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

Use validateToken to exchange refresh token in OAuth getFetcher #3750

Merged
merged 2 commits into from
Jun 8, 2023

Conversation

garrettjstevens
Copy link
Collaborator

One tiny bugfix in this (was missing a "$" in a template literal substitution). Feel free to cherry-pick that commit out if the other commit requires more discussion.

Side note, it seems like there should be some sort of eslint rule that warns about { instead of ${ in template literals, but the closest thing I could find was disallowing backticks unless they contain a substitution or a newline:

{
  "quotes":  ["warn", "single", { "avoidEscape": true }]
}

The other commit is something where when I was reviewing #3742 I kept feeling like I'd tried to do something similar before, and I finally remembered what it was. It's possible to use validateToken to accomplish what I think is the same thing using getPreAuthorizationInformation does, which is basically making sure the refresh token for something that doesn't use RPC gets exchanged correctly. It seems to work fine in my testing.

I thought it might be a bit less confusing since getPreAuthorizationInformation was originally created specifically for RPC, so calling it in the main thread might be counterintuitive.

@garrettjstevens garrettjstevens self-assigned this Jun 7, 2023
@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
@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Merging #3750 (be6553a) into refresh_token_refactor (bce2414) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@                   Coverage Diff                   @@
##           refresh_token_refactor    #3750   +/-   ##
=======================================================
  Coverage                   64.09%   64.10%           
=======================================================
  Files                         987      987           
  Lines                       29669    29664    -5     
  Branches                     7095     7094    -1     
=======================================================
- Hits                        19016    19015    -1     
+ Misses                      10489    10485    -4     
  Partials                      164      164           
Impacted Files Coverage Δ
...tication/src/HTTPBasicModel/HTTPBasicLoginForm.tsx 78.57% <ø> (ø)
plugins/authentication/src/OAuthModel/model.tsx 8.33% <0.00%> (+0.25%) ⬆️

... and 1 file with indirect coverage changes

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

@cmdcolin
Copy link
Collaborator

cmdcolin commented Jun 7, 2023

nice! validateToken seems like a nice semantic way to do it. we could possibly add some more javadoc around this, but happy to merge

@cmdcolin cmdcolin merged commit 2df65a6 into refresh_token_refactor Jun 8, 2023
11 checks passed
@cmdcolin
Copy link
Collaborator

cmdcolin commented Jun 8, 2023

I proposed possibly making this a new eslint rule in the unicorn set here sindresorhus/eslint-plugin-unicorn#2150

@cmdcolin cmdcolin deleted the oauth_refresh_validate branch June 8, 2023 00:08
cmdcolin added a commit that referenced this pull request Jun 8, 2023
* Refresh token refactor

* Change the if/else into a boolean flag

* Use validateToken to exchange refresh token in OAuth getFetcher (#3750)

* Add missing "$" in template

* Use validateToken to ensure refresh token is exchanged

---------

Co-authored-by: Garrett Stevens <stevens.garrett.j@gmail.com>
@garrettjstevens
Copy link
Collaborator Author

I proposed possibly making this a new eslint rule in the unicorn set here sindresorhus/eslint-plugin-unicorn#2150

ooh, that would be nice if someone implemented a rule like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants