From b20a258b72d3130747118c6f3c535fc5d1b3a15f Mon Sep 17 00:00:00 2001 From: gotjosh Date: Thu, 19 Sep 2019 16:13:38 +0100 Subject: [PATCH] LDAP: Show non-matched groups returned from LDAP (#19208) * LDAP: Show all LDAP groups * Use the returned LDAP groups as the reference when debugging LDAP We need to use the LDAP groups returned as the main reference for assuming what we were able to match and what wasn't. Before, we were using the configured groups in LDAP TOML configuration file. * s/User name/Username * Add a title to for the LDAP mapping results * LDAP: UI Updates to debug view * LDAP: Make it explicit when we weren't able to match teams --- pkg/api/ldap_debug.go | 50 +++++++++++-------- pkg/api/ldap_debug_test.go | 17 ++++--- pkg/services/ldap/ldap.go | 4 +- pkg/services/ldap/ldap_private_test.go | 9 ++-- pkg/services/ldap/settings.go | 6 +-- public/app/features/admin/ldap/LdapPage.tsx | 6 +-- .../features/admin/ldap/LdapUserGroups.tsx | 38 ++++++++------ .../app/features/admin/ldap/LdapUserInfo.tsx | 15 +++++- .../app/features/admin/ldap/LdapUserTeams.tsx | 47 +++++++++-------- public/sass/layout/_page.scss | 2 +- 10 files changed, 115 insertions(+), 79 deletions(-) diff --git a/pkg/api/ldap_debug.go b/pkg/api/ldap_debug.go index b5f9d91bf4a..de87c128a89 100644 --- a/pkg/api/ldap_debug.go +++ b/pkg/api/ldap_debug.go @@ -34,7 +34,7 @@ type LDAPAttribute struct { } // RoleDTO is a serializer for mapped roles from LDAP -type RoleDTO struct { +type LDAPRoleDTO struct { OrgId int64 `json:"orgId"` OrgName string `json:"orgName"` OrgRole models.RoleType `json:"orgRole"` @@ -49,7 +49,7 @@ type LDAPUserDTO struct { Username *LDAPAttribute `json:"login"` IsGrafanaAdmin *bool `json:"isGrafanaAdmin"` IsDisabled bool `json:"isDisabled"` - OrgRoles []RoleDTO `json:"roles"` + OrgRoles []LDAPRoleDTO `json:"roles"` Teams []models.TeamOrgGroupDTO `json:"teams"` } @@ -90,6 +90,10 @@ func (user *LDAPUserDTO) FetchOrgs() error { } for i, orgDTO := range user.OrgRoles { + if orgDTO.OrgId < 1 { + continue + } + orgName := orgNamesById[orgDTO.OrgId] if orgName != "" { @@ -256,7 +260,7 @@ func (server *HTTPServer) GetUserFromLDAP(c *models.ReqContext) Response { user, serverConfig, err := ldap.User(username) if user == nil { - return Error(http.StatusNotFound, "No user was found on the LDAP server(s)", err) + return Error(http.StatusNotFound, "No user was found in the LDAP server(s) with that username", err) } logger.Debug("user found", "user", user) @@ -272,22 +276,32 @@ func (server *HTTPServer) GetUserFromLDAP(c *models.ReqContext) Response { IsDisabled: user.IsDisabled, } - orgRoles := []RoleDTO{} + orgRoles := []LDAPRoleDTO{} - for _, g := range serverConfig.Groups { - role := &RoleDTO{} + // First, let's find the groupDN that we did match by inspecting the assigned user OrgRoles. + for _, group := range serverConfig.Groups { + orgRole, ok := user.OrgRoles[group.OrgId] - if isMatchToLDAPGroup(user, g) { - role.OrgId = g.OrgID - role.OrgRole = user.OrgRoles[g.OrgID] - role.GroupDN = g.GroupDN + if ok && orgRole == group.OrgRole { + r := &LDAPRoleDTO{GroupDN: group.GroupDN, OrgId: group.OrgId, OrgRole: group.OrgRole} + orgRoles = append(orgRoles, *r) + } + } - orgRoles = append(orgRoles, *role) - } else { - role.OrgId = g.OrgID - role.GroupDN = g.GroupDN + // Then, we find what we did not match by inspecting the list of groups returned from + // LDAP against what we have already matched above. + for _, userGroup := range user.Groups { + var matches int - orgRoles = append(orgRoles, *role) + for _, orgRole := range orgRoles { + if orgRole.GroupDN == userGroup { // we already matched it + matches++ + } + } + + if matches < 1 { + r := &LDAPRoleDTO{GroupDN: userGroup} + orgRoles = append(orgRoles, *r) } } @@ -312,12 +326,6 @@ func (server *HTTPServer) GetUserFromLDAP(c *models.ReqContext) Response { return JSON(200, u) } -// isMatchToLDAPGroup determines if we were able to match an LDAP group to an organization+role. -// Since we allow one role per organization. If it's set, we were able to match it. -func isMatchToLDAPGroup(user *models.ExternalUserInfo, groupConfig *ldap.GroupToOrgRole) bool { - return user.OrgRoles[groupConfig.OrgID] == groupConfig.OrgRole -} - // splitName receives the full name of a user and splits it into two parts: A name and a surname. func splitName(name string) (string, string) { names := util.SplitString(name) diff --git a/pkg/api/ldap_debug_test.go b/pkg/api/ldap_debug_test.go index 938c3b1b228..b5815503f61 100644 --- a/pkg/api/ldap_debug_test.go +++ b/pkg/api/ldap_debug_test.go @@ -94,7 +94,7 @@ func TestGetUserFromLDAPApiEndpoint_UserNotFound(t *testing.T) { sc := getUserFromLDAPContext(t, "/api/admin/ldap/user-that-does-not-exist") require.Equal(t, sc.resp.Code, http.StatusNotFound) - assert.JSONEq(t, "{\"message\":\"No user was found on the LDAP server(s)\"}", sc.resp.Body.String()) + assert.JSONEq(t, "{\"message\":\"No user was found in the LDAP server(s) with that username\"}", sc.resp.Body.String()) } func TestGetUserFromLDAPApiEndpoint_OrgNotfound(t *testing.T) { @@ -103,6 +103,7 @@ func TestGetUserFromLDAPApiEndpoint_OrgNotfound(t *testing.T) { Name: "John Doe", Email: "john.doe@example.com", Login: "johndoe", + Groups: []string{"cn=admins,ou=groups,dc=grafana,dc=org"}, OrgRoles: map[int64]models.RoleType{1: models.ROLE_ADMIN, 2: models.ROLE_VIEWER}, IsGrafanaAdmin: &isAdmin, } @@ -117,12 +118,12 @@ func TestGetUserFromLDAPApiEndpoint_OrgNotfound(t *testing.T) { Groups: []*ldap.GroupToOrgRole{ { GroupDN: "cn=admins,ou=groups,dc=grafana,dc=org", - OrgID: 1, + OrgId: 1, OrgRole: models.ROLE_ADMIN, }, { GroupDN: "cn=admins,ou=groups,dc=grafana2,dc=org", - OrgID: 2, + OrgId: 2, OrgRole: models.ROLE_VIEWER, }, }, @@ -164,6 +165,7 @@ func TestGetUserFromLDAPApiEndpoint(t *testing.T) { Name: "John Doe", Email: "john.doe@example.com", Login: "johndoe", + Groups: []string{"cn=admins,ou=groups,dc=grafana,dc=org", "another-group-not-matched"}, OrgRoles: map[int64]models.RoleType{1: models.ROLE_ADMIN}, IsGrafanaAdmin: &isAdmin, } @@ -178,7 +180,7 @@ func TestGetUserFromLDAPApiEndpoint(t *testing.T) { Groups: []*ldap.GroupToOrgRole{ { GroupDN: "cn=admins,ou=groups,dc=grafana,dc=org", - OrgID: 1, + OrgId: 1, OrgRole: models.ROLE_ADMIN, }, }, @@ -203,7 +205,7 @@ func TestGetUserFromLDAPApiEndpoint(t *testing.T) { sc := getUserFromLDAPContext(t, "/api/admin/ldap/johndoe") - require.Equal(t, sc.resp.Code, http.StatusOK) + assert.Equal(t, sc.resp.Code, http.StatusOK) expected := ` { @@ -222,7 +224,8 @@ func TestGetUserFromLDAPApiEndpoint(t *testing.T) { "isGrafanaAdmin": true, "isDisabled": false, "roles": [ - { "orgId": 1, "orgRole": "Admin", "orgName": "Main Org.", "groupDN": "cn=admins,ou=groups,dc=grafana,dc=org" } + { "orgId": 1, "orgRole": "Admin", "orgName": "Main Org.", "groupDN": "cn=admins,ou=groups,dc=grafana,dc=org" }, + { "orgId": 0, "orgRole": "", "orgName": "", "groupDN": "another-group-not-matched" } ], "teams": null } @@ -251,7 +254,7 @@ func TestGetUserFromLDAPApiEndpoint_WithTeamHandler(t *testing.T) { Groups: []*ldap.GroupToOrgRole{ { GroupDN: "cn=admins,ou=groups,dc=grafana,dc=org", - OrgID: 1, + OrgId: 1, OrgRole: models.ROLE_ADMIN, }, }, diff --git a/pkg/services/ldap/ldap.go b/pkg/services/ldap/ldap.go index d079d632f25..a6718ce69d9 100644 --- a/pkg/services/ldap/ldap.go +++ b/pkg/services/ldap/ldap.go @@ -408,12 +408,12 @@ func (server *Server) buildGrafanaUser(user *ldap.Entry) (*models.ExternalUserIn for _, group := range server.Config.Groups { // only use the first match for each org - if extUser.OrgRoles[group.OrgID] != "" { + if extUser.OrgRoles[group.OrgId] != "" { continue } if isMemberOf(memberOf, group.GroupDN) { - extUser.OrgRoles[group.OrgID] = group.OrgRole + extUser.OrgRoles[group.OrgId] = group.OrgRole if extUser.IsGrafanaAdmin == nil || !*extUser.IsGrafanaAdmin { extUser.IsGrafanaAdmin = group.IsGrafanaAdmin } diff --git a/pkg/services/ldap/ldap_private_test.go b/pkg/services/ldap/ldap_private_test.go index ff8bc430ed9..0382cdc5987 100644 --- a/pkg/services/ldap/ldap_private_test.go +++ b/pkg/services/ldap/ldap_private_test.go @@ -3,11 +3,10 @@ package ldap import ( "testing" - . "github.com/smartystreets/goconvey/convey" - "gopkg.in/ldap.v3" - "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" + . "github.com/smartystreets/goconvey/convey" + "gopkg.in/ldap.v3" ) func TestLDAPPrivateMethods(t *testing.T) { @@ -124,7 +123,7 @@ func TestLDAPPrivateMethods(t *testing.T) { Config: &ServerConfig{ Groups: []*GroupToOrgRole{ { - OrgID: 1, + OrgId: 1, }, }, }, @@ -162,7 +161,7 @@ func TestLDAPPrivateMethods(t *testing.T) { Config: &ServerConfig{ Groups: []*GroupToOrgRole{ { - OrgID: 1, + OrgId: 1, }, }, }, diff --git a/pkg/services/ldap/settings.go b/pkg/services/ldap/settings.go index beba4c76c19..720180716f6 100644 --- a/pkg/services/ldap/settings.go +++ b/pkg/services/ldap/settings.go @@ -55,7 +55,7 @@ type AttributeMap struct { // config "group_mappings" setting type GroupToOrgRole struct { GroupDN string `toml:"group_dn"` - OrgID int64 `toml:"org_id"` + OrgId int64 `toml:"org_id"` // This pointer specifies if setting was set (for backwards compatibility) IsGrafanaAdmin *bool `toml:"grafana_admin"` @@ -139,8 +139,8 @@ func readConfig(configFile string) (*Config, error) { } for _, groupMap := range server.Groups { - if groupMap.OrgID == 0 { - groupMap.OrgID = 1 + if groupMap.OrgId == 0 { + groupMap.OrgId = 1 } } } diff --git a/public/app/features/admin/ldap/LdapPage.tsx b/public/app/features/admin/ldap/LdapPage.tsx index 57b865badc3..9f65470ff74 100644 --- a/public/app/features/admin/ldap/LdapPage.tsx +++ b/public/app/features/admin/ldap/LdapPage.tsx @@ -89,12 +89,12 @@ export class LdapPage extends PureComponent { {config.buildInfo.isEnterprise && ldapSyncInfo && } -

User mapping

+

Test user mapping

- +
diff --git a/public/app/features/admin/ldap/LdapUserGroups.tsx b/public/app/features/admin/ldap/LdapUserGroups.tsx index d38fbb956cc..a21cd4da876 100644 --- a/public/app/features/admin/ldap/LdapUserGroups.tsx +++ b/public/app/features/admin/ldap/LdapUserGroups.tsx @@ -9,7 +9,6 @@ interface Props { export const LdapUserGroups: FC = ({ groups, showAttributeMapping }) => { const items = showAttributeMapping ? groups : groups.filter(item => item.orgRole); - const roleColumnClass = showAttributeMapping && 'width-14'; return (
@@ -17,32 +16,39 @@ export const LdapUserGroups: FC = ({ groups, showAttributeMapping }) => { + {showAttributeMapping && } - {showAttributeMapping && } {items.map((group, index) => { return ( - - {showAttributeMapping && ( <> - + {!group.orgRole && ( + <> + + + )} + + )} + {group.orgName && ( + <> + + )} diff --git a/public/app/features/admin/ldap/LdapUserInfo.tsx b/public/app/features/admin/ldap/LdapUserInfo.tsx index 3aed200c4df..c1c3298ccb1 100644 --- a/public/app/features/admin/ldap/LdapUserInfo.tsx +++ b/public/app/features/admin/ldap/LdapUserInfo.tsx @@ -18,8 +18,21 @@ export const LdapUserInfo: FC = ({ ldapUser, showAttributeMapping }) => { {ldapUser.roles && ldapUser.roles.length > 0 && ( )} - {ldapUser.teams && ldapUser.teams.length > 0 && ( + + {ldapUser.teams && ldapUser.teams.length > 0 ? ( + ) : ( +
+
+
LDAP GroupOrganisation RoleLDAP Group
{group.orgName}{group.orgRole}{group.groupDN} - {!group.orgRole && ( - - No match - -
- -
- - - )} - 		+ + + No match + + + + + + + {group.orgName}{group.orgRole}
+ + + + + +
No teams found via LDAP
+
+ )} ); diff --git a/public/app/features/admin/ldap/LdapUserTeams.tsx b/public/app/features/admin/ldap/LdapUserTeams.tsx index 30ac81b5231..e8a87d2bfaa 100644 --- a/public/app/features/admin/ldap/LdapUserTeams.tsx +++ b/public/app/features/admin/ldap/LdapUserTeams.tsx @@ -1,5 +1,4 @@ import React, { FC } from 'react'; -import { css } from 'emotion'; import { Tooltip } from '@grafana/ui'; import { LdapTeam } from 'app/types'; @@ -10,10 +9,6 @@ interface Props { export const LdapUserTeams: FC = ({ teams, showAttributeMapping }) => { const items = showAttributeMapping ? teams : teams.filter(item => item.teamName); - const teamColumnClass = showAttributeMapping && 'width-14'; - const noMatchPlaceholderStyle = css` - display: flex; - `; return (
@@ -21,29 +16,41 @@ export const LdapUserTeams: FC = ({ teams, showAttributeMapping }) => { + {showAttributeMapping && } - {showAttributeMapping && } {items.map((team, index) => { return ( - - - {showAttributeMapping && } + {showAttributeMapping && ( + <> + + {!team.orgName && ( + <> + + + )} + + )} + {team.orgName && ( + <> + + + + )} ); })} diff --git a/public/sass/layout/_page.scss b/public/sass/layout/_page.scss index 6b2fa95730e..66b6c47356a 100644 --- a/public/sass/layout/_page.scss +++ b/public/sass/layout/_page.scss @@ -99,7 +99,7 @@ .page-heading { font-size: $font-size-h4; margin-top: 0; - margin-bottom: $spacer * 0.7; + margin-bottom: $spacer; } .page-action-bar {
LDAP GroupOrganisation TeamLDAP
- {team.orgName || ( -
- No match - -
- -
- -
- )} - 		{team.teamName}{team.groupDN}{team.groupDN} + +
+ No match + + + + + +
+ 		{team.orgName}{team.teamName}