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
8000

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

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.

Sign up for GitHub

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

Merged
pedroigor merged 2 commits into from
Aug 22, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e31ce0
13647 fixed wrong feature flag for checking admin fine-grained authz
sschu Aug 8, 2022
11f5ec6
13647 Added null checks and some comments/questions for discussions. …
sschu Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ...es/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ private String getExchangeToPermissionName(ClientModel client) {

private void initialize(ClientModel client) {
ResourceServer server = root.findOrCreateResourceServer(client);
if (server==null) return;
Scope manageScope = manageScope(server);
if (manageScope == null) {
manageScope = authz.getStoreFactory().getScopeStore().create(server, AdminPermissionManagement.MANAGE_SCOPE);
Expand Down Expand Up @@ -291,6 +292,7 @@ public Resource resource(ClientModel client) {

@Override
public Map<String, String> getPermissions(ClientModel client) {
if (authz == null) return null;
initialize(client);
Map<String, String> scopes = new LinkedHashMap<>();
scopes.put(AdminPermissionManagement.VIEW_SCOPE, viewPermission(client).getId());
Expand Down
7 changes: 4 additions & 3 deletions ...ces/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class GroupPermissions implements GroupPermissionEvaluator, GroupPermissionManag
GroupPermissions(AuthorizationProvider authz, MgmtPermissions root) {
this.authz = authz;
this.root = root;
if (Profile.isFeatureEnabled(Profile.Feature.AUTHORIZATION)) {
if (authz!=null) {
resourceStore = authz.getStoreFactory().getResourceStore();
policyStore = authz.getStoreFactory().getPolicyStore();
} else {
Expand Down Expand Up @@ -96,9 +96,9 @@ private static String getViewMembersPermissionGroup(GroupModel group) {
}

private void initialize(GroupModel group) {
root.initializeRealmResourceServer();
ResourceServer server = root.initializeRealmResourceServer();
if (server == null) return;
root.initializeRealmDefaultScopes();
ResourceServer server = root.realmResourceServer();
Scope manageScope = root.realmManageScope();
Scope viewScope = root.realmViewScope();
Scope manageMembersScope = root.initializeRealmScope(MANAGE_MEMBERS_SCOPE);
Expand Down Expand Up @@ -221,6 +221,7 @@ public Resource resource(GroupModel group) {

@Override
public Map<String, String> getPermissions(GroupModel group) {
if (authz == null) return null;
initialize(group);
Map<String, String> scopes = new LinkedHashMap<>();
scopes.put(AdminPermissionManagement.VIEW_SCOPE, viewPermission(group).getId());
Expand Down
2 changes: 2 additions & 0 deletions ...n/java/org/keycloak/services/resources/admin/permissions/IdentityProviderPermissions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ private String getExchangeToPermissionName(IdentityProviderModel idp) {

private void initialize(IdentityProviderModel idp) {
ResourceServer server = root.initializeRealmResourceServer();
if (server == null) return;
Scope exchangeToScope = root.initializeScope(TOKEN_EXCHANGE, server);

String resourceName = getResourceName(idp);
Expand Down Expand Up @@ -139,6 +140,7 @@ public Resource resource(IdentityProviderModel idp) {

@Override
public Map<String, String> getPermissions(IdentityProviderModel idp) {
if (authz==null) return null;
initialize(idp);
Map<String, String> scopes = new LinkedHashMap<>();
scopes.put(TOKEN_EXCHANGE, exchangeToPermission(idp).getId());
Expand Down
18 changes: 12 additions & 6 deletions ...ices/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,14 @@ class MgmtPermissions implements AdminPermissionEvaluator, AdminPermissionManage
protected RealmPermissions realmPermissions;
protected ClientPermissions clientPermissions;
protected IdentityProviderPermissions idpPermissions;
protected RolePermissions rolePermissions;


MgmtPermissions(KeycloakSession session, RealmModel realm) {
this.session = session;
this.realm = realm;
KeycloakSessionFactory keycloakSessionFactory = session.getKeycloakSessionFactory();
if (Profile.isFeatureEnabled(Profile.Feature.AUTHORIZATION)) {
if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ)) {
AuthorizationProviderFactory factory = (AuthorizationProviderFactory) keycloakSessionFactory.getProviderFactory(AuthorizationProvider.class);
this.authz = factory.create(session, realm);
}
Expand Down Expand Up @@ -203,7 +204,9 @@ public RealmModel adminsRealm() {

@Override
public RolePermissions roles() {
return new RolePermissions(session, realm, authz, this);
if (rolePermissions!=null) return rolePermissions;
rolePermissions = new RolePermissions(session, realm, authz, this);
return rolePermissions;
}

@Override
Expand Down Expand Up @@ -251,20 +254,20 @@ public ResourceServer resourceServer(ClientModel client) {

@Override
public ResourceServer realmResourceServer() {
if (!Profile.isFeatureEnabled(Profile.Feature.AUTHORIZATION)) return null;
if (authz == null) return null;
if (realmResourceServer != null) return realmResourceServer;
ClientModel client = getRealmManagementClient();
if (client == null) return null;
ResourceServerStore resourceServerStore = authz.getStoreFactory().getResourceServerStore();
realmResourceServer = resourceServerStore.findByClient(client);
realmResourceServer = authz.getStoreFactory().getResourceServerStore().findByClient(client);
return realmResourceServer;

}

public ResourceServer initializeRealmResourceServer() {
if (!Profile.isFeatureEnabled(Profile.Feature.AUTHORIZATION)) return null;
if (authz == null) return null;
if (realmResourceServer != null) return realmResourceServer;
ClientModel client = getRealmManagementClient();
if (client == null) return null;
realmResourceServer = authz.getStoreFactory().getResourceServerStore().findByClient(client);
if (realmResourceServer == null) {
realmResourceServer = authz.getStoreFactory().getResourceServerStore().create(client);
Expand All @@ -277,12 +280,14 @@ public ResourceServer initializeRealmResourceServer() {

public void initializeRealmDefaultScopes() {
ResourceServer server = initializeRealmResourceServer();
if (server == null) return;
manageScope = initializeRealmScope(MgmtPermissions.MANAGE_SCOPE);
viewScope = initializeRealmScope(MgmtPermissions.VIEW_SCOPE);
}

public Scope initializeRealmScope(String name) {
ResourceServer server = initializeRealmResourceServer();
if (server == null) return null;
Scope scope = authz.getStoreFactory().getScopeStore().findByName(server, name);
if (scope == null) {
scope = authz.getStoreFactory().getScopeStore().create(server, name);
Expand All @@ -291,6 +296,7 @@ public Scope initializeRealmScope(String name) {
}

public Scope initializeScope(String na 10000 me, ResourceServer server) {
if (authz == null) return null;
Scope scope = authz.getStoreFactory().getScopeStore().findByName(server, name);
if (scope == null) {
scope = authz.getStoreFactory().getScopeStore().create(server, name);
Expand Down
9 changes: 3 additions & 6 deletions ...ices/src/main/java/org/keycloak/services/resources/admin/permissions/RolePermissions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ private void disablePermissions(RoleModel role) {

@Override
public Map<String, String> getPermissions(RoleModel role) {
if (authz == null) return null;
initialize(role);
Map<String, String> scopes = new LinkedHashMap<>();
scopes.put(RolePermissionManagement.MAP_ROLE_SCOPE, mapRolePermission(role).getId());
Expand Down Expand Up @@ -123,9 +124,9 @@ public Policy mapClientScopePermission(RoleModel role) {

@Override
public Resource resource(RoleModel role) {
ResourceStore resourceStore = authz.getStoreFactory().getResourceStore();
ResourceServer server = resourceServer(role);
if (server == null) return null;
ResourceStore resourceStore = authz.getStoreFactory().getResourceStore();
return resourceStore.findByName(server, getRoleResourceName(role));
}

Expand Down Expand Up @@ -546,6 +547,7 @@ private void initialize(RoleModel role) {
if (server == null) {
ClientModel client = getRoleClient(role);
server = root.findOrCreateResourceServer(client);
if (server == null ) return;
}
Scope mapRoleScope = mapRoleScope(server);
if (mapRoleScope == null) {
Expand Down Expand Up @@ -602,11 +604,6 @@ private String getMapCompositePermissionName(RoleModel role) {
return MAP_ROLE_COMPOSITE_SCOPE + ".permission." + role.getId();
}

private ResourceServer sdfgetResourceServer(RoleModel role) {
ClientModel client = getRoleClient(role);
return root.findOrCreateResourceServer(client);
}

private static String getRoleResourceName(RoleModel role) {
return "role.resource." + role.getId();
}
Expand Down
7 changes: 4 additions & 3 deletions ...ices/src/main/java/org/keycloak/services/resources/admin/permissions/UserPermissions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class UserPermissions implements UserPermissionEvaluator, UserPermissionManageme
this.session = session;
this.authz = authz;
this.root = root;
if (Profile.isFeatureEnabled(Profile.Feature.AUTHORIZATION)) {
if (authz != null) {
policyStore = authz.getStoreFactory().getPolicyStore();
resourceStore = authz.getStoreFactory().getResourceStore();
} else {
Expand All @@ -94,9 +94,9 @@ class UserPermissions implements UserPermissionEvaluator, UserPermissionManageme


private void initialize() {
root.initializeRealmResourceServer();
ResourceServer server = root.initializeRealmResourceServer();
if (server == null) return;
root.initializeRealmDefaultScopes();
ResourceServer server = root.realmResourceServer();
Scope manageScope = root.realmManageScope();
Scope viewScope = root.realmViewScope();
Scope mapRolesScope = root.initializeRealmScope(MAP_ROLES_SCOPE);
Expand Down Expand Up @@ -144,6 +144,7 @@ private void initialize() {

@Override
public Map<String, String> getPermissions() {
if (authz == null) return null;
initialize();
Map<String, String> scopes = new LinkedHashMap<>();
scopes.put(AdminPermissionManagement.VIEW_SCOPE, viewPermission().getId());
Expand Down
6 changes: 1 addition & 5 deletions .../src/test/java/org/keycloak/testsuite/adapter/servlet/BrokerLinkAndTokenExchangeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@
@AppServerContainer(ContainerConstants.APP_SERVER_EAP6)
@AppServerContainer(ContainerConstants.APP_SERVER_EAP71)
@EnableFeature(value = Profile.Feature.TOKEN_EXCHANGE, skipRestart = true)
@EnableFeature(value = Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ, skipRestart = true)
public class BrokerLinkAndTokenExchangeTest extends AbstractServletsAdapterTest {
public static final String CHILD_IDP = "child";
public static final String PARENT_IDP = "parent-idp";
Expand All @@ -106,11 +107,6 @@ public class BrokerLinkAndTokenExchangeTest extends AbstractServletsAdapterTest
public static final String UNAUTHORIZED_CHILD_CLIENT = "unauthorized-child-client";
public static final String PARENT_CLIENT = "parent-client";

@BeforeClass
public static void enabled() {
ProfileAssume.assumeFeatureEnabled(Profile.Feature.AUTHORIZATION);
}

@Deployment(name = ClientApp.DEPLOYMENT_NAME)
protected static WebArchive accountLink() {
return servletDeployment(ClientApp.DEPLOYMENT_NAME, LinkAndExchangeServlet.class, ServletTestUtils.class);
Expand Down
7 changes: 0 additions & 7 deletions ...uillian/tests/base/src/test/java/org/keycloak/testsuite/admin/FineGrainAdminUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import org.hamcrest.Matchers;
import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.Test;
import org.keycloak.admin.client.Keycloak;
import org.keycloak.authorization.AuthorizationProvider;
Expand Down Expand Up @@ -53,7 +52,6 @@
import org.keycloak.services.resources.admin.permissions.ClientPermissionManagement;
import org.keycloak.services.resources.admin.permissions.GroupPermissionManagement;
import org.keycloak.testsuite.AbstractKeycloakTest;
import org.keycloak.testsuite.ProfileAssume;
import org.keycloak.testsuite.arquillian.annotation.EnableFeature;
import org.keycloak.testsuite.arquillian.annotation.UncaughtServerErrorExpected;
import org.keycloak.testsuite.auth.page.AuthRealm;
Expand Down Expand Up @@ -88,11 +86,6 @@ public class FineGrainAdminUnitTest extends AbstractKeycloakTest {

public static final String CLIENT_NAME = "application";

@BeforeClass
public static void enabled() {
ProfileAssume.assumeFeatureEnabled(Profile.Feature.AUTHORIZATION);
}

@Override
public void addTestRealms(List<RealmRepresentation> testRealms) {
RealmRepresentation testRealmRep = new RealmRepresentation();
Expand Down
9 changes: 2 additions & 7 deletions ...lian/tests/base/src/test/java/org/keycloak/testsuite/admin/ManagementPermissionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
*/
package org.keycloak.testsuite.admin;

import org.junit.BeforeClass;
import org.junit.Test;
import org.keycloak.admin.client.resource.ClientResource;
import org.keycloak.admin.client.resource.GroupResource;
Expand All @@ -25,7 +24,7 @@
import org.keycloak.common.Profile;
import org.keycloak.representations.idm.*;
import org.keycloak.testsuite.AbstractTestRealmKeycloakTest;
import org.keycloak.testsuite.ProfileAssume;
import org.keycloak.testsuite.arquillian.annotation.EnableFeature;

import javax.ws.rs.core.Response;

Expand All @@ -36,13 +35,9 @@
/**
* @author <a href="mailto:leon.graser@bosch-si.com">Leon Graser</a>
*/
@EnableFeature(value = Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ, skipRestart = true)
public class ManagementPermissionsTest extends AbstractTestRealmKeycloakTest {

@BeforeClass
public static void enabled() {
ProfileAssume.assumeFeatureEnabled(Profile.Feature.AUTHORIZATION);
}

@Override
public void configureTestRealm(RealmRepresentation testRealm) {

Expand Down
20 changes: 7 additions & 13 deletions ...tegration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UsersTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
import org.keycloak.representations.idm.authorization.PolicyRepresentation;
import org.keycloak.representations.idm.authorization.ScopePermissionRepresentation;
import org.keycloak.representations.idm.authorization.UserPolicyRepresentation;
import org.keycloak.testsuite.ProfileAssume;
import org.keycloak.testsuite.arquillian.annotation.EnableFeature;
import org.keycloak.testsuite.util.AdminClientUtil;

import java.io.IOException;
Expand Down Expand Up @@ -224,17 +224,15 @@ public void countUsersByFiltersWithViewPermission() {
}

@Test
@EnableFeature(value = Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ, skipRestart = true)
public void countUsersWithGroupViewPermission() throws CertificateException, NoSuchAlgorithmException, KeyStoreException, KeyManagementException, IOException {
ProfileAssume.assumeFeatureEnabled(Profile.Feature.AUTHORIZATION);

RealmResource testRealmResource = setupTestEnvironmentWithPermissions(true);
assertThat(testRealmResource.users().count(), is(3));
}

@Test
@EnableFeature(value = Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ, skipRestart = true)
public void countUsersBySearchWithGroupViewPermission() throws CertificateException, NoSuchAlgorithmException, KeyStoreException, KeyManagementException, IOException {
ProfileAssume.assumeFeatureEnabled(Profile.Feature.AUTHORIZATION);

RealmResource testRealmResource = setupTestEnvironmentWithPermissions(true);
//search all
assertThat(testRealmResource.users().count("user"), is(3));
Expand All @@ -256,9 +254,8 @@ public void countUsersBySearchWithGroupViewPermission() throws CertificateExcept
}

@Test
@EnableFeature(value = Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ, skipRestart = true)
public void countUsersByFiltersWithGroupViewPermission() throws CertificateException, NoSuchAlgorithmException, KeyStoreException, KeyManagementException, IOException {
ProfileAssume.assumeFeatureEnabled(Profile.Feature.AUTHORIZATION);

RealmResource testRealmResource = setupTestEnvironmentWithPermissions(true);
//search username
assertThat(testRealmResource.users().count(null, null, null, "user"), is(3));
Expand Down Expand Up @@ -293,17 +290,15 @@ public void countUsersByFiltersWithGroupViewPermission() throws CertificateExcep
}

@Test
@EnableFeature(value = Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ, skipRestart = true)
public void countUsersWithNoViewPermission() throws CertificateException, NoSuchAlgorithmException, KeyStoreException, IOException, KeyManagementException {
ProfileAssume.assumeFeatureEnabled(Profile.Feature.AUTHORIZATION);

RealmResource testRealmResource = setupTestEnvironmentWithPermissions(false);
assertThat(testRealmResource.users().count(), is(0));
}

@Test
@EnableFeature(value = Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ, skipRestart = true)
public void countUsersBySearchWithNoViewPermission() throws CertificateException, NoSuchAlgorithmException, KeyStoreException, KeyManagementException, IOException {
ProfileAssume.assumeFeatureEnabled(Profile.Feature.AUTHORIZATION);

RealmResource testRealmResource = setupTestEnvironmentWithPermissions(false);
//search all
assertThat(testRealmResource.users().count("user"), is(0));
Expand All @@ -325,9 +320,8 @@ public void countUsersBySearchWithNoViewPermission() throws CertificateException
}

@Test
@EnableFeature(value = Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ, skipRestart = true)
public void countUsersByFiltersWithNoViewPermission() throws CertificateException, NoSuchAlgorithmException, KeyStoreException, KeyManagementException, IOException {
ProfileAssume.assumeFeatureEnabled(Profile.Feature.AUTHORIZATION);

RealmResource testRealmResource = setupTestEnvironmentWithPermissions(false);
//search username
assertThat(testRealmResource.users().count(null, null, null, "user"), is(0));
Expand Down
9 changes: 1 addition & 8 deletions ...n/tests/base/src/test/java/org/keycloak/testsuite/oauth/ClientTokenExchangeSAML2Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

package org.keycloak.testsuite.oauth;

import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.Test;
import org.keycloak.OAuth2Constants;
Expand Down Expand Up @@ -54,7 +53,6 @@
import org.keycloak.testsuite.AbstractKeycloakTest;
import org.keycloak.testsuite.Assert;
import org.keycloak.testsuite.AssertEvents;
import org.keycloak.testsuite.ProfileAssume;
import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude;
import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer;
import org.keycloak.testsuite.arquillian.annotation.EnableFeature;
Expand All @@ -78,7 +76,6 @@
import java.util.Map;

import static org.junit.Assert.assertNotNull;
import static org.keycloak.common.Profile.Feature.AUTHORIZATION;
import static org.keycloak.models.ImpersonationSessionNote.IMPERSONATOR_ID;
import static org.keycloak.models.ImpersonationSessionNote.IMPERSONATOR_USERNAME;
import static org.keycloak.protocol.saml.SamlProtocol.SAML_ASSERTION_CONSUMER_URL_POST_ATTRIBUTE;
Expand All @@ -89,6 +86,7 @@
*/
@AuthServerContainerExclude(AuthServer.REMOTE)
@EnableFeature(value = Profile.Feature.TOKEN_EXCHANGE, skipRestart = true)
@EnableFeature(value = Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ, skipRestart = true)
public class ClientTokenExchangeSAML2Test extends AbstractKeycloakTest {

private static final String SAML_SIGNED_TARGET = "http://localhost:8080/saml-signed-assertion/";
Expand All @@ -104,11 +102,6 @@ public class ClientTokenExchangeSAML2Test extends AbstractKeycloakTest {
@Rule
public AssertEvents events = new AssertEvents(this);

@BeforeClass
public static void enabled() {
ProfileAssume.assumeFeatureEnabled(AUTHORIZATION);
}

@Override
public void addTestRealms(List<RealmRepresentation> testRealms) {
RealmRepresentation testRealmRep = new RealmRepresentation();
Expand Down
Loading
0