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

Add option to use OAuth "state" param in internet accounts #3296

Merged
merged 2 commits into from
Nov 1, 2022

Conversation

garrettjstevens
Copy link
Collaborator

This adds to the option to use the state parameter in an OAuth authorization. By default it is read from the config, but if dynamic state is needed, the get state getter can be overridden in an extending model.

Motivation is to use this in Apollo to keep track of some state across the OAuth flow.

@garrettjstevens garrettjstevens self-assigned this Oct 27, 2022
@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Oct 27, 2022
@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Merging #3296 (3bab368) into main (cb4ebf9) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #3296      +/-   ##
==========================================
- Coverage   59.59%   59.58%   -0.01%     
==========================================
  Files         731      731              
  Lines       28887    28891       +4     
  Branches     6984     6987       +3     
==========================================
+ Hits        17215    17216       +1     
- Misses      11428    11431       +3     
  Partials      244      244              
Impacted Files Coverage Δ
...gins/authentication/src/OAuthModel/configSchema.ts 50.00% <ø> (ø)
plugins/authentication/src/OAuthModel/model.tsx 5.91% <0.00%> (-0.15%) ⬇️
...rative-view/src/ServerSideRenderedBlockContent.tsx 68.00% <0.00%> (+4.00%) ⬆️

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

@cmdcolin
Copy link
Collaborator

from previous work we've done, we have to use a non-getter (which i call a method) to be able to override it in an extending model. examples are things like trackMenuItems

@cmdcolin
Copy link
Collaborator

maybe that is only if you need to use super though...but might not be a bad thing to use anyways

@garrettjstevens garrettjstevens added enhancement New feature or request and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Oct 27, 2022
@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Oct 27, 2022
@garrettjstevens
Copy link
Collaborator Author

Good point, made it a method

@cmdcolin
Copy link
Collaborator

can you clarify how state is intended to be used? it seems related to various security concerns so knowing what the intent is might be helpful (at least for me :)) https://auth0.com/docs/secure/attack-protection/state-parameters#use-the-stored-url-to-redirect-users

@garrettjstevens
Copy link
Collaborator Author

Specifically for Apollo, this is going to be used in the way the article you linked describes:

You can use the state parameter to encode an application state that will put the user where they were before the authentication process started

Since in Apollo 3, the JBrowse 2 instance does not necessarily have to be on the same server as the Apollo Collaboration Server, the URL of the JBrowse 2 instance is sent in the state so the redirect goes to the correct place.

In Apollo 3 the server itself is doing the signing in instead of the client, and it is does the same thing described in that article, using a stored nonce to keep track of the state data. It uses the passport-oauth2 library to do that.

@cmdcolin cmdcolin removed the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Nov 1, 2022
@cmdcolin cmdcolin merged commit 14aa9fa into main Nov 1, 2022
@cmdcolin cmdcolin changed the title Add option to use OAuth "state" param Add option to use OAuth "state" param in internet accounts Nov 1, 2022
@cmdcolin cmdcolin deleted the add_state_to_oauth branch January 10, 2023 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants