-
Notifications
You must be signed in to change notification settings - Fork 33
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
add dynamic group mapping for AD provider #146
add dynamic group mapping for AD provider #146
Conversation
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.
@rshlin left some comments! otherwise, it looks good!
providers/active_directory.go
Outdated
@@ -243,9 +244,13 @@ func (s *ADProvider) getUserData(username string, password string) (goth.User, e | |||
if j.Name == s.config.LDAPLastNameAttribute { | |||
thisUser.LastName = j.Values[0] | |||
} | |||
if j.Name == s.config.LDAPGroupMembershipAttribute { |
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.
@rshlin what do you think if here, instead of adding a new config field (s.config.LDAPGroupMembershipAttribute
) we just use s.profile.CustomUserGroupField
?
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.
@rshlin we are thinking of implementing this using the suggestion made by @tbuchaillot -- Any objections? Thanks a lot for this!
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 had concerns on using s.profile.CustomUserGroupField
because this field had different semantics(see GetGroupId): providers collect relevant identity attributes into a single container and at the end SSOAccessData
is uniformly assembled. I wanted to leave these layers(i.e. adapter & core logic) low coupled and introduce a minimum amount of changes(preferably incremental) to make AD mapping work.
Anyway, required design decisions produce insignificant effects at this stage, and I agree with @tbuchaillot suggestion.
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.
lgtm
thanks a lot @rshlin for your contribution! We are going to notify you when this is released :) |
* add dynamic group mapping feature for AD provider * reuse s.profile.CustomUserGroupField config key for groups retrievement Co-authored-by: rsharifullin <rsharifullin@cinimex.ru>
Description
implemented dynamic group mapping for AD provider.
Related Issue
https://support.tyk.io/hc/requests/11700
https://tyktech.atlassian.net/browse/TT-2400
Motivation and Context
After deserializing ldap response with user info previous implementation of ADProvider was grabbing the first element from user attribute with type array and hence all valuable group membership information was cutted off.
How This Has Been Tested
Ran TIB as standalone, AD, dashboard. Used the following profile for test:
Screenshots (if appropriate)
Types of changes
Checklist
fork, don't request your
master
!master
branch (left side). Also, you should startyour branch off our latest
master
.go mod tidy && go mod vendor
go fmt -s
go vet