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 unlink social accounts #253

Merged
merged 26 commits into from
Apr 13, 2024

Conversation

diegosperes
Copy link
Contributor

@diegosperes diegosperes commented Mar 28, 2024

Where the discussion started. Authn issue.

Summary

This PR is adding the ability of share oauth account information and delete them.

Changes

  • Adding email column to oauth_accounts table in order to facilitate users identify their accounts.
  • Updating GET /account/{id} to return oauth_providers containing OAuth information.
  • Adding new public endpoint DELETE /oauth/{provider} allowing user unlink OAuth accounts.
  • Adding new public endpoint GET /oauth/info returning oauth information for linked accounts.
  • Adding new private endpoint DELETE /account/{id}/oauth allowing unlink OAuth accounts for the given user.

There's a particular use case that requires special attention. When a user registers via the OAuth flow, a random password is generated. To unlink the social account, Authn verifies whether the user needs to set a new password. As a side effect, a new error, NEW_PASSWORD_REQUIRED, has been introduced.

docker-compose.yml Outdated Show resolved Hide resolved
server/handlers/delete_account_oauth.go Outdated Show resolved Hide resolved
app/data/mysql/migrations.go Show resolved Hide resolved
app/services/account_oauth_ender.go Outdated Show resolved Hide resolved
app/services/account_oauth_ender_test.go Outdated Show resolved Hide resolved
server/handlers/get_account_test.go Outdated Show resolved Hide resolved
server/handlers/get_account_test.go Outdated Show resolved Hide resolved
server/handlers/get_oauth_info_test.go Outdated Show resolved Hide resolved
server/handlers/get_oauth_info_test.go Outdated Show resolved Hide resolved
server/public_routes.go Outdated Show resolved Hide resolved
@diegosperes diegosperes force-pushed the oauth-unlink-social-accounts branch 2 times, most recently from 7f6cee1 to ab77e1c Compare April 1, 2024 22:05
@AlexCuse AlexCuse mentioned this pull request Apr 3, 2024
@AlexCuse AlexCuse self-requested a review April 3, 2024 01:31
Copy link
Member

@cainlevy cainlevy left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I'm sorry for the delay in reviewing.

app/data/mysql/migrations.go Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
app/services/account_oauth_ender.go Outdated Show resolved Hide resolved
app/services/account_oauth_ender.go Outdated Show resolved Hide resolved
app/services/account_oauth_getter.go Outdated Show resolved Hide resolved
server/handlers/delete_account_oauth.go Show resolved Hide resolved
server/handlers/get_account.go Outdated Show resolved Hide resolved
server/public_routes.go Outdated Show resolved Hide resolved
app/services/account_oauth_ender.go Outdated Show resolved Hide resolved
app/services/account_oauth_ender.go Outdated Show resolved Hide resolved
app/services/account_oauth_ender.go Outdated Show resolved Hide resolved
server/handlers/delete_account_oauth.go Show resolved Hide resolved
server/handlers/delete_account_oauth_test.go Outdated Show resolved Hide resolved
app/services/account_getter.go Outdated Show resolved Hide resolved
server/handlers/get_account_test.go Outdated Show resolved Hide resolved
@diegosperes
Copy link
Contributor Author

diegosperes commented Apr 8, 2024

@AlexCuse @cainlevy Thanks very much for your comments, I have updated the code accordingly to them 😄

Copy link
Contributor

@AlexCuse AlexCuse left a comment

Choose a reason for hiding this comment

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

This is looking really good think this is my last round of comments.

app/models/account.go Outdated Show resolved Hide resolved
app/services/account_getter_test.go Show resolved Hide resolved
app/services/account_oauth_ender.go Outdated Show resolved Hide resolved
@@ -0,0 +1,62 @@
package services_test
Copy link
Contributor

Choose a reason for hiding this comment

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

rename file

app/services/identity_reconciler.go Outdated Show resolved Hide resolved
app/services/identity_reconciler.go Show resolved Hide resolved
app/services/identity_reconciler.go Outdated Show resolved Hide resolved
app/services/identity_reconciler_test.go Show resolved Hide resolved
@AlexCuse
Copy link
Contributor

@cainlevy do you think we need to worry about revoking the stored access token here or is it sufficient to "forget"? Looking at apple's implementation it would be fairly simple, though it does require an additional URL to be defined https://developer.apple.com/documentation/sign_in_with_apple/revoke_tokens/

A quick search on google and microsoft seems to indicate the tokens expire after an hour anyway so may be nothing to worry about there.

@AlexCuse AlexCuse mentioned this pull request Apr 10, 2024
app/services/identity_reconciler.go Outdated Show resolved Hide resolved
app/services/identity_reconciler_test.go Outdated Show resolved Hide resolved
server/handlers/get_oauth_accounts_test.go Outdated Show resolved Hide resolved
Copy link
Member

@cainlevy cainlevy left a comment

Choose a reason for hiding this comment

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

thank you! it's looking good.

docs/api.md Show resolved Hide resolved
@cainlevy
Copy link
Member

do you think we need to worry about revoking the stored access token here or is it sufficient to "forget"?

Personally I would expect it's enough to forget them. Revoking them sounds very thorough but also would require a higher level of integration with each identity provider.

@AlexCuse AlexCuse merged commit 9c9929a into keratin:main Apr 13, 2024
2 checks passed
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.

4 participants