diff --git a/pkg/login/social/connectors/grafana_com_oauth.go b/pkg/login/social/connectors/grafana_com_oauth.go index 1ded4a18f7b..3f016b1f01c 100644 --- a/pkg/login/social/connectors/grafana_com_oauth.go +++ b/pkg/login/social/connectors/grafana_com_oauth.go @@ -11,6 +11,7 @@ import ( "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/login/social" "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/ssosettings" ssoModels "github.com/grafana/grafana/pkg/services/ssosettings/models" "github.com/grafana/grafana/pkg/services/ssosettings/validation" @@ -22,8 +23,10 @@ var ExtraGrafanaComSettingKeys = map[string]ExtraKeyInfo{ allowedOrganizationsKey: {Type: String, DefaultValue: ""}, } -var _ social.SocialConnector = (*SocialGrafanaCom)(nil) -var _ ssosettings.Reloadable = (*SocialGrafanaCom)(nil) +var ( + _ social.SocialConnector = (*SocialGrafanaCom)(nil) + _ ssosettings.Reloadable = (*SocialGrafanaCom)(nil) +) type SocialGrafanaCom struct { *SocialBase @@ -133,7 +136,6 @@ func (s *SocialGrafanaCom) UserInfo(ctx context.Context, client *http.Client, _ } response, err := s.httpGet(ctx, client, s.url+"/api/oauth2/user") - if err != nil { return nil, fmt.Errorf("Error getting user info: %s", err) } @@ -151,7 +153,7 @@ func (s *SocialGrafanaCom) UserInfo(ctx context.Context, client *http.Client, _ } if !s.info.SkipOrgRoleSync { - userInfo.OrgRoles = s.orgRoleMapper.MapOrgRoles(&MappingConfiguration{strictRoleMapping: false}, nil, identity.RoleType(data.Role)) + userInfo.OrgRoles = s.orgRoleMapper.MapOrgRoles(NewMappingConfiguration(map[string]map[int64]org.RoleType{}, false), nil, identity.RoleType(data.Role)) } if !s.isOrganizationMember(data.Orgs) { diff --git a/pkg/login/social/connectors/org_role_mapper.go b/pkg/login/social/connectors/org_role_mapper.go index dac4157ea4f..0e0a317b644 100644 --- a/pkg/login/social/connectors/org_role_mapper.go +++ b/pkg/login/social/connectors/org_role_mapper.go @@ -34,6 +34,13 @@ type MappingConfiguration struct { strictRoleMapping bool } +func NewMappingConfiguration(orgMapping map[string]map[int64]org.RoleType, strictRoleMapping bool) MappingConfiguration { + return MappingConfiguration{ + orgMapping, + strictRoleMapping, + } +} + func ProvideOrgRoleMapper(cfg *setting.Cfg, orgService org.Service) *OrgRoleMapper { return &OrgRoleMapper{ cfg: cfg, @@ -50,7 +57,7 @@ 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( - mappingCfg *MappingConfiguration, + mappingCfg MappingConfiguration, externalOrgs []string, directlyMappedRole org.RoleType, ) map[int64]org.RoleType { @@ -131,7 +138,7 @@ func (m *OrgRoleMapper) handleGlobalOrgMapping(orgRoles map[int64]org.RoleType) // 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 { +func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings []string, roleStrict bool) MappingConfiguration { res := map[string]map[int64]org.RoleType{} for _, v := range mappings { @@ -140,7 +147,7 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] 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} + return NewMappingConfiguration(map[string]map[int64]org.RoleType{}, roleStrict) } continue } @@ -150,7 +157,7 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] 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} + return NewMappingConfiguration(map[string]map[int64]org.RoleType{}, roleStrict) } continue } @@ -158,7 +165,7 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] 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} + return NewMappingConfiguration(map[string]map[int64]org.RoleType{}, roleStrict) } orga := kv[0] @@ -169,7 +176,7 @@ func (m *OrgRoleMapper) ParseOrgMappingSettings(ctx context.Context, mappings [] res[orga][int64(orgID)] = getRoleForInternalOrgMapping(kv) } - return &MappingConfiguration{orgMapping: res, strictRoleMapping: roleStrict} + return NewMappingConfiguration(res, roleStrict) } func (m *OrgRoleMapper) getOrgIDForInternalMapping(ctx context.Context, orgIdCfg string) (int, error) { @@ -177,6 +184,10 @@ func (m *OrgRoleMapper) getOrgIDForInternalMapping(ctx context.Context, orgIdCfg return mapperMatchAllOrgID, nil } + if orgIdCfg == "" { + return 0, fmt.Errorf("the org name or id is empty") + } + orgID, err := strconv.Atoi(orgIdCfg) if err != nil { res, getErr := m.orgService.GetByName(ctx, &org.GetOrgByNameQuery{Name: orgIdCfg}) diff --git a/pkg/login/social/connectors/org_role_mapper_test.go b/pkg/login/social/connectors/org_role_mapper_test.go index 3c9760e5d4a..7d2e4b5f26b 100644 --- a/pkg/login/social/connectors/org_role_mapper_test.go +++ b/pkg/login/social/connectors/org_role_mapper_test.go @@ -91,6 +91,14 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { strictRoleMapping: true, expected: nil, }, + { + name: "should map the directly mapped role if the org mapping contains at least one invalid setting and strict role mapping is disabled", + externalOrgs: []string{"First"}, + orgMappingSettings: []string{"First:1", "Second:"}, + directlyMappedRole: "Editor", + strictRoleMapping: false, + expected: map[int64]org.RoleType{1: 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"}, @@ -217,65 +225,66 @@ func TestOrgRoleMapper_MapOrgRoles(t *testing.T) { } } +func TestOrgRoleMapper_MapOrgRoles_ReturnsDefaultOnNilMapping(t *testing.T) { + orgService := orgtest.NewOrgServiceFake() + cfg := setting.NewCfg() + cfg.AutoAssignOrg = true + cfg.AutoAssignOrgId = 2 + cfg.AutoAssignOrgRole = string(org.RoleViewer) + mapper := ProvideOrgRoleMapper(cfg, orgService) + + actual := mapper.MapOrgRoles(NewMappingConfiguration(map[string]map[int64]org.RoleType{}, false), []string{"First"}, org.RoleNone) + + assert.EqualValues(t, map[int64]org.RoleType{2: org.RoleNone}, actual) +} + func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { testCases := []struct { name string rawMapping []string roleStrict bool setupMock func(*orgtest.MockService) - expected *MappingConfiguration + expected MappingConfiguration }{ { name: "should return empty mapping when no org mapping settings are provided", rawMapping: []string{}, - expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{}, - strictRoleMapping: false, - }, + expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{}, false), }, { name: "should return empty mapping when role is invalid and role strict is enabled", rawMapping: []string{"Second:1:SuperEditor"}, roleStrict: true, - expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{}, - strictRoleMapping: true, - }, + expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{}, true), + }, + { + name: "should return correct mapping when org mapping part is missing from a mapping and role strict is disabled", + rawMapping: []string{"Second:1", "First:"}, + roleStrict: false, + expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, false), }, { name: "should return default mapping when role is invalid and strict role mapping is disabled", rawMapping: []string{"Second:1:SuperEditor"}, roleStrict: false, - expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, - strictRoleMapping: false, - }, + expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, false), }, { 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{ - orgMapping: map[string]map[int64]org.RoleType{}, - strictRoleMapping: true, - }, + expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{}, true), }, { 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}}, - strictRoleMapping: false, - }, + expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, false), }, { name: "should return correct mapping when the first part contains multiple colons", rawMapping: []string{"Groups\\:IT\\:ops:1:Viewer"}, roleStrict: false, - expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{"Groups:IT:ops": {1: org.RoleViewer}}, - strictRoleMapping: false, - }, + expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{"Groups:IT:ops": {1: org.RoleViewer}}, false), }, { name: "should return correct mapping when the org name contains multiple colons", @@ -286,36 +295,24 @@ func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { return query.Name == "Org:1" })).Return(&org.Org{ID: 1}, nil) }, - expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{"Group1": {1: org.RoleViewer}}, - strictRoleMapping: false, - }, + expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{"Group1": {1: org.RoleViewer}}, false), }, { name: "should return empty mapping when org mapping is nil", rawMapping: nil, - expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{}, - strictRoleMapping: false, - }, + expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{}, false), }, { name: "should return empty mapping when one of the org mappings are not in the correct format and strict role mapping is enabled", rawMapping: []string{"Second:Group:1:SuperEditor", "Second:1:Viewer"}, roleStrict: true, - expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{}, - strictRoleMapping: true, - }, + expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{}, true), }, { name: "should skip org mapping when one of the org mappings are not in the correct format and strict role mapping is enabled", rawMapping: []string{"Second:Group:1:SuperEditor", "Second:1:Admin"}, roleStrict: false, - expected: &MappingConfiguration{ - orgMapping: map[string]map[int64]org.RoleType{"Second": {1: org.RoleAdmin}}, - strictRoleMapping: false, - }, + expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{"Second": {1: org.RoleAdmin}}, false), }, { name: "should return empty mapping if at least one org was not found or the resolution failed and strict role mapping is enabled", @@ -327,10 +324,7 @@ func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { })).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, - }, + expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{}, true), }, { name: "should skip org mapping if org was not found or the resolution fails and strict role mapping is disabled", @@ -342,10 +336,7 @@ func TestOrgRoleMapper_ParseOrgMappingSettings(t *testing.T) { })).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, - }, + expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{"ExternalOrg1": {1: org.RoleEditor}}, false), }, } for _, tc := range testCases { diff --git a/pkg/login/social/connectors/social_base.go b/pkg/login/social/connectors/social_base.go index 46410c985eb..24e76d0c5fd 100644 --- a/pkg/login/social/connectors/social_base.go +++ b/pkg/login/social/connectors/social_base.go @@ -35,7 +35,7 @@ type SocialBase struct { log log.Logger features featuremgmt.FeatureToggles orgRoleMapper *OrgRoleMapper - orgMappingCfg *MappingConfiguration + orgMappingCfg MappingConfiguration } func newSocialBase(name string, @@ -43,7 +43,6 @@ func newSocialBase(name string, info *social.OAuthInfo, features featuremgmt.FeatureToggles, cfg *setting.Cfg, - ) *SocialBase { logger := log.New("oauth." + name)