From d9595f21333a536f9912ae857cd406caa45cb3b7 Mon Sep 17 00:00:00 2001 From: Mihaly Gyongyosi Date: Fri, 26 Apr 2024 11:52:18 +0200 Subject: [PATCH 01/16] Add OrgRoleMapper --- .../social/connectors/org_role_mapper.go | 185 ++++++++++++++++++ .../social/connectors/org_role_mapper_test.go | 147 ++++++++++++++ 2 files changed, 332 insertions(+) create mode 100644 pkg/login/social/connectors/org_role_mapper.go create mode 100644 pkg/login/social/connectors/org_role_mapper_test.go diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go new file mode 100644 index 0000000000000..1dc460a476114 --- /dev/null +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -0,0 +1,185 @@ +package connectors + +import ( + "context" + "fmt" + "strconv" + "strings" + + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/setting" +) + +const MapperMatchAllOrgID = -1 + +type OrgRoleMapper struct { + cfg *setting.Cfg + logger log.Logger + orgService org.Service +} + +func ProvideOrgRoleMapper(cfg *setting.Cfg, orgService org.Service) *OrgRoleMapper { + return &OrgRoleMapper{ + cfg: cfg, + logger: log.New("orgrole.mapper"), + orgService: orgService, + } +} + +func (m *OrgRoleMapper) MapOrgRoles(ctx context.Context, externalOrgs []string, orgMappingSettings []string, directlyMappedRole org.RoleType) (map[int64]org.RoleType, error) { + orgMapping := m.splitOrgMappingSettings(ctx, orgMappingSettings) + + userOrgRoles := getMappedOrgRoles(externalOrgs, orgMapping) + + m.handleGlobalOrgMapping(userOrgRoles) + + if len(userOrgRoles) == 0 { + userOrgRoles = m.GetDefaultOrgMapping(directlyMappedRole) + } + + if directlyMappedRole == "" { + m.logger.Debug("No direct role mapping found") + return userOrgRoles, nil + } + + m.logger.Debug("Direct role mapping found", "role", directlyMappedRole) + + // Merge roles from org mapping `org_mapping` with role from direct mapping + for orgID, role := range userOrgRoles { + userOrgRoles[orgID] = getTopRole(directlyMappedRole, role) + } + + return userOrgRoles, nil +} + +func (m *OrgRoleMapper) GetDefaultOrgMapping(directlyMappedRole org.RoleType) map[int64]org.RoleType { + orgRoles := make(map[int64]org.RoleType, 0) + + orgID := int64(1) + if m.cfg.AutoAssignOrg && m.cfg.AutoAssignOrgId > 0 { + orgID = int64(m.cfg.AutoAssignOrgId) + } + + if directlyMappedRole == "" || !directlyMappedRole.IsValid() { + orgRoles[orgID] = org.RoleType(m.cfg.AutoAssignOrgRole) + } else { + orgRoles[orgID] = directlyMappedRole + } + + return orgRoles +} + +func (m *OrgRoleMapper) handleGlobalOrgMapping(orgRoles map[int64]org.RoleType) { + // No global role mapping => return + globalRole, ok := orgRoles[MapperMatchAllOrgID] + if !ok { + return + } + + allOrgIDs, err := m.getAllOrgs() + if err != nil { + // Prevent resetting assignments + orgRoles = map[int64]org.RoleType{} + m.logger.Warn("error fetching all orgs, removing org mapping to prevent org sync") + } + + // Remove the global role mapping + delete(orgRoles, MapperMatchAllOrgID) + + // Global mapping => for all orgs get top role mapping + for orgID := range allOrgIDs { + orgRoles[orgID] = getTopRole(orgRoles[orgID], globalRole) + } +} + +func (m *OrgRoleMapper) splitOrgMappingSettings(ctx context.Context, mappings []string) map[string]map[int64]org.RoleType { + res := map[string]map[int64]org.RoleType{} + + for _, v := range mappings { + kv := strings.Split(v, ":") + if len(kv) > 1 { + orgID, err := strconv.Atoi(kv[1]) + if err != nil && kv[1] != "*" { + res, getErr := m.orgService.GetByName(ctx, &org.GetOrgByNameQuery{Name: kv[1]}) + + if getErr != nil { + // ignore not existing org + m.logger.Warn("Could not fetch organization. Skipping.", "mapping", fmt.Sprintf("%v", v)) + continue + } + orgID, err = int(res.ID), nil + } + if kv[1] == "*" { + orgID, err = MapperMatchAllOrgID, nil + } + if err == nil { + orga := kv[0] + if res[orga] == nil { + res[orga] = map[int64]org.RoleType{} + } + + if len(kv) > 2 && org.RoleType(kv[2]).IsValid() { + res[orga][int64(orgID)] = org.RoleType(kv[2]) + } else { + res[orga][int64(orgID)] = org.RoleViewer + } + } + } + } + + return res +} + +func (m *OrgRoleMapper) getAllOrgs() (map[int64]bool, error) { + allOrgIDs := map[int64]bool{} + allOrgs, err := m.orgService.Search(context.Background(), &org.SearchOrgsQuery{}) + if err != nil { + // In case of error, return no orgs + return nil, err + } + + for _, org := range allOrgs { + allOrgIDs[org.ID] = true + } + return allOrgIDs, nil +} + +func getMappedOrgRoles(externalOrgs []string, orgMapping map[string]map[int64]org.RoleType) map[int64]org.RoleType { + userOrgRoles := map[int64]org.RoleType{} + + if len(orgMapping) == 0 { + return nil + } + + if orgRoles, ok := orgMapping["*"]; ok { + for orgID, role := range orgRoles { + userOrgRoles[orgID] = role + } + } + + for _, org := range externalOrgs { + orgRoles, ok := orgMapping[org] + if !ok { + continue + } + + for orgID, role := range orgRoles { + userOrgRoles[orgID] = getTopRole(userOrgRoles[orgID], role) + } + } + + return userOrgRoles +} + +func getTopRole(currRole org.RoleType, otherRole org.RoleType) org.RoleType { + if currRole == "" { + return otherRole + } + + if currRole.Includes(otherRole) { + return currRole + } + + return otherRole +} diff --git a/pkg/login/social/connectors/org_role_mapper_test.go b/pkg/login/social/connectors/org_role_mapper_test.go new file mode 100644 index 0000000000000..07e8fedf2c8fb --- /dev/null +++ b/pkg/login/social/connectors/org_role_mapper_test.go @@ -0,0 +1,147 @@ +package connectors + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/services/org/orgtest" + "github.com/grafana/grafana/pkg/setting" +) + +func TestExternalOrgRoleMapper_MapOrgRoles(t *testing.T) { + testCases := []struct { + name string + externalOrgs []string + orgMappingSettings []string + directlyMappedRole org.RoleType + expected map[int64]org.RoleType + }{ + { + name: "should return the default mapping when no org mapping settings are provided and directly mapped role is not set", + externalOrgs: []string{}, + orgMappingSettings: []string{}, + directlyMappedRole: "", + expected: map[int64]org.RoleType{2: org.RoleViewer}, + }, + { + name: "should return the default mapping when no org mapping settings are provided", + externalOrgs: []string{}, + orgMappingSettings: []string{}, + directlyMappedRole: org.RoleEditor, + 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", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"Second:1:Editor"}, + directlyMappedRole: "", + expected: map[int64]org.RoleType{2: org.RoleViewer}, + }, + { + name: "should map the higher role if directly mapped role is lower than the role found in the org mapping", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"First:1:Editor"}, + directlyMappedRole: org.RoleViewer, + expected: map[int64]org.RoleType{1: org.RoleEditor}, + }, + { + name: "should map the directly mapped role if it is higher than the role found in the org mapping", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"First:1:Viewer"}, + directlyMappedRole: org.RoleEditor, + expected: map[int64]org.RoleType{1: org.RoleEditor}, + }, + { + name: "should map to multiple organizations and set the directly mapped role for those if it is higher than the role found in the org mapping", + externalOrgs: []string{"First", "Second"}, + orgMappingSettings: []string{"First:1:Viewer", "Second:2:Viewer"}, + directlyMappedRole: org.RoleEditor, + expected: map[int64]org.RoleType{1: org.RoleEditor, 2: org.RoleEditor}, + }, + { + name: "should map to multiple organizations and set the directly mapped role for those if the directly mapped role is not set", + externalOrgs: []string{"First", "Second"}, + orgMappingSettings: []string{"First:1:Viewer", "Second:2:Viewer"}, + directlyMappedRole: "", + expected: map[int64]org.RoleType{1: org.RoleViewer, 2: org.RoleViewer}, + }, + { + name: "should map to all of the organizations if global org mapping is provided and the directly mapped role is set", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"First:*:Editor"}, + directlyMappedRole: org.RoleViewer, + expected: map[int64]org.RoleType{1: org.RoleEditor, 2: org.RoleEditor, 3: org.RoleEditor}, + }, + { + name: "should map to all of the organizations if global org mapping is provided and the directly mapped role is not set", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"First:*:Editor"}, + directlyMappedRole: "", + expected: map[int64]org.RoleType{1: org.RoleEditor, 2: org.RoleEditor, 3: org.RoleEditor}, + }, + { + name: "should map to all of the organizations if global org mapping is provided and the directly mapped role is higher than the role found in the org mappings", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"First:*:Viewer"}, + directlyMappedRole: org.RoleEditor, + expected: map[int64]org.RoleType{1: org.RoleEditor, 2: org.RoleEditor, 3: org.RoleEditor}, + }, + { + name: "should map correctly when global org mapping is provided", + externalOrgs: []string{"First", "Second", "Third"}, + orgMappingSettings: []string{"First:1:Viewer", "*:1:Editor", "Second:2:Viewer"}, + directlyMappedRole: "", + expected: map[int64]org.RoleType{1: org.RoleEditor, 2: org.RoleViewer}, + }, + { + name: "should map correctly and respect wildcard precedence when global org mapping is provided", + externalOrgs: []string{"First", "Second"}, + orgMappingSettings: []string{"*:1:Editor", "First:1:Viewer", "Second:2:Viewer"}, + directlyMappedRole: "", + expected: map[int64]org.RoleType{1: org.RoleEditor, 2: org.RoleViewer}, + }, + { + name: "should map directly mapped role when global org mapping is provided and the directly mapped role is higher than the role found in the org mappings", + externalOrgs: []string{"First", "Second", "Third"}, + orgMappingSettings: []string{"First:1:Viewer", "*:1:Editor", "Second:2:Viewer"}, + directlyMappedRole: org.RoleAdmin, + expected: map[int64]org.RoleType{1: org.RoleAdmin, 2: org.RoleAdmin}, + }, + { + name: "should map correctly and respect the mapping precedence when multiple org mappings are provided for the same org", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"First:1:Editor", "First:1:Viewer"}, + directlyMappedRole: "", + expected: map[int64]org.RoleType{1: org.RoleViewer}, + }, + { + name: "should map to all organizations when global org mapping is provided and the directly mapped role is higher than the role found in the org mappings", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"First:*:Editor"}, + directlyMappedRole: org.RoleAdmin, + expected: map[int64]org.RoleType{1: org.RoleAdmin, 2: org.RoleAdmin, 3: org.RoleAdmin}, + }, + } + orgService := orgtest.NewOrgServiceFake() + orgService.ExpectedOrgs = []*org.OrgDTO{ + {Name: "First", ID: 1}, + {Name: "Second", ID: 2}, + {Name: "Third", ID: 3}, + } + cfg := setting.NewCfg() + cfg.AutoAssignOrg = true + cfg.AutoAssignOrgId = 2 + cfg.AutoAssignOrgRole = string(org.RoleViewer) + mapper := ProvideOrgRoleMapper(cfg, orgService) + + for _, tc := range testCases { + actual, err := mapper.MapOrgRoles(context.Background(), tc.externalOrgs, tc.orgMappingSettings, tc.directlyMappedRole) + require.NoError(t, err) + + assert.EqualValues(t, tc.expected, actual) + } +} From 2423f46d55108ed14874c76e0dab44b0a4eefb11 Mon Sep 17 00:00:00 2001 From: Mihaly Gyongyosi Date: Fri, 26 Apr 2024 14:59:29 +0200 Subject: [PATCH 02/16] Address feedback, add more tests --- .../social/connectors/org_role_mapper.go | 8 +- .../social/connectors/org_role_mapper_test.go | 80 ++- pkg/services/org/org.go | 1 + pkg/services/org/orgtest/mock.go | 454 ++++++++++++++++++ 4 files changed, 535 insertions(+), 8 deletions(-) create mode 100644 pkg/services/org/orgtest/mock.go diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index 1dc460a476114..bb1ec314f2ea5 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -29,13 +29,16 @@ func ProvideOrgRoleMapper(cfg *setting.Cfg, orgService org.Service) *OrgRoleMapp func (m *OrgRoleMapper) MapOrgRoles(ctx context.Context, externalOrgs []string, orgMappingSettings []string, directlyMappedRole org.RoleType) (map[int64]org.RoleType, error) { orgMapping := m.splitOrgMappingSettings(ctx, orgMappingSettings) + if len(orgMapping) == 0 { + return m.GetDefaultOrgMapping(directlyMappedRole), nil + } userOrgRoles := getMappedOrgRoles(externalOrgs, orgMapping) m.handleGlobalOrgMapping(userOrgRoles) if len(userOrgRoles) == 0 { - userOrgRoles = m.GetDefaultOrgMapping(directlyMappedRole) + return m.GetDefaultOrgMapping(directlyMappedRole), nil } if directlyMappedRole == "" { @@ -80,8 +83,9 @@ func (m *OrgRoleMapper) handleGlobalOrgMapping(orgRoles map[int64]org.RoleType) allOrgIDs, err := m.getAllOrgs() if err != nil { // Prevent resetting assignments - orgRoles = map[int64]org.RoleType{} + clear(orgRoles) m.logger.Warn("error fetching all orgs, removing org mapping to prevent org sync") + return } // Remove the global role mapping diff --git a/pkg/login/social/connectors/org_role_mapper_test.go b/pkg/login/social/connectors/org_role_mapper_test.go index 07e8fedf2c8fb..3da83936f8345 100644 --- a/pkg/login/social/connectors/org_role_mapper_test.go +++ b/pkg/login/social/connectors/org_role_mapper_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/services/org" @@ -12,12 +13,13 @@ import ( "github.com/grafana/grafana/pkg/setting" ) -func TestExternalOrgRoleMapper_MapOrgRoles(t *testing.T) { +func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { testCases := []struct { name string externalOrgs []string orgMappingSettings []string directlyMappedRole org.RoleType + getAllOrgsError error expected map[int64]org.RoleType }{ { @@ -41,6 +43,13 @@ func TestExternalOrgRoleMapper_MapOrgRoles(t *testing.T) { directlyMappedRole: "", expected: map[int64]org.RoleType{2: org.RoleViewer}, }, + { + name: "should return Viewer role for the org if the specified role is invalid", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"First:1:SuperEditor"}, + directlyMappedRole: "", + expected: map[int64]org.RoleType{1: org.RoleViewer}, + }, { name: "should map the higher role if directly mapped role is lower than the role found in the org mapping", externalOrgs: []string{"First"}, @@ -125,13 +134,16 @@ func TestExternalOrgRoleMapper_MapOrgRoles(t *testing.T) { directlyMappedRole: org.RoleAdmin, expected: map[int64]org.RoleType{1: org.RoleAdmin, 2: org.RoleAdmin, 3: org.RoleAdmin}, }, + { + name: "should skip map to all organizations if org service returns an error", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"First:*:Editor"}, + getAllOrgsError: assert.AnError, + directlyMappedRole: org.RoleAdmin, + expected: map[int64]org.RoleType{2: org.RoleAdmin}, + }, } orgService := orgtest.NewOrgServiceFake() - orgService.ExpectedOrgs = []*org.OrgDTO{ - {Name: "First", ID: 1}, - {Name: "Second", ID: 2}, - {Name: "Third", ID: 3}, - } cfg := setting.NewCfg() cfg.AutoAssignOrg = true cfg.AutoAssignOrgId = 2 @@ -139,9 +151,65 @@ func TestExternalOrgRoleMapper_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}, + } + } actual, err := mapper.MapOrgRoles(context.Background(), tc.externalOrgs, tc.orgMappingSettings, tc.directlyMappedRole) require.NoError(t, err) assert.EqualValues(t, tc.expected, actual) } } + +func TestOrgRoleMapper_MapOrgRoles_OrgNameResolution(t *testing.T) { + testCases := []struct { + name string + orgMapping []string + setupMock func(*orgtest.MockService) + expectedOrgRoles map[int64]org.RoleType + }{ + { + name: "should skip org mapping if org name is not found or the resolution fails", + orgMapping: []string{"ExternalOrg1:First:Editor", "ExternalOrg1:NonExistent:Viewer"}, + setupMock: func(orgService *orgtest.MockService) { + orgService.On("GetByName", mock.Anything, mock.MatchedBy(func(query *org.GetOrgByNameQuery) bool { + return query.Name == "First" + })).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}, + }, + { + name: "should return default mapping 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}, + }, + } + + cfg := setting.NewCfg() + cfg.AutoAssignOrg = true + cfg.AutoAssignOrgId = 2 + cfg.AutoAssignOrgRole = string(org.RoleViewer) + + orgService := orgtest.NewMockService(t) + mapper := ProvideOrgRoleMapper(cfg, orgService) + + for _, tc := range testCases { + orgService.ExpectedCalls = nil + tc.setupMock(orgService) + + actual, err := mapper.MapOrgRoles(context.Background(), []string{"ExternalOrg1"}, tc.orgMapping, org.RoleViewer) + require.NoError(t, err) + + assert.EqualValues(t, tc.expectedOrgRoles, actual) + } +} diff --git a/pkg/services/org/org.go b/pkg/services/org/org.go index b41c7a80a6fd2..cb531aea33e7f 100644 --- a/pkg/services/org/org.go +++ b/pkg/services/org/org.go @@ -4,6 +4,7 @@ import ( "context" ) +//go:generate mockery --name Service --structname MockService --outpkg orgtest --filename mock.go --output ./orgtest/ type Service interface { GetIDForNewUser(context.Context, GetOrgIDForNewUserCommand) (int64, error) InsertOrgUser(context.Context, *OrgUser) (int64, error) diff --git a/pkg/services/org/orgtest/mock.go b/pkg/services/org/orgtest/mock.go new file mode 100644 index 0000000000000..7dc2b16d34d62 --- /dev/null +++ b/pkg/services/org/orgtest/mock.go @@ -0,0 +1,454 @@ +// Code generated by mockery v2.42.1. DO NOT EDIT. + +package orgtest + +import ( + context "context" + + org "github.com/grafana/grafana/pkg/services/org" + mock "github.com/stretchr/testify/mock" +) + +// MockService is an autogenerated mock type for the Service type +type MockService struct { + mock.Mock +} + +// AddOrgUser provides a mock function with given fields: _a0, _a1 +func (_m *MockService) AddOrgUser(_a0 context.Context, _a1 *org.AddOrgUserCommand) error { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for AddOrgUser") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, *org.AddOrgUserCommand) error); ok { + r0 = rf(_a0, _a1) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// CreateWithMember provides a mock function with given fields: _a0, _a1 +func (_m *MockService) CreateWithMember(_a0 context.Context, _a1 *org.CreateOrgCommand) (*org.Org, error) { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for CreateWithMember") + } + + var r0 *org.Org + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, *org.CreateOrgCommand) (*org.Org, error)); ok { + return rf(_a0, _a1) + } + if rf, ok := ret.Get(0).(func(context.Context, *org.CreateOrgCommand) *org.Org); ok { + r0 = rf(_a0, _a1) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*org.Org) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, *org.CreateOrgCommand) error); ok { + r1 = rf(_a0, _a1) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// Delete provides a mock function with given fields: _a0, _a1 +func (_m *MockService) Delete(_a0 context.Context, _a1 *org.DeleteOrgCommand) error { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for Delete") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, *org.DeleteOrgCommand) error); ok { + r0 = rf(_a0, _a1) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// DeleteUserFromAll provides a mock function with given fields: _a0, _a1 +func (_m *MockService) DeleteUserFromAll(_a0 context.Context, _a1 int64) error { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for DeleteUserFromAll") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, int64) error); ok { + r0 = rf(_a0, _a1) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// GetByID provides a mock function with given fields: _a0, _a1 +func (_m *MockService) GetByID(_a0 context.Context, _a1 *org.GetOrgByIDQuery) (*org.Org, error) { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for GetByID") + } + + var r0 *org.Org + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, *org.GetOrgByIDQuery) (*org.Org, error)); ok { + return rf(_a0, _a1) + } + if rf, ok := ret.Get(0).(func(context.Context, *org.GetOrgByIDQuery) *org.Org); ok { + r0 = rf(_a0, _a1) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*org.Org) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, *org.GetOrgByIDQuery) error); ok { + r1 = rf(_a0, _a1) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetByName provides a mock function with given fields: _a0, _a1 +func (_m *MockService) GetByName(_a0 context.Context, _a1 *org.GetOrgByNameQuery) (*org.Org, error) { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for GetByName") + } + + var r0 *org.Org + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, *org.GetOrgByNameQuery) (*org.Org, error)); ok { + return rf(_a0, _a1) + } + if rf, ok := ret.Get(0).(func(context.Context, *org.GetOrgByNameQuery) *org.Org); ok { + r0 = rf(_a0, _a1) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*org.Org) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, *org.GetOrgByNameQuery) error); ok { + r1 = rf(_a0, _a1) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetIDForNewUser provides a mock function with given fields: _a0, _a1 +func (_m *MockService) GetIDForNewUser(_a0 context.Context, _a1 org.GetOrgIDForNewUserCommand) (int64, error) { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for GetIDForNewUser") + } + + var r0 int64 + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, org.GetOrgIDForNewUserCommand) (int64, error)); ok { + return rf(_a0, _a1) + } + if rf, ok := ret.Get(0).(func(context.Context, org.GetOrgIDForNewUserCommand) int64); ok { + r0 = rf(_a0, _a1) + } else { + r0 = ret.Get(0).(int64) + } + + if rf, ok := ret.Get(1).(func(context.Context, org.GetOrgIDForNewUserCommand) error); ok { + r1 = rf(_a0, _a1) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetOrCreate provides a mock function with given fields: _a0, _a1 +func (_m *MockService) GetOrCreate(_a0 context.Context, _a1 string) (int64, error) { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for GetOrCreate") + } + + var r0 int64 + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string) (int64, error)); ok { + return rf(_a0, _a1) + } + if rf, ok := ret.Get(0).(func(context.Context, string) int64); ok { + r0 = rf(_a0, _a1) + } else { + r0 = ret.Get(0).(int64) + } + + if rf, ok := ret.Get(1).(func(context.Context, string) error); ok { + r1 = rf(_a0, _a1) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetOrgUsers provides a mock function with given fields: _a0, _a1 +func (_m *MockService) GetOrgUsers(_a0 context.Context, _a1 *org.GetOrgUsersQuery) ([]*org.OrgUserDTO, error) { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for GetOrgUsers") + } + + var r0 []*org.OrgUserDTO + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, *org.GetOrgUsersQuery) ([]*org.OrgUserDTO, error)); ok { + return rf(_a0, _a1) + } + if rf, ok := ret.Get(0).(func(context.Context, *org.GetOrgUsersQuery) []*org.OrgUserDTO); ok { + r0 = rf(_a0, _a1) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*org.OrgUserDTO) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, *org.GetOrgUsersQuery) error); ok { + r1 = rf(_a0, _a1) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// GetUserOrgList provides a mock function with given fields: _a0, _a1 +func (_m *MockService) GetUserOrgList(_a0 context.Context, _a1 *org.GetUserOrgListQuery) ([]*org.UserOrgDTO, error) { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for GetUserOrgList") + } + + var r0 []*org.UserOrgDTO + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, *org.GetUserOrgListQuery) ([]*org.UserOrgDTO, error)); ok { + return rf(_a0, _a1) + } + if rf, ok := ret.Get(0).(func(context.Context, *org.GetUserOrgListQuery) []*org.UserOrgDTO); ok { + r0 = rf(_a0, _a1) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*org.UserOrgDTO) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, *org.GetUserOrgListQuery) error); ok { + r1 = rf(_a0, _a1) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// InsertOrgUser provides a mock function with given fields: _a0, _a1 +func (_m *MockService) InsertOrgUser(_a0 context.Context, _a1 *org.OrgUser) (int64, error) { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for InsertOrgUser") + } + + var r0 int64 + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, *org.OrgUser) (int64, error)); ok { + return rf(_a0, _a1) + } + if rf, ok := ret.Get(0).(func(context.Context, *org.OrgUser) int64); ok { + r0 = rf(_a0, _a1) + } else { + r0 = ret.Get(0).(int64) + } + + if rf, ok := ret.Get(1).(func(context.Context, *org.OrgUser) error); ok { + r1 = rf(_a0, _a1) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// RegisterDelete provides a mock function with given fields: query +func (_m *MockService) RegisterDelete(query string) { + _m.Called(query) +} + +// RemoveOrgUser provides a mock function with given fields: _a0, _a1 +func (_m *MockService) RemoveOrgUser(_a0 context.Context, _a1 *org.RemoveOrgUserCommand) error { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for RemoveOrgUser") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, *org.RemoveOrgUserCommand) error); ok { + r0 = rf(_a0, _a1) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// Search provides a mock function with given fields: _a0, _a1 +func (_m *MockService) Search(_a0 context.Context, _a1 *org.SearchOrgsQuery) ([]*org.OrgDTO, error) { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for Search") + } + + var r0 []*org.OrgDTO + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, *org.SearchOrgsQuery) ([]*org.OrgDTO, error)); ok { + return rf(_a0, _a1) + } + if rf, ok := ret.Get(0).(func(context.Context, *org.SearchOrgsQuery) []*org.OrgDTO); ok { + r0 = rf(_a0, _a1) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*org.OrgDTO) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, *org.SearchOrgsQuery) error); ok { + r1 = rf(_a0, _a1) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// SearchOrgUsers provides a mock function with given fields: _a0, _a1 +func (_m *MockService) SearchOrgUsers(_a0 context.Context, _a1 *org.SearchOrgUsersQuery) (*org.SearchOrgUsersQueryResult, error) { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for SearchOrgUsers") + } + + var r0 *org.SearchOrgUsersQueryResult + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, *org.SearchOrgUsersQuery) (*org.SearchOrgUsersQueryResult, error)); ok { + return rf(_a0, _a1) + } + if rf, ok := ret.Get(0).(func(context.Context, *org.SearchOrgUsersQuery) *org.SearchOrgUsersQueryResult); ok { + r0 = rf(_a0, _a1) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*org.SearchOrgUsersQueryResult) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, *org.SearchOrgUsersQuery) error); ok { + r1 = rf(_a0, _a1) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// UpdateAddress provides a mock function with given fields: _a0, _a1 +func (_m *MockService) UpdateAddress(_a0 context.Context, _a1 *org.UpdateOrgAddressCommand) error { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for UpdateAddress") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, *org.UpdateOrgAddressCommand) error); ok { + r0 = rf(_a0, _a1) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// UpdateOrg provides a mock function with given fields: _a0, _a1 +func (_m *MockService) UpdateOrg(_a0 context.Context, _a1 *org.UpdateOrgCommand) error { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for UpdateOrg") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, *org.UpdateOrgCommand) error); ok { + r0 = rf(_a0, _a1) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// UpdateOrgUser provides a mock function with given fields: _a0, _a1 +func (_m *MockService) UpdateOrgUser(_a0 context.Context, _a1 *org.UpdateOrgUserCommand) error { + ret := _m.Called(_a0, _a1) + + if len(ret) == 0 { + panic("no return value specified for UpdateOrgUser") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, *org.UpdateOrgUserCommand) error); ok { + r0 = rf(_a0, _a1) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// NewMockService creates a new instance of MockService. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewMockService(t interface { + mock.TestingT + Cleanup(func()) +}) *MockService { + mock := &MockService{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} From c7cddbc904252ae53981717b2166bb0ef2c6654b Mon Sep 17 00:00:00 2001 From: Mihaly Gyongyosi Date: Fri, 26 Apr 2024 17:57:12 +0200 Subject: [PATCH 03/16] Prevent resetting assignments when global org mapping fails --- .../social/connectors/org_role_mapper.go | 24 ++++++++++++------- .../social/connectors/org_role_mapper_test.go | 9 +++---- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index bb1ec314f2ea5..1be149fca0f0b 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -27,23 +27,27 @@ func ProvideOrgRoleMapper(cfg *setting.Cfg, orgService org.Service) *OrgRoleMapp } } -func (m *OrgRoleMapper) MapOrgRoles(ctx context.Context, externalOrgs []string, orgMappingSettings []string, directlyMappedRole org.RoleType) (map[int64]org.RoleType, error) { +func (m *OrgRoleMapper) MapOrgRoles(ctx context.Context, externalOrgs []string, orgMappingSettings []string, directlyMappedRole org.RoleType) map[int64]org.RoleType { orgMapping := m.splitOrgMappingSettings(ctx, orgMappingSettings) + if len(orgMapping) == 0 { - return m.GetDefaultOrgMapping(directlyMappedRole), nil + return m.GetDefaultOrgMapping(directlyMappedRole) } userOrgRoles := getMappedOrgRoles(externalOrgs, orgMapping) - m.handleGlobalOrgMapping(userOrgRoles) + if err := m.handleGlobalOrgMapping(userOrgRoles); err != nil { + // Cannot map global org roles, return nil (prevent resetting asignments) + return nil + } if len(userOrgRoles) == 0 { - return m.GetDefaultOrgMapping(directlyMappedRole), nil + return m.GetDefaultOrgMapping(directlyMappedRole) } if directlyMappedRole == "" { m.logger.Debug("No direct role mapping found") - return userOrgRoles, nil + return userOrgRoles } m.logger.Debug("Direct role mapping found", "role", directlyMappedRole) @@ -53,7 +57,7 @@ func (m *OrgRoleMapper) MapOrgRoles(ctx context.Context, externalOrgs []string, userOrgRoles[orgID] = getTopRole(directlyMappedRole, role) } - return userOrgRoles, nil + return userOrgRoles } func (m *OrgRoleMapper) GetDefaultOrgMapping(directlyMappedRole org.RoleType) map[int64]org.RoleType { @@ -73,11 +77,11 @@ func (m *OrgRoleMapper) GetDefaultOrgMapping(directlyMappedRole org.RoleType) ma return orgRoles } -func (m *OrgRoleMapper) handleGlobalOrgMapping(orgRoles map[int64]org.RoleType) { +func (m *OrgRoleMapper) handleGlobalOrgMapping(orgRoles map[int64]org.RoleType) error { // No global role mapping => return globalRole, ok := orgRoles[MapperMatchAllOrgID] if !ok { - return + return nil } allOrgIDs, err := m.getAllOrgs() @@ -85,7 +89,7 @@ func (m *OrgRoleMapper) handleGlobalOrgMapping(orgRoles map[int64]org.RoleType) // Prevent resetting assignments clear(orgRoles) m.logger.Warn("error fetching all orgs, removing org mapping to prevent org sync") - return + return err } // Remove the global role mapping @@ -95,6 +99,8 @@ func (m *OrgRoleMapper) handleGlobalOrgMapping(orgRoles map[int64]org.RoleType) for orgID := range allOrgIDs { orgRoles[orgID] = getTopRole(orgRoles[orgID], globalRole) } + + return nil } func (m *OrgRoleMapper) splitOrgMappingSettings(ctx context.Context, mappings []string) map[string]map[int64]org.RoleType { diff --git a/pkg/login/social/connectors/org_role_mapper_test.go b/pkg/login/social/connectors/org_role_mapper_test.go index 3da83936f8345..514064f1d2120 100644 --- a/pkg/login/social/connectors/org_role_mapper_test.go +++ b/pkg/login/social/connectors/org_role_mapper_test.go @@ -6,7 +6,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/org/orgtest" @@ -140,7 +139,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { orgMappingSettings: []string{"First:*:Editor"}, getAllOrgsError: assert.AnError, directlyMappedRole: org.RoleAdmin, - expected: map[int64]org.RoleType{2: org.RoleAdmin}, + expected: nil, }, } orgService := orgtest.NewOrgServiceFake() @@ -160,8 +159,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { {Name: "Third", ID: 3}, } } - actual, err := mapper.MapOrgRoles(context.Background(), tc.externalOrgs, tc.orgMappingSettings, tc.directlyMappedRole) - require.NoError(t, err) + actual := mapper.MapOrgRoles(context.Background(), tc.externalOrgs, tc.orgMappingSettings, tc.directlyMappedRole) assert.EqualValues(t, tc.expected, actual) } @@ -207,8 +205,7 @@ func TestOrgRoleMapper_MapOrgRoles_OrgNameResolution(t *testing.T) { orgService.ExpectedCalls = nil tc.setupMock(orgService) - actual, err := mapper.MapOrgRoles(context.Background(), []string{"ExternalOrg1"}, tc.orgMapping, org.RoleViewer) - require.NoError(t, err) + actual := mapper.MapOrgRoles(context.Background(), []string{"ExternalOrg1"}, tc.orgMapping, org.RoleViewer) assert.EqualValues(t, tc.expectedOrgRoles, actual) } From e606e95ea55c3be3ce3d303c1f65e843b143a76f Mon Sep 17 00:00:00 2001 From: Mihaly Gyongyosi Date: Mon, 29 Apr 2024 10:21:37 +0200 Subject: [PATCH 04/16] Provide a parsing function instead of recalculating the org_mapping on every mapping --- pkg/login/social/connectors/org_role_mapper.go | 18 ++++++++++++------ .../social/connectors/org_role_mapper_test.go | 8 +++++--- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index 1be149fca0f0b..146fd668c8fb2 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -27,9 +27,13 @@ func ProvideOrgRoleMapper(cfg *setting.Cfg, orgService org.Service) *OrgRoleMapp } } -func (m *OrgRoleMapper) MapOrgRoles(ctx context.Context, externalOrgs []string, orgMappingSettings []string, directlyMappedRole org.RoleType) map[int64]org.RoleType { - orgMapping := m.splitOrgMappingSettings(ctx, orgMappingSettings) - +// MapOrgRoles maps the external orgs/groups to Grafana orgs and roles. It returns a map or orgID to role. +// +// externalOrgs: list of external orgs/groups +// 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) } @@ -103,7 +107,9 @@ func (m *OrgRoleMapper) handleGlobalOrgMapping(orgRoles map[int64]org.RoleType) return nil } -func (m *OrgRoleMapper) splitOrgMappingSettings(ctx context.Context, mappings []string) map[string]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 { res := map[string]map[int64]org.RoleType{} for _, v := range mappings { @@ -114,8 +120,8 @@ func (m *OrgRoleMapper) splitOrgMappingSettings(ctx context.Context, mappings [] res, getErr := m.orgService.GetByName(ctx, &org.GetOrgByNameQuery{Name: kv[1]}) if getErr != nil { - // ignore not existing org - m.logger.Warn("Could not fetch organization. Skipping.", "mapping", fmt.Sprintf("%v", v)) + // skip in case of error + m.logger.Warn("Could not fetch organization. Skipping.", "err", getErr, "mapping", fmt.Sprintf("%v", v), "org", kv[1]) continue } orgID, err = int(res.ID), nil diff --git a/pkg/login/social/connectors/org_role_mapper_test.go b/pkg/login/social/connectors/org_role_mapper_test.go index 514064f1d2120..a403ae5009297 100644 --- a/pkg/login/social/connectors/org_role_mapper_test.go +++ b/pkg/login/social/connectors/org_role_mapper_test.go @@ -159,7 +159,8 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { {Name: "Third", ID: 3}, } } - actual := mapper.MapOrgRoles(context.Background(), tc.externalOrgs, tc.orgMappingSettings, tc.directlyMappedRole) + orgMapping := mapper.ParseOrgMappingSettings(context.Background(), tc.orgMappingSettings) + actual := mapper.MapOrgRoles(context.Background(), tc.externalOrgs, orgMapping, tc.directlyMappedRole) assert.EqualValues(t, tc.expected, actual) } @@ -173,7 +174,7 @@ func TestOrgRoleMapper_MapOrgRoles_OrgNameResolution(t *testing.T) { expectedOrgRoles map[int64]org.RoleType }{ { - name: "should skip org mapping if org name is not found or the resolution fails", + name: "should skip org mapping if org was not found or the resolution fails", orgMapping: []string{"ExternalOrg1:First:Editor", "ExternalOrg1:NonExistent:Viewer"}, setupMock: func(orgService *orgtest.MockService) { orgService.On("GetByName", mock.Anything, mock.MatchedBy(func(query *org.GetOrgByNameQuery) bool { @@ -205,7 +206,8 @@ func TestOrgRoleMapper_MapOrgRoles_OrgNameResolution(t *testing.T) { orgService.ExpectedCalls = nil tc.setupMock(orgService) - actual := mapper.MapOrgRoles(context.Background(), []string{"ExternalOrg1"}, tc.orgMapping, org.RoleViewer) + orgMapping := mapper.ParseOrgMappingSettings(context.Background(), tc.orgMapping) + actual := mapper.MapOrgRoles(context.Background(), []string{"ExternalOrg1"}, orgMapping, org.RoleViewer) assert.EqualValues(t, tc.expectedOrgRoles, actual) } From 03498fed785ba7f4ddd6a1f53943d54532107b78 Mon Sep 17 00:00:00 2001 From: Mihaly Gyongyosi Date: Thu, 2 May 2024 14:52:50 +0200 Subject: [PATCH 05/16] 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 146fd668c8fb2..8e8e7a8eef1d1 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 a403ae5009297..7dd3670d14915 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) + }) } } From d8bce8e4ef59cb2c22b018daba3a302439f252b2 Mon Sep 17 00:00:00 2001 From: Mihaly Gyongyosi Date: Fri, 3 May 2024 11:24:03 +0200 Subject: [PATCH 06/16] Introduce MappingConfiguration --- .../social/connectors/org_role_mapper.go | 21 +++++--- .../social/connectors/org_role_mapper_test.go | 49 +++++++++++++------ 2 files changed, 49 insertions(+), 21 deletions(-) diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index 8e8e7a8eef1d1..e0a94581ffa88 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -19,6 +19,11 @@ type OrgRoleMapper struct { orgService org.Service } +type MappingConfiguration struct { + orgMapping map[string]map[int64]org.RoleType + roleStrict bool +} + func ProvideOrgRoleMapper(cfg *setting.Cfg, orgService org.Service) *OrgRoleMapper { return &OrgRoleMapper{ cfg: cfg, @@ -30,23 +35,25 @@ func ProvideOrgRoleMapper(cfg *setting.Cfg, orgService org.Service) *OrgRoleMapp // MapOrgRoles maps the external orgs/groups to Grafana orgs and roles. It returns a map or orgID to role. // // externalOrgs: list of external orgs/groups +// // 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 +// // roleStrict: if true, either the evaluated role from orgMapping or the directlyMappedRole must be a valid role. func (m *OrgRoleMapper) MapOrgRoles( ctx context.Context, + mappingCfg *MappingConfiguration, 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 { + if len(mappingCfg.orgMapping) == 0 && !isValidRole(directlyMappedRole) && mappingCfg.roleStrict { // No org mappings are configured and the directly mapped role is not set and roleStrict is enabled return nil } - userOrgRoles := getMappedOrgRoles(externalOrgs, orgMapping) + userOrgRoles := getMappedOrgRoles(externalOrgs, mappingCfg.orgMapping) if err := m.handleGlobalOrgMapping(userOrgRoles); err != nil { // Cannot map global org roles, return nil (prevent resetting asignments) @@ -54,7 +61,7 @@ func (m *OrgRoleMapper) MapOrgRoles( } if len(userOrgRoles) == 0 { - if roleStrict && !isValidRole(directlyMappedRole) { + if mappingCfg.roleStrict && !isValidRole(directlyMappedRole) { // No org mapping found and roleStrict is enabled return nil } @@ -124,7 +131,7 @@ 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. // 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 { +func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings []string, roleStrict bool) *MappingConfiguration { res := map[string]map[int64]org.RoleType{} for _, v := range mappings { @@ -164,7 +171,7 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] } } - return res + return &MappingConfiguration{orgMapping: res, roleStrict: roleStrict} } func (m *OrgRoleMapper) getAllOrgs() (map[int64]bool, error) { diff --git a/pkg/login/social/connectors/org_role_mapper_test.go b/pkg/login/social/connectors/org_role_mapper_test.go index 7dd3670d14915..3504263b188a6 100644 --- a/pkg/login/social/connectors/org_role_mapper_test.go +++ b/pkg/login/social/connectors/org_role_mapper_test.go @@ -38,7 +38,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { expected: nil, }, { - name: "should return the default mapping when no org mapping settings are provided", + name: "should return the default mapping when no org mapping settings are provided and direcly mapped role is set", externalOrgs: []string{}, orgMappingSettings: []string{}, directlyMappedRole: org.RoleEditor, @@ -76,7 +76,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { 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", + name: "should map the direclty mapped role to default org if the org mapping is invalid and role strict is enabled", externalOrgs: []string{"First"}, orgMappingSettings: []string{"First:1:SuperEditor"}, directlyMappedRole: "Editor", @@ -194,8 +194,8 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { {Name: "Third", ID: 3}, } } - orgMapping := mapper.ParseOrgMappingSettings(context.Background(), tc.orgMappingSettings, tc.roleStrict) - actual := mapper.MapOrgRoles(context.Background(), tc.externalOrgs, orgMapping, tc.directlyMappedRole, tc.roleStrict) + mappingCfg := mapper.ParseOrgMappingSettings(context.Background(), tc.orgMappingSettings, tc.roleStrict) + actual := mapper.MapOrgRoles(context.Background(), mappingCfg, tc.externalOrgs, tc.directlyMappedRole) assert.EqualValues(t, tc.expected, actual) }) @@ -244,8 +244,8 @@ func TestOrgRoleMapper_MapOrgRoles_OrgNameResolution(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) + mappingCfg := mapper.ParseOrgMappingSettings(context.Background(), tc.orgMapping, false) + actual := mapper.MapOrgRoles(context.Background(), mappingCfg, []string{"ExternalOrg1"}, org.RoleViewer) assert.EqualValues(t, tc.expectedOrgRoles, actual) }) @@ -257,45 +257,66 @@ func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { name string rawMapping []string roleStrict bool - expected map[string]map[int64]org.RoleType + expected *MappingConfiguration }{ { name: "should return empty mapping when no org mapping settings are provided", rawMapping: []string{}, - expected: map[string]map[int64]org.RoleType{}, + expected: &MappingConfiguration{ + orgMapping: map[string]map[int64]org.RoleType{}, + roleStrict: false, + }, }, { 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{}, + expected: &MappingConfiguration{ + orgMapping: map[string]map[int64]org.RoleType{}, + roleStrict: true, + }, }, { 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}}, + expected: &MappingConfiguration{ + orgMapping: map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, + roleStrict: false, + }, }, { 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{}, + expected: &MappingConfiguration{ + orgMapping: map[string]map[int64]org.RoleType{}, + roleStrict: true, + }, }, { 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}}, + expected: &MappingConfiguration{ + orgMapping: map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, + roleStrict: false, + }, }, { name: "should return empty mapping when org mapping is empty", rawMapping: []string{}, - expected: map[string]map[int64]org.RoleType{}, + expected: &MappingConfiguration{ + orgMapping: map[string]map[int64]org.RoleType{}, + roleStrict: false, + }, }, { name: "should return empty mapping when org mapping is nil", rawMapping: nil, - expected: map[string]map[int64]org.RoleType{}, + expected: &MappingConfiguration{ + orgMapping: map[string]map[int64]org.RoleType{}, + roleStrict: false, + }, }, } for _, tc := range testCases { From de1d94b0a0e89f741f9b8b51bbb4beab1825cb6d Mon Sep 17 00:00:00 2001 From: Mihaly Gyongyosi Date: Fri, 3 May 2024 11:54:28 +0200 Subject: [PATCH 07/16] Handle other edge case --- .../social/connectors/org_role_mapper.go | 23 +++---- .../social/connectors/org_role_mapper_test.go | 62 +++++++++++-------- 2 files changed, 47 insertions(+), 38 deletions(-) diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index e0a94581ffa88..df3cf5d50c072 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -19,9 +19,12 @@ type OrgRoleMapper struct { orgService org.Service } +// MappingConfiguration represents the mapping configuration from external orgs to Grafana orgs and roles. +// orgMapping: mapping from external orgs to Grafana orgs and roles +// strictRoleMapping: if true, the mapper ensuers that the evaluated role from orgMapping or the directlyMappedRole is a valid role, otherwise it will return nil. type MappingConfiguration struct { - orgMapping map[string]map[int64]org.RoleType - roleStrict bool + orgMapping map[string]map[int64]org.RoleType + strictRoleMapping bool } func ProvideOrgRoleMapper(cfg *setting.Cfg, orgService org.Service) *OrgRoleMapper { @@ -34,21 +37,18 @@ func ProvideOrgRoleMapper(cfg *setting.Cfg, orgService org.Service) *OrgRoleMapp // MapOrgRoles maps the external orgs/groups to Grafana orgs and roles. It returns a map or orgID to role. // -// externalOrgs: list of external orgs/groups +// mappingCfg: mapping configuration from external orgs to Grafana orgs and roles. Use `ParseOrgMappingSettings` to convert the raw setting to this format. // -// 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. +// externalOrgs: list of external orgs/groups // // directlyMappedRole: role that is directly mapped to the user -// -// roleStrict: if true, either the evaluated role from orgMapping or the directlyMappedRole must be a valid role. func (m *OrgRoleMapper) MapOrgRoles( ctx context.Context, mappingCfg *MappingConfiguration, externalOrgs []string, directlyMappedRole org.RoleType, ) map[int64]org.RoleType { - if len(mappingCfg.orgMapping) == 0 && !isValidRole(directlyMappedRole) && mappingCfg.roleStrict { + if len(mappingCfg.orgMapping) == 0 && !isValidRole(directlyMappedRole) && mappingCfg.strictRoleMapping { // No org mappings are configured and the directly mapped role is not set and roleStrict is enabled return nil } @@ -61,7 +61,7 @@ func (m *OrgRoleMapper) MapOrgRoles( } if len(userOrgRoles) == 0 { - if mappingCfg.roleStrict && !isValidRole(directlyMappedRole) { + if mappingCfg.strictRoleMapping && !isValidRole(directlyMappedRole) { // No org mapping found and roleStrict is enabled return nil } @@ -153,8 +153,9 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] } if err == nil { if roleStrict && (len(kv) < 3 || !org.RoleType(kv[2]).IsValid()) { + // Return empty mapping if at least one org mapping is invalid (missing role, invalid role) m.logger.Warn("Skipping org mapping due to missing or invalid role in mapping when roleStrict is enabled.", "mapping", fmt.Sprintf("%v", v)) - continue + return &MappingConfiguration{orgMapping: map[string]map[int64]org.RoleType{}, strictRoleMapping: roleStrict} } orga := kv[0] @@ -171,7 +172,7 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] } } - return &MappingConfiguration{orgMapping: res, roleStrict: roleStrict} + return &MappingConfiguration{orgMapping: res, strictRoleMapping: roleStrict} } func (m *OrgRoleMapper) getAllOrgs() (map[int64]bool, error) { diff --git a/pkg/login/social/connectors/org_role_mapper_test.go b/pkg/login/social/connectors/org_role_mapper_test.go index 3504263b188a6..52fa9aa0557b8 100644 --- a/pkg/login/social/connectors/org_role_mapper_test.go +++ b/pkg/login/social/connectors/org_role_mapper_test.go @@ -29,14 +29,6 @@ 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 and direcly mapped role is set", externalOrgs: []string{}, @@ -52,19 +44,27 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { 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", + name: "should return Viewer role for the org if the specified role is invalid", externalOrgs: []string{"First"}, - orgMappingSettings: []string{"Second:1:Editor"}, + orgMappingSettings: []string{"First:1:SuperEditor"}, + directlyMappedRole: "", + expected: map[int64]org.RoleType{1: 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 Viewer role for the org if the specified role is invalid", + 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{"First:1:SuperEditor"}, + orgMappingSettings: []string{"Second:1:Editor"}, directlyMappedRole: "", - expected: map[int64]org.RoleType{1: org.RoleViewer}, + roleStrict: true, + expected: nil, }, // In this case the parsed org mapping will be empty because the role is invalid { @@ -83,6 +83,14 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { roleStrict: true, expected: map[int64]org.RoleType{2: org.RoleEditor}, }, + { + name: "should return nil if the org mapping contains at least one invalid setting and directly mapped role is empty and role strict is enabled", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"First:1:SuperEditor", "First:1:Admin"}, + directlyMappedRole: "", + roleStrict: true, + expected: nil, + }, { name: "should map the higher role if directly mapped role is lower than the role found in the org mapping", externalOrgs: []string{"First"}, @@ -263,8 +271,8 @@ func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { name: "should return empty mapping when no org mapping settings are provided", rawMapping: []string{}, expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{}, - roleStrict: false, + orgMapping: map[string]map[int64]org.RoleType{}, + strictRoleMapping: false, }, }, { @@ -272,8 +280,8 @@ func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { rawMapping: []string{"Second:1:SuperEditor"}, roleStrict: true, expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{}, - roleStrict: true, + orgMapping: map[string]map[int64]org.RoleType{}, + strictRoleMapping: true, }, }, { @@ -281,8 +289,8 @@ func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { rawMapping: []string{"Second:1:SuperEditor"}, roleStrict: false, expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, - roleStrict: false, + orgMapping: map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, + strictRoleMapping: false, }, }, { @@ -290,32 +298,32 @@ func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { rawMapping: []string{"Second:1"}, roleStrict: true, expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{}, - roleStrict: true, + orgMapping: map[string]map[int64]org.RoleType{}, + strictRoleMapping: true, }, }, { name: "should return empty mapping when org mapping doesn't contain the role and strict is disabled", rawMapping: []string{"Second:1"}, expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, - roleStrict: false, + orgMapping: map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, + strictRoleMapping: false, }, }, { name: "should return empty mapping when org mapping is empty", rawMapping: []string{}, expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{}, - roleStrict: false, + orgMapping: map[string]map[int64]org.RoleType{}, + strictRoleMapping: false, }, }, { name: "should return empty mapping when org mapping is nil", rawMapping: nil, expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{}, - roleStrict: false, + orgMapping: map[string]map[int64]org.RoleType{}, + strictRoleMapping: false, }, }, } From 1d282543acd943cc728bcd4bed3680f6f05cdd4c Mon Sep 17 00:00:00 2001 From: Mihaly Gyongyosi Date: Fri, 3 May 2024 12:20:24 +0200 Subject: [PATCH 08/16] Add tests --- .../social/connectors/org_role_mapper.go | 4 + .../social/connectors/org_role_mapper_test.go | 96 ++++++++----------- 2 files changed, 44 insertions(+), 56 deletions(-) diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index df3cf5d50c072..372202ef35df6 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -144,6 +144,10 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] if getErr != nil { // skip in case of error m.logger.Warn("Could not fetch organization. Skipping.", "err", getErr, "mapping", fmt.Sprintf("%v", v), "org", kv[1]) + if roleStrict { + // Return empty mapping if at least one org name cannot be resolved when roleStrict is enabled + return &MappingConfiguration{orgMapping: map[string]map[int64]org.RoleType{}, strictRoleMapping: roleStrict} + } continue } orgID, err = int(res.ID), nil diff --git a/pkg/login/social/connectors/org_role_mapper_test.go b/pkg/login/social/connectors/org_role_mapper_test.go index 52fa9aa0557b8..f94564402da97 100644 --- a/pkg/login/social/connectors/org_role_mapper_test.go +++ b/pkg/login/social/connectors/org_role_mapper_test.go @@ -51,7 +51,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { expected: map[int64]org.RoleType{1: 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", + name: "should return nil when no org mapping settings are provided and directly mapped role is not set and strict role mapping is enabled", externalOrgs: []string{}, orgMappingSettings: []string{}, directlyMappedRole: "", @@ -59,7 +59,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { expected: nil, }, { - 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", + name: "should return nil when org mapping doesn't match any of the external orgs and no directly mapped role is provided and strict role mapping is enabled", externalOrgs: []string{"First"}, orgMappingSettings: []string{"Second:1:Editor"}, directlyMappedRole: "", @@ -68,7 +68,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { }, // 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", + name: "should return nil for the org if the specified role is invalid and strict role mapping is enabled", externalOrgs: []string{"First"}, orgMappingSettings: []string{"First:1:SuperEditor"}, directlyMappedRole: "", @@ -76,7 +76,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { expected: nil, }, { - name: "should map the direclty mapped role to default org if the org mapping is invalid and role strict is enabled", + name: "should map the direclty mapped role to default org if the org mapping is invalid and strict role mapping is enabled", externalOrgs: []string{"First"}, orgMappingSettings: []string{"First:1:SuperEditor"}, directlyMappedRole: "Editor", @@ -84,7 +84,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { expected: map[int64]org.RoleType{2: org.RoleEditor}, }, { - name: "should return nil if the org mapping contains at least one invalid setting and directly mapped role is empty and role strict is enabled", + name: "should return nil if the org mapping contains at least one invalid setting and directly mapped role is empty and strict role mapping is enabled", externalOrgs: []string{"First"}, orgMappingSettings: []string{"First:1:SuperEditor", "First:1:Admin"}, directlyMappedRole: "", @@ -210,61 +210,12 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { } } -func TestOrgRoleMapper_MapOrgRoles_OrgNameResolution(t *testing.T) { - t.Skip() - testCases := []struct { - name string - orgMapping []string - setupMock func(*orgtest.MockService) - expectedOrgRoles map[int64]org.RoleType - }{ - { - name: "should skip org mapping if org was not found or the resolution fails", - orgMapping: []string{"ExternalOrg1:First:Editor", "ExternalOrg1:NonExistent:Viewer"}, - setupMock: func(orgService *orgtest.MockService) { - orgService.On("GetByName", mock.Anything, mock.MatchedBy(func(query *org.GetOrgByNameQuery) bool { - return query.Name == "First" - })).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}, // TODO: should this be nil or error? - }, - { - 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: nil, - }, - } - - cfg := setting.NewCfg() - cfg.AutoAssignOrg = true - cfg.AutoAssignOrgId = 2 - cfg.AutoAssignOrgRole = string(org.RoleViewer) - - orgService := orgtest.NewMockService(t) - mapper := ProvideOrgRoleMapper(cfg, orgService) - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - orgService.ExpectedCalls = nil - tc.setupMock(orgService) - - mappingCfg := mapper.ParseOrgMappingSettings(context.Background(), tc.orgMapping, false) - actual := mapper.MapOrgRoles(context.Background(), mappingCfg, []string{"ExternalOrg1"}, org.RoleViewer) - - assert.EqualValues(t, tc.expectedOrgRoles, actual) - }) - } -} - func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { testCases := []struct { name string rawMapping []string roleStrict bool + setupMock func(*orgtest.MockService) expected *MappingConfiguration }{ { @@ -326,11 +277,44 @@ func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { strictRoleMapping: false, }, }, + { + name: "should return empty mapping if at least one org was not found or the resolution failed and strict role mapping is enabled", + rawMapping: []string{"ExternalOrg1:First:Editor", "ExternalOrg1:NonExistent:Viewer"}, + roleStrict: true, + setupMock: func(orgService *orgtest.MockService) { + orgService.On("GetByName", mock.Anything, mock.MatchedBy(func(query *org.GetOrgByNameQuery) bool { + return query.Name == "First" + })).Return(&org.Org{ID: 1}, nil) + orgService.On("GetByName", mock.Anything, mock.Anything).Return(nil, assert.AnError) + }, + expected: &MappingConfiguration{ + orgMapping: map[string]map[int64]org.RoleType{}, + strictRoleMapping: true, + }, + }, + { + name: "should skip org mapping if org was not found or the resolution fails and strict role mapping is disabled", + rawMapping: []string{"ExternalOrg1:First:Editor", "ExternalOrg1:NonExistent:Viewer"}, + roleStrict: false, + setupMock: func(orgService *orgtest.MockService) { + orgService.On("GetByName", mock.Anything, mock.MatchedBy(func(query *org.GetOrgByNameQuery) bool { + return query.Name == "First" + })).Return(&org.Org{ID: 1}, nil) + orgService.On("GetByName", mock.Anything, mock.Anything).Return(nil, assert.AnError) + }, + expected: &MappingConfiguration{ + orgMapping: map[string]map[int64]org.RoleType{"ExternalOrg1": {1: org.RoleEditor}}, + strictRoleMapping: false, + }, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { cfg := setting.NewCfg() - orgService := orgtest.NewOrgServiceFake() + orgService := orgtest.NewMockService(t) + if tc.setupMock != nil { + tc.setupMock(orgService) + } mapper := ProvideOrgRoleMapper(cfg, orgService) actual := mapper.ParseOrgMappingSettings(context.Background(), tc.rawMapping, tc.roleStrict) From 86774881920ec45fec40fec4fcc1093f5f73db20 Mon Sep 17 00:00:00 2001 From: Mihaly Gyongyosi Date: Fri, 3 May 2024 12:44:37 +0200 Subject: [PATCH 09/16] lint --- pkg/login/social/connectors/org_role_mapper_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/login/social/connectors/org_role_mapper_test.go b/pkg/login/social/connectors/org_role_mapper_test.go index f94564402da97..3f88b1347ec04 100644 --- a/pkg/login/social/connectors/org_role_mapper_test.go +++ b/pkg/login/social/connectors/org_role_mapper_test.go @@ -76,7 +76,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { expected: nil, }, { - name: "should map the direclty mapped role to default org if the org mapping is invalid and strict role mapping is enabled", + name: "should map the directly mapped role to default org if the org mapping is invalid and strict role mapping is enabled", externalOrgs: []string{"First"}, orgMappingSettings: []string{"First:1:SuperEditor"}, directlyMappedRole: "Editor", From 2a39346dca6f810391ac3854a34e910012271fca Mon Sep 17 00:00:00 2001 From: Misi Date: Fri, 3 May 2024 17:10:04 +0200 Subject: [PATCH 10/16] Apply documentation update suggestions Co-authored-by: Gabriel MABILLE --- pkg/login/social/connectors/org_role_mapper.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index 372202ef35df6..5d7e845854072 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -21,7 +21,7 @@ type OrgRoleMapper struct { // MappingConfiguration represents the mapping configuration from external orgs to Grafana orgs and roles. // orgMapping: mapping from external orgs to Grafana orgs and roles -// strictRoleMapping: if true, the mapper ensuers that the evaluated role from orgMapping or the directlyMappedRole is a valid role, otherwise it will return nil. +// strictRoleMapping: if true, the mapper ensures that the evaluated role from orgMapping or the directlyMappedRole is a valid role, otherwise it will return nil. type MappingConfiguration struct { orgMapping map[string]map[int64]org.RoleType strictRoleMapping bool @@ -39,9 +39,9 @@ func ProvideOrgRoleMapper(cfg *setting.Cfg, orgService org.Service) *OrgRoleMapp // // mappingCfg: mapping configuration from external orgs to Grafana orgs and roles. Use `ParseOrgMappingSettings` to convert the raw setting to this format. // -// externalOrgs: list of external orgs/groups +// externalOrgs: list of orgs/groups from the provider // -// directlyMappedRole: role that is directly mapped to the user +// directlyMappedRole: role that is directly mapped to the user (ex: through `role_attribute_path`) func (m *OrgRoleMapper) MapOrgRoles( ctx context.Context, mappingCfg *MappingConfiguration, From b4b96a1bf5641e454ca4a7d4ff5961515fcdd566 Mon Sep 17 00:00:00 2001 From: Misi Date: Fri, 3 May 2024 17:12:07 +0200 Subject: [PATCH 11/16] Apply GetDefaultOrgMapping suggestions from code review Co-authored-by: Gabriel MABILLE --- .../social/connectors/org_role_mapper.go | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index 5d7e845854072..bb6a4bff99e3f 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -48,9 +48,9 @@ func (m *OrgRoleMapper) MapOrgRoles( externalOrgs []string, directlyMappedRole org.RoleType, ) map[int64]org.RoleType { - if len(mappingCfg.orgMapping) == 0 && !isValidRole(directlyMappedRole) && mappingCfg.strictRoleMapping { - // No org mappings are configured and the directly mapped role is not set and roleStrict is enabled - return nil + if len(mappingCfg.orgMapping) == 0 { + // Org mapping is not configured + return m.GetDefaultOrgMapping(mappingCfg.strictRoleMapping, directlyMappedRole) } userOrgRoles := getMappedOrgRoles(externalOrgs, mappingCfg.orgMapping) @@ -61,13 +61,7 @@ func (m *OrgRoleMapper) MapOrgRoles( } if len(userOrgRoles) == 0 { - if mappingCfg.strictRoleMapping && !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) + return m.GetDefaultOrgMapping(mappingCfg.strictRoleMapping, directlyMappedRole) } if directlyMappedRole == "" { @@ -85,7 +79,11 @@ func (m *OrgRoleMapper) MapOrgRoles( return userOrgRoles } -func (m *OrgRoleMapper) GetDefaultOrgMapping(directlyMappedRole org.RoleType) map[int64]org.RoleType { +func (m *OrgRoleMapper) GetDefaultOrgMapping(strictRoleMapping bool, directlyMappedRole org.RoleType) map[int64]org.RoleType { + if strictRoleMapping && !directlyMappedRole.IsValid() { + m.logger.Debug("Prevent default org role mapping, role attribute strict requested") + return nil + } orgRoles := make(map[int64]org.RoleType, 0) orgID := int64(1) From e471fb337476ff9539ffc84fdbe4dfd9a875f62e Mon Sep 17 00:00:00 2001 From: Misi Date: Fri, 3 May 2024 17:18:55 +0200 Subject: [PATCH 12/16] Apply suggestions for cleaning up unnecessary err in ParseOrgMappingSettings Co-authored-by: Gabriel MABILLE --- .../social/connectors/org_role_mapper.go | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index bb6a4bff99e3f..99cfbd47f6c50 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -148,28 +148,26 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] } continue } - orgID, err = int(res.ID), nil + orgID = int(res.ID) } if kv[1] == "*" { - orgID, err = MapperMatchAllOrgID, nil + orgID = MapperMatchAllOrgID + } + if roleStrict && (len(kv) < 3 || !org.RoleType(kv[2]).IsValid()) { + // Return empty mapping if at least one org mapping is invalid (missing role, invalid role) + m.logger.Warn("Skipping org mapping due to missing or invalid role in mapping with roleStrict is enabled.", "mapping", fmt.Sprintf("%v", v)) + return &MappingConfiguration{orgMapping: map[string]map[int64]org.RoleType{}, strictRoleMapping: roleStrict} } - if err == nil { - if roleStrict && (len(kv) < 3 || !org.RoleType(kv[2]).IsValid()) { - // Return empty mapping if at least one org mapping is invalid (missing role, invalid role) - m.logger.Warn("Skipping org mapping due to missing or invalid role in mapping when roleStrict is enabled.", "mapping", fmt.Sprintf("%v", v)) - return &MappingConfiguration{orgMapping: map[string]map[int64]org.RoleType{}, strictRoleMapping: roleStrict} - } - orga := kv[0] - if res[orga] == nil { - res[orga] = map[int64]org.RoleType{} - } + orga := kv[0] + if res[orga] == nil { + res[orga] = map[int64]org.RoleType{} + } - if len(kv) > 2 && org.RoleType(kv[2]).IsValid() { - res[orga][int64(orgID)] = org.RoleType(kv[2]) - } else { - res[orga][int64(orgID)] = org.RoleViewer - } + if len(kv) > 2 && org.RoleType(kv[2]).IsValid() { + res[orga][int64(orgID)] = org.RoleType(kv[2]) + } else { + res[orga][int64(orgID)] = org.RoleViewer } } } From a174f27b6cefc1e0552b456482120710bbb2efd5 Mon Sep 17 00:00:00 2001 From: Misi Date: Fri, 3 May 2024 17:21:31 +0200 Subject: [PATCH 13/16] Apply suggestions from code review Co-authored-by: Gabriel MABILLE --- pkg/login/social/connectors/org_role_mapper.go | 5 ++--- pkg/login/social/connectors/org_role_mapper_test.go | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index 99cfbd47f6c50..26c8da72d2f1f 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -91,10 +91,9 @@ func (m *OrgRoleMapper) GetDefaultOrgMapping(strictRoleMapping bool, directlyMap orgID = int64(m.cfg.AutoAssignOrgId) } - if directlyMappedRole == "" || !directlyMappedRole.IsValid() { + orgRoles[orgID] = directlyMappedRole + if !directlyMappedRole.IsValid() { orgRoles[orgID] = org.RoleType(m.cfg.AutoAssignOrgRole) - } else { - orgRoles[orgID] = directlyMappedRole } return orgRoles diff --git a/pkg/login/social/connectors/org_role_mapper_test.go b/pkg/login/social/connectors/org_role_mapper_test.go index 3f88b1347ec04..96dbe7001b65e 100644 --- a/pkg/login/social/connectors/org_role_mapper_test.go +++ b/pkg/login/social/connectors/org_role_mapper_test.go @@ -141,7 +141,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { expected: map[int64]org.RoleType{1: org.RoleEditor, 2: org.RoleEditor, 3: org.RoleEditor}, }, { - name: "should map correctly when global org mapping is provided", + name: "should map correctly when fallback org mapping is provided", externalOrgs: []string{"First", "Second", "Third"}, orgMappingSettings: []string{"First:1:Viewer", "*:1:Editor", "Second:2:Viewer"}, directlyMappedRole: "", From eddf871312476f4896bf55e9dcc2666e059f4437 Mon Sep 17 00:00:00 2001 From: Mihaly Gyongyosi Date: Fri, 3 May 2024 17:50:20 +0200 Subject: [PATCH 14/16] Address feedback suggestions --- .../social/connectors/org_role_mapper.go | 23 +++++---- .../social/connectors/org_role_mapper_test.go | 47 +++++++++++++------ 2 files changed, 46 insertions(+), 24 deletions(-) diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index 26c8da72d2f1f..b1c184ed1b48e 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -48,7 +48,7 @@ func (m *OrgRoleMapper) MapOrgRoles( externalOrgs []string, directlyMappedRole org.RoleType, ) map[int64]org.RoleType { - if len(mappingCfg.orgMapping) == 0 { + if len(mappingCfg.orgMapping) == 0 { // Org mapping is not configured return m.GetDefaultOrgMapping(mappingCfg.strictRoleMapping, directlyMappedRole) } @@ -80,10 +80,10 @@ func (m *OrgRoleMapper) MapOrgRoles( } func (m *OrgRoleMapper) GetDefaultOrgMapping(strictRoleMapping bool, directlyMappedRole org.RoleType) map[int64]org.RoleType { - if strictRoleMapping && !directlyMappedRole.IsValid() { + if strictRoleMapping && !directlyMappedRole.IsValid() { m.logger.Debug("Prevent default org role mapping, role attribute strict requested") return nil - } + } orgRoles := make(map[int64]org.RoleType, 0) orgID := int64(1) @@ -133,6 +133,15 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] for _, v := range mappings { kv := strings.Split(v, ":") + if len(kv) > 3 { + // Log and skip the mapping if the format is invalid + m.logger.Error("Skipping org mapping due to invalid format.", "mapping", fmt.Sprintf("%v", v)) + if roleStrict { + // Return empty mapping if the mapping format is invalied and roleStrict is enabled + return &MappingConfiguration{orgMapping: map[string]map[int64]org.RoleType{}, strictRoleMapping: roleStrict} + } + continue + } if len(kv) > 1 { orgID, err := strconv.Atoi(kv[1]) if err != nil && kv[1] != "*" { @@ -152,9 +161,9 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] if kv[1] == "*" { orgID = MapperMatchAllOrgID } - if roleStrict && (len(kv) < 3 || !org.RoleType(kv[2]).IsValid()) { + if roleStrict && (len(kv) < 3 || !org.RoleType(kv[2]).IsValid()) { // Return empty mapping if at least one org mapping is invalid (missing role, invalid role) - m.logger.Warn("Skipping org mapping due to missing or invalid role in mapping with roleStrict is enabled.", "mapping", fmt.Sprintf("%v", v)) + m.logger.Warn("Skipping org mapping due to missing or invalid role in mapping when roleStrict is enabled.", "mapping", fmt.Sprintf("%v", v)) return &MappingConfiguration{orgMapping: map[string]map[int64]org.RoleType{}, strictRoleMapping: roleStrict} } @@ -226,7 +235,3 @@ 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 96dbe7001b65e..a72e35d75a875 100644 --- a/pkg/login/social/connectors/org_role_mapper_test.go +++ b/pkg/login/social/connectors/org_role_mapper_test.go @@ -18,7 +18,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { externalOrgs []string orgMappingSettings []string directlyMappedRole org.RoleType - roleStrict bool + strictRoleMapping bool getAllOrgsError error expected map[int64]org.RoleType }{ @@ -55,7 +55,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { externalOrgs: []string{}, orgMappingSettings: []string{}, directlyMappedRole: "", - roleStrict: true, + strictRoleMapping: true, expected: nil, }, { @@ -63,7 +63,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { externalOrgs: []string{"First"}, orgMappingSettings: []string{"Second:1:Editor"}, directlyMappedRole: "", - roleStrict: true, + strictRoleMapping: true, expected: nil, }, // In this case the parsed org mapping will be empty because the role is invalid @@ -72,7 +72,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { externalOrgs: []string{"First"}, orgMappingSettings: []string{"First:1:SuperEditor"}, directlyMappedRole: "", - roleStrict: true, + strictRoleMapping: true, expected: nil, }, { @@ -80,7 +80,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { externalOrgs: []string{"First"}, orgMappingSettings: []string{"First:1:SuperEditor"}, directlyMappedRole: "Editor", - roleStrict: true, + strictRoleMapping: true, expected: map[int64]org.RoleType{2: org.RoleEditor}, }, { @@ -88,7 +88,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { externalOrgs: []string{"First"}, orgMappingSettings: []string{"First:1:SuperEditor", "First:1:Admin"}, directlyMappedRole: "", - roleStrict: true, + strictRoleMapping: true, expected: nil, }, { @@ -141,12 +141,19 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { expected: map[int64]org.RoleType{1: org.RoleEditor, 2: org.RoleEditor, 3: org.RoleEditor}, }, { - name: "should map correctly when fallback org mapping is provided", + name: "should map correctly when fallback org mapping is provided and fallback has a higher role", externalOrgs: []string{"First", "Second", "Third"}, orgMappingSettings: []string{"First:1:Viewer", "*:1:Editor", "Second:2:Viewer"}, directlyMappedRole: "", expected: map[int64]org.RoleType{1: org.RoleEditor, 2: org.RoleViewer}, }, + { + name: "should map correctly when fallback org mapping is provided and fallback has a lower role", + externalOrgs: []string{"First", "Second", "Third"}, + orgMappingSettings: []string{"First:1:Editor", "*:1:Viewer", "Second:2:Viewer"}, + directlyMappedRole: "", + expected: map[int64]org.RoleType{1: org.RoleEditor, 2: org.RoleViewer}, + }, { name: "should map correctly and respect wildcard precedence when global org mapping is provided", externalOrgs: []string{"First", "Second"}, @@ -202,7 +209,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { {Name: "Third", ID: 3}, } } - mappingCfg := mapper.ParseOrgMappingSettings(context.Background(), tc.orgMappingSettings, tc.roleStrict) + mappingCfg := mapper.ParseOrgMappingSettings(context.Background(), tc.orgMappingSettings, tc.strictRoleMapping) actual := mapper.MapOrgRoles(context.Background(), mappingCfg, tc.externalOrgs, tc.directlyMappedRole) assert.EqualValues(t, tc.expected, actual) @@ -236,7 +243,7 @@ func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { }, }, { - name: "should return default mapping when role is invalid and role strict is disabled", + name: "should return default mapping when role is invalid and strict role mapping is disabled", rawMapping: []string{"Second:1:SuperEditor"}, roleStrict: false, expected: &MappingConfiguration{ @@ -245,7 +252,7 @@ func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { }, }, { - name: "should return empty mapping when org mapping doesn't contain the role and strict is enabled", + name: "should return empty mapping when org mapping doesn't contain any role and strict role mapping is enabled", rawMapping: []string{"Second:1"}, roleStrict: true, expected: &MappingConfiguration{ @@ -254,7 +261,7 @@ func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { }, }, { - name: "should return empty mapping when org mapping doesn't contain the role and strict is disabled", + name: "should return default mapping when org mapping doesn't contain any role and strict is disabled", rawMapping: []string{"Second:1"}, expected: &MappingConfiguration{ orgMapping: map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, @@ -262,18 +269,28 @@ func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { }, }, { - name: "should return empty mapping when org mapping is empty", - rawMapping: []string{}, + name: "should return empty mapping when org mapping is nil", + rawMapping: nil, expected: &MappingConfiguration{ orgMapping: map[string]map[int64]org.RoleType{}, strictRoleMapping: false, }, }, { - name: "should return empty mapping when org mapping is nil", - rawMapping: nil, + name: "should return empty mapping when the org mapping format is invalid and strict role mapping is enabled", + rawMapping: []string{"External:Org1:First:Organization:Editor"}, + roleStrict: true, expected: &MappingConfiguration{ orgMapping: map[string]map[int64]org.RoleType{}, + strictRoleMapping: true, + }, + }, + { + name: "should return only the valid mappings from the raw mappings when strict role mapping is disabled", + rawMapping: []string{"External:Org1:First:Organization:Editor", "Second:1:Editor"}, + roleStrict: false, + expected: &MappingConfiguration{ + orgMapping: map[string]map[int64]org.RoleType{"Second": {1: org.RoleEditor}}, strictRoleMapping: false, }, }, From 6c5ebe0a0250bd2791b929483c936615b5a84cd0 Mon Sep 17 00:00:00 2001 From: Mihaly Gyongyosi Date: Fri, 3 May 2024 18:00:37 +0200 Subject: [PATCH 15/16] Cleanup --- pkg/login/social/connectors/org_role_mapper.go | 18 +++++++++--------- .../social/connectors/org_role_mapper_test.go | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index b1c184ed1b48e..c8a8770dc00be 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -11,8 +11,9 @@ import ( "github.com/grafana/grafana/pkg/setting" ) -const MapperMatchAllOrgID = -1 +const mapperMatchAllOrgID = -1 +// OrgRoleMapper maps external orgs/groups to Grafana orgs and basic roles. type OrgRoleMapper struct { cfg *setting.Cfg logger log.Logger @@ -43,14 +44,13 @@ func ProvideOrgRoleMapper(cfg *setting.Cfg, orgService org.Service) *OrgRoleMapp // // directlyMappedRole: role that is directly mapped to the user (ex: through `role_attribute_path`) func (m *OrgRoleMapper) MapOrgRoles( - ctx context.Context, mappingCfg *MappingConfiguration, externalOrgs []string, directlyMappedRole org.RoleType, ) map[int64]org.RoleType { if len(mappingCfg.orgMapping) == 0 { // Org mapping is not configured - return m.GetDefaultOrgMapping(mappingCfg.strictRoleMapping, directlyMappedRole) + return m.getDefaultOrgMapping(mappingCfg.strictRoleMapping, directlyMappedRole) } userOrgRoles := getMappedOrgRoles(externalOrgs, mappingCfg.orgMapping) @@ -61,7 +61,7 @@ func (m *OrgRoleMapper) MapOrgRoles( } if len(userOrgRoles) == 0 { - return m.GetDefaultOrgMapping(mappingCfg.strictRoleMapping, directlyMappedRole) + return m.getDefaultOrgMapping(mappingCfg.strictRoleMapping, directlyMappedRole) } if directlyMappedRole == "" { @@ -79,7 +79,7 @@ func (m *OrgRoleMapper) MapOrgRoles( return userOrgRoles } -func (m *OrgRoleMapper) GetDefaultOrgMapping(strictRoleMapping bool, directlyMappedRole org.RoleType) map[int64]org.RoleType { +func (m *OrgRoleMapper) getDefaultOrgMapping(strictRoleMapping bool, directlyMappedRole org.RoleType) map[int64]org.RoleType { if strictRoleMapping && !directlyMappedRole.IsValid() { m.logger.Debug("Prevent default org role mapping, role attribute strict requested") return nil @@ -101,7 +101,7 @@ func (m *OrgRoleMapper) GetDefaultOrgMapping(strictRoleMapping bool, directlyMap func (m *OrgRoleMapper) handleGlobalOrgMapping(orgRoles map[int64]org.RoleType) error { // No global role mapping => return - globalRole, ok := orgRoles[MapperMatchAllOrgID] + globalRole, ok := orgRoles[mapperMatchAllOrgID] if !ok { return nil } @@ -115,7 +115,7 @@ func (m *OrgRoleMapper) handleGlobalOrgMapping(orgRoles map[int64]org.RoleType) } // Remove the global role mapping - delete(orgRoles, MapperMatchAllOrgID) + delete(orgRoles, mapperMatchAllOrgID) // Global mapping => for all orgs get top role mapping for orgID := range allOrgIDs { @@ -125,9 +125,9 @@ func (m *OrgRoleMapper) handleGlobalOrgMapping(orgRoles map[int64]org.RoleType) return nil } -// 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. // If the roleStrict is enabled, the mapping should contain a valid role for each org. +// FIXME: Consider introducing a struct to represent the org mapping settings func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings []string, roleStrict bool) *MappingConfiguration { res := map[string]map[int64]org.RoleType{} @@ -159,7 +159,7 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] orgID = int(res.ID) } if kv[1] == "*" { - orgID = MapperMatchAllOrgID + orgID = mapperMatchAllOrgID } if roleStrict && (len(kv) < 3 || !org.RoleType(kv[2]).IsValid()) { // Return empty mapping if at least one org mapping is invalid (missing role, invalid role) diff --git a/pkg/login/social/connectors/org_role_mapper_test.go b/pkg/login/social/connectors/org_role_mapper_test.go index a72e35d75a875..75aebe596be5d 100644 --- a/pkg/login/social/connectors/org_role_mapper_test.go +++ b/pkg/login/social/connectors/org_role_mapper_test.go @@ -210,7 +210,7 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { } } mappingCfg := mapper.ParseOrgMappingSettings(context.Background(), tc.orgMappingSettings, tc.strictRoleMapping) - actual := mapper.MapOrgRoles(context.Background(), mappingCfg, tc.externalOrgs, tc.directlyMappedRole) + actual := mapper.MapOrgRoles(mappingCfg, tc.externalOrgs, tc.directlyMappedRole) assert.EqualValues(t, tc.expected, actual) }) From 7c8748629b830afd90df435521b711f8fbda77e7 Mon Sep 17 00:00:00 2001 From: Mihaly Gyongyosi Date: Fri, 3 May 2024 19:13:12 +0200 Subject: [PATCH 16/16] Reduce cognitive complexity of ParseOrgMappingSettings --- .../social/connectors/org_role_mapper.go | 84 +++++++++++-------- 1 file changed, 51 insertions(+), 33 deletions(-) diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index c8a8770dc00be..bb3d7974cb788 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -133,8 +133,7 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] for _, v := range mappings { kv := strings.Split(v, ":") - if len(kv) > 3 { - // Log and skip the mapping if the format is invalid + if !isValidOrgMappingFormat(kv) { m.logger.Error("Skipping org mapping due to invalid format.", "mapping", fmt.Sprintf("%v", v)) if roleStrict { // Return empty mapping if the mapping format is invalied and roleStrict is enabled @@ -142,47 +141,54 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] } continue } - if len(kv) > 1 { - orgID, err := strconv.Atoi(kv[1]) - if err != nil && kv[1] != "*" { - res, getErr := m.orgService.GetByName(ctx, &org.GetOrgByNameQuery{Name: kv[1]}) - - if getErr != nil { - // skip in case of error - m.logger.Warn("Could not fetch organization. Skipping.", "err", getErr, "mapping", fmt.Sprintf("%v", v), "org", kv[1]) - if roleStrict { - // Return empty mapping if at least one org name cannot be resolved when roleStrict is enabled - return &MappingConfiguration{orgMapping: map[string]map[int64]org.RoleType{}, strictRoleMapping: roleStrict} - } - continue - } - orgID = int(res.ID) - } - if kv[1] == "*" { - orgID = mapperMatchAllOrgID - } - if roleStrict && (len(kv) < 3 || !org.RoleType(kv[2]).IsValid()) { - // Return empty mapping if at least one org mapping is invalid (missing role, invalid role) - m.logger.Warn("Skipping org mapping due to missing or invalid role in mapping when roleStrict is enabled.", "mapping", fmt.Sprintf("%v", v)) + + orgID, err := m.getOrgIDForInternalMapping(ctx, kv[1]) + if err != nil { + m.logger.Warn("Could not fetch OrgID. Skipping.", "err", err, "mapping", fmt.Sprintf("%v", v), "org", kv[1]) + if roleStrict { + // Return empty mapping if at least one org name cannot be resolved when roleStrict is enabled return &MappingConfiguration{orgMapping: map[string]map[int64]org.RoleType{}, strictRoleMapping: roleStrict} } + continue + } - orga := kv[0] - if res[orga] == nil { - res[orga] = map[int64]org.RoleType{} - } + if roleStrict && (len(kv) < 3 || !org.RoleType(kv[2]).IsValid()) { + // Return empty mapping if at least one org mapping is invalid (missing role, invalid role) + m.logger.Warn("Skipping org mapping due to missing or invalid role in mapping when roleStrict is enabled.", "mapping", fmt.Sprintf("%v", v)) + return &MappingConfiguration{orgMapping: map[string]map[int64]org.RoleType{}, strictRoleMapping: roleStrict} + } - if len(kv) > 2 && org.RoleType(kv[2]).IsValid() { - res[orga][int64(orgID)] = org.RoleType(kv[2]) - } else { - res[orga][int64(orgID)] = org.RoleViewer - } + orga := kv[0] + if res[orga] == nil { + res[orga] = map[int64]org.RoleType{} } + + res[orga][int64(orgID)] = getRoleForInternalOrgMapping(kv) } return &MappingConfiguration{orgMapping: res, strictRoleMapping: roleStrict} } +func (m *OrgRoleMapper) getOrgIDForInternalMapping(ctx context.Context, orgIdCfg string) (int, error) { + if orgIdCfg == "*" { + return mapperMatchAllOrgID, nil + } + + orgID, err := strconv.Atoi(orgIdCfg) + if err != nil { + res, getErr := m.orgService.GetByName(ctx, &org.GetOrgByNameQuery{Name: orgIdCfg}) + + if getErr != nil { + // skip in case of error + m.logger.Warn("Could not fetch organization. Skipping.", "err", err, "org", orgIdCfg) + return 0, getErr + } + orgID = int(res.ID) + } + + return orgID, nil +} + func (m *OrgRoleMapper) getAllOrgs() (map[int64]bool, error) { allOrgIDs := map[int64]bool{} allOrgs, err := m.orgService.Search(context.Background(), &org.SearchOrgsQuery{}) @@ -197,6 +203,18 @@ func (m *OrgRoleMapper) getAllOrgs() (map[int64]bool, error) { return allOrgIDs, nil } +func getRoleForInternalOrgMapping(kv []string) org.RoleType { + if len(kv) > 2 && org.RoleType(kv[2]).IsValid() { + return org.RoleType(kv[2]) + } + + return org.RoleViewer +} + +func isValidOrgMappingFormat(kv []string) bool { + return len(kv) > 1 && len(kv) < 4 +} + func getMappedOrgRoles(externalOrgs []string, orgMapping map[string]map[int64]org.RoleType) map[int64]org.RoleType { userOrgRoles := map[int64]org.RoleType{}