KEYCLOAK-10695 - fixed view permissions of a group by br0xpl · Pull Request #6114 · keycloak/keycloak

br0xpl · 2019-06-21T14:35:49Z

Hi,
According to the issue discribed here:
https://issues.jboss.org/browse/KEYCLOAK-10695
I'm sending a PR with a proposition of fix.

Please let me know if this seems good?

pedroigor

Thanks, @br0xpl. Overall, it looks good to me.

However, there is something that I think you are missing, pagination. If the parameters used for pagination (first and max) are provided you will end up enforcing access for a specific page and, eventually, responding with less or no entries although you may have groups in other pages that the user has access.

We had the same issue with the users pagination which we end up doing some hacks to get this working. For instance, getting the groups beforehand prior to making the query.

For groups, we would need something similar or any other more clever logic.

br0xpl · 2019-06-26T22:27:33Z

Thanks, @br0xpl. Overall, it looks good to me.

However, there is something that I think you are missing, pagination. If the parameters used for pagination (first and max) are provided you will end up enforcing access for a specific page and, eventually, responding with less or no entries although you may have groups in other pages that the user has access.

We had the same issue with the users pagination which we end up doing some hacks to get this working. For instance, getting the groups beforehand prior to making the query.

For groups, we would need something similar or any other more clever logic.

Hi!
I've done the filtering just like in Users case, where all groups are fetched from db each time. This fixes the issue and it's similar to what is in Users but in my opinion we should open another issue to fix the pagination (as I wrote in the comment in the source file).

@pedroigor What do you think?

Thanks.

br0xpl · 2019-06-27T09:06:11Z

@pedroigor Another thing is that the travis build failed but because of some maven dependencies being unresolvable (forbidden).. seems some problem with maven artifactory rather than with the PR..

Can we somehow rerun the travis build?

abstractj · 2019-06-27T11:10:59Z

@pedroigor Another thing is that the travis build failed but because of some maven dependencies being unresolvable (forbidden).. seems some problem with maven artifactory rather than with the PR..

Can we somehow rerun the travis build?

That's unrelated with this PR, we are trying to figure how to fix this. Meanwhile, I will try to trigger the job again.

pedroigor · 2019-07-02T12:08:18Z

Thanks @br0xpl . Like we discussed, it is indeed the most obvious solution although not the most optimal. You also need to provide some tests but before doing so, I will ask @mposolda for his feedback about the current solution.

Regarding the other options to pagination, when doing users pagination I also had the same ideas. I think #2 is the best one as it does keep clients simple and requires some relatively simple work on the server side.

mposolda

+1 for the approach instead of handle pagination on the client. I added few more comments inline.

mposolda · 2019-07-02T13:22:47Z

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

-            results = ModelToRepresentation.searchForGroupByName(realm, search.trim(), firstResult, maxResults);
-        } else if(Objects.nonNull(firstResult) && Objects.nonNull(maxResults)) {
-            results = ModelToRepresentation.toGroupHierarchy(realm, false, firstResult, maxResults);
+            results = realm.searchForGroupByName(search.trim(),null, null);


Shouldn't be here "firstResult, maxResults" instead of "null, null" ?

Indeed. @br0xpl, could you consider both firstResult and maxResults when searching by name ?

The problem is we are getting maxResults then and some of the groups might get filtered out. So we would have to:
a) fetch maxResults in a loop until we get maxResults after filtering
b) fetch all
I chose b - but if you prefer a just let me know?

@pedroigor , @mposolda : let me know how would you see it done?

But indeed my code misses the filtering first and max in after filtering - this has to be done.

Filter first and max results out of the filtered results

@br0xpl, another option would be:

Check if the user has permissions to view groups (granted by view-users or manage-users)

If not, obtain the groups that have permission enabled and return those that the user can view

You can then use the list of groups to filter the results when searching by name

Look at this code in users

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

Line 239 in b4b3527

if (!auth.users().canView()) {

. We set the list of groups that the user has view permission in the KeycloakSession. Then we use this list to filter results in the database.

You should also consider filtering top level groups. Just like it is done today.

@br0xpl, see https://github.com/pedroigor/keycloak/tree/KEYCLOAK-10695. Similar to users. But still missing the top-level group pagination. Just a quick work to give you some ideas :)

@pedroigor , @mposolda : I've fixed it according to your description - please see it.

I've used the getGroupsWithViewPermission function of GroupPermissions but I have a doubt about it's behaviour, see here:

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

Lines 306 to 329 in b4b3527

@Override

public Set<String> getGroupsWithViewPermission() {

if (root.users().canView() || root.users().canManage()) return Collections.emptySet();

if (!root.isAdminSameRealm()) {

return Collections.emptySet();

}

ResourceServer server = root.realmResourceServer();

if (server == null) {

return Collections.emptySet();

}

Set<String> granted = new HashSet<>();

resourceStore.findByType("Group", server.getId(), resource -> {

if (hasPermission(resource, null, VIEW_MEMBERS_SCOPE, MANAGE_MEMBERS_SCOPE)) {

granted.add(resource.getName().substring(RESOURCE_NAME_PREFIX.length()));

}

});

return granted;

}

and line 323 - it is checking if user has manage _members or view _members permission. Shouldn't it be checking if user has view or manage permission for that group? The function name suggest just "view", not view_members..?

Seems we need to either change function name and create another function with view or we need to change this function?

We can change the method name. Its intent is to obtain the groups that a user has either view or manage members permission. Good catch.

I've done that. I moved the common part to generic getGroupsWithPermissions(String... scopes) function and created two functions: getGroupsWithViewPermission /new function/, and renamed the previous: getGroupsWithViewMembersPermission.

@pedroigor : please review all the changes -> I've tested and the groups work as expected now.

mposolda · 2019-07-02T13:28:31Z

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

+
        if (Objects.nonNull(search)) {
-            results = realm.getGroupsCountByNameContaining(search);
+            results = realm.searchForGroupByName(search, null, null);


This is potential performance killer, but I don't have better idea how to address this. So I assume we need to live with that. Fortunately there will be thousands of groups at most (not millions) and it is just about admin requests, so hopefully it is ok.

IMO in general, it is probably better to have pagination approach, which doesn't require counts. Something, which always returns the token for next page? Although this approach also has limitations (EG. it is not easily possible to move to the exact page like "Go directly to page 50" etc), and there are probably other limitations. But maybe something to consider in the design of new REST API (CC @pedroigor ). Sorry for hijack the PR :)

@mposolda, the guideline is now updated with the pagination practice. In a nutshell, we are keeping the existing behavior when paginating results with small improvements on how we communicate whether or not the client reached the last page. We can discuss more there :)

mposolda · 2019-07-02T13:29:06Z

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

+         * TODO: IMHO we should consider different pagination technique:
+         * (1) REST return filtered results (in some cases even epty page) and JS
+         * client retrieves next pages until it gets all page of results or reaches the end of list
+         * (2) or we fetch results in backend page by page until we get all required results.


+1 for this

pedroigor · 2019-07-04T13:28:10Z

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

-            results = ModelToRepresentation.searchForGroupByName(realm, search.trim(), firstResult, maxResults);
-        } else if(Objects.nonNull(firstResult) && Objects.nonNull(maxResults)) {
-            results = ModelToRepresentation.toGroupHierarchy(realm, false, firstResult, maxResults);
+            results = realm.searchForGroupByName(search.trim(),null, null);


Indeed. @br0xpl, could you consider both firstResult and maxResults when searching by name ?

pedroigor

Only a few comments. We should also consider adding a test

keycloak/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/group/GroupTest.java

Line 676 in 7654793

public void searchAndCountGroups() throws Exception {

.

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

pedroigor · 2019-07-29T13:38:06Z

@br0xpl, this test is failing GroupTest.searchAndCountGroups.

…ionsResource adding delete and update methods.

br0xpl · 2019-08-12T21:15:15Z

@pedroigor: Thanks for your valuable comments. I've added a test for the fine grained permission. While writing the test I've found some methods missing in the keycloak-admin-client code - (update and delete in Scope/Resource PermissionsResource classes) - I've added them.

Please verify - thanks!

abstractj · 2019-08-15T10:55:43Z

@br0xpl I believe there were some changes in master, which is causing some conflicts with your changes. Could you please rebase?

pedroigor · 2019-08-15T12:03:43Z

...min-client/src/main/java/org/keycloak/admin/client/resource/ResourcePermissionsResource.java

    Response create(ResourcePermissionRepresentation representation);

    @Path("{id}")
+    @PUT


These changes are not needed because you have these same methods in org.keycloak.admin.client.resource.ResourcePermissionResource.

pedroigor · 2019-08-15T12:04:35Z

.../admin-client/src/main/java/org/keycloak/admin/client/resource/ScopePermissionsResource.java

    Response create(ScopePermissionRepresentation representation);

    @Path("{id}")
+    @PUT


These changes are not needed because you have these same methods in org.keycloak.admin.client.resource.ScopePermissionResource.

pedroigor · 2019-08-15T12:13:34Z

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

-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
+import java.util.*;


Please, don't use wildcards. Import specific types instead. This change is not really necessary. The new types you are importing are fine.

pedroigor · 2019-08-15T12:23:10Z

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

-        }
+            Set<String> groups = auth.groups().getGroupsWithViewPermission();
+            Stream<String> s = groups.stream();
+            if (search!=null) s=s.filter(group->group.contains(search.trim()));


Don't you have to search by name? Also, I think you should consider filtering by name just like the [code](https://github.com/pedroigor/keycloak/commit/81e718bf9af893d335c4cb50ddc369c813b0ed26) I shared with you.

pedroigor · 2019-08-15T12:32:21Z

...ration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/group/GroupTest.java

-import org.keycloak.admin.client.resource.RealmResource;
-import org.keycloak.admin.client.resource.RoleMappingResource;
-import org.keycloak.admin.client.resource.UsersResource;
+import org.keycloak.admin.client.resource.*;


Same thing here in regards to import statements. Prefer importing types. Most of these changes to import statements can be avoided.

pedroigor · 2019-08-15T12:40:34Z

@br0xpl, thanks again for your work. In addition to rebasing your branch, could you also squash your commits so that we have a single one ?

br0xpl · 2019-10-13T18:15:44Z

@pedroigor, @abstractj : I'll try to come back and finish this once again. @pedroigor I sorry I missed your contribution, I'll make sure to take it as indeed we need to search by the name of course. Please give me some time to fix this and squash all the changes together. Thanks!

abstractj

@br0xpl this PR has some conflicts with master at the moment. Could you please follow up on @pedroigor's considerations and rebase your changes?

stianst · 2020-06-15T11:07:11Z

Closing this as out of date due to lack of feedback

KEYCLOAK-10695 - fixed view permissions of a group

2a02796

stianst assigned abstractj Jun 24, 2019

stianst requested a review from pedroigor June 24, 2019 08:16

pedroigor requested changes Jun 24, 2019

View reviewed changes

Fixed pagination and subgroups permission check.

f38c234

pedroigor requested a review from mposolda July 2, 2019 12:08

mposolda reviewed Jul 2, 2019

View reviewed changes

pedroigor requested changes Jul 4, 2019

View reviewed changes

abstractj requested review from mposolda and pedroigor July 18, 2019 13:52

Adamczyk Błażej added 2 commits July 24, 2019 10:14

Fixed filtering according to discussion.

aff70a1

Fixed getGroupsWithPermissions methods in GroupPermissions class.

a1f303c

pedroigor requested changes Jul 29, 2019

View reviewed changes

services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java Show resolved Hide resolved

services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java Outdated Show resolved Hide resolved

Fixed count method. Added tests. Fixed keycloak-admin-client *Permiss…

fc6d406

…ionsResource adding delete and update methods.

abstractj requested a review from pedroigor August 15, 2019 10:54

pedroigor requested changes Aug 15, 2019

View reviewed changes

abstractj requested changes Oct 31, 2019

View reviewed changes

stianst added the Prioritize label Nov 6, 2019

abstractj requested a review from douglaspalmer January 10, 2020 13:04

stianst added this to the 10.0.0 milestone Mar 17, 2020

stianst removed this from the 10.0.0 milestone Apr 21, 2020

stianst added this to the 11.0.0 milestone Apr 21, 2020

stianst closed this Jun 15, 2020

	@Override
	public Set<String> getGroupsWithViewPermission() {
	if (root.users().canView() \|\| root.users().canManage()) return Collections.emptySet();

	if (!root.isAdminSameRealm()) {
	return Collections.emptySet();
	}

	ResourceServer server = root.realmResourceServer();

	if (server == null) {
	return Collections.emptySet();
	}

	Set<String> granted = new HashSet<>();

	resourceStore.findByType("Group", server.getId(), resource -> {
	if (hasPermission(resource, null, VIEW_MEMBERS_SCOPE, MANAGE_MEMBERS_SCOPE)) {
	granted.add(resource.getName().substring(RESOURCE_NAME_PREFIX.length()));
	}
	});

	return granted;
	}

Conversation

br0xpl commented Jun 21, 2019

Uh oh!

pedroigor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

br0xpl commented Jun 26, 2019

Uh oh!

br0xpl commented Jun 27, 2019

Uh oh!

abstractj commented Jun 27, 2019

Uh oh!

pedroigor commented Jul 2, 2019

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

br0xpl Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedroigor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pedroigor commented Jul 29, 2019

Uh oh!

br0xpl commented Aug 12, 2019

Uh oh!

abstractj commented Aug 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedroigor commented Aug 15, 2019

Uh oh!

br0xpl commented Oct 13, 2019

Uh oh!

abstractj left a comment

Choose a reason for hiding this comment

Uh oh!

stianst commented Jun 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

pedroigor left a comment •

edited

Loading

br0xpl Jul 24, 2019 •

edited

Loading