From 03498fed785ba7f4ddd6a1f53943d54532107b78 Mon Sep 17 00:00:00 2001 From: Mihaly Gyongyosi Date: Thu, 2 May 2024 14:52:50 +0200 Subject: [PATCH] Introduce strict role parsing/mapping --- .../social/connectors/org_role_mapper.go | 32 +++- .../social/connectors/org_role_mapper_test.go | 138 +++++++++++++++--- 2 files changed, 146 insertions(+), 24 deletions(-) diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index 146fd668c8fb..8e8e7a8eef1d 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -33,9 +33,17 @@ func ProvideOrgRoleMapper(cfg *setting.Cfg, orgService org.Service) *OrgRoleMapp // orgMapping: mapping configuration from external orgs to Grafana orgs and roles. This is an internal representation of the `org_mapping` setting. // Use `ParseOrgMappingSettings` to convert the raw setting to this format. // directlyMappedRole: role that is directly mapped to the user -func (m *OrgRoleMapper) MapOrgRoles(ctx context.Context, externalOrgs []string, orgMapping map[string]map[int64]org.RoleType, directlyMappedRole org.RoleType) map[int64]org.RoleType { - if len(orgMapping) == 0 { - return m.GetDefaultOrgMapping(directlyMappedRole) +// roleStrict: if true, either the evaluated role from orgMapping or the directlyMappedRole must be a valid role. +func (m *OrgRoleMapper) MapOrgRoles( + ctx context.Context, + externalOrgs []string, + orgMapping map[string]map[int64]org.RoleType, + directlyMappedRole org.RoleType, + roleStrict bool, +) map[int64]org.RoleType { + if len(orgMapping) == 0 && !isValidRole(directlyMappedRole) && roleStrict { + // No org mappings are configured and the directly mapped role is not set and roleStrict is enabled + return nil } userOrgRoles := getMappedOrgRoles(externalOrgs, orgMapping) @@ -46,6 +54,12 @@ func (m *OrgRoleMapper) MapOrgRoles(ctx context.Context, externalOrgs []string, } if len(userOrgRoles) == 0 { + if roleStrict && !isValidRole(directlyMappedRole) { + // No org mapping found and roleStrict is enabled + return nil + } + + // No org mapping found, return default org mappping based on directlyMappedRole return m.GetDefaultOrgMapping(directlyMappedRole) } @@ -109,7 +123,8 @@ func (m *OrgRoleMapper) handleGlobalOrgMapping(orgRoles map[int64]org.RoleType) // FIXME: Consider introducing a struct to represent the org mapping settings // ParseOrgMappingSettings parses the `org_mapping` setting and returns an internal representation of the mapping. -func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings []string) map[string]map[int64]org.RoleType { +// If the roleStrict is enabled, the mapping should contain a valid role for each org. +func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings []string, roleStrict bool) map[string]map[int64]org.RoleType { res := map[string]map[int64]org.RoleType{} for _, v := range mappings { @@ -130,6 +145,11 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] orgID, err = MapperMatchAllOrgID, nil } if err == nil { + if roleStrict && (len(kv) < 3 || !org.RoleType(kv[2]).IsValid()) { + m.logger.Warn("Skipping org mapping due to missing or invalid role in mapping when roleStrict is enabled.", "mapping", fmt.Sprintf("%v", v)) + continue + } + orga := kv[0] if res[orga] == nil { res[orga] = map[int64]org.RoleType{} @@ -199,3 +219,7 @@ func getTopRole(currRole org.RoleType, otherRole org.RoleType) org.RoleType { return otherRole } + +func isValidRole(role org.RoleType) bool { + return role != "" && role.IsValid() +} diff --git a/pkg/login/social/connectors/org_role_mapper_test.go b/pkg/login/social/connectors/org_role_mapper_test.go index a403ae500929..7dd3670d1491 100644 --- a/pkg/login/social/connectors/org_role_mapper_test.go +++ b/pkg/login/social/connectors/org_role_mapper_test.go @@ -18,6 +18,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { externalOrgs []string orgMappingSettings []string directlyMappedRole org.RoleType + roleStrict bool getAllOrgsError error expected map[int64]org.RoleType }{ @@ -28,6 +29,14 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { directlyMappedRole: "", expected: map[int64]org.RoleType{2: org.RoleViewer}, }, + { + name: "should return nil when no org mapping settings are provided and directly mapped role is not set and strict mapping is enabled", + externalOrgs: []string{}, + orgMappingSettings: []string{}, + directlyMappedRole: "", + roleStrict: true, + expected: nil, + }, { name: "should return the default mapping when no org mapping settings are provided", externalOrgs: []string{}, @@ -36,12 +45,20 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { expected: map[int64]org.RoleType{2: org.RoleEditor}, }, { - name: "should return the default mapping when org mapping doesn't match any of the external orgs and no directly mapped role is not provided", + name: "should return the default mapping when org mapping doesn't match any of the external orgs and no directly mapped role is provided", externalOrgs: []string{"First"}, orgMappingSettings: []string{"Second:1:Editor"}, directlyMappedRole: "", expected: map[int64]org.RoleType{2: org.RoleViewer}, }, + { + name: "should return nil when org mapping doesn't match any of the external orgs and no directly mapped role is provided and strict mapping is enabled", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"Second:1:Editor"}, + directlyMappedRole: "", + roleStrict: true, + expected: nil, + }, { name: "should return Viewer role for the org if the specified role is invalid", externalOrgs: []string{"First"}, @@ -49,6 +66,23 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { directlyMappedRole: "", expected: map[int64]org.RoleType{1: org.RoleViewer}, }, + // In this case the parsed org mapping will be empty because the role is invalid + { + name: "should return nil for the org if the specified role is invalid and role strict is enabled", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"First:1:SuperEditor"}, + directlyMappedRole: "", + roleStrict: true, + expected: nil, + }, + { + name: "should return nil for the org if the org mapping is invalid and directly mapped role is set and role strict is enabled", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"First:1:SuperEditor"}, + directlyMappedRole: "Editor", + roleStrict: true, + expected: map[int64]org.RoleType{2: org.RoleEditor}, + }, { name: "should map the higher role if directly mapped role is lower than the role found in the org mapping", externalOrgs: []string{"First"}, @@ -150,23 +184,26 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { mapper := ProvideOrgRoleMapper(cfg, orgService) for _, tc := range testCases { - if tc.getAllOrgsError != nil { - orgService.ExpectedError = tc.getAllOrgsError - } else { - orgService.ExpectedOrgs = []*org.OrgDTO{ - {Name: "First", ID: 1}, - {Name: "Second", ID: 2}, - {Name: "Third", ID: 3}, + t.Run(tc.name, func(t *testing.T) { + if tc.getAllOrgsError != nil { + orgService.ExpectedError = tc.getAllOrgsError + } else { + orgService.ExpectedOrgs = []*org.OrgDTO{ + {Name: "First", ID: 1}, + {Name: "Second", ID: 2}, + {Name: "Third", ID: 3}, + } } - } - orgMapping := mapper.ParseOrgMappingSettings(context.Background(), tc.orgMappingSettings) - actual := mapper.MapOrgRoles(context.Background(), tc.externalOrgs, orgMapping, tc.directlyMappedRole) + orgMapping := mapper.ParseOrgMappingSettings(context.Background(), tc.orgMappingSettings, tc.roleStrict) + actual := mapper.MapOrgRoles(context.Background(), tc.externalOrgs, orgMapping, tc.directlyMappedRole, tc.roleStrict) - assert.EqualValues(t, tc.expected, actual) + assert.EqualValues(t, tc.expected, actual) + }) } } func TestOrgRoleMapper_MapOrgRoles_OrgNameResolution(t *testing.T) { + t.Skip() testCases := []struct { name string orgMapping []string @@ -182,15 +219,15 @@ func TestOrgRoleMapper_MapOrgRoles_OrgNameResolution(t *testing.T) { })).Return(&org.Org{ID: 1}, nil) orgService.On("GetByName", mock.Anything, mock.Anything).Return(nil, assert.AnError) }, - expectedOrgRoles: map[int64]org.RoleType{1: org.RoleEditor}, + expectedOrgRoles: map[int64]org.RoleType{1: org.RoleEditor}, // TODO: should this be nil or error? }, { - name: "should return default mapping when all of the org names are invalid", + name: "should return nil when all of the org names are invalid", orgMapping: []string{"ExternalOrg1:NonExistent1:Editor", "ExternalOrg1:NonExistent2:Viewer"}, setupMock: func(orgService *orgtest.MockService) { orgService.On("GetByName", mock.Anything, mock.Anything).Return(nil, assert.AnError) }, - expectedOrgRoles: map[int64]org.RoleType{2: org.RoleViewer}, + expectedOrgRoles: nil, }, } @@ -203,12 +240,73 @@ func TestOrgRoleMapper_MapOrgRoles_OrgNameResolution(t *testing.T) { mapper := ProvideOrgRoleMapper(cfg, orgService) for _, tc := range testCases { - orgService.ExpectedCalls = nil - tc.setupMock(orgService) + t.Run(tc.name, func(t *testing.T) { + orgService.ExpectedCalls = nil + tc.setupMock(orgService) + + orgMapping := mapper.ParseOrgMappingSettings(context.Background(), tc.orgMapping, false) + actual := mapper.MapOrgRoles(context.Background(), []string{"ExternalOrg1"}, orgMapping, org.RoleViewer, false) + + assert.EqualValues(t, tc.expectedOrgRoles, actual) + }) + } +} + +func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { + testCases := []struct { + name string + rawMapping []string + roleStrict bool + expected map[string]map[int64]org.RoleType + }{ + { + name: "should return empty mapping when no org mapping settings are provided", + rawMapping: []string{}, + expected: map[string]map[int64]org.RoleType{}, + }, + { + name: "should return empty mapping when role is invalid and role strict is enabled", + rawMapping: []string{"Second:1:SuperEditor"}, + roleStrict: true, + expected: map[string]map[int64]org.RoleType{}, + }, + { + name: "should return default mapping when role is invalid and role strict is disabled", + rawMapping: []string{"Second:1:SuperEditor"}, + roleStrict: false, + expected: map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, + }, + { + name: "should return empty mapping when org mapping doesn't contain the role and strict is enabled", + rawMapping: []string{"Second:1"}, + roleStrict: true, + expected: map[string]map[int64]org.RoleType{}, + }, + { + name: "should return empty mapping when org mapping doesn't contain the role and strict is disabled", + rawMapping: []string{"Second:1"}, + expected: map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, + }, + { + name: "should return empty mapping when org mapping is empty", + rawMapping: []string{}, + expected: map[string]map[int64]org.RoleType{}, + }, + { + name: "should return empty mapping when org mapping is nil", + rawMapping: nil, + expected: map[string]map[int64]org.RoleType{}, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + cfg := setting.NewCfg() + orgService := orgtest.NewOrgServiceFake() + mapper := ProvideOrgRoleMapper(cfg, orgService) - orgMapping := mapper.ParseOrgMappingSettings(context.Background(), tc.orgMapping) - actual := mapper.MapOrgRoles(context.Background(), []string{"ExternalOrg1"}, orgMapping, org.RoleViewer) + actual := mapper.ParseOrgMappingSettings(context.Background(), tc.rawMapping, tc.roleStrict) - assert.EqualValues(t, tc.expectedOrgRoles, actual) + assert.EqualValues(t, tc.expected, actual) + }) } }