-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Auth: Fix orgrole picker disabled if isSynced user #64033
Auth: Fix orgrole picker disabled if isSynced user #64033
Conversation
pkg/api/org_users.go
Outdated
if err != nil { | ||
if errors.Is(err, user.ErrUserNotFound) { | ||
hs.log.Warn("Failed to get user auth info for basic auth user", cmd.UserID, nil) | ||
} else { | ||
hs.log.Error("Failed to get user auth info for external sync check", cmd.UserID, err) | ||
return response.Error(http.StatusInternalServerError, "Failed to get user auth info", nil) | ||
} | ||
} | ||
if qAuth.Result != nil && qAuth.Result.AuthModule != "" && login.IsExternallySynced(hs.Cfg, qAuth.Result.AuthModule) { |
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.
we need to check for Auth.Results == nil for a basic Auth user. here we look at the error if the userNotFound for a basic Auth user.
desc: "should not be able to change basicRole with a different provider", | ||
SkipOrgRoleSync: false, | ||
AuthEnabled: true, | ||
AuthModule: login.GenericOAuthModule, | ||
expectedCode: http.StatusForbidden, | ||
}, | ||
{ | ||
desc: "should be able to change basicRole with a basic Auth", | ||
SkipOrgRoleSync: false, | ||
AuthEnabled: false, | ||
AuthModule: "", | ||
expectedCode: http.StatusOK, | ||
}, | ||
{ | ||
desc: "should be able to change basicRole with a basic Auth", | ||
SkipOrgRoleSync: true, | ||
AuthEnabled: true, | ||
AuthModule: "", | ||
expectedCode: http.StatusOK, | ||
}, | ||
} |
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.
added tests for basic Auth as well
@@ -67,7 +67,7 @@ func (hs *HTTPServer) getUserUserProfile(c *contextmodel.ReqContext, userID int6 | |||
authLabel := login.GetAuthProviderLabel(getAuthQuery.Result.AuthModule) | |||
userProfile.AuthLabels = append(userProfile.AuthLabels, authLabel) | |||
userProfile.IsExternal = true | |||
userProfile.IsExternallySynced = login.IsExternallySynced(hs.Cfg, authLabel) | |||
userProfile.IsExternallySynced = login.IsExternallySynced(hs.Cfg, getAuthQuery.Result.AuthModule) |
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.
using AuthModule instead of Labels
@@ -150,7 +150,7 @@ const UserListAdminPageUnConnected = ({ | |||
<Icon name="question-circle" /> | |||
</Tooltip> | |||
</th> | |||
<th style={{ width: '1%' }}>Synced from</th> | |||
<th style={{ width: '1%' }}>Origin</th> |
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.
changed to Origin instead, wdyt?
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've just tested it again, and everything works great!
Please lower the level of the log before merging (and maybe fix the way the error is used). But apart from that this looks good 👌
pkg/api/org_users.go
Outdated
err := hs.authInfoService.GetAuthInfo(c.Req.Context(), &qAuth) | ||
if err != nil { | ||
if errors.Is(err, user.ErrUserNotFound) { | ||
hs.log.Warn("Failed to get user auth info for basic auth user", cmd.UserID, nil) |
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 would put it at debug level - it is currently expected that a basic auth user won't have an auth info entry.
pkg/api/org_users.go
Outdated
} | ||
} | ||
if qAuth.Result != nil && qAuth.Result.AuthModule != "" && login.IsExternallySynced(hs.Cfg, qAuth.Result.AuthModule) { | ||
return response.ErrOrFallback(http.StatusForbidden, "Cannot change role for externally synced user", org.ErrCannotChangeRoleForExternallySyncedUser) |
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 think the intended use of errutil
errors is that they are propagated down from other methods that are called from handlers. There's not much benefit of returning it directly from the handler I think. But if you want to do it, you'll need something like this:
return response.ErrOrFallback(http.StatusForbidden, "Cannot change role for externally synced user", org.ErrCannotChangeRoleForExternallySyncedUser) | |
return response.Err(org.ErrCannotChangeRoleForExternallySyncedUser.Errorf("Cannot change role for externally synced user")) |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-64033-to-v9.4.x origin/v9.4.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 3cd952b8bad8d23daee10fcc303a6d19c8b6d0a0
# Push it to GitHub
git push --set-upstream origin backport-64033-to-v9.4.x
git switch main
# Remove the local backport branch
git branch -D backport-64033-to-v9.4.x Then, create a pull request where the |
* fix: disable orgrolepicker if externaluser is synced * add disable to role picker * just took me 2 hours to center the icon * wip * fix: check externallySyncedUser for API call * remove check from store * add: tests * refactor authproxy and made tests run * add: feature toggle * set feature toggle for tests * add: IsProviderEnabled * refactor: featuretoggle name * IsProviderEnabled tests * add specific tests for isProviderEnabled * fix: org_user tests * add: owner to featuretoggle * add missing authlabels * remove fmt * feature toggle * change config * add test for a different authmodule * test refactor * gen feature toggle again * fix basic auth user able to change the org role * test for basic auth role * make err.base to error * lowered lvl of log and input mesg (cherry picked from commit 3cd952b)
* fix: disable orgrolepicker if externaluser is synced * add disable to role picker * just took me 2 hours to center the icon * wip * fix: check externallySyncedUser for API call * remove check from store * add: tests * refactor authproxy and made tests run * add: feature toggle * set feature toggle for tests * add: IsProviderEnabled * refactor: featuretoggle name * IsProviderEnabled tests * add specific tests for isProviderEnabled * fix: org_user tests * add: owner to featuretoggle * add missing authlabels * remove fmt * feature toggle * change config * add test for a different authmodule * test refactor * gen feature toggle again * fix basic auth user able to change the org role * test for basic auth role * make err.base to error * lowered lvl of log and input mesg (cherry picked from commit 3cd952b)
* fix: disable orgrolepicker if externaluser is synced * add disable to role picker * just took me 2 hours to center the icon * wip * fix: check externallySyncedUser for API call * remove check from store * add: tests * refactor authproxy and made tests run * add: feature toggle * set feature toggle for tests * add: IsProviderEnabled * refactor: featuretoggle name * IsProviderEnabled tests * add specific tests for isProviderEnabled * fix: org_user tests * add: owner to featuretoggle * add missing authlabels * remove fmt * feature toggle * change config * add test for a different authmodule * test refactor * gen feature toggle again * fix basic auth user able to change the org role * test for basic auth role * make err.base to error * lowered lvl of log and input mesg (cherry picked from commit 3cd952b)
Auth: Fix orgrole picker disabled if isSynced user (#64033) * fix: disable orgrolepicker if externaluser is synced * add disable to role picker * just took me 2 hours to center the icon * wip * fix: check externallySyncedUser for API call * remove check from store * add: tests * refactor authproxy and made tests run * add: feature toggle * set feature toggle for tests * add: IsProviderEnabled * refactor: featuretoggle name * IsProviderEnabled tests * add specific tests for isProviderEnabled * fix: org_user tests * add: owner to featuretoggle * add missing authlabels * remove fmt * feature toggle * change config * add test for a different authmodule * test refactor * gen feature toggle again * fix basic auth user able to change the org role * test for basic auth role * make err.base to error * lowered lvl of log and input mesg (cherry picked from commit 3cd952b)
@eleijonmarck Should this be backported to |
What is this feature?
TODO:
Fixes #
#63835
Special notes for your reviewer:
Goes together w. enterprise - https://github.com/grafana/grafana-enterprise/pull/4774
oss test
![image](https://user-images.githubusercontent.com/7727602/222470155-abb892ed-6d2b-4efd-ae35-8c945107763a.png)
enterprise test
![image](https://user-images.githubusercontent.com/7727602/222691038-442e7a58-62dc-41ad-82c3-93b7c96db733.png)