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

Avoid re-prompting a user login when refresh token is processed #3749

Merged
merged 3 commits into from
Jun 8, 2023

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented Jun 7, 2023

Follow up to the oauth PR

Also does a couple refactors

a) disables the need to have any config slot for "hasRefreshToken". it is just there or not in the responses, and eagerly attempted to use
b) misc immutable js syntax sugar
c) adds more console.error logs where before they were silent, which may make the log noisier but may improve debugging

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Jun 7, 2023
// may want to improve handling
// eslint-disable-next-line @typescript-eslint/no-floating-promises
this.useEndpointForAuthorization(resolve, reject)
if (doUserFlow) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only prompts the user flow if !refreshToken or refreshToken fails

@cmdcolin cmdcolin added bug Something isn't working and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Jun 7, 2023
@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Merging #3749 (bce2414) into main (d3219d6) will decrease coverage by 0.02%.
The diff coverage is 10.00%.

❗ Current head bce2414 differs from pull request most recent head 2df65a6. Consider uploading reports for the commit 2df65a6 to get more accurate results

@@            Coverage Diff             @@
##             main    #3749      +/-   ##
==========================================
- Coverage   64.10%   64.09%   -0.02%     
==========================================
  Files         987      987              
  Lines       29668    29669       +1     
  Branches     7099     7095       -4     
==========================================
- Hits        19019    19016       -3     
- Misses      10485    10489       +4     
  Partials      164      164              
Impacted Files Coverage Δ
...thentication/src/DropboxOAuthModel/configSchema.ts 50.00% <ø> (ø)
...ins/authentication/src/DropboxOAuthModel/model.tsx 16.66% <0.00%> (ø)
...gins/authentication/src/OAuthModel/configSchema.ts 50.00% <ø> (ø)
plugins/authentication/src/OAuthModel/model.tsx 8.07% <0.00%> (+0.47%) ⬆️
plugins/authentication/src/OAuthModel/util.ts 0.00% <0.00%> (ø)
...uggableElementTypes/models/InternetAccountModel.ts 62.35% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin cmdcolin force-pushed the refresh_token_refactor branch from 2bb0941 to bce2414 Compare June 7, 2023 22:07
* Add missing "$" in template

* Use validateToken to ensure refresh token is exchanged
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Jun 8, 2023

maybe can just go ahead with merge, and test on main...I think it should be good to go

@cmdcolin cmdcolin merged commit 27484b9 into main Jun 8, 2023
@cmdcolin cmdcolin deleted the refresh_token_refactor branch June 8, 2023 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants