Add groups scope and LDAP implementation #510
Conversation
| @@ -65,5 +69,9 @@ func (s *Session) Claims(issuerURL string) jose.Claims { | |||
| if s.Nonce != "" { | |||
| claims["nonce"] = s.Nonce | |||
| } | |||
| log.Println(s.Scope.HasScope(scope.ScopeGroups), s.Groups) | |||
There was a problem hiding this comment.
woops, need to remove this
|
Oh, and cc @johnw188 per our conversation from #sig-auth. |
| @@ -447,8 +484,10 @@ func (c *LDAPConnector) Identity(username, password string) (*oidc.Identity, err | |||
| if c.searchBeforeAuth { | |||
| err = c.ldapPool.Do(func(conn *ldap.Conn) error { | |||
| if err := conn.Bind(c.searchBindDN, c.searchBindPw); err != nil { | |||
| // Don't wrap error as it may be a specific LDAP error. | |||
| return err | |||
| if !invalidBindCredentials(err) { | |||
There was a problem hiding this comment.
Why remove all these calls to invalidBindCredentials()?
There was a problem hiding this comment.
Remove? I'm confused, those calls aren't being removed by this PR.
There was a problem hiding this comment.
Oh sorry, still early, eyes are playing tricks on me.
|
As per in-person conversation with @chancez we might want to have some way for a connector to pass data between logging the user in and getting the groups. An obvious use case is access tokens. E.g. I login to GitHub, and need the access token from the login to get my groups. Going to defer that point to a different PR, as it involves refactoring the oidc.LoginFunc. |
|
👍 This looks fairly solid to me overall. |
|
Going to squash and merge unless there are objections. |
ericchiang
force-pushed
the
add-groups-scope-and-ldap-implementation
branch
from
4a9f462 to
607d992
Compare
|
Is it possible to restructure this so that group searches can be performed with the user's bind as opposed to requiring a service account? It would simplify deployment of this if we could avoid setting up a service account just for Dex. (tacking this on to this issue for context; can open a separate issue if desired) |
|
@wyattanderson not currently. For refresh tokens, we insist on updating the groups information in the returned ID Token, to prevent stale authentication data. When a client tries to refresh an ID Token, we don't have the users credentials at that point. This is a problem for some possible groups implementations (see #514), but is especially problematic for LDAP since we'd have to hold onto the user's plain text password. |
|
Oh yeah, totally forgot about the refresh token situation which makes perfect sense. Thanks! Also, thanks in general for this project. It's saving me months of work. |
|
Glad to hear! |
This commit adds the new "groups" scope and an LDAP implementation. Tests are largely updated to accomidate changes to function signatures. Need to add some tests that directly try the groups functionality.
cc @sym3tri @chancez
Closes #432
Updates #480