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

OAuth implementation #16

Merged
merged 45 commits into from
Oct 12, 2023
Merged

OAuth implementation #16

merged 45 commits into from
Oct 12, 2023

Conversation

augustuswm
Copy link
Contributor

@augustuswm augustuswm commented Sep 8, 2023

This replaces the openid (Google) and access token (GitHub) authentication methods with an OAuth implementation. This implementation forwards authentication to Google or GitHub and transforms the returned user into an RFD API user. Changes here fix #1, #2, and #9.

There are still a few outstanding issues:

  • All state for authentication attempts is stored in the database, which means if the state parameter is corrupted (or not returned) by a remote OAuth system we can not determine what login attempt the call was for. This likely needs to be fixed by storing short-lived session data on the client system prior to redirecting to the remote provider.
  • Account merging is not implemented. If I log in with the Google and GitHub accounts that both have the verified email address of [email protected], the API will generate to separate user for each. In a future state, I should be able to log in to my RFD API accoutn via my Google account and then connect my GitHub account as an authn provider as well. This will require work to determine how to merge two or more RFD API accounts.

@augustuswm augustuswm changed the title Initial work on changing to oauth OAuth implementation Sep 11, 2023
@augustuswm augustuswm marked this pull request as draft September 11, 2023 19:26
rfd-api/src/authn/key.rs Outdated Show resolved Hide resolved

pub struct NewApiKey {
pub struct RawApiKey {
clear: String,

Choose a reason for hiding this comment

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

Suggested change
clear: String,
clear: secrecy::SecretString,

Sounds like a good candidate for secrecy::SecretString here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been changed to a Vec<u8> and I'm looking at wrapping it with zeroize

rfd-api/src/authn/key.rs Outdated Show resolved Hide resolved
rfd-api/src/authn/key.rs Outdated Show resolved Hide resolved
@augustuswm augustuswm marked this pull request as ready for review September 28, 2023 14:01
@augustuswm
Copy link
Contributor Author

augustuswm commented Sep 28, 2023

This should be ready for initial review. Few known issues:

  1. expires_at on LoginAttempt should not be optional
  2. Access token duration is hardcoded instead of configurable
  3. error_description and error_uri are not included when constructing failed redirect uris

@augustuswm augustuswm merged commit 90f987d into main Oct 12, 2023
6 checks passed
@augustuswm augustuswm deleted the oauth branch October 12, 2023 18:28
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.

Determine API token implementation
2 participants