MG-2201 - Add Policy to Fail Role Update if User Not on Platform #2234

JeffMboya · 2024-05-14T05:54:29Z

What type of PR is this?

This is a feature because it adds the following functionality: client check check in policy layer when updating client role.

What does this do?

This PR modifies the update client role functionality to check for client existence in the policy layer (spiceDB) instead of the repo layer. This prevents the system from adding a policy for a non-existent user and then having to roll it back.

Which issue(s) does this PR fix/relate to?

Related Issue Feature: Update User role should fail fast for unknown user IDs #2201

Have you included tests for your changes?

No

Did you document any new/modified feature?

No

rodneyosodo

Since we check if the user is SuperAdmin first before updating the role, then they are certainly in the platform @arvindh123 what do you think?

auth/api/grpc/server.go

arvindh123

Since we check if the user is SuperAdmin first before updating the role, then they are certainly in the platform @arvindh123 what do you think?

@rodneyosodo
In previous part of the code, we are checking the Request User is SuperAdmin or Normal User.
we are not checking the role change user ID .
It is like super admin can make other user as super admin or normal user
In role change API endpoint, SuperAdmin will be sending the user id of the other user to change the role.
So we should check the user id given by super admin in request , is exists in platform or not.

arvindh123 · 2024-05-20T13:36:41Z

users/service_test.go

@@ -1075,7 +1075,7 @@ func TestUpdateClientRole(t *testing.T) {
 updateRoleResponse: mgclients.Client{},
 token: validToken,
 deletePolicyErr: svcerr.ErrMalformedEntity,
- err: svcerr.ErrDeletePolicies,


@JeffMboya
Instead of this changing this test response, Please add new test case for invalid / non existance user

rodneyosodo · 2024-05-22T12:42:14Z

users/service.go

@@ -694,6 +694,19 @@ func (svc service) addClientPolicyRollback(ctx context.Context, userID string, r
 }

 func (svc service) updateClientPolicy(ctx context.Context, userID string, role mgclients.Role) error {
+ res, err := svc.auth.Authorize(ctx, &magistrala.AuthorizeReq{


This authorization should happen once not during rolling back. This is because when we roll back we are sure the user is in the platform

client := mgclients.Client{ ID: cli.ID, Role: cli.Role, UpdatedAt: time.Now(), UpdatedBy: tokenUserID, } if _, err := svc.authorize(ctx, auth.UserType, auth.UsersKind, tokenUserID, auth.MembershipPermission, auth.PlatformType, auth.MagistralaObject); err != nil { return mgclients.Client{}, err } if err := svc.updateClientPolicy(ctx, cli.ID, cli.Role); err != nil { return mgclients.Client{}, err }

@rodneyosodo, since we want to verify if the user whose role the super admin wants to update is on the platform, I think the subject should be client.ID.

Update accordingly

Signed-off-by: JeffMboya <[email protected]> refactor: add memberRelation policy Signed-off-by: JeffMboya <[email protected]> refactor: add memberRelation policy Signed-off-by: JeffMboya <[email protected]> refactor:Add memberRelation policy Signed-off-by: JeffMboya <[email protected]> refactor:Add memberRelation policy Signed-off-by: JeffMboya <[email protected]> refactor:Add memberRelation policy Signed-off-by: JeffMboya <[email protected]> refactor:Add memberRelation policy Signed-off-by: JeffMboya <[email protected]> refactor:Add memberRelation policy Signed-off-by: JeffMboya <[email protected]> refactor:Add memberRelation policy Signed-off-by: JeffMboya <[email protected]> refactor: Revert changes in grpcServer struct Signed-off-by: JeffMboya <[email protected]> refactor: Revert changes in grpcServer struct Signed-off-by: JeffMboya <[email protected]> refactor: Fix authorization error in updateClientPolicy Signed-off-by: JeffMboya <[email protected]> refactor: Fix authorization error in updateClientPolicy Signed-off-by: JeffMboya <[email protected]> refactor: Fix authorization error in updateClientPolicy Signed-off-by: JeffMboya <[email protected]>

Signed-off-by: JeffMboya <[email protected]>

rodneyosodo · 2024-05-23T08:24:51Z

users/service_test.go

@@ -1101,6 +1103,14 @@ func TestUpdateClientRole(t *testing.T) {
 updateRoleErr: svcerr.ErrAuthentication,
 err: svcerr.ErrAuthentication,
 },
+ {


Add missing test case. You can run the following command and look at the HTML output for the missing case

cd users && go test -v --race -covermode=atomic -coverprofile=cover.out . && go tool cover -html cover.out -o cover.html

Signed-off-by: JeffMboya <[email protected]>

WashingtonKK

Please update tests.

WashingtonKK · 2024-05-23T13:34:38Z

users/service_test.go

+ desc: "update client role with failed MembershipPermission authorization",
+ client: client,
+ identifyResponse: &magistrala.IdentityRes{UserId: client.ID},
+ authorizeResponse: &magistrala.AuthorizeRes{Authorized: false},
+ token: validToken,
+ err: svcerr.ErrAuthorization,
+ },
+ {
+ desc: "Update client role for non-existent user",
+ client: mgclients.Client{},
+ token: validToken,
+ identifyResponse: &magistrala.IdentityRes{},
+ identifyErr: svcerr.ErrAuthorization,
+ err: svcerr.ErrAuthorization,
+ },


These test cases are not correct since the mock call for authorize returns tc.authorizeErr yet you have only provided err meaning that authorize will not return an error when it should.

Suggested change

desc: "update client role with failed MembershipPermission authorization",

client: client,

identifyResponse: &magistrala.IdentityRes{UserId: client.ID},

authorizeResponse: &magistrala.AuthorizeRes{Authorized: false},

token: validToken,

err: svcerr.ErrAuthorization,

},

{

desc: "Update client role for non-existent user",

client: mgclients.Client{},

token: validToken,

identifyResponse: &magistrala.IdentityRes{},

identifyErr: svcerr.ErrAuthorization,

err: svcerr.ErrAuthorization,

},

desc: "update client role with failed MembershipPermission authorization",

client: client,

identifyResponse: &magistrala.IdentityRes{UserId: client.ID},

authorizeResponse: &magistrala.AuthorizeRes{Authorized: false},

token: validToken,

err: svcerr.ErrAuthorization,

authorizeErr: svcerr.ErrAuthorization,

},

{

desc: "Update client role for non-existent user",

client: mgclients.Client{},

token: validToken,

identifyResponse: &magistrala.IdentityRes{},

identifyErr: svcerr.ErrAuthorization,

err: svcerr.ErrAuthorization,

authorizeErr: svcerr.ErrAuthorization,

},

WashingtonKK · 2024-05-23T13:51:26Z

users/service_test.go

@@ -1101,6 +1103,22 @@ func TestUpdateClientRole(t *testing.T) {
 updateRoleErr: svcerr.ErrAuthentication,
 err: svcerr.ErrAuthentication,
 },
+ {
+ desc: "update client role with failed MembershipPermission authorization",


This test does not cover your changes for a failed membership permission. It fails at svc.checkSuperAdmin when checking for admin permissions at svc.auth.Authorize.

Update the test and add a mock call which will pass in this case, and cover your changes.

WashingtonKK · 2024-05-23T13:55:26Z

users/service_test.go

+ identifyResponse: &magistrala.IdentityRes{},
+ identifyErr: svcerr.ErrAuthorization,


If svc.Identify fails, it means that the token provided is not valid, or the user who is trying to update the role does not exist and not that the client is non-existent.

Effectively this means this test will still also not cover your changes for this PR.

Signed-off-by: JeffMboya <[email protected]>

Signed-off-by: JeffMboya <[email protected]> fix: failing tests Signed-off-by: JeffMboya <[email protected]> fix: failing tests Signed-off-by: JeffMboya <[email protected]> fix: failing tests Signed-off-by: JeffMboya <[email protected]> fix: failing tests Signed-off-by: JeffMboya <[email protected]>

Signed-off-by: JeffMboya <[email protected]>

WashingtonKK

LGTM

JeffMboya force-pushed the MG-2201 branch 11 times, most recently from 1768f0a to 3bf4593 Compare May 15, 2024 09:47

JeffMboya marked this pull request as ready for review May 15, 2024 10:57

JeffMboya force-pushed the MG-2201 branch from 6b01491 to c4b2235 Compare May 15, 2024 11:04

rodneyosodo requested changes May 17, 2024

View reviewed changes

auth/api/grpc/server.go Outdated Show resolved Hide resolved

JeffMboya force-pushed the MG-2201 branch from c4b2235 to 8e6461d Compare May 20, 2024 09:07

arvindh123 approved these changes May 20, 2024

View reviewed changes

arvindh123 reviewed May 20, 2024

View reviewed changes

arvindh123 requested changes May 20, 2024

View reviewed changes

JeffMboya force-pushed the MG-2201 branch 3 times, most recently from 6c58f06 to d519516 Compare May 21, 2024 08:28

JeffMboya self-assigned this May 21, 2024

arvindh123 previously approved these changes May 21, 2024

View reviewed changes

rodneyosodo requested a review from WashingtonKK May 22, 2024 10:18

rodneyosodo requested changes May 22, 2024

View reviewed changes

JeffMboya force-pushed the MG-2201 branch from 6b7d335 to 7ab0c20 Compare May 22, 2024 14:47

JeffMboya dismissed arvindh123’s stale review via 4c5cd54 May 22, 2024 17:41

JeffMboya changed the title ~~MG-2201 - In update user role , Add policy for role should fail if user id is not member of Magistrala platform~~ MG-2201 - Add Policy to Fail Role Update if User Not on Platform May 22, 2024

JeffMboya added 2 commits May 22, 2024 21:06

refactor: Fix authorization error in updateClientPolicy

6a10fcd

Signed-off-by: JeffMboya <[email protected]>

refactor: Fix authorization error in updateClientPolicy

00cf3da

Signed-off-by: JeffMboya <[email protected]>

JeffMboya force-pushed the MG-2201 branch from 4c5cd54 to 00cf3da Compare May 22, 2024 18:06

rodneyosodo requested changes May 23, 2024

View reviewed changes

refactor: Add failed MembershipPermission authorization test case

bce311c

Signed-off-by: JeffMboya <[email protected]>

WashingtonKK requested changes May 23, 2024

View reviewed changes

refactor: Add failed MembershipPermission authorization test case

9b2c46b

Signed-off-by: JeffMboya <[email protected]>

JeffMboya force-pushed the MG-2201 branch from 9eeacd9 to 377c470 Compare May 24, 2024 04:34

JeffMboya force-pushed the MG-2201 branch from 3137712 to 8c077be Compare May 24, 2024 04:51

JeffMboya added 2 commits May 24, 2024 08:22

fix: failing tests

b904215

Signed-off-by: JeffMboya <[email protected]>

fix: failing tests

9cc8fe7

Signed-off-by: JeffMboya <[email protected]>

WashingtonKK approved these changes May 24, 2024

View reviewed changes

arvindh123 approved these changes May 24, 2024

View reviewed changes

rodneyosodo approved these changes May 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MG-2201 - Add Policy to Fail Role Update if User Not on Platform #2234

MG-2201 - Add Policy to Fail Role Update if User Not on Platform #2234

JeffMboya commented May 14, 2024 •

edited

rodneyosodo left a comment

arvindh123 left a comment •

edited

arvindh123 May 20, 2024

rodneyosodo May 22, 2024

JeffMboya May 22, 2024

rodneyosodo May 23, 2024

JeffMboya May 23, 2024

rodneyosodo May 23, 2024

WashingtonKK left a comment

WashingtonKK May 23, 2024

WashingtonKK May 23, 2024

WashingtonKK May 23, 2024

WashingtonKK May 23, 2024

WashingtonKK left a comment

		identifyResponse: &magistrala.IdentityRes{},
		identifyErr: svcerr.ErrAuthorization,

MG-2201 - Add Policy to Fail Role Update if User Not on Platform #2234

Are you sure you want to change the base?

MG-2201 - Add Policy to Fail Role Update if User Not on Platform #2234

Conversation

JeffMboya commented May 14, 2024 • edited

What type of PR is this?

What does this do?

Which issue(s) does this PR fix/relate to?

Have you included tests for your changes?

Did you document any new/modified feature?

rodneyosodo left a comment

Choose a reason for hiding this comment

arvindh123 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WashingtonKK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WashingtonKK left a comment

Choose a reason for hiding this comment

JeffMboya commented May 14, 2024 •

edited

arvindh123 left a comment •

edited