KEYCLOAK-18061 On Linked account, paged listing of IdPs #8022

laskasn · 2021-05-10T15:16:40Z

more details here or here

mposolda

@laskasn Thanks for the contribution! And sorry it took so long to review :-(

I've added inline comment regarding the backwards compatibility of admin REST API. Also it will be good to agree on the admin REST API format with @jonkoops from UI team, who had some ideas for improving pagination to allow better consume it in UI.

mposolda · 2023-08-01T10:17:12Z

services/src/main/java/org/keycloak/services/resources/account/LinkedAccountsResource.java

+        final String term = keyword.toLowerCase().trim();
+        List<LinkedAccountRepresentation> accounts = realm.getIdentityProvidersStream().filter(IdentityProviderModel::isEnabled)
+                .filter(idp -> {
+                    if(keyword.isEmpty())


Suggested change

if(keyword.isEmpty())

if (term.isEmpty())

Term might be better as it is already trimmed. In case keyword is something like " ", it doesn't need to be filtered IMO

mposolda · 2023-08-01T10:22:11Z

services/src/main/java/org/keycloak/services/resources/account/LinkedAccountsResource.java

+                })
+                .map(provider -> toLinkedAccountRepresentation(provider, socialIds, federatedIdentities))
+                .filter(rep -> {
+                    return linked ? rep.isConnected() : !rep.isConnected();


For backwards compatibility, it will be good to preserve same behaviour like before when linked parameter is not present when this endpoint is invoked. This means that when linked parameter is not present, all identity providers should be included, when unlinked would have empty linkedUsername field inside the JSON.

IMO it should be fine as in case you have linked=true, you can include only linked and when linked=false, you can include only unlinked.

For backwards compatibility, it would be also good to keep the same response format in case that linked parameter is omited (which means to avoid wrapping response data within the ResultSet, but just return the list as it was before).

So here is the question if it is rather clear to introduce new endpoint for the new format of data (perhaps something like /v2).

@jonkoops WDYT? In account REST API, we don't yet have any pagination, so perhaps here is possibly good to start with some cleaner API from the beginning?

I think a V2 API is needed, but it doesn't have to happen right now. Rather than getting it stamped out quickly I'd prefer we take a moment to sit down and spec it out. I am especially interested in taking strong approach on having a good schema, so that we can generate API clients, rather than having to keep maintaining one.

For backwards compatibility, it will be good to preserve same behaviour like before when linked parameter is not present when this endpoint is invoked. This means that when linked parameter is not present, all identity providers should be included, when unlinked would have empty linkedUsername field inside the JSON.

IMO it should be fine as in case you have linked=true, you can include only linked and when linked=false, you can include only unlinked.

For backwards compatibility, it would be also good to keep the same response format in case that linked parameter is omited (which means to avoid wrapping response data within the ResultSet, but just return the list as it was before).

So here is the question if it is rather clear to introduce new endpoint for the new format of data (perhaps something like /v2).

@jonkoops WDYT? In account REST API, we don't yet have any pagination, so perhaps here is possibly good to start with some cleaner API from the beginning?

@mposolda So you want to have old response (all Identity Providers) if linked is empty, don't you?
We do not have problem. In new account console linked will be either true either false.

@jonkoops So are you ok with have this PR in relatively quickly with the approach like in current API and then at some point, move everything to V2? For me, personally it would work.

@cgeorgilakis @jonkoops I have just one additional point regarding API. Sorry I did not bringed it before, but it came to me when looking at some related issue with groups pagination in admin console...

I vote that we remove the ResultSet with the variable totalHits from this PR, but we keep returning just the sorted list of links as we did until now. This means that UI/client won't know the counts . That is the same approach like we have for instance for resources (in ResourcesService class).

My point is, that having proper "counts" can be very tricky and expensive in some cases. For instance if there are fine-grained permissions for identity providers, you cannot automatically just use 1 simple DB query for select count(*) from identity_providers where realm = 'my_realm', but every identity-provider may be evaluated separately to know if the current user has permission to view it. This is very expensive and hence non-realistic to have proper counts if there are 1000s of identity providers.

Right now, we don't support fine-grained permissions for identity providers yet AFAIK. But if we later want to add them, this can hit us, as we might need to keep counts (for backwards compatibility), which could be performance killer.

In general, I would say that for pagination, it is usually sufficient information to know if next page exists? In this case, you can display arrow (next page). The total number of results and total number of pages is not needed then... AFAIK we already do something like this in the admin console for various objects (EG. clients).

@mposolda I disagree with last comment.

We are in account console (Identity Provider linking). User must be able to see and link with all Identity Providers he could.
Now, he could not only lick his account with a disable Identity Provider. Maybe in future a new config attribute need to be added. However, fine-grained permissions is only for admin console - not for account console.

The only thing that we may investigate is adding new method in RealmModel for filtering and count.
Stream<IdentityProviderModel> filterIdentityProvidersStream(boolean linked, List<String> federatedAlias, String keyword, Integer firstResult, Integer maxResults)

So are you ok with have this PR in relatively quickly with the approach like in current API and then at some point, move everything to V2? For me, personally it would work.

Yes, this is not an issue. I would prefer we land this sooner, and start thinking about a V2 API later (perhaps a nice topic for a face-to-face).

As for the pagination issue, we can deal with pagination without a count, but it's obviously a less than ideal user-experience.

@jonkoops Thanks. Yes, agree that we can discuss it later and could be good topic for face-to-face

@cgeorgilakis @jonkoops Yes, we don't support fine-grained permissions for account console (yet). Also we don't support fine-grained admin permissions for identity providers (yet).
My take is, that we support fine-grained admin permissions for some other objects (like clients for instance) and for those, we are not able to effectively compute correct count.

In theory, we can support count only for objects, which do not support fine-grained permissions. But IMO this approach has potential issues with:

APIs won't be compatible if some objects support count and some are not

It is also strange to me if admin REST API doesn't support count when account REST API supports count

We may want later to add fine-grained permissions for some objects (EG. for identity-providers). This means that even if count is easily possible now, it may not be possible later to effectively compute it. But removing stuff from the API is much harder and more expensive (broken backwards compatibility etc) than avoid to add it from the beginning.

That's my take for why to rather avoid to support count .

@cgeorgilakis I can also add that for pagination support, you don't need to have count (or totalHits support)? UI doesn't need to know the exact count of items or exact count of pages. It just needs to know that there is next page, so that UI can display "arrow" for next page? We do the same already for instance for clients in the admin console or for resources in the account console. So you can possibly look here for some inspiration thought?

cgeorgilakis · 2023-08-02T08:48:31Z

I have done the requested change and I have rebased to current master.

Maybe I should also change the message based on related new github issue.

jonkoops

We should probably also include this functionality for the V3 account console?

mposolda

@jonkoops So do I understand correctly, that every account console UI change now needs to be duplicated in both V2 and V3 account console?

mposolda · 2023-08-03T15:04:10Z

services/src/main/java/org/keycloak/services/resources/account/LinkedAccountsResource.java

+                })
+                .map(provider -> toLinkedAccountRepresentation(provider, socialIds, federatedIdentities))
+                .filter(rep -> {
+                    return linked ? rep.isConnected() : !rep.isConnected();


@jonkoops So are you ok with have this PR in relatively quickly with the approach like in current API and then at some point, move everything to V2? For me, personally it would work.

@cgeorgilakis @jonkoops I have just one additional point regarding API. Sorry I did not bringed it before, but it came to me when looking at some related issue with groups pagination in admin console...

I vote that we remove the ResultSet with the variable totalHits from this PR, but we keep returning just the sorted list of links as we did until now. This means that UI/client won't know the counts . That is the same approach like we have for instance for resources (in ResourcesService class).

My point is, that having proper "counts" can be very tricky and expensive in some cases. For instance if there are fine-grained permissions for identity providers, you cannot automatically just use 1 simple DB query for select count(*) from identity_providers where realm = 'my_realm', but every identity-provider may be evaluated separately to know if the current user has permission to view it. This is very expensive and hence non-realistic to have proper counts if there are 1000s of identity providers.

Right now, we don't support fine-grained permissions for identity providers yet AFAIK. But if we later want to add them, this can hit us, as we might need to keep counts (for backwards compatibility), which could be performance killer.

In general, I would say that for pagination, it is usually sufficient information to know if next page exists? In this case, you can display arrow (next page). The total number of results and total number of pages is not needed then... AFAIK we already do something like this in the admin console for various objects (EG. clients).

cgeorgilakis · 2023-08-04T14:20:09Z

I have keep the same behaviour like before when linked parameter is not present when this endpoint is invoked - as requested.

jonkoops · 2023-08-04T16:03:24Z

So do I understand correctly, that every account console UI change now needs to be duplicated in both V2 and V3 account console?

@mposolda we're going to be pushing v3 of the Account Console as the default in ~2 majors. So yeah, any changes for the Account Console should land there as well. In my opinion, if it's an enhancement I would even go as far as not to implement it in v2 unless absolutely necessary, but I will defer that judgement to @ssilvert.

ssilvert · 2023-08-07T18:16:17Z

So do I understand correctly, that every account console UI change now needs to be duplicated in both V2 and V3 account console?

@mposolda we're going to be pushing v3 of the Account Console as the default in ~2 majors. So yeah, any changes for the Account Console should land there as well. In my opinion, if it's an enhancement I would even go as far as not to implement it in v2 unless absolutely necessary, but I will defer that judgement to @ssilvert.

Right. We are trying to get rid of Account V2 ASAP. We don't want anything new in V2 if we can avoid it.

jonkoops · 2023-08-21T17:08:25Z

We are trying to get rid of Account V2 ASAP. We don't want anything new in V2 if we can avoid it.

Pretty much yeah.

cgeorgilakis · 2023-11-07T09:45:59Z

Account V2 ASAP

@ssilvert any progress in Account V2 ASAP?
Where can I see it?

ssilvert · 2023-11-07T19:09:10Z

Account V2 ASAP

@ssilvert any progress in Account V2 ASAP? Where can I see it?

@cgeorgilakis I'm not sure what you mean. We have effectively stopped development of Account Console V2. So the changes to V2 in this PR should be removed.

If everyone is ready to go with the rest of this PR, then UI changes need to go in Account Console V3, along with new tests.

Source for Account Console V3 lives under /js/apps/account-ui

jonkoops

This PR will have to be updated to target the v3 version of the Account Console instead.

jonkoops · 2024-01-02T11:38:38Z

Closing this PR due to lack of activity on pending review comments. Feel free to let us know if there is still interest in landing this, and we can re-open the PR.

laskasn force-pushed the #50 branch 4 times, most recently from 53d901c to 3a3d0ff Compare October 21, 2021 08:21

laskasn force-pushed the #50 branch 2 times, most recently from 9f77a9c to a36ab7a Compare May 5, 2022 17:11

laskasn mentioned this pull request May 5, 2022

Linked account - paged listing of IdPs #11867

Open

laskasn force-pushed the #50 branch from a36ab7a to 4071349 Compare May 7, 2022 00:46

cgeorgilakis mentioned this pull request Jun 21, 2023

Keycloak being able to handle 1000+ identity providers in a realm #21071

Open

mposolda linked an issue Jun 27, 2023 that may be closed by this pull request

Identity providers: Pagination in account console (and account REST API) #21261

Open

stianst added the team/core label Jul 17, 2023

mposolda requested changes Aug 1, 2023

View reviewed changes

mposolda self-assigned this Aug 1, 2023

mposolda requested a review from jonkoops August 1, 2023 10:45

mposolda added the team/ui label Aug 1, 2023

cgeorgilakis requested review from a team as code owners August 1, 2023 13:31

cgeorgilakis requested a review from a team August 1, 2023 13:31

cgeorgilakis requested a review from a team as a code owner August 1, 2023 13:31

ghost added team/cloud-native team/store labels Aug 1, 2023

cgeorgilakis force-pushed the #50 branch 2 times, most recently from cfc2a2e to dc38946 Compare August 2, 2023 06:44

jonkoops reviewed Aug 2, 2023

View reviewed changes

mposolda reviewed Aug 3, 2023

View reviewed changes

KEYCLOAK-18061 On Linked account, paged listing of IdPs

29007c1

cgeorgilakis force-pushed the #50 branch from bb57690 to 29007c1 Compare August 4, 2023 14:17

jonkoops requested changes Nov 14, 2023

View reviewed changes

jonkoops closed this Jan 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEYCLOAK-18061 On Linked account, paged listing of IdPs #8022

KEYCLOAK-18061 On Linked account, paged listing of IdPs #8022

laskasn commented May 10, 2021 •

edited

mposolda left a comment •

edited

mposolda Aug 1, 2023

mposolda Aug 1, 2023

jonkoops Aug 1, 2023

cgeorgilakis Aug 1, 2023

mposolda Aug 3, 2023

cgeorgilakis Aug 4, 2023

jonkoops Aug 4, 2023

mposolda Aug 7, 2023

mposolda Aug 7, 2023

cgeorgilakis commented Aug 2, 2023 •

edited

jonkoops left a comment

mposolda left a comment

mposolda Aug 3, 2023

cgeorgilakis commented Aug 4, 2023

jonkoops commented Aug 4, 2023

ssilvert commented Aug 7, 2023

jonkoops commented Aug 21, 2023

cgeorgilakis commented Nov 7, 2023

ssilvert commented Nov 7, 2023

jonkoops left a comment

jonkoops commented Jan 2, 2024

KEYCLOAK-18061 On Linked account, paged listing of IdPs #8022

KEYCLOAK-18061 On Linked account, paged listing of IdPs #8022

Conversation

laskasn commented May 10, 2021 • edited

mposolda left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgeorgilakis commented Aug 2, 2023 • edited

jonkoops left a comment

Choose a reason for hiding this comment

mposolda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgeorgilakis commented Aug 4, 2023

jonkoops commented Aug 4, 2023

ssilvert commented Aug 7, 2023

jonkoops commented Aug 21, 2023

cgeorgilakis commented Nov 7, 2023

ssilvert commented Nov 7, 2023

jonkoops left a comment

Choose a reason for hiding this comment

jonkoops commented Jan 2, 2024

laskasn commented May 10, 2021 •

edited

mposolda left a comment •

edited

cgeorgilakis commented Aug 2, 2023 •

edited