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.

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

Document correct OAuth2 handling of invalid_credentials errors vs deprecated methods #575

Closed
clocked0ne opened this issue Dec 19, 2018 · 7 comments · Fixed by #804
Closed
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@clocked0ne
Copy link

clocked0ne commented Dec 19, 2018

We have encountered some difficulties using server side OAuth2 with offline access granted using a refresh token that are not made clear from the breaking changes introduced in the latest version of the googleapis library that depends on this package:

[google-auth-library:DEP007] DeprecationWarning: The refreshAccessToken method has been deprecated, and will be removed in the 3.0 release of google-auth-library. Please use the getRequestHeaders method instead.

This library is documented as of v2.0.0 as having The refreshAccessToken method deprecated and sugests different replacement methods to the actual deprecation warning:

The OAuth2.refreshAccessToken method has been deprecated. The getAccessToken, getRequestMetadata, and request methods will all refresh the token if needed automatically. There is no need to ever manually refresh the token.

However The getRequestMetadata method has also been deprecated:
https://github.com/googleapis/google-auth-library-nodejs/releases/tag/v2.0.0

It is described that the above methods will refresh the token if needed automatically, but this is not always the case. invalid_credentials errors (which do not indicate the underlying cause) do not make an attempt to generate a new access token using the refresh token, yet calling the deprecated refreshAccessToken method often resolves this and refreshes the access token successfully; as a useful side effect the tokens retrieved can be automatically saved under the tokens event.

This raises some questions that really should have documented answers:
Why does an invalid_credentials error not automatically try to refresh the tokens as a token session expired error would?
Why is there no direct replacement for the refreshAccessToken method?

If a refresh_token is set again on OAuth2Client.credentials.refresh_token, you can can [sic] refreshAccessToken():
const tokens = await oauth2Client.refreshAccessToken(); // your access_token is now refreshed and stored in oauth2Client store these new tokens in a safe place (e.g. database)

So why is it deprecated?

Under normal circustances the library handles expired tokens fine. For invalid_grant errors we disable further processing for the user until they have re-authed seperately via our frontend and obtained completely new access/refresh tokens. It does not seem like we should be doing the same for invalid_credentials errors given it appears to be possible to refresh the access token.

I have tried to look at all related issues to this deprecation and how these scenarios should be handled but there does not seem to be a defined solution anywhere that doesn't involve the still documented yet deprecated refreshAccessToken method.

@clocked0ne
Copy link
Author

#570 relates to this

@JustinBeckwith JustinBeckwith added triage me I really want to be triaged. 🚨 This issue needs some love. labels Dec 20, 2018
@JustinBeckwith JustinBeckwith added type: docs Improvement to the documentation for an API. and removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Jan 21, 2019
@JustinBeckwith
Copy link
Contributor

Thank you for this really well written issue! I want to split this up into a few parts.

invalid_credentials

I have personally never run into this error. Can you share more about what happens? Does this happen when calling refreshAccessToken? Do you have a status code that comes back from the server handy? If we understand the situations where this happens, I can look into automatic retries if it makes sense. Any details here would be super helpful!

getRequestHeaders

This may be a very naive question - but is there any reason the getRequestHeaders method wouldn't work here? It feels like you're saying:

  • access_token can fall into an invalid state (somehow)
  • expiry_date is set, and not expired or close to expiring
  • refresh_token is set

With those conditions - it is accurate that we won't attempt to refresh the access token automatically. Instead, you'll get the same ol cached one.

I'm getting closer to being convinced we should un-deprecate this, but I want to make sure I 100% understand the details first :)

@JustinBeckwith JustinBeckwith self-assigned this Apr 13, 2019
@JustinBeckwith JustinBeckwith added needs more info This issue needs more information from the customer to proceed. type: question Request for information or clarification. Not an issue. and removed type: docs Improvement to the documentation for an API. labels Apr 13, 2019
@clocked0ne
Copy link
Author

clocked0ne commented Apr 15, 2019

Hi @JustinBeckwith thanks for getting involved.

invalid_credentials

I have personally never run into this error. Can you share more about what happens? Does this happen when calling refreshAccessToken? Do you have a status code that comes back from the server handy? If we understand the situations where this happens, I can look into automatic retries if it makes sense. Any details here would be super helpful!

This error occurs quite frequently from looking at our logs, we have a few thousand active users and its occuring hundreds of times a day; I'm guessing your assumptions about the access_token or expiry_date are correct. From looking at the logs and the code below I've been able to deduce that in every case simply calling refreshAccessToken fixes this, as I can see no examples where a failure/error has been the result logged instead.

Our requests use retries, so on Google API errors we pass the errors to a function which subsequently calls refreshAccessToken and looks like this (its old code so please forgive the quality):

const googleHandleAPIErrors = function (callerPath, errorMessage, callback) {
	if (errorMessage.match(/invalid.+credentials/gi)) {
		log.info(
			{ req_id: self._user._id, module: 'GmailAPIWrapper' },
			`Invalid credentials for User`
		);
		// This method is deprecated
		return oauth2Client.refreshAccessToken((error, tokens, response) => {
			if(error) {
				if (response && response.body) {
					log.info(
						{ req_id: self._user._id, module: 'GmailAPIWrapper', error: response.body },
						`oauth2Client.refreshAccessToken API call response`
					);
				}
				if(error.message && error.message.indexOf('invalid_grant') > -1) {
					return googleHandleInvalidGrant(callerPath, error)
						.catch(err => callback(err));
				} else {
					return callback(error);
				}
			} else {
				log.info(
					{ req_id: self._user._id, module: 'GmailAPIWrapper', tokens },
					'New AccessToken retrieved for user'
				);
				// Tokens are automatically updated by oauth2Client so force request retry
				return callback(new Error('Got new token, please retry the request'));
			}
		});
	}

	if (errorMessage === 'invalid_grant') {
		log.info(
			{ req_id: self._user._id, module: 'GmailAPIWrapper' },
			`User's Google grant is invalid`
		);
		return googleHandleInvalidGrant(callerPath, errorMessage)
			.catch(err => callback(err));
	}

	if(errorMessage.match(/Mail service not enabled/gi))
		return setUserUnsupported(callerPath)
			.catch(err => callback(err));

	return callback(errorMessage);
};

googleHandleInvalidGrant simply marks the user in the db as having an invalid grant/requiring re-auth.

As you can see we only pass in the error message currently, I would need to add some additional logging/modify this to capture more such as the response status code, etc. Happy to do this if you deem it necessary.

May/may not be relevant, but this is how we set up the client for use on instantiating our wrapper:

const oauth2Client = new google.auth.OAuth2(clientId, clientSecret, callbackURL);

oauth2Client.on('tokens', tokens => updateUserAccessToken(tokens)); // Saves to DB

oauth2Client.setCredentials({
	access_token: _user.accessToken,
	refresh_token: _user.refreshToken,
	expiry_date: new Date(_user.expiryDate).valueOf() || true
});

@clocked0ne
Copy link
Author

clocked0ne commented Sep 4, 2019

Just to add to this as there hasn't been any noticable changes in newer versions, we now periodically see the error response No access, refresh token or API key is set but cannot see a root cause, it would be good to know how this situation can also come about.

@bcoe bcoe added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. and removed needs more info This issue needs more information from the customer to proceed. type: question Request for information or clarification. Not an issue. labels Sep 5, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Sep 5, 2019
@bcoe
Copy link
Contributor

bcoe commented Sep 10, 2019

@clocked0ne could you provide a full stack trace of one of the errors you are receiving when you fail to refresh your token (err.stack).

We have multiple checks and balances in place, so I'm a bit surprised you're needing to manually call refreshAccessToken.

@bcoe
Copy link
Contributor

bcoe commented Sep 27, 2019

@clocked0ne how are you instantiating your client, are you creating a new JWT instance, similar to the approach discussed here. If so, I believe we just patched a major issue with token refresh logic, mind taking it for a spin?

If you're using new OAuth2Client() directly, perhaps I could bother you to provide a reproduction modeled after @aaleksandrov's? for starters, what service are you interacting with?

@clocked0ne
Copy link
Author

clocked0ne commented Oct 2, 2019

@bcoe Thanks for getting back about this.

As this is on company time unfortunately I'm not at liberty to dive back into it as readily, however I can show you our current client instantiation:

const { google } = require('googleapis');
const oauth2Client = new google.auth.OAuth2(
    config.googleAuth.clientId,
    config.googleAuth.clientSecret,
    config.googleAuth.callbackURL
);

oauth2Client.on('tokens', tokens => updateUserAccessToken(tokens)); // Writes to DB & `_user`

oauth2Client.setCredentials({
    access_token: _user.accessToken,
    refresh_token: _user.refreshToken,
    expiry_date: _user.expiryDate
        ? new Date(self._user.expiryDate).valueOf()
        : Date.now()
});

We then make requests such as:

const gmail = google.gmail('v1');
gmail.users.watch(
    {
        userId: 'me',
        auth: oauth2Client,
        resource: {
            topicName: 'name'
        },
    },
    (err, watchResponse) => {}
);

If there is an error, we pass this to an error handler function to try to resolve, if it is an invalid_grant response we save this to the DB/user as their grant has been revoked so they can re-auth on the frontend. In these specific instances, the error response causing issues is 'invalid credentials':

function googleHandleAPIErrors(errorMessage, callback) {
    if (errorMessage.match(/invalid credentials/gi)) {
        log.info({ req_id: _user.id }, `Invalid credentials for User`);

        return oauth2Client.refreshAccessToken((error, tokens, response) => {
            if(error) {
                if(error.message && error.message.indexOf('invalid_grant') > -1) {
                    return googleHandleInvalidGrant(error).catch(callback);
                }
                return callback(error);
            }
            log.info({ req_id: _user.id, tokens }, 'New AccessToken retrieved for user');
            // Tokens are automatically updated by oauth2Client so force request retry
            return callback(new Error('Got new token, please retry the request'));
        });
    }

    if (errorMessage === 'invalid_grant') {
        log.info({ req_id: _user.id }, `User's Google grant is invalid`);
        return self.googleHandleInvalidGrant(errorMessage).catch(callback);
    }
    if(errorMessage.match(/Mail service not enabled/gi))
        return self.setUserUnsupportedTrue().catch(callback);
    return callback(errorMessage);
}

If time becomes available I will try and introduce some extra debug logging to capture the API responses in more detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
4 participants