8000 13647 fixed wrong feature flag for checking admin fine-grained authz by sschu · Pull Request #13648 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

13647 fixed wrong feature flag for checking admin fine-grained authz - #13648

Merged
pedroigor merged 2 commits into
keycloak:mainfrom
bosch-io:13647_fix_wrong_feature_flag_for_checking_admin_fine_grained_authz
Aug 22, 2022
Merged

13647 fixed wrong feature flag for checking admin fine-grained authz#13648
pedroigor merged 2 commits into
keycloak:mainfrom
bosch-io:13647_fix_wrong_feature_flag_for_checking_admin_fine_grained_authz

Conversation

@sschu
@sschu sschu commented Aug 8, 2022
Copy link
Copy Markdown
Contributor

Fixes #13647.

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

@pedroigor

Here is the PR for fixing other wrong usages of the profile flag.

@pedroigor
pedroigor commented Aug 8, 2022
Copy link
Copy Markdown
Contributor

@sschu Thanks. Looks like some tests are failing because now the authz field in MgmtPermissions is null and no check exists if this reference is null.

@sschu
sschu commented Aug 8, 2022
Copy link
Copy Markdown
Contributor Author

I'll have a look.

@sschu
sschu force-pushed the 13647_fix_wrong_feature_flag_for_checking_admin_fine_grained_authz branch from 24a9e8d to a9382b7 Compare August 11, 2022 17:16
@sschu
sschu commented Aug 11, 2022 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

@pedroigor I had to adapt some tests to also enable the profile feature correctly. I hope that does the trick.

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

@sschu Thanks again! Makes more sense for me too.

However, I'm afraid we still have some null checks to make like:

The only reason why they don't fail is because of checks like

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

Line 36 in c3ea847

boolean isPermissionsEnabled(RoleModel role);
.

So the places still using the authz even though null are only called if permissions are enabled for a specific area.

I would say we can leave those as is and merge the PR. But if you think you have a better idea to improve the code, please let me know.

@sschu
sschu commented Aug 12, 2022
8000
Copy link
Copy Markdown
Contributor Author

@pedroigor
I also saw the the code for the XyzPermissions classes is not very safe and relies on being used in a safe way from external. I did not like that so I added another commit for easier review adding checks. I am somewhat confident I caught all cases.

The general idea is to only check for the profile feature in one place in 'MgmtPermissions' and use authz!=null as an equivalent internal check down the road (not sure on this one, maybe checking for the feature directly is more explicit - but its also more code to read). In general ADMIN_FINE_GRAINED_AUTHZ enabled should be equivalent to authz!=null. Also, 'resourceServer!=null' means 'authz!=null' so it is safe to make calls to it. I am not 100% sure the reverse is also true so maybe there are cases where resourceServer==null and authz!=null. But most checks just check resourceServer!=null and can safely use authz afterwards.

I also added some source code comments/questions, feel free to respond to them. If you like the changes I would finalise the second commit and finally squash it before merging.

@sschu
sschu force-pushed the 13647_fix_wrong_feature_flag_for_checking_admin_fine_grained_authz branch from 536f673 to 042b5d7 Compare August 12, 2022 08:01
pedroigor
pedroigor reviewed Aug 12, 2022
View reviewed changes
Comment thread services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissions.java Outdated
@pedroigor pedroigor Aug 12, 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.

I don't think it does. The calling code does not call this method if the feature is disabled. See

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

Line 660 in 2b996b1

public ManagementPermissionReference getManagementPermissions() {
.

Comment thread services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.java Outdated
@pedroigor pedroigor Aug 12, 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.

Same comment and code pattern from the previous comments on clients.

Comment thread ...main/java/org/keycloak/services/resources/admin/permissions/IdentityProviderPermissions.java Outdated
@pedroigor pedroigor Aug 12, 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.

Same comment and code pattern from the previous comments on clients.

Comment thread services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissions.java Outdated
@pedroigor pedroigor Aug 12, 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.

I don't think so. We can do the same as others and see how it goes.

Comment thread services/src/main/java/org/keycloak/services/resources/admin/permissions/RolePermissions.java Outdated
@pedroigor pedroigor Aug 12, 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.

Same comment and code pattern from the previous comments on clients.

Comment thread services/src/main/java/org/keycloak/services/resources/admin/permissions/RolePermissions.java Outdated
@pedroigor pedroigor Aug 12, 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.

Definitely :) Let's remove it.

Comment thread services/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissions.java Outdated
@pedroigor pedroigor Aug 12, 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.

Same comment and code pattern from the previous comments on clients.

@pedroigor
pedroigor commented Aug 12, 2022
Copy link
Copy Markdown
Contributor

@sschu I'm really pleased to collaborate with you on these changes and your willingness to improve this area. Thanks a lot for your time and effort.

Despite the answers to some of your comments, the proposed changes LGTM and they should help us to create a baseline to perform more aggressive refactoring in the future.

@sschu
sschu force-pushed the 13647_fix_wrong_feature_flag_for_checking_admin_fine_grained_authz branch from 042b5d7 to b0f086d Compare August 12, 2022 12:21
@sschu
sschu commented Aug 12, 2022
Copy link
Copy Markdown
Contributor Author

@pedroigor Thanks for the roses! ;) I am also happy to contribute. I implemented your suggestions. Should I then squash the commits if everything is fine or can it be automatically squash-merged?

@pedroigor
pedroigor commented Aug 12, 2022
Copy link
Copy Markdown
Contributor

I can squash before merging.

@sschu
sschu commented Aug 12, 2022
Copy link
Copy Markdown
Contributor Author

Hmm, the failing test ran fine locally.

@sschu
sschu force-pushed the 13647_fix_wrong_feature_flag_for_checking_admin_fine_grained_authz branch from b0f086d to 11f5ec6 Compare August 22, 2022 07:32
@sschu
sschu commented Aug 22, 2022
Copy link
Copy Markdown
Contributor Author

@pedroigor The test failure is most likely not related to my changes.

@sschu sschu mentioned this pull request Aug 22, 2022
Merged
@pedroigor
pedroigor merged commit 916cfbb 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.

Wrong feature flag used to decide if admin fine-grained-permissions should be enabled

2 participants

@sschu @pedroigor
0