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

Implement custom oauth state #269

Closed
wants to merge 1 commit into from

Conversation

anderspitman
Copy link

Fixes #267

@sugyan let me know if this isn't what you're looking for

@sugyan
Copy link
Owner

sugyan commented Dec 14, 2024

Thank you for the pull-request. But I think this is wrong.
The state is the key that the oauth-client uses to share data between the two requests, and should not be specified by the application implementer (it could easily cause a conflict)

The official SDK implementation of TypeScript is as follows. The state passed as an option to .authorize() is simply named appState and the value is stored directly in stateData. The data is then restored in .callback() and the value of appState is returned.
https://github.com/bluesky-social/atproto/blob/51b0c48ce7d502f355cecd956b7773069abff432/packages/oauth/oauth-client/src/oauth-client.ts#L294-L301
https://github.com/bluesky-social/atproto/blob/51b0c48ce7d502f355cecd956b7773069abff432/packages/oauth/oauth-client/src/oauth-client.ts#L466
This is the use of the state option in .authorize() that I mentioned in #267.

The branch I am working on will port this official SDK implementation as is (not yet to the point where I can merge it...).
https://github.com/sugyan/atrium/pull/243/files#diff-9cd4899596d78f0372c6f0fe0a39cc4b293c99dc03d12b953e94f8ed1db93edcR168
https://github.com/sugyan/atrium/pull/243/files#diff-9cd4899596d78f0372c6f0fe0a39cc4b293c99dc03d12b953e94f8ed1db93edcR250

@anderspitman
Copy link
Author

Ah ok I misunderstood. I believe that approach will also work for me. Since my fork is simple and works for now, there's no rush on my end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Custom OAuth state
2 participants