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

Add a READER role that gives read access to users and groups info #842

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

garaimanoj
Copy link
Contributor

No description provided.

@garaimanoj garaimanoj self-assigned this Sep 9, 2024
@garaimanoj garaimanoj linked an issue Sep 9, 2024 that may be closed by this pull request
@garaimanoj garaimanoj marked this pull request as ready for review September 11, 2024 14:19
@rmiccoli rmiccoli changed the title Issue 794 role reader Add ROLE_READER Sep 12, 2024
@rmiccoli
Copy link
Contributor

I notice that a user with monitoring privileges (ROLE_READER) is not able to access to /scim/Users and /iam/scope_policies - they get 403. Is it intentional?

Also, user can try to remove members from a group getting, of course, a 403 error after clicking. Should the Remove button be hidden?

Copy link
Member

@enricovianello enricovianello left a comment

Choose a reason for hiding this comment

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

It seems to me all ok

@enricovianello enricovianello changed the title Add ROLE_READER Add a READER role that gives read access to users and groups info Sep 13, 2024
Copy link
Member

@enricovianello enricovianello left a comment

Choose a reason for hiding this comment

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

These changes are ok to me and I have nothing to correct 👍
Coverage is ok but we didn't add specific tests about this role. I'd say it's important to add some specific tests in particular that check those users are not allowed to get for example x509 PEM certificates of these users. But the real problem is that admins currently can see them and probably what we wants is that both admins and readers can't access them and tests should be oriented to this behavior. Then, probably, we need a different issue that addresses this change, which is more complex: currently there are no views on SCIM model objects. This change is required before introducing this READER role. I'm realizing this now. We can talk about this during today IAM community meeting.

@enricovianello enricovianello added the component/db Issue that includes one or more db migrations label Sep 13, 2024
@garaimanoj
Copy link
Contributor Author

I notice that a user with monitoring privileges (ROLE_READER) is not able to access to /scim/Users and /iam/scope_policies - they get 403. Is it intentional?

Also, user can try to remove members from a group getting, of course, a 403 error after clicking. Should the Remove button be hidden?

Now ROLE_READER can access to /scim/Users and /iam/scope_policies. Remove button has been disabled.

@rmiccoli
Copy link
Contributor

Quality Gate Passed Quality Gate passed

Issues 59 New issues 0 Accepted issues

Measures 0 Security Hotspots 100.0% Coverage on New Code 0.0% Duplication on New Code

See analysis details on SonarCloud

Try to solve these issues where possible

@garaimanoj
Copy link
Contributor Author

garaimanoj commented Sep 16, 2024

Organization Management is available in sidebar menu when registration is enabled. I have added additional check for READER.
image

@garaimanoj
Copy link
Contributor Author

garaimanoj commented Sep 16, 2024

These changes are ok to me and I have nothing to correct 👍 Coverage is ok but we didn't add specific tests about this role. I'd say it's important to add some specific tests in particular that check those users are not allowed to get for example x509 PEM certificates of these users. But the real problem is that admins currently can see them and probably what we wants is that both admins and readers can't access them and tests should be oriented to this behavior. Then, probably, we need a different issue that addresses this change, which is more complex: currently there are no views on SCIM model objects. This change is required before introducing this READER role. I'm realizing this now. We can talk about this during today IAM community meeting.

As of now I have hidden the x509 certificate section from Admin and Reader from UI. Could you please raise an issue with more details regarding the new view of SCIM modal task.

@rmiccoli
Copy link
Contributor

rmiccoli commented Sep 18, 2024

These changes are ok to me and I have nothing to correct 👍 Coverage is ok but we didn't add specific tests about this role. I'd say it's important to add some specific tests in particular that check those users are not allowed to get for example x509 PEM certificates of these users. But the real problem is that admins currently can see them and probably what we wants is that both admins and readers can't access them and tests should be oriented to this behavior. Then, probably, we need a different issue that addresses this change, which is more complex: currently there are no views on SCIM model objects. This change is required before introducing this READER role. I'm realizing this now. We can talk about this during today IAM community meeting.

As of now I have hidden the x509 certificate section from Admin and Reader from UI. Could you please raise an issue with more details regarding the new view of SCIM modal task.

Hi Manoj, both admins and users with ROLE_READER should see users' x509 certificates from UI. The idea was to hide only the pem-encoded certificate from SCIM API.

@garaimanoj
Copy link
Contributor Author

I am checking the Sonar analysis issues

@enricovianello
Copy link
Member

enricovianello commented Sep 24, 2024

These changes are ok to me and I have nothing to correct 👍 Coverage is ok but we didn't add specific tests about this role. I'd say it's important to add some specific tests in particular that check those users are not allowed to get for example x509 PEM certificates of these users. But the real problem is that admins currently can see them and probably what we wants is that both admins and readers can't access them and tests should be oriented to this behavior. Then, probably, we need a different issue that addresses this change, which is more complex: currently there are no views on SCIM model objects. This change is required before introducing this READER role. I'm realizing this now. We can talk about this during today IAM community meeting.

As of now I have hidden the x509 certificate section from Admin and Reader from UI. Could you please raise an issue with more details regarding the new view of SCIM modal task.

Hi Manoj, both admins and users with ROLE_READER should see users' x509 certificates from UI. The idea was to hide only the pem-encoded certificate from SCIM API.

The idea is to use some mechanism to hide the PEM encoded certificate from the ScimUser object returned by SCIM endpoints. The Jackson Java Views can be used (they are already used into ClientDetails object for example) or we can add something like this JSONFilter, used to filter passwords from the returned SCIMUser object (used as an annotation).
Probably this second approach is the easier and which doesn't mean lots of changes.

We could probably add a similar annotation to ScimX509Certificate object which is used inside the ScimIndigoUser extension.

Copy link

sonarcloud bot commented Sep 25, 2024

@enricovianello enricovianello changed the base branch from master to develop October 17, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ROLE_READ to allow seeing account details of all users
3 participants