Skip to content

Commit 41ac5b5

Browse files
IevaVasiljevaIevaVasiljeva
authored
RBAC: Fix an issue with server admins not being able to manage users in orgs that they don't belong to (grafana#92024)
* look at global perms if user is not a part of the target org * use constant * update tests
1 parent 40cdfeb commit 41ac5b5

File tree

2 files changed

+41
-61
lines changed

2 files changed

+41
-61
lines changed

‎pkg/services/accesscontrol/authorize_in_org_test.go

Lines changed: 37 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package accesscontrol_test
22

33
import (
4-
"context"
54
"fmt"
65
"net/http"
76
"net/http/httptest"
@@ -13,7 +12,6 @@ import (
1312
"github.com/grafana/authlib/claims"
1413
"github.com/grafana/grafana/pkg/services/accesscontrol"
1514
"github.com/grafana/grafana/pkg/services/accesscontrol/acimpl"
16-
"github.com/grafana/grafana/pkg/services/accesscontrol/actest"
1715
"github.com/grafana/grafana/pkg/services/authn"
1816
"github.com/grafana/grafana/pkg/services/authn/authntest"
1917
"github.com/grafana/grafana/pkg/services/authz/zanzana"
@@ -22,7 +20,6 @@ import (
2220
"github.com/grafana/grafana/pkg/services/team"
2321
"github.com/grafana/grafana/pkg/services/team/teamtest"
2422
"github.com/grafana/grafana/pkg/services/user"
25-
"github.com/grafana/grafana/pkg/services/user/usertest"
2623
"github.com/grafana/grafana/pkg/web"
2724
)
2825

@@ -37,8 +34,8 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) {
3734
orgIDGetter accesscontrol.OrgIDGetter
3835
evaluator accesscontrol.Evaluator
3936
accessControl accesscontrol.AccessControl
40-
acService accesscontrol.Service
41-
userCache user.Service
37+
userIdentities []*authn.Identity
38+
authnErrors []error
4239
ctxSignedInUser *user.SignedInUser
4340
teamService team.Service
4441
expectedStatus int
@@ -48,7 +45,6 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) {
4845
targetOrgId: accesscontrol.GlobalOrgID,
4946
evaluator: accesscontrol.EvalPermission("users:read", "users:*"),
5047
accessControl: ac,
51-
userCache: &usertest.FakeUserService{},
5248
ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}},
5349
targerOrgPermissions: []accesscontrol.Permission{{Action: "users:read", Scope: "users:*"}},
5450
teamService: &teamtest.FakeService{},
@@ -60,7 +56,6 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) {
6056
targerOrgPermissions: []accesscontrol.Permission{{Action: "users:read", Scope: "users:*"}},
6157
evaluator: accesscontrol.EvalPermission("users:read", "users:*"),
6258
accessControl: ac,
63-
userCache: &usertest.FakeUserService{},
6459
ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}},
6560
teamService: &teamtest.FakeService{},
6661
expectedStatus: http.StatusOK,
@@ -71,7 +66,6 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) {
7166
targerOrgPermissions: []accesscontrol.Permission{},
7267
evaluator: accesscontrol.EvalPermission("users:read", "users:*"),
7368
accessControl: ac,
74-
userCache: &usertest.FakeUserService{},
7569
ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{}},
7670
teamService: &teamtest.FakeService{},
7771
expectedStatus: http.StatusForbidden,
@@ -82,7 +76,6 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) {
8276
targerOrgPermissions: []accesscontrol.Permission{{Action: "users:read", Scope: "users:*"}},
8377
evaluator: accesscontrol.EvalPermission("users:read", "users:*"),
8478
accessControl: ac,
85-
userCache: &usertest.FakeUserService{},
8679
ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}},
8780
teamService: &teamtest.FakeService{},
8881
expectedStatus: http.StatusOK,
@@ -93,47 +86,19 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) {
9386
targerOrgPermissions: []accesscontrol.Permission{},
9487
evaluator: accesscontrol.EvalPermission("users:read", "users:*"),
9588
accessControl: ac,
96-
userCache: &usertest.FakeUserService{},
9789
ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}},
9890
teamService: &teamtest.FakeService{},
9991
expectedStatus: http.StatusForbidden,
10092
},
101-
{
102-
name: "should return 403 when user org ID doesn't match and user does not exist in org 2",
103-
targetOrgId: 2,
104-
targerOrgPermissions: []accesscontrol.Permission{},
105-
evaluator: accesscontrol.EvalPermission("users:read", "users:*"),
106-
accessControl: ac,
107-
userCache: &usertest.FakeUserService{ExpectedError: fmt.Errorf("user not found")},
108-
ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}},
109-
teamService: &teamtest.FakeService{},
110-
expectedStatus: http.StatusForbidden,
111-
},
112-
{
113-
name: "should return 403 early when api key org ID doesn't match",
114-
targetOrgId: 2,
115-
targerOrgPermissions: []accesscontrol.Permission{},
116-
evaluator: accesscontrol.EvalPermission("users:read", "users:*"),
117-
accessControl: ac,
118-
userCache: &usertest.FakeUserService{},
119-
ctxSignedInUser: &user.SignedInUser{ApiKeyID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}},
120-
teamService: &teamtest.FakeService{},
121-
expectedStatus: http.StatusForbidden,
122-
},
12393
{
12494
name: "should fetch user permissions when org ID doesn't match",
12595
targetOrgId: 2,
12696
targerOrgPermissions: []accesscontrol.Permission{{Action: "users:read", Scope: "users:*"}},
12797
evaluator: accesscontrol.EvalPermission("users:read", "users:*"),
12898
accessControl: ac,
12999
teamService: &teamtest.FakeService{},
130-
userCache: &usertest.FakeUserService{
131-
GetSignedInUserFn: func(ctx context.Context, query *user.GetSignedInUserQuery) (*user.SignedInUser, error) {
132-
return &user.SignedInUser{UserID: 1, OrgID: 2, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}}, nil
133-
},
134-
},
135-
ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:write": {"users:*"}}}},
136-
expectedStatus: http.StatusOK,
100+
ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:write": {"users:*"}}}},
101+
expectedStatus: http.StatusOK,
137102
},
138103
{
139104
name: "fails to fetch user permissions when org ID doesn't match",
@@ -142,16 +107,9 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) {
142107
evaluator: accesscontrol.EvalPermission("users:read", "users:*"),
143108
accessControl: ac,
144109
teamService: &teamtest.FakeService{},
145-
acService: &actest.FakeService{
146-
ExpectedErr: fmt.Errorf("failed to get user permissions"),
147-
},
148-
userCache: &usertest.FakeUserService{
149-
GetSignedInUserFn: func(ctx context.Context, query *user.GetSignedInUserQuery) (*user.SignedInUser, error) {
150-
return &user.SignedInUser{UserID: 1, OrgID: 2, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}}, nil
151-
},
152-
},
153-
ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}},
154-
expectedStatus: http.StatusForbidden,
110+
authnErrors: []error{fmt.Errorf("failed to get user permissions")},
111+
ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}},
112+
expectedStatus: http.StatusForbidden,
155113
},
156114
{
157115
name: "unable to get target org",
@@ -160,24 +118,35 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) {
160118
},
161119
evaluator: accesscontrol.EvalPermission("users:read", "users:*"),
162120
accessControl: ac,
163-
userCache: &usertest.FakeUserService{},
164121
ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:read": {"users:*"}}}},
165122
teamService: &teamtest.FakeService{},
166123
expectedStatus: http.StatusForbidden,
167124
},
168125
{
169-
name: "should fetch global user permissions when user is not a member of the target org",
170-
targetOrgId: 2,
171-
targerOrgPermissions: []accesscontrol.Permission{{Action: "users:read", Scope: "users:*"}},
172-
evaluator: accesscontrol.EvalPermission("users:read", "users:*"),
173-
accessControl: ac,
174-
userCache: &usertest.FakeUserService{
175-
GetSignedInUserFn: func(ctx context.Context, query *user.GetSignedInUserQuery) (*user.SignedInUser, error) {
176-
return &user.SignedInUser{UserID: 1, OrgID: -1, Permissions: map[int64]map[string][]string{}}, nil
177-
},
126+
name: "should fetch global user permissions when user is not a member of the target org",
127+
targetOrgId: 2,
128+
evaluator: accesscontrol.EvalPermission("users:read", "users:*"),
129+
accessControl: ac,
130+
ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:write": {"users:*"}}}},
131+
userIdentities: []*authn.Identity{
132+
{ID: "1", OrgID: -1, Permissions: map[int64]map[string][]string{}},
133+
{ID: "1", OrgID: accesscontrol.GlobalOrgID, Permissions: map[int64]map[string][]string{accesscontrol.GlobalOrgID: {"users:read": {"users:*"}}}},
178134
},
135+
authnErrors: []error{nil, nil},
136+
expectedStatus: http.StatusOK,
137+
},
138+
{
139+
name: "should fail if user is not a member of the target org and doesn't have the right permissions globally",
140+
targetOrgId: 2,
141+
evaluator: accesscontrol.EvalPermission("users:read", "users:*"),
142+
accessControl: ac,
179143
ctxSignedInUser: &user.SignedInUser{UserID: 1, OrgID: 1, Permissions: map[int64]map[string][]string{1: {"users:write": {"users:*"}}}},
180-
expectedStatus: http.StatusOK,
144+
userIdentities: []*authn.Identity{
145+
{ID: "1", OrgID: -1, Permissions: map[int64]map[string][]string{}},
146+
{ID: "1", OrgID: accesscontrol.GlobalOrgID, Permissions: map[int64]map[string][]string{accesscontrol.GlobalOrgID: {"folders:read": {"folders:*"}}}},
147+
},
148+
authnErrors: []error{nil, nil},
149+
expectedStatus: http.StatusForbidden,
181150
},
182151
}
183152

@@ -194,9 +163,16 @@ func TestAuthorizeInOrgMiddleware(t *testing.T) {
194163
Permissions: map[int64]map[string][]string{},
195164
}
196165
expectedIdentity.Permissions[tc.targetOrgId] = accesscontrol.GroupScopesByAction(tc.targerOrgPermissions)
166+
var expectedErr error
167+
if len(tc.authnErrors) > 0 {
168+
expectedErr = tc.authnErrors[0]
169+
}
197170

198171
authnService := &authntest.FakeService{
199-
ExpectedIdentity: expectedIdentity,
172+
ExpectedIdentity: expectedIdentity,
173+
ExpectedIdentities: tc.userIdentities,
174+
ExpectedErr: expectedErr,
175+
ExpectedErrs: tc.authnErrors,
200176
}
201177

202178
var orgIDGetter accesscontrol.OrgIDGetter

‎pkg/services/accesscontrol/middleware.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,10 @@ func AuthorizeInOrgMiddleware(ac AccessControl, authnService authn.Service) func
205205
var orgUser identity.Requester = c.SignedInUser
206206
if targetOrgID != c.SignedInUser.GetOrgID() {
207207
orgUser, err = authnService.ResolveIdentity(c.Req.Context(), targetOrgID, c.SignedInUser.GetID())
208+
if err == nil && orgUser.GetOrgID() == NoOrgID {
209+
// User is not a member of the target org, so only their global permissions are relevant
210+
orgUser, err = authnService.ResolveIdentity(c.Req.Context(), GlobalOrgID, c.SignedInUser.GetID())
211+
}
208212
if err != nil {
209213
deny(c, nil, fmt.Errorf("failed to authenticate user in target org: %w", err))
210214
return

0 commit comments

Comments
 (0)