8000 12653 check if fine-grained permissions are enabled before retrieving… by sschu · Pull Request #12654 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

12653 check if fine-grained permissions are enabled before retrieving… - #12654

Merged
pedroigor merged 1 commit into
keycloak:mainfrom
bosch-io:12653_fix_n+1_queries_to_group_memberships_when_fine_grained_permissions_are_disabled
Aug 22, 2022
Merged

12653 check if fine-grained permissions are enabled before retrieving…#12654
pedroigor merged 1 commit into
keycloak:mainfrom
bosch-io:12653_fix_n+1_queries_to_group_memberships_when_fine_grained_permissions_are_disabled

Conversation

@sschu
@sschu sschu commented Jun 22, 2022
Copy link
Copy Markdown
Contributor

… group memberships of users

Fixes #12653

@sschu sschu mentioned this pull request Jun 22, 2022
Closed
@sschu
sschu commented Jun 22, 2022
Copy link
Copy Markdown
Contributor Author

@hmlnarik I isolated this one out of the other PR.. I will continue with the rest tomorrow...

@stianst
stianst requested a review from pedroigor June 23, 2022 07:31
pedroigor
pedroigor reviewed Jun 28, 2022
View reviewed changes
Comment thread services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissions.java Outdated
@pedroigor pedroigor Jun 28, 2022
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't we check org.keycloak.common.Profile.Feature#ADMIN_FINE_GRAINED_AUTHZ instead?

@sschu sschu Jun 28, 2022
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@pedroigor Could be possible, but I basically just copied how it is done elsewhere like here:

keycloak/services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissions.java

Line 254 in 99c06d1

if (!Profile.isFeatureEnabled(Profile.Feature.AUTHORIZATION)) return null;

Its basically like that all over the place. ;)

@sschu sschu Jul 18, 2022
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@pedroigor Any feedback from your side?

@pedroigor pedroigor Jul 21, 2022
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry. My last comment is because AUTHORIZATION should always be enabled. But not the ADMIN_FINE_GRAINED_AUTHZ feature.

So I'm wondering what difference that condition makes if you remove it or at least change it to ADMIN_FINE_GRAINED_AUTHZ if there is a difference.

@sschu 8000 sschu Jul 22, 2022
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@pedroigor If AUTHORIZATIONis always enabled but the ADMIN_FINE_GRAINED_AUTHZ is the correct one to use, it is probably used wrong all over the place. There is only one place where ADMIN_FINE_GRAINED_AUTHZ is used:

keycloak/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java

Line 191 in 820ab52

if (!Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ)) {

AUTHORIZATION is used much more often and I assume it's used wrong in these places:

keycloak/services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissions.java

Line 86 in cdef39f

if (Profile.isFeatureEnabled(Profile.Feature.AUTHORIZATION)) {

keycloak/services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.java

Line 64 in cdef39f

if (Profile.isFeatureEnabled(Profile.Feature.AUTHORIZATION)) {

keycloak/services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissions.java

Line 76 in 99c06d1

if (Profile.isFeatureEnabled(Profile.Feature.AUTHORIZATION)) {

keycloak/services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissions.java

Line 254 in 99c06d1

if (!Profile.isFeatureEnabled(Profile.Feature.AUTHORIZATION)) return null;

keycloak/services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissions.java

Line 265 in 99c06d1

if (!Profile.isFeatureEnabled(Profile.Feature.AUTHORIZATION)) return null;

I especially find

keycloak/services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissions.java

Line 254 in 99c06d1

if (!Profile.isFeatureEnabled(Profile.Feature.AUTHORIZATION)) return null;

dangerous, as the decision that fine-grained permissions are not used is essentially taken by searching for a resource server for the realm-management client and not finding it in:

keycloak/services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissions.java

Line 259 in 99c06d1

realmResourceServer = resourceServerStore.findByClient(client);

This would mean that if fine-grained permissions were enabled before and a resource server was created for the realm management client, they would still be used even after the profile was changed to not support fine-grained permissions.

WDYT?

@sschu
sschu commented Jul 29, 2022
Copy link
Copy Markdown
Contributor Author

@pedroigor Any feedback?

@pedroigor
pedroigor commented Aug 8, 2022 •
edited
Loading
Copy link
Copy Markdown
Contributor

@sschu You are right, I think those checks are outdated and referring to the time when authorization services were still tech preview.

The reason it works, partially, is that there is a check for resource servers and policies. So if you don't have them created fine-frained admin permissions won't be evaluated. Otherwise, if you enable, set up permissions, and disable, they will be still running.

From a security perspective, I think this is not so bad because fine-grained permissions should be more restrictive. But definitely not the behavior we want.

So yeah, I think we should update everything to use fine-grained feature instead.

@sschu
sschu force-pushed the 12653_fix_n+1_queries_to_group_memberships_when_fine_grained_permissions_are_disabled branch from 48ddc5a to a9ca6b0 Compare August 8, 2022 16:29
@sschu
sschu commented Aug 8, 2022
Copy link
Copy Markdown
Contributor Author

@pedroigor I updated this PR to use the correct profile feature name. I will file a bug and send a PR to change all the other places where the feature name was not used correctly.

@sschu sschu mentioned this pull request Aug 8, 2022
Closed
@sschu
sschu commented Aug 12, 2022
Copy link
Copy Markdown
Contributor Author

@pedroigor If you like my updates to the other PR, I would probably change this to not check explicitly for the feature flag but for authz!=null and move it into can(View|Manage)ByGroup.

@sschu
sschu force-pushed the 12653_fix_n+1_queries_to_group_memberships_when_fine_grained_permissions_are_disabled branch from a9ca6b0 to 24dd1cd Compare August 22, 2022 07:42
@sschu
sschu commented Aug 22, 2022
Copy link
Copy Markdown
Contributor Author

@pedroigor I changed the implementation to be in line with the changes in #13648.
The test failures seem to be unrelated...

@pedroigor
pedroigor merged commit fb978de into keycloak:main Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pedroigor pedroigor pedroigor approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Brief representation user retrieval on Admin API: only check group memberships when fine-grained permissions are enabled

2 participants

@sschu @pedroigor
0