Fix disabling of admin permissions in fine grained authz v2#1519
Open
michalvavrik wants to merge 1 commit into
Open
Fix disabling of admin permissions in fine grained authz v2#1519michalvavrik wants to merge 1 commit into
michalvavrik wants to merge 1 commit into
Conversation
* Closes: keycloak#1509 * Makes sure that `admin_permissions_enabled` is not omitted from PUT when it is explicitly set in `.tf` resource. It is done by distinguishing between true (set explicitly to true), false (set explicitly to false) and nil (unset, hence omitted) I think this might not be the only case, e.g. `organizations_enabled` looks like it behaves same way, but I didn't explore that as I am only fixing the linked issue. * Conditionally switches `admin-fine-grained-authz` from `v1` to `v2` in docker compose and test CI so that we can test this PR. Currently `admin_permissions_enabled` is not tested because even by the existing test. It is ignored due to the fact that tests are using `admin-fine-grained-authz:v1`. However v1 has been deprecated since Keycloak 26.5.0 keycloak/keycloak#44121. We should test it and make sure that the flag works. * For version lesser than 26.5.0, 7 tests are disabled due to the fact that v2 does not support these scenarios: * https://github.com/keycloak/keycloak/blob/1ca15660f576eec7a81e60a00651d551d5045def/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java#L727 * https://github.com/keycloak/keycloak/blob/1ca15660f576eec7a81e60a00651d551d5045def/services/src/main/java/org/keycloak/services/resources/admin/fgap/ClientPermissionsV2.java#L185 Signed-off-by: Michal Vavřík <michal.vavrik@aol.com>
michalvavrik
changed the title
Member
Author
|
\cc @sschu |
Member
Author
|
Looking at the #1187 it might not be desirable to switch to v2 for now, however the bug is legit and the |
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: keycloak_realm: admin_permissions_enabled is not disabled if previously enabled #1509
Makes sure that
admin_permissions_enabledis not omitted from PUT when it is explicitly set in.tfresource. It is done by distinguishing between true (set explicitly to true), false (set explicitly to false) and nil (unset, hence omitted) I think this might not be the only case, e.g.organizations_enabledlooks like it behaves same way, but I didn't explore that as I am only fixing the linked issue.Conditionally switches
admin-fine-grained-authzfromv1tov2in docker compose and test CI so that we can test this PR. Currentlyadmin_permissions_enabledis not tested because even by the existing test. It is ignored due to the fact that tests are usingadmin-fine-grained-authz:v1. However v1 has been deprecated since Keycloak 26.5.0 Deprecate Fine-Grained Admin Permissions v1 keycloak#44121. We should test it and make sure that the flag works.For version lesser than 26.5.0, 7 tests are disabled due to the fact that v2 does not support these scenarios: