From 3df0611f81114268c706d1ae2ade9c761f71e35f Mon Sep 17 00:00:00 2001 From: Gabriel MABILLE Date: Thu, 1 Feb 2024 12:37:01 +0100 Subject: [PATCH] RBAC: Fix authorize in org (#81552) * RBAC: Fix authorize in org * Implement option 2 * Fix typo * Fix alerting test * Add test to cover the not member case --- .../accesscontrol/acimpl/accesscontrol.go | 12 +++++++--- .../accesscontrol/authorize_in_org_test.go | 18 +++++++++++++++ pkg/services/accesscontrol/middleware.go | 23 +++++++++++++------ pkg/services/accesscontrol/models.go | 1 + pkg/services/auth/identity/requester.go | 2 ++ pkg/services/authn/identity.go | 14 +++++++++++ pkg/services/user/identity.go | 17 ++++++++++++++ .../api/alerting/api_backtesting_test.go | 4 +++- 8 files changed, 80 insertions(+), 11 deletions(-) diff --git a/pkg/services/accesscontrol/acimpl/accesscontrol.go b/pkg/services/accesscontrol/acimpl/accesscontrol.go index f99e4edbe82..f5d8774292d 100644 --- a/pkg/services/accesscontrol/acimpl/accesscontrol.go +++ b/pkg/services/accesscontrol/acimpl/accesscontrol.go @@ -39,13 +39,19 @@ func (a *AccessControl) Evaluate(ctx context.Context, user identity.Requester, e } namespace, identifier := user.GetNamespacedID() - if len(user.GetPermissions()) == 0 { + + // If the user is in no organization, then the evaluation must happen based on the user's global permissions + permissions := user.GetPermissions() + if user.GetOrgID() == accesscontrol.NoOrgID { + permissions = user.GetGlobalPermissions() + } + if len(permissions) == 0 { a.log.Debug("No permissions set for entity", "namespace", namespace, "id", identifier, "orgID", user.GetOrgID(), "login", user.GetLogin()) return false, nil } // Test evaluation without scope resolver first, this will prevent 403 for wildcard scopes when resource does not exist - if evaluator.Evaluate(user.GetPermissions()) { + if evaluator.Evaluate(permissions) { return true, nil } @@ -57,7 +63,7 @@ func (a *AccessControl) Evaluate(ctx context.Context, user identity.Requester, e return false, err } - return resolvedEvaluator.Evaluate(user.GetPermissions()), nil + return resolvedEvaluator.Evaluate(permissions), nil } func (a *AccessControl) RegisterScopeAttributeResolver(prefix string, resolver accesscontrol.ScopeAttributeResolver) { diff --git a/pkg/services/accesscontrol/authorize_in_org_test.go b/pkg/services/accesscontrol/authorize_in_org_test.go index 812f56634fa..8a9f6d2b814 100644 --- a/pkg/services/accesscontrol/authorize_in_org_test.go +++ b/pkg/services/accesscontrol/authorize_in_org_test.go @@ -202,6 +202,24 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) { teamService: &teamtest.FakeService{}, expectedStatus: http.StatusForbidden, }, + { + name: "should fetch global user permissions when user is not a member of the target org", + orgIDGetter: func(c *contextmodel.ReqContext) (int64, error) { + return 2, nil + }, + evaluator: accesscontrol.EvalPermission("users:read", "users:*"), + accessControl: ac, + acService: &actest.FakeService{ + ExpectedPermissions: []accesscontrol.Permission{{Action: "users:read", Scope: "users:*"}}, + }, + userCache: &usertest.FakeUserService{ + GetSignedInUserFn: func(ctx context.Context, query *user.GetSignedInUserQuery) (*user.SignedInUser, error) { + return &user.SignedInUser{UserID: 1, OrgID: -1, Permissions: map[int64]map[string][]string{}}, nil + }, + }, + ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:write": {"users:*"}}}}, + expectedStatus: http.StatusOK, + }, } // Run test cases diff --git a/pkg/services/accesscontrol/middleware.go b/pkg/services/accesscontrol/middleware.go index 6c76060542d..5ef36a8a216 100644 --- a/pkg/services/accesscontrol/middleware.go +++ b/pkg/services/accesscontrol/middleware.go @@ -260,23 +260,32 @@ func makeTmpUser(ctx context.Context, service Service, cache userCache, tmpUser.OrgName = queryResult.OrgName tmpUser.OrgRole = queryResult.OrgRole - if teamService != nil { - teamIDs, err := teamService.GetTeamIDsByUser(ctx, &team.GetTeamIDsByUserQuery{OrgID: targetOrgID, UserID: tmpUser.UserID}) - if err != nil { - return nil, err + // Only fetch the team membership is the user is a member of the organization + if queryResult.OrgID == targetOrgID { + if teamService != nil { + teamIDs, err := teamService.GetTeamIDsByUser(ctx, &team.GetTeamIDsByUserQuery{OrgID: targetOrgID, UserID: tmpUser.UserID}) + if err != nil { + return nil, err + } + tmpUser.Teams = teamIDs } - tmpUser.Teams = teamIDs } } } - if tmpUser.Permissions[targetOrgID] == nil || len(tmpUser.Permissions[targetOrgID]) == 0 { + // If the user is not a member of the organization + // evaluation must happen based on global permissions. + evaluationOrg := targetOrgID + if tmpUser.OrgID == NoOrgID { + evaluationOrg = GlobalOrgID + } + if tmpUser.Permissions[evaluationOrg] == nil || len(tmpUser.Permissions[evaluationOrg]) == 0 { permissions, err := service.GetUserPermissions(ctx, tmpUser, Options{}) if err != nil { return nil, err } - tmpUser.Permissions[targetOrgID] = GroupScopesByAction(permissions) + tmpUser.Permissions[evaluationOrg] = GroupScopesByAction(permissions) } return tmpUser, nil diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index ce353e721c5..68af6d37ddb 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -330,6 +330,7 @@ func (cmd *SaveExternalServiceRoleCommand) Validate() error { const ( GlobalOrgID = 0 + NoOrgID = int64(-1) GeneralFolderUID = "general" RoleGrafanaAdmin = "Grafana Admin" diff --git a/pkg/services/auth/identity/requester.go b/pkg/services/auth/identity/requester.go index b88120e88f2..aa7670d6c79 100644 --- a/pkg/services/auth/identity/requester.go +++ b/pkg/services/auth/identity/requester.go @@ -40,6 +40,8 @@ type Requester interface { GetOrgRole() roletype.RoleType // GetPermissions returns the permissions of the active entity. GetPermissions() map[string][]string + // GetGlobalPermissions returns the permissions of the active entity that are available across all organizations. + GetGlobalPermissions() map[string][]string // DEPRECATED: GetTeams returns the teams the entity is a member of. // Retrieve the teams from the team service instead of using this method. GetTeams() []int64 diff --git a/pkg/services/authn/identity.go b/pkg/services/authn/identity.go index dc8f8cce04c..4f618cd4424 100644 --- a/pkg/services/authn/identity.go +++ b/pkg/services/authn/identity.go @@ -31,6 +31,7 @@ const ( const ( AnonymousNamespaceID = NamespaceAnonymous + ":0" + GlobalOrgID = int64(0) ) var _ identity.Requester = (*Identity)(nil) @@ -164,6 +165,19 @@ func (i *Identity) GetPermissions() map[string][]string { return i.Permissions[i.GetOrgID()] } +// GetGlobalPermissions returns the permissions of the active entity that are available across all organizations +func (i *Identity) GetGlobalPermissions() map[string][]string { + if i.Permissions == nil { + return make(map[string][]string) + } + + if i.Permissions[GlobalOrgID] == nil { + return make(map[string][]string) + } + + return i.Permissions[GlobalOrgID] +} + func (i *Identity) GetTeams() []int64 { return i.Teams } diff --git a/pkg/services/user/identity.go b/pkg/services/user/identity.go index 49b4edc2c8c..2ab4b1f3661 100644 --- a/pkg/services/user/identity.go +++ b/pkg/services/user/identity.go @@ -8,6 +8,10 @@ import ( "github.com/grafana/grafana/pkg/services/auth/identity" ) +const ( + GlobalOrgID = int64(0) +) + type SignedInUser struct { UserID int64 `xorm:"user_id"` OrgID int64 `xorm:"org_id"` @@ -159,6 +163,19 @@ func (u *SignedInUser) GetPermissions() map[string][]string { return u.Permissions[u.GetOrgID()] } +// GetGlobalPermissions returns the permissions of the active entity that are available across all organizations +func (u *SignedInUser) GetGlobalPermissions() map[string][]string { + if u.Permissions == nil { + return make(map[string][]string) + } + + if u.Permissions[GlobalOrgID] == nil { + return make(map[string][]string) + } + + return u.Permissions[GlobalOrgID] +} + // DEPRECATED: GetTeams returns the teams the entity is a member of // Retrieve the teams from the team service instead of using this method. func (u *SignedInUser) GetTeams() []int64 { diff --git a/pkg/tests/api/alerting/api_backtesting_test.go b/pkg/tests/api/alerting/api_backtesting_test.go index de83add0e0f..08c60978bb7 100644 --- a/pkg/tests/api/alerting/api_backtesting_test.go +++ b/pkg/tests/api/alerting/api_backtesting_test.go @@ -11,6 +11,7 @@ import ( "github.com/grafana/grafana-plugin-sdk-go/data" "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/models/roletype" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" "github.com/grafana/grafana/pkg/services/datasources" @@ -92,9 +93,10 @@ func TestBacktesting(t *testing.T) { t.Run("if user does not have permissions", func(t *testing.T) { testUserId := createUser(t, env.SQLStore, user.CreateUserCommand{ - DefaultOrgRole: "", + DefaultOrgRole: string(roletype.RoleNone), Password: "test", Login: "test", + OrgID: 1, }) testUserApiCli := newAlertingApiClient(grafanaListedAddr, "test", "test")