Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

error-reporting: Skip authentication for error-reporting when API key is provided #2554

Merged
merged 9 commits into from
Sep 15, 2017
Merged

error-reporting: Skip authentication for error-reporting when API key is provided #2554

merged 9 commits into from
Sep 15, 2017

Conversation

kjin
Copy link
Contributor

@kjin kjin commented Aug 22, 2017

Addresses #2302

When key is provided, the error reporting service instance doesn't go through the OAuth2 process. This means that if a provided API key is invalid, errors will never be sent (there is no fallback); however, an error will be printed in this case.

@kjin kjin requested review from ofrobots and DominicKramer August 22, 2017 22:16
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 22, 2017
DominicKramer
DominicKramer reviewed Aug 22, 2017
View reviewed changes
packages/error-reporting/src/google-apis/auth-client.js Outdated
}
});
} else {
that._logger.info('API key provided; skipping OAuth2 token generation.');

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

packages/error-reporting/src/google-apis/auth-client.js
].join(' '));
}
});
if (tryAuthenticate) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@DominicKramer DominicKramer changed the title Skip authentication for error-reporting when API key is provided error-reporting: Skip authentication for error-reporting when API key is provided Aug 22, 2017
DominicKramer
DominicKramer reviewed Aug 23, 2017
View reviewed changes
packages/error-reporting/src/google-apis/auth-client.js Outdated
@@ -127,7 +119,9 @@ class RequestHandler extends common.Service {
if (this._config.getShouldReportErrorsToAPI()) {
this.request({
uri: 'events:report',
qs: RequestHandler.manufactureQueryString(this._config.getKey()),
qs: {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

DominicKramer
DominicKramer reviewed Aug 23, 2017
View reviewed changes
packages/error-reporting/test/unit/google-apis/auth-client.js Outdated
key: 'key'
};
var message = 'Made OAuth2 Token Request';
verifyReportedMessage(config, new Error(message), '');

This comment was marked as spam.

Sign in to view

packages/error-reporting/src/google-apis/auth-client.js
].join(' '));
}
});
if (tryAuthenticate) {

This comment was marked as spam.

Sign in to view

@googleapis googleapis deleted a comment from coveralls Aug 24, 2017
@googleapis googleapis deleted a comment from coveralls Aug 24, 2017
@googleapis googleapis deleted a comment from coveralls Aug 24, 2017
@googleapis googleapis deleted a comment from coveralls Aug 24, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 689a233 on kjin:e-r-a into f243e26 on GoogleCloudPlatform:master.

@googleapis googleapis deleted a comment from coveralls Aug 28, 2017
@kjin
Copy link
Contributor Author

kjin commented Aug 31, 2017

@ofrobots PTAL. The most recent commit warns if the API key is invalid ahead of time, but depends on the error message in the response body. This works, but if you think it's too fragile I'll revert to the previous commit, which adds in the README a message informing users that the we won't be able to detect invalid API keys ahead of time.

ofrobots
ofrobots reviewed Aug 31, 2017
View reviewed changes
packages/error-reporting/README.md Outdated

We recommend storing the API key in a file rather than hard-coding it into your application's source code.

**Note:** The Error Reporting instance will check if the provided API key is invalid shortly after it is instantiated. If the key is invalid, an error-level message will be logged.

This comment was marked as spam.

Sign in to view

packages/error-reporting/src/google-apis/auth-client.js
qs: RequestHandler.manufactureQueryString(this._config.getKey()),
method: 'POST',
json: {}
}, (err, body, response) => {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@lukesneeringer
Copy link
Contributor

@ofrobots Another question that we should ask is: Is this something we want to support? I was of the understanding that not supporting API keys in the client libraries was an intentional decision. I am definitely hesitant to add it as a one-off in a single library.

@ofrobots
Copy link
Contributor

@lukesneeringer The API is not being added, but rather fixed in this PR. For whatever historical reasons, error-reporting started with API key support and removing it at this point is going to break existing users. I'm 👎 on that.

@kjin
Copy link
Contributor Author

kjin commented Sep 14, 2017

@ofrobots @DominicKramer I've added a system test to check that the messages supplied from the Stackdriver Errors API are in line with what we expect. I think this is a reasonable way to act upon any future changes in the API - let me know your thoughts.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 80b5dd0 on kjin:e-r-a into 9e78eba on GoogleCloudPlatform:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8b7f9bc on kjin:e-r-a into 9e78eba on GoogleCloudPlatform:master.

ofrobots
ofrobots approved these changes Sep 15, 2017
View reviewed changes
Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once nits are addressed.

packages/error-reporting/README.md Outdated

If a key is provided, the module will not attempt to authenticate using the methods associated with locally-stored credentials as mentioned in the previous section.

We recommend storing the API key in a file rather than hard-coding it into your application's source code.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a2dff29 on kjin:e-r-a into 9e78eba on GoogleCloudPlatform:master.

@kjin kjin merged commit 8ca2a85 into googleapis:master Sep 15, 2017
stephenplusplus pushed a commit to lukesneeringer/google-cloud-node that referenced this pull request Sep 18, 2017
@kjin @stephenplusplus
error-reporting: Skip authentication for error-reporting when API key…
9064626 
… is provided (googleapis#2554)

PR-URL: googleapis#2554
@kjin
Copy link
Contributor Author

kjin commented Oct 3, 2017

@stephenplusplus Could you do a release of the error-reporting library? I think this is a semver-patch change. Thanks!

@stephenplusplus
Copy link
Contributor

Published 0.2.2!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DominicKramer DominicKramer DominicKramer left review comments

@0x4b 0x4b 0x4b left review comments

@ofrobots ofrobots ofrobots approved these changes

Assignees
No one assigned
Labels
api: error-reporting cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kjin @coveralls @lukesneeringer @ofrobots @stephenplusplus @0x4b @DominicKramer @googlebot