Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[ENG-5769] Oauth 1.0a integration #78
[ENG-5769] Oauth 1.0a integration #78
Changes from 6 commits
18f491f
d84a2c1
271cba0
f9ec901
8552be5
7db1040
5375b52
7c17719
6d2200f
1ebf5d4
da9b7e3
08f8d9a
05b1873
63d76db
2541c24
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do we have to store the
authorized_account.pk
in the session? Is there anything in the payload of the request to the callback url that can be used to identify which authorized account this is?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.
There isn't anything in payload which'd allow for is to identify it, I don't like this approach too, but I don't have any better ideas
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 callback request includes the same temporary token received when initiating oauth1 -- could add a model to hold oauth1 temporary state (parallel
OAuth2TokenMetadata
) with indexedtemporary_token
field(edit: nevermind -- i think that's true in specs but not for zotero, oh well)
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.
For Oauth 1a, it should still be
AccessTokenCredentials
, right? I believe this affects how the header is constructed for the http requests.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.
There are
oauth_token
andoauth_token_secret
, which in their nature and usage are drastically different from OAuth2 access and refresh tokensThere 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.
Is there not an
auth_callback_url
?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.
auth_url
isauth_callback_url
, I'll change it if it's confusingThere 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.
For the record: we're storing the
auth_callback_url
explicitly in case there are services that won't allow us to update the list of callbacks without invalidating our client and/or old credentials. That way, if necessary, we can encode this ashttps://[environment.]osf.io/oauth/callback/[provider]
-- where we already have things set up to forward requests to GV if appropriate.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.
hmm, the client secret should be stored encrypted -- but since OAuth2ClientConfig is the same, maybe worth splitting that into another ticket? (...and i'm starting to reconsider the EncryptedDataclassModel abstract base, now that we have three potential uses for it...)
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 agree, storing any unencrypted credentials in the db, poses security risks
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.
Yes, separate ticket. Need input from cloud eng/devops as to whether they want this in the database at all or if they'd prefer an approach like this one