Skip to content

Commit

Permalink
Introduce strict role parsing/mapping
Browse files Browse the repository at this point in the history
  • Loading branch information
@mgyongyosi
mgyongyosi committed May 2, 2024
1 parent e606e95 commit 0438541
Show file tree
Hide file tree
Showing 2 changed files with 146 additions and 24 deletions.
32 changes: 28 additions & 4 deletions pkg/login/social/connectors/org_role_mapper.go
View file
Expand Up @@ -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)
Expand All @@ -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 m.GetDefaultOrgMapping(directlyMappedRole)
}

Expand Down Expand Up @@ -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 {
Expand All @@ -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{}
Expand Down Expand Up @@ -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()
}
138 changes: 118 additions & 20 deletions pkg/login/social/connectors/org_role_mapper_test.go
View file
Expand Up @@ -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
}{
Expand All @@ -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{},
Expand All @@ -36,19 +45,44 @@ 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"},
orgMappingSettings: []string{"First:1:SuperEditor"},
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"},
Expand Down Expand Up @@ -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
Expand All @@ -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,
},
}

Expand All @@ -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)
})
}
}

0 comments on commit 0438541

Please sign in to comment.