From da500a56b39098029772a11653e9984fd1a44a2a Mon Sep 17 00:00:00 2001 From: "grafana-delivery-bot[bot]" <132647405+grafana-delivery-bot[bot]@users.noreply.github.com> Date: Thu, 22 Aug 2024 11:24:08 +0100 Subject: [PATCH] [v11.1.x] RBAC: Fix an issue with server admins not being able to manage users in orgs that they don't belong to (#92273) * RBAC: Fix an issue with server admins not being able to manage users in orgs that they don't belong to (#92024) * look at global perms if user is not a part of the target org * use constant * update tests (cherry picked from commit 41ac5b5ae767b1cd1872e4fc4803f08197acbffb) * fix tests --------- Co-authored-by: Ieva --- .../accesscontrol/authorize_in_org_test.go | 101 +++++++----------- pkg/services/accesscontrol/middleware.go | 4 + 2 files changed, 43 insertions(+), 62 deletions(-) diff --git a/pkg/services/accesscontrol/authorize_in_org_test.go b/pkg/services/accesscontrol/authorize_in_org_test.go index f9cc616c73a..b9bd6235b4f 100644 --- a/pkg/services/accesscontrol/authorize_in_org_test.go +++ b/pkg/services/accesscontrol/authorize_in_org_test.go @@ -1,7 +1,6 @@ package accesscontrol_test import ( - "context" "fmt" "net/http" "net/http/httptest" @@ -11,7 +10,7 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" - "github.com/grafana/grafana/pkg/services/accesscontrol/actest" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/authn" "github.com/grafana/grafana/pkg/services/authn/authntest" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" @@ -19,7 +18,6 @@ import ( "github.com/grafana/grafana/pkg/services/team" "github.com/grafana/grafana/pkg/services/team/teamtest" "github.com/grafana/grafana/pkg/services/user" - "github.com/grafana/grafana/pkg/services/user/usertest" "github.com/grafana/grafana/pkg/web" ) @@ -34,8 +32,8 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) { orgIDGetter accesscontrol.OrgIDGetter evaluator accesscontrol.Evaluator accessControl accesscontrol.AccessControl - acService accesscontrol.Service - userCache user.Service + userIdentities []*authn.Identity + authnErrors []error ctxSignedInUser *user.SignedInUser teamService team.Service expectedStatus int @@ -45,7 +43,6 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) { targetOrgId: accesscontrol.GlobalOrgID, evaluator: accesscontrol.EvalPermission("users:read", "users:*"), accessControl: ac, - userCache: &usertest.FakeUserService{}, ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}}, targerOrgPermissions: []accesscontrol.Permission{{Action: "users:read", Scope: "users:*"}}, teamService: &teamtest.FakeService{}, @@ -57,7 +54,6 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) { targerOrgPermissions: []accesscontrol.Permission{{Action: "users:read", Scope: "users:*"}}, evaluator: accesscontrol.EvalPermission("users:read", "users:*"), accessControl: ac, - userCache: &usertest.FakeUserService{}, ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}}, teamService: &teamtest.FakeService{}, expectedStatus: http.StatusOK, @@ -68,7 +64,6 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) { targerOrgPermissions: []accesscontrol.Permission{}, evaluator: accesscontrol.EvalPermission("users:read", "users:*"), accessControl: ac, - userCache: &usertest.FakeUserService{}, ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{}}, teamService: &teamtest.FakeService{}, expectedStatus: http.StatusForbidden, @@ -79,7 +74,6 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) { targerOrgPermissions: []accesscontrol.Permission{{Action: "users:read", Scope: "users:*"}}, evaluator: accesscontrol.EvalPermission("users:read", "users:*"), accessControl: ac, - userCache: &usertest.FakeUserService{}, ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}}, teamService: &teamtest.FakeService{}, expectedStatus: http.StatusOK, @@ -90,33 +84,10 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) { targerOrgPermissions: []accesscontrol.Permission{}, evaluator: accesscontrol.EvalPermission("users:read", "users:*"), accessControl: ac, - userCache: &usertest.FakeUserService{}, ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}}, teamService: &teamtest.FakeService{}, expectedStatus: http.StatusForbidden, }, - { - name: "should return 403 when user org ID doesn't match and user does not exist in org 2", - targetOrgId: 2, - targerOrgPermissions: []accesscontrol.Permission{}, - evaluator: accesscontrol.EvalPermission("users:read", "users:*"), - accessControl: ac, - userCache: &usertest.FakeUserService{ExpectedError: fmt.Errorf("user not found")}, - ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}}, - teamService: &teamtest.FakeService{}, - expectedStatus: http.StatusForbidden, - }, - { - name: "should return 403 early when api key org ID doesn't match", - targetOrgId: 2, - targerOrgPermissions: []accesscontrol.Permission{}, - evaluator: accesscontrol.EvalPermission("users:read", "users:*"), - accessControl: ac, - userCache: &usertest.FakeUserService{}, - ctxSignedInUser: &user.SignedInUser{ApiKeyID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}}, - teamService: &teamtest.FakeService{}, - expectedStatus: http.StatusForbidden, - }, { name: "should fetch user permissions when org ID doesn't match", targetOrgId: 2, @@ -124,13 +95,8 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) { evaluator: accesscontrol.EvalPermission("users:read", "users:*"), accessControl: ac, teamService: &teamtest.FakeService{}, - userCache: &usertest.FakeUserService{ - GetSignedInUserFn: func(ctx context.Context, query *user.GetSignedInUserQuery) (*user.SignedInUser, error) { - return &user.SignedInUser{UserID: 1, OrgID: 2, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}}, nil - }, - }, - ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:write": {"users:*"}}}}, - expectedStatus: http.StatusOK, + ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:write": {"users:*"}}}}, + expectedStatus: http.StatusOK, }, { name: "fails to fetch user permissions when org ID doesn't match", @@ -139,16 +105,9 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) { evaluator: accesscontrol.EvalPermission("users:read", "users:*"), accessControl: ac, teamService: &teamtest.FakeService{}, - acService: &actest.FakeService{ - ExpectedErr: fmt.Errorf("failed to get user permissions"), - }, - userCache: &usertest.FakeUserService{ - GetSignedInUserFn: func(ctx context.Context, query *user.GetSignedInUserQuery) (*user.SignedInUser, error) { - return &user.SignedInUser{UserID: 1, OrgID: 2, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}}, nil - }, - }, - ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}}, - expectedStatus: http.StatusForbidden, + authnErrors: []error{fmt.Errorf("failed to get user permissions")}, + ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}}, + expectedStatus: http.StatusForbidden, }, { name: "unable to get target org", @@ -157,24 +116,35 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) { }, evaluator: accesscontrol.EvalPermission("users:read", "users:*"), accessControl: ac, - userCache: &usertest.FakeUserService{}, ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}}, teamService: &teamtest.FakeService{}, expectedStatus: http.StatusForbidden, }, { - name: "should fetch global user permissions when user is not a member of the target org", - targetOrgId: 2, - targerOrgPermissions: []accesscontrol.Permission{{Action: "users:read", Scope: "users:*"}}, - evaluator: accesscontrol.EvalPermission("users:read", "users:*"), - accessControl: ac, - 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 - }, - }, + name: "should fetch global user permissions when user is not a member of the target org", + targetOrgId: 2, + evaluator: accesscontrol.EvalPermission("users:read", "users:*"), + accessControl: ac, ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:write": {"users:*"}}}}, - expectedStatus: http.StatusOK, + userIdentities: []*authn.Identity{ + {ID: identity.MustParseNamespaceID("user:1"), OrgID: -1, Permissions: map[int64]map[string][]string{}}, + {ID: identity.MustParseNamespaceID("user:1"), OrgID: accesscontrol.GlobalOrgID, Permissions: map[int64]map[string][]string{accesscontrol.GlobalOrgID: {"users:read": {"users:*"}}}}, + }, + authnErrors: []error{nil, nil}, + expectedStatus: http.StatusOK, + }, + { + name: "should fail if user is not a member of the target org and doesn't have the right permissions globally", + targetOrgId: 2, + evaluator: accesscontrol.EvalPermission("users:read", "users:*"), + accessControl: ac, + ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:write": {"users:*"}}}}, + userIdentities: []*authn.Identity{ + {ID: identity.MustParseNamespaceID("user:1"), OrgID: -1, Permissions: map[int64]map[string][]string{}}, + {ID: identity.MustParseNamespaceID("user:1"), OrgID: accesscontrol.GlobalOrgID, Permissions: map[int64]map[string][]string{accesscontrol.GlobalOrgID: {"folders:read": {"folders:*"}}}}, + }, + authnErrors: []error{nil, nil}, + expectedStatus: http.StatusForbidden, }, } @@ -190,9 +160,16 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) { Permissions: map[int64]map[string][]string{}, } expectedIdentity.Permissions[tc.targetOrgId] = accesscontrol.GroupScopesByAction(tc.targerOrgPermissions) + var expectedErr error + if len(tc.authnErrors) > 0 { + expectedErr = tc.authnErrors[0] + } authnService := &authntest.FakeService{ - ExpectedIdentity: expectedIdentity, + ExpectedIdentity: expectedIdentity, + ExpectedIdentities: tc.userIdentities, + ExpectedErr: expectedErr, + ExpectedErrs: tc.authnErrors, } var orgIDGetter accesscontrol.OrgIDGetter diff --git a/pkg/services/accesscontrol/middleware.go b/pkg/services/accesscontrol/middleware.go index 714f37b9041..6cb77dd454c 100644 --- a/pkg/services/accesscontrol/middleware.go +++ b/pkg/services/accesscontrol/middleware.go @@ -195,6 +195,10 @@ func AuthorizeInOrgMiddleware(ac AccessControl, authnService authn.Service) func var orgUser identity.Requester = c.SignedInUser if targetOrgID != c.SignedInUser.GetOrgID() { orgUser, err = authnService.ResolveIdentity(c.Req.Context(), targetOrgID, c.SignedInUser.GetID()) + if err == nil && orgUser.GetOrgID() == NoOrgID { + // User is not a member of the target org, so only their global permissions are relevant + orgUser, err = authnService.ResolveIdentity(c.Req.Context(), GlobalOrgID, c.SignedInUser.GetID()) + } if err != nil { deny(c, nil, fmt.Errorf("failed to authenticate user in target org: %w", err)) return