Address feedback, add more tests

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

pkg/login/social/connectors/org_role_mapper_test.go

@@ -5,19 +5,21 @@ import (
 	"testing"
 
 	"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"
 	"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,23 +134,82 @@ 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
 	cfg.AutoAssignOrgRole = string(org.RoleViewer)
 	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)
+	}
+}

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)