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

fix: Fixes refresh_token not being obtained all the time. #10

Conversation

JJetmar
Copy link

@JJetmar JJetmar commented Jan 3, 2023

fix: Fixes refresh_token not being obtained all the time.
fix: Obtained code is not being propagated and processed by the server.
fix: No more active waiting when code is being obtained.

fix: Obtained code is not being propagated and processed by the server.
fix: No more active waiting when code is being obtained.
fix: Obtained code is not being propagated and processed by the server.
fix: No more active waiting when code is being obtained.
Copy link
Owner

@metalwarrior665 metalwarrior665 left a comment

Choose a reason for hiding this comment

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

I am not sure now about the reason for this

@@ -49,8 +49,13 @@ module.exports.apifyGoogleAuth = async ({ scope, tokensStore, credentials, googl
const authorizeUrl = oAuth2Client.generateAuthUrl({
access_type: 'offline',
scope: `https://www.googleapis.com/auth/${scope}`,
// Always prompt for user consent otherwise, there is a chance to not obtain refresh_token
// https://stackoverflow.com/a/10857806
prompt: 'consent'
Copy link
Owner

Choose a reason for hiding this comment

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

Does this help us in current situation? Because by default we will use the code stored in KV store even if it is not viable and it will fail. But I didn't really have problems with needing a refresh. What is the actual problem you are solving?

Copy link
Author

@JJetmar JJetmar Jan 3, 2023

Choose a reason for hiding this comment

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

As mentioned in the stackoverflow - It helps in development when you are testing/using the same account for the same app and redirectUrl. The refresh_token seems to be returned only for the first time in this case. Then you have to manually go to your google account and remove the consent and then give it again to the application I noticed this because otherwise I had to give new consent every 30minutes (which is pretty annoying) since the credentials were saved without refresh_token which is required for obtaining the new tokens.

See googleapis/google-api-nodejs-client#750 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

you are not storing the code, you are storing the received tokens.

});
let code;

const codeHolder = { code: null }; // To be able to access code as reference from any scope
Copy link
Owner

Choose a reason for hiding this comment

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

What is the scope we want to access this? We are sending it somewhere?

Copy link
Author

Choose a reason for hiding this comment

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

The change in code was not detected in the setInterval body (I think due to different closure).

]);

clearInterval(timeoutInterval); // clear Interval no matter what happened
codeBeenSet(); // Resolve in case of timeout
Copy link
Owner

Choose a reason for hiding this comment

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

What is the reason for this rewrite, it seems like the same thing with more complicated code, am I missing something?

Copy link
Author

Choose a reason for hiding this comment

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

You don't have to wait another 10sec after the code was already set.

Copy link
Author

Choose a reason for hiding this comment

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

I may use synchronize it with EventEmitter without creating extra promises.

@JJetmar
Copy link
Author

JJetmar commented Apr 6, 2023

This could be closed without merge when #11 will be accepted.

@JJetmar
Copy link
Author

JJetmar commented Jun 19, 2023

negotiated by #11

@JJetmar JJetmar closed this Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants