-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
LDAP: Fix LDAP users authenticated via auth proxy not being able to use LDAP active sync #83715
Conversation
This PR must be merged before a backport PR will be created. |
2 similar comments
This PR must be merged before a backport PR will be created. |
This PR must be merged before a backport PR will be created. |
pkg/services/authn/clients/proxy.go
Outdated
// Get the authentication information for the user | ||
authedBy, err := c.authInfoService.GetAuthInfo(ctx, &login.GetAuthInfoQuery{UserId: usr.UserID}) | ||
if err != nil || authedBy == nil { | ||
c.log.FromContext(ctx).Warn("Cached user had no valid auth info", "error", err, "userId", string(entry)) | ||
} else { | ||
c.log.FromContext(ctx).Debug("User was loaded from cache, skip syncs", "userId", usr.UserID) | ||
return authn.IdentityFromSignedInUser(authn.NamespacedID(authn.NamespaceUser, usr.UserID), usr, authn.ClientParams{SyncPermissions: true}, authedBy.AuthModule), 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.
Do we really need this part. We are not doing any syncing.
If we want to always display the correct authenticated by, even when using session, maybe this should be part of user fetch hook instead. Otherwise only auth proxy would always display this info correctly
Then we should probably use that hook to fetch all user info instead of doing it manually here?
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.
👍 let me see how that looks like
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.
Looks good just a question / suggestion before we merge this
… able to use LDAP active sync (#83751) LDAP: Fix LDAP users authenticated via auth proxy not being able to use LDAP active sync (#83715) * fix LDAP users authenticated via auth proxy not being able to use ldap sync * simplify id resolution at the cost of no fallthrough * remove unused services * remove unused cache key (cherry picked from commit 2182cc4) Co-authored-by: Jo <joao.guerreiro@grafana.com>
… able to use LDAP active sync (#83750) LDAP: Fix LDAP users authenticated via auth proxy not being able to use LDAP active sync (#83715) * fix LDAP users authenticated via auth proxy not being able to use ldap sync * simplify id resolution at the cost of no fallthrough * remove unused services * remove unused cache key (cherry picked from commit 2182cc4) Co-authored-by: Jo <joao.guerreiro@grafana.com>
What is this feature?
Users authenticated with auth proxy via the LDAP client were being stored as auth_proxy (grafana db) users instead of ldap users.
The changes to the auth client should gradually fix this users to have an LDAP entry instead.
(Possible research avenue into changing behavior of search to ignore the latest login method used)
grafana/pkg/services/user/userimpl/store.go
Line 632 in 9c9e5e6
Why do we need this feature?
[Add a description of the problem the feature is trying to solve.]
Who is this feature for?
[Add information on what kind of user the feature is for.]
Which issue(s) does this PR fix?:
Fixes #
Special notes for your reviewer:
Please check that: