Skip to content

Commit

Permalink
Handle other edge case
Browse files Browse the repository at this point in the history
  • Loading branch information
@mgyongyosi
mgyongyosi committed May 3, 2024
1 parent d8bce8e commit de1d94b
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 38 deletions.
23 changes: 12 additions & 11 deletions pkg/login/social/connectors/org_role_mapper.go
View file
Expand Up @@ -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 {
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down Expand Up @@ -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]
Expand All @@ -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) {
Expand Down
62 changes: 35 additions & 27 deletions pkg/login/social/connectors/org_role_mapper_test.go
View file
Expand Up @@ -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{},
Expand All @@ -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
{
Expand All @@ -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"},
Expand Down Expand Up @@ -263,59 +271,59 @@ 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,
},
},
{
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{},
roleStrict: true,
orgMapping: map[string]map[int64]org.RoleType{},
strictRoleMapping: true,
},
},
{
name: "should return default mapping when role is invalid and role strict is disabled",
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,
},
},
{
name: "should return empty mapping when org mapping doesn't contain the role and strict is enabled",
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,
},
},
}
Expand Down

0 comments on commit de1d94b

Please sign in to comment.