Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

JeffMboya
Copy link
Contributor

@JeffMboya JeffMboya commented May 14, 2024

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?

Have you included tests for your changes?

No

Did you document any new/modified feature?

No

@JeffMboya JeffMboya force-pushed the MG-2201 branch 11 times, most recently from 1768f0a to 3bf4593 Compare May 15, 2024 09:47
@JeffMboya JeffMboya marked this pull request as ready for review May 15, 2024 10:57
Copy link
Member

@rodneyosodo rodneyosodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Copy link
Contributor

@arvindh123 arvindh123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

users/service_test.go Outdated Show resolved Hide resolved
@JeffMboya JeffMboya force-pushed the MG-2201 branch 3 times, most recently from 6c58f06 to d519516 Compare May 21, 2024 08:28
@JeffMboya JeffMboya self-assigned this May 21, 2024
arvindh123
arvindh123 previously approved these changes May 21, 2024
users/service.go Outdated Show resolved Hide resolved
@JeffMboya 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
users/service_test.go Show resolved Hide resolved
Copy link
Contributor

@WashingtonKK WashingtonKK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update tests.

users/service_test.go Outdated Show resolved Hide resolved
users/service_test.go Show resolved Hide resolved
users/service_test.go Outdated Show resolved Hide resolved
@JeffMboya JeffMboya force-pushed the MG-2201 branch 2 times, most recently from 3137712 to 8c077be Compare May 24, 2024 04:51
WashingtonKK
WashingtonKK previously approved these changes May 24, 2024
Copy link
Contributor

@WashingtonKK WashingtonKK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

rodneyosodo
rodneyosodo previously approved these changes May 24, 2024
users/service.go Outdated Show resolved Hide resolved
@JeffMboya JeffMboya dismissed stale reviews from rodneyosodo and WashingtonKK via e133122 May 30, 2024 07:15
arvindh123
arvindh123 previously approved these changes May 30, 2024
felixgateru
felixgateru previously approved these changes May 31, 2024
rodneyosodo
rodneyosodo previously approved these changes May 31, 2024
Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JeffMboya Please resolve conflicts.

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor: add memberRelation policy

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor: add memberRelation policy

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor:Add memberRelation policy

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor:Add memberRelation policy

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor:Add memberRelation policy

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor:Add memberRelation policy

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor:Add memberRelation policy

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor:Add memberRelation policy

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor: Revert changes in grpcServer struct

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor: Revert changes in grpcServer struct

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor: Fix authorization error in updateClientPolicy

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor: Fix authorization error in updateClientPolicy

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor: Fix authorization error in updateClientPolicy

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

fix: failing tests

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

fix: failing tests

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

fix: failing tests

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

fix: failing tests

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Update User role should fail fast for unknown user IDs
6 participants