Chore: Extend OrgRoleMapper (#97336)

* Extend OrgRoleMapper

* Cover mappingCfg eq nil

* Use struct instead of a pointer in MapOrgRoles

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 (
-var _ ssosettings.Reloadable = (*SocialGrafanaCom)(nil)
+	_ 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) {

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})

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{
+			expected:   NewMappingConfiguration(map[string]map[int64]org.RoleType{}, false),
-				orgMapping:        map[string]map[int64]org.RoleType{},
-				strictRoleMapping: false,
-			},
 		},
 		{
 			name:       "should return empty mapping when role is invalid and role strict is enabled",
 			rawMapping: []string{"Second:1:SuperEditor"},
 			roleStrict: true,
-			expected: &MappingConfiguration{
+			expected:   NewMappingConfiguration(map[string]map[int64]org.RoleType{}, true),
-				orgMapping:        map[string]map[int64]org.RoleType{},
+		},
-				strictRoleMapping: 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{
+			expected:   NewMappingConfiguration(map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, false),
-				orgMapping:        map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}},
-				strictRoleMapping: 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{
+			expected:   NewMappingConfiguration(map[string]map[int64]org.RoleType{}, true),
-				orgMapping:        map[string]map[int64]org.RoleType{},
-				strictRoleMapping: true,
-			},
 		},
 		{
 			name:       "should return default mapping when org mapping doesn't contain any role and strict is disabled",
 			rawMapping: []string{"Second:1"},
-			expected: &MappingConfiguration{
+			expected:   NewMappingConfiguration(map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}}, false),
-				orgMapping:        map[string]map[int64]org.RoleType{"Second": {1: org.RoleViewer}},
-				strictRoleMapping: false,
-			},
 		},
 		{
 			name:       "should return correct mapping when the first part contains multiple colons",
 			rawMapping: []string{"Groups\\:IT\\:ops:1:Viewer"},
 			roleStrict: false,
-			expected: &MappingConfiguration{
+			expected:   NewMappingConfiguration(map[string]map[int64]org.RoleType{"Groups:IT:ops": {1: org.RoleViewer}}, false),
-				orgMapping:        map[string]map[int64]org.RoleType{"Groups:IT:ops": {1: org.RoleViewer}},
-				strictRoleMapping: 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{
+			expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{"Group1": {1: org.RoleViewer}}, false),
-				orgMapping:        map[string]map[int64]org.RoleType{"Group1": {1: org.RoleViewer}},
-				strictRoleMapping: false,
-			},
 		},
 		{
 			name:       "should return empty mapping when org mapping is nil",
 			rawMapping: nil,
-			expected: &MappingConfiguration{
+			expected:   NewMappingConfiguration(map[string]map[int64]org.RoleType{}, false),
-				orgMapping:        map[string]map[int64]org.RoleType{},
-				strictRoleMapping: 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{
+			expected:   NewMappingConfiguration(map[string]map[int64]org.RoleType{}, true),
-				orgMapping:        map[string]map[int64]org.RoleType{},
-				strictRoleMapping: 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{
+			expected:   NewMappingConfiguration(map[string]map[int64]org.RoleType{"Second": {1: org.RoleAdmin}}, false),
-				orgMapping:        map[string]map[int64]org.RoleType{"Second": {1: org.RoleAdmin}},
-				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",
@@ -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{
+			expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{}, true),
-				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",
@@ -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{
+			expected: NewMappingConfiguration(map[string]map[int64]org.RoleType{"ExternalOrg1": {1: org.RoleEditor}}, false),
-				orgMapping:        map[string]map[int64]org.RoleType{"ExternalOrg1": {1: org.RoleEditor}},
-				strictRoleMapping: false,
-			},
 		},
 	}
 	for _, tc := range testCases {

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)