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.

Sign up for GitHub

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

Jump to bottom

Generic OAuth: Prevent adding duplicated users #32286

Merged
merged 14 commits into from
Apr 9, 2021
Merged

Generic OAuth: Prevent adding duplicated users #32286

merged 14 commits into from
Apr 9, 2021

Conversation

dsotirakis
Copy link
Contributor

What this PR does / why we need it:

The purpose of this PR is to prevent adding duplicated users in user_auth column, when logging in.
Essentially, since we'll have to deal with many duplicated entries already, we add a check where:

image

Notes:

  1. Check for auth_id is now removed, since no auth_id exists when using Generic OAuth. This was the main cause of the problem, since every time a new user with Generic OAuth login existed was re-added on the database.
  2. The new DELETE operation, can be potentially removed in the future.

Which issue(s) this PR fixes:

Fixes #23077

Special notes for your reviewer:

Steps to reproduce:

  • Make sure you have a Generic OAuth provider config ready. (The most easy and straight-forward one should be to add a Bitbucket config)
  • Set up the [auth.generic_oauth] config in your custom.ini file inside grafana repo.
  • Make sure you are on master, then try to login and logout 3-4 times.
  • Then go to grafana.db, browse user_auth table, and you'll see something like:
id user_id auth_module auth_id created o_auth_access_token o_auth_refresh_token o_auth_token_type o_auth_expiry
33 4 oauth_generic_oauth 2021-03-24 12:02:25 <o_auth_access_token> <o_auth_refresh_token> <o_auth_token_type> 2021-03-24 14:02:11
33 4 oauth_generic_oauth 2021-03-24 12:02:25 <o_auth_access_token> <o_auth_refresh_token> <o_auth_token_type> 2021-03-24 14:02:11
33 4 oauth_generic_oauth 2021-03-24 12:02:25 <o_auth_access_token> <o_auth_refresh_token> <o_auth_token_type> 2021-03-24 14:02:11
  • Checkout branch and run against it.
  • Try to login again.
  • You'll see:
id user_id auth_module auth_id created o_auth_access_token o_auth_refresh_token o_auth_token_type o_auth_expiry
33 4 oauth_generic_oauth 2021-03-24 12:22:25 <o_auth_access_token> <o_auth_refresh_token> <o_auth_token_type> 2021-03-24 14:22:11

@dsotirakis dsotirakis self-assigned this Mar 24, 2021
@dsotirakis dsotirakis added this to the 8.0.0 milestone Mar 24, 2021
@dsotirakis dsotirakis requested a review from marefr March 24, 2021 12:42
dsotirakis
dsotirakis commented Mar 29, 2021
View reviewed changes
pkg/services/sqlstore/user_auth.go Outdated
}
}
// create authInfo record to link accounts
} else if authQuery.Result == nil && query.AuthModule != "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to make this less ugly.

@dsotirakis dsotirakis force-pushed the generic-oauth-duplicates branch from 764faa6 to 18fce06 Compare March 29, 2021 12:34
@dsotirakis dsotirakis requested a review from marefr March 29, 2021 12:34
marefr
marefr suggested changes Mar 29, 2021
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Great work with the conversion of tests 🎉

I added some additional comments. Let me know if you don't understand and we can discuss it.

@dsotirakis dsotirakis requested a review from marefr March 30, 2021 09:33
marefr
marefr suggested changes Mar 30, 2021
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

See comment and let me know if I misunderstood

marefr
marefr suggested changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Great. Now I think it should work according to my idea. Not sure I'm correct though and that it works :) I will test it manually as well. Added a comment regarding a missing error check.

Would be great to add a test for generic_oauth in sqlstore/user_auth_test.go as well to validate the changes works as expected.

@dsotirakis dsotirakis requested a review from marefr April 1, 2021 08:36
@dsotirakis dsotirakis marked this pull request as ready for review April 1, 2021 08:36
@dsotirakis dsotirakis requested a review from a team as a code owner April 1, 2021 08:36
@dsotirakis dsotirakis requested review from wbrowne and removed request for a team April 1, 2021 08:36
@dsotirakis dsotirakis force-pushed the generic-oauth-duplicates branch from a38c2ad to 7cdfd72 Compare April 1, 2021 08:40
marefr
marefr suggested changes Apr 1, 2021
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Works as expected 👍

Verified no new rows in user_auth is created when signing in with existing generic_oauth user that exists in user_auth and that a new row is created for generic_oauth user when there are no rows in the user_auth table.

One thing left, I added a comment. Otherwise this should be good to merge I think since it prevents adding duplicate rows in the user_auth table. Great work

marefr
marefr approved these changes Apr 9, 2021
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM left a couple comments/suggestions.

After this been merged need to think about what's left in regard to duplicate rows

pkg/services/sqlstore/user_auth_test.go Outdated

// Expect to pass since there's a matching login user
getTime = func() time.Time { return time.Now().AddDate(0, 0, -2) }
query := &models.GetUserByAuthInfoQuery{Login: login, AuthModule: "oauth_generic_oauth", AuthId: ""}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put this in a const variable since used in 3 different places now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used with different Login field so we shall create a method / func for that - think it's not necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Was referring to the magic string oauth_generic_oauth. Sorry for being unclear

@dsotirakis dsotirakis enabled auto-merge (squash) April 9, 2021 10:39
@dsotirakis dsotirakis disabled auto-merge April 9, 2021 10:59
@dsotirakis dsotirakis enabled auto-merge (squash) April 9, 2021 11:08
@dsotirakis dsotirakis merged commit b867ced into master Apr 9, 2021
@dsotirakis dsotirakis deleted the generic-oauth-duplicates branch April 9, 2021 11:28
@marefr marefr mentioned this pull request Apr 9, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marefr marefr marefr approved these changes

@wbrowne wbrowne Awaiting requested review from wbrowne

Assignees

@dsotirakis dsotirakis

Labels
add to changelog area/auth/oauth area/backend type/bug
Projects
None yet
Milestone
8.0.0-beta1
Development

Successfully merging this pull request may close these issues.

Generic OAuth: Creating multiple user auth rows for the same user and provider
3 participants
@dsotirakis @marefr @grafanabot