-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix: Fixes refresh_token not being obtained all the time. #10
Conversation
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.
There was a problem hiding this 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' |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This could be closed without merge when #11 will be accepted. |
negotiated by #11 |
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.