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.

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: No longer shows incorrectly matching groups based on role in debug page #20018

Merged

Conversation

xlson
Copy link
Contributor

@xlson xlson commented Oct 25, 2019

What this PR does / why we need it:

No longer show all groups that gives a specific Org Role for a user that gets that role from one of them.

Which issue(s) this PR fixes:

Fixes #20016

Special notes for your reviewer:

Org Role was used as a shortcut to figure out what groups were matching
and which weren't. That lead to too all groups matching a specific role
to show up for a user if that user got that role.
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

I think we can simplify the code a bit by combining the to for loops:

So instead of looping over config groups then user groups (nested), and then user groups again. Should be the same as below:

	orgRoles := []LDAPRoleDTO{}

	for _, userGroup := range user.Groups {
		matches := false

		for _, configGroup := range serverConfig.Groups {
			if configGroup.GroupDN == userGroup {
				r := &LDAPRoleDTO{GroupDN: configGroup.GroupDN, OrgId: configGroup.OrgId, OrgRole: configGroup.OrgRole}
				orgRoles = append(orgRoles, *r)
				matches = true
				break
			}
		}

		if !matches {
			r := &LDAPRoleDTO{GroupDN: userGroup}
			orgRoles = append(orgRoles, *r)
		}
	}

The order of groups in the ldap.toml file is important, only the first
match for an organisation will be used. This means we have to iterate
based on the config and stop matching when a match is found.

We might want to think about showing further matches as potential
matches that are shadowed by the first match. That would possibly make
it easier to understand why one match is used instead of another one.
@xlson
Copy link
Contributor Author

xlson commented Oct 27, 2019

@torkelo As we match groups based on the order of configs in the toml file I don't think we can do it quite this cleanly (thanks for pointing this out @gotjosh). It seems we need to match based on the toml config as only the first matching role for an org will be used. (We could consider choosing to show the others as matching as well if we have a way to signify that they are being shadowed by the first match). If we choose to iterate based on the user's groups we wouldn't know what the first match was.

I've rewritten the code in a way that seems to match what the real LDAP mapping code is doing. It doesn't look great though as it ends up with a for -> if -> for cycle. I'll see if it's possible to clean that up a bit tomorrow. I am wondering if it would be possible to expose more meta-data about the mappings from the LDAP code rather than re-creating the logic here.

@torkelo
Copy link
Member

torkelo commented Oct 28, 2019

Regarding the fact that only the first match is used (per org). Not sure what is best here. Not showing "Match" and listing them as "No Match" feels wrong as well as it might give the impression that the group is specified wrongly in the ldap toml. As long as we list them in the same way as they mappings are specified in the toml file so that the used match & role is first then maybe that is best?

We should probably change so the order or mappings is not used and that the highest role/permission instead wins. (But not in this PR).

@xlson
Copy link
Contributor Author

xlson commented Oct 28, 2019

I agree, showing as No Match is very confusing. Yeah, that might be for the best. We could add an info icon with an explain that only the first match is used.

I agree. While the it is working now makes it possible to create more creative configuration, I think it's likely that we're paying too much for that power by making our code more complicated and the LDAP config harder to understand.

@gotjosh gotjosh self-requested a review October 31, 2019 09:38
@xlson xlson requested a review from torkelo October 31, 2019 14:45
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Looks good

public/app/features/admin/ldap/LdapUserGroups.tsx Outdated Show resolved Hide resolved
public/app/features/admin/ldap/LdapUserGroups.tsx Outdated Show resolved Hide resolved
xlson and others added 2 commits November 1, 2019 14:54
Co-Authored-By: gotjosh <josue.abreu@gmail.com>
Co-Authored-By: gotjosh <josue.abreu@gmail.com>
@xlson xlson merged commit 730bedf into grafana:master Nov 1, 2019
@xlson xlson deleted the ldap-debug-fix-incorrectly-matched-groups branch November 1, 2019 14:42
papagian pushed a commit that referenced this pull request Nov 6, 2019
…#20018)

* LDAP Debug: No longer shows incorrectly matching groups based on role

Org Role was used as a shortcut to figure out what groups were matching
and which weren't. That lead to too all groups matching a specific role
to show up for a user if that user got that role.

* LDAP Debug: Fixes ordering of matches

The order of groups in the ldap.toml file is important, only the first
match for an organisation will be used. This means we have to iterate
based on the config and stop matching when a match is found.

We might want to think about showing further matches as potential
matches that are shadowed by the first match. That would possibly make
it easier to understand why one match is used instead of another one.

* LDAP Debug: never display more than one match for the same LDAP group/mapping.

* LDAP Debug: show all matches, even if they aren't used

* Update public/app/features/admin/ldap/LdapUserGroups.tsx

Co-Authored-By: gotjosh <josue.abreu@gmail.com>

* Update public/app/features/admin/ldap/LdapUserGroups.tsx

Co-Authored-By: gotjosh <josue.abreu@gmail.com>
(cherry picked from commit 730bedf)
@marefr marefr added the type/bug label Nov 6, 2019
papagian added a commit that referenced this pull request Nov 6, 2019
* SingleStat: apply mappings to no data response (#19951)

(cherry picked from commit 8232659)

* DataLinks: Fix blur issues (#19883)


(cherry picked from commit a1e8157)

* Docker: makes it possible to parse timezones in the docker image (#20081)


(cherry picked from commit e940edc)

* Backport: Bump crewjam/saml to the latest master

Ref: #20126

* LDAP Debug: No longer shows incorrectly matching groups based on role (#20018)

* LDAP Debug: No longer shows incorrectly matching groups based on role

Org Role was used as a shortcut to figure out what groups were matching
and which weren't. That lead to too all groups matching a specific role
to show up for a user if that user got that role.

* LDAP Debug: Fixes ordering of matches

The order of groups in the ldap.toml file is important, only the first
match for an organisation will be used. This means we have to iterate
based on the config and stop matching when a match is found.

We might want to think about showing further matches as potential
matches that are shadowed by the first match. That would possibly make
it easier to understand why one match is used instead of another one.

* LDAP Debug: never display more than one match for the same LDAP group/mapping.

* LDAP Debug: show all matches, even if they aren't used

* Update public/app/features/admin/ldap/LdapUserGroups.tsx

Co-Authored-By: gotjosh <josue.abreu@gmail.com>

* Update public/app/features/admin/ldap/LdapUserGroups.tsx

Co-Authored-By: gotjosh <josue.abreu@gmail.com>
(cherry picked from commit 730bedf)

* LDAP: All LDAP servers should be tried even if one of them returns a connection error (#20077)

All ldap servers are now being tried and the first one that gives back an answer is used if a previous one is failing. Applies to login and syncing

(cherry picked from commit cce5557)

* mysql: fix encoding in connection string (#20192)


(cherry picked from commit 19dbd27)

* release 6.4.4

* Settings: fix deprecation error
@marefr marefr changed the title LDAP Debug: No longer shows incorrectly matching groups based on role LDAP: No longer shows incorrectly matching groups based on role in debug view Nov 6, 2019
@marefr marefr changed the title LDAP: No longer shows incorrectly matching groups based on role in debug view LDAP: No longer shows incorrectly matching groups based on role in debug page Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LDAP Debug page incorrectly lists matching groups based on role
5 participants