Support for granular permissions in Account Console by cgeorgilakis · Pull Request #10568 · keycloak/keycloak

cgeorgilakis · 2022-03-03T13:39:04Z

PR for supporting granular permissions in Account Console.

We propose to add two extra roles (‘manage-account-basic-auth’ and ‘manage-account-2fa’). A user from external Identity Provider can not be able to login as Keycloak User (add password) but he will be able to add two factor authedicator. However, we can remove them if you want or add others instead.

My PR works with adding a composite role containing all account roles in account-console client. I want to add all scopes of "account" client to "account-console" client in order not to needed adding extra role. However, such a change make testAccountConsoleClient method of AbstractMigrationTest works different for masterRealm and migrationRealm. What else change should I do? Could you help me with it?

cgeorgilakis · 2022-08-31T08:12:52Z

@ssilvert could you review this PR? It is serious problem that only manage-account users can view/manage anything in account console. We do not want to give users the possibility to change their basic info.

Do you want to rebase it? Do you want to remove extra account roles and discuss account roles in another issue?

sschu · 2022-08-31T10:29:42Z

@cgeorgilakis Have you looked at the user profile (https://www.keycloak.org/docs/latest/server_admin/#defining-a-user-profile?) You can use it to prevent users from changing a defined set of attributes.

cgeorgilakis · 2022-08-31T12:57:12Z

@sschu We have checked User profile feature in version 18 and it is interesting.

The main problem is that you can not change permissions in main attributes ( email, first name, last name).
Although this may be fixed in feature, this PR is a different needed part of Keycloak in order to account console work fine.
User with 'manage-account' role can do extra things with this role. F.e. he can add/change his password. User from external IdP must not login as Keycloak users with this trick. Moreover, User profile is not user specific. All users will have same functionality.

sschu · 2022-08-31T13:05:32Z

At least the configuration for main attributes was added, see #12620. You can also forbid changing password by disabling this action on the realm. But indeed, all of these things are realm-level settings.

cgeorgilakis · 2022-08-31T13:11:04Z

Good news for Keycloak 20 version!
Although User profile is a very good feature, this PR is needed for proper user specific roles.

ssilvert · 2022-09-03T18:43:31Z

Hi @cgeorgilakis, I definitely like the idea of having some fine-grained authorization in account console. Has this been discussed on the discussions board or anywhere else with members of the Keycloak team?

I was going to try running your code but there are conflicts. Can you resolve them?

cgeorgilakis · 2022-09-05T14:18:38Z

@ssilvert I will rebase, resolve conflicts and I will inform you.

I have open a github discussion where I have not given any reply after mentioning about this PR.
Moreover, github issue is opened from a person that is now Keycloak maintainer.

stianst · 2022-09-06T06:29:42Z

+import static org.keycloak.models.AccountRoles.MANAGE_ACCOUNT_2FA;
+import static org.keycloak.models.AccountRoles.MANAGE_ACCOUNT_BASIC_AUTH;


I'd rather have a single role "MANAGE_CREDENTIALS" than these two.

When user is from external IdP, he must not be able to add Keycloak password. However, he must be able to add two factor credentials. Can we block external users from login via Keycloak Username form in another way?

stianst · 2022-09-06T06:30:35Z

                isTotpConfigured = session.userCredentialManager().isConfiguredFor(realm, user, realm.getOTPPolicy().getType());
                RoleModel deleteAccountRole = realm.getClientByClientId(Constants.ACCOUNT_MANAGEMENT_CLIENT_ID).getRole(AccountRoles.DELETE_ACCOUNT);
                deleteAccountAllowed = deleteAccountRole != null && user.hasRole(deleteAccountRole) && realm.getRequiredActionProviderByAlias(DeleteAccount.PROVIDER_ID).isEnabled();
+                RoleModel manageAccount = realm.getClientByClientId(Constants.ACCOUNT_MANAGEMENT_CLIENT_ID).getRole(AccountRoles.MANAGE_ACCOUNT);


By adding scope on all the account client roles to account-console client the roles are available in the token and can be read directly by the admin console rather than having to pass these as variables in the landing page for the console.

Although I agree that this is best solution, I need Keycloak team help.
getToken() function exists in 'themes/src/main/resources/theme/keycloak.v2/account/src/app/keycloak-service/keycloak.service.ts' for getting token. How can I use this function or something similar in index.ftl in order to hide some parts of themes/src/main/resources/theme/keycloak.v2/account/resources/content.json? For tsx files, I believe change can be implemented.

@ssilvert @jonkoops @edewit anyone that can provide some pointers here?

@cgeorgilakis As you know, there is a "hidden" attribute on each ContentItem in content.json.

When the account console starts up, the value of that attribute is evaluated as javascript. So you should be able to call out to a function that reads the token from there. As long as your function returns a boolean value true, the ContentItem will be hidden.

stianst · 2022-09-06T06:31:16Z

@@ -0,0 +1,52 @@
+package org.keycloak.migration.migrators;


Need to update to MigrateTo20_0_0

Done during rebase

stianst · 2022-09-06T06:31:55Z

+            RoleModel manageAccount2fa = accountClient.addRole(MANAGE_ACCOUNT_2FA);
+            manageAccount2fa.setDescription("${role_" + MANAGE_ACCOUNT_2FA + "}");
+        }
+    }


Need to add scope on all roles in account client to account-console client

stianst

See comments. Haven't done a full review yet, but let's start with deciding what the credentials role should be, and have the account-console read roles directly from the token rather than passing as variables to the landing page.

cgeorgilakis · 2022-09-06T07:38:57Z

I have rebased it resolving conflicts. I will check tests.

sschu · 2022-09-21T12:51:42Z

Originally, manage-consent and view-consent were introduced for symmetry/consistency with other roles. I agree one of view-applications and view-consents is superfluous, but to keep the symmetry (view|manage)-consent or (view|manage)-applications could be used instead.

jonkoops · 2022-09-26T15:20:06Z

Let's not add commented out code.

Suggested change

// kc.hasAtLeastOneResourceRoles = function(resource, ...roles) {

// if (!kc.resourceAccess) {

// return false;

// }

//

// var access = kc.resourceAccess[resource || kc.clientId];

// if (!!access) {

// return roles.some(function(role) {

// return access.roles.indexOf(role) >= 0;

// });

// } else {

// return false;

// }

// }

Sorry, it is temporary.
Although Keycloak have been working with this code, some tests related to ui were failing.
I try in another way. Finnally, I will remove it.

jonkoops · 2022-09-26T15:23:15Z

The obj variable does not exist.

jonkoops · 2022-09-26T15:25:35Z

Since all cases here rely on the existence of the access variable, it would be best to add early return to avoid repeating logic.

if (!access) { return false; }

ok I will do the change

jonkoops · 2022-09-27T10:26:34Z

Since all of the branches of this statement terminate (the all return). There is no need to use else if or else.

Suggested change

if (!access) {

return false;

} else if (Array.isArray(role)) {

return role.some(function(r) {

return access.roles.indexOf(r) >= 0;

});

} else{

return !!access && access.roles.indexOf(role) >= 0;

}

if (!access) {

return false;

}

if (Array.isArray(role)) {

return role.some(function(r) {

return access.roles.indexOf(r) >= 0;

});

}

return access.roles.indexOf(role) >= 0;

jonkoops

The type definition for the hasResourceRole() method should be updated as well.

cgeorgilakis · 2022-09-28T06:45:28Z

The type definition for the hasResourceRole() method should be updated as well.

You are right. How could I define that role could be either String either String Array? Or should I create a seperate method?

@ssilvert @stianst I have kept interface features in account console. I have only changed that its values is taken from account role from user access token. Are we ok with this functionality?

jonkoops · 2022-09-29T13:41:20Z

You can provide the type as either a string, or a string array so string | string[].

cgeorgilakis · 2022-09-30T07:42:52Z

You can provide the type as either a string, or a string array so string | string[].

Done

jonkoops · 2022-10-05T08:09:38Z

Hmmm, taking a closer look at this change I am left wondering if it's really needed. If the consumer of this would like to check an array one could simply do the following:

const hasRole = roles.some(role => keycloak.hasResourceRole(role))

Apologies for the back an forth, but I think I'd rather keep the implementation of Keycloak JS itself unchanged. Could you remove this change from it?

Hmmm, taking a closer look at this change I am left wondering if it's really needed. If the consumer of this would like to check an array one could simply do the following:

const hasRole = roles.some(role => keycloak.hasResourceRole(role))

Apologies for the back an forth, but I think I'd rather keep the implementation of Keycloak JS itself unchanged. Could you remove this change from it?

@jonkoops I agree that it is more clear. My only thought was that keycloak.hasResourceRole will be call 2-3 times even if user has no account roles. However, this is rare. So I will procceed with requested change soon.

Closes keycloak#8734

cgeorgilakis · 2022-10-06T10:30:56Z

I have made requested change. keycloak.js does not change.

jonkoops

Changes for the front-end look good, can't say anything about the code changes on the Java side.

cgeorgilakis · 2022-10-13T09:28:12Z

@ssilvert @stianst Could we proceed with the review of this PR?
If desired account roles is the problem, we can keep them as it is and merging the PR without any change in account roles.
After it, I (or you) could open a github discussion about desired account roles. For us, it is essential to take into account that many users is getting from external Identity Providers. These users should not be able to add password, change their basic attributes received from external IdentityProvider.

ssilvert · 2023-02-21T14:52:50Z

@cgeorgilakis I'm sorry this is taking so long and I do owe you an explanation.

We are working on a refactored version of the account console that will use modern React (hooks instead of classes). Also, we are switching to Vite instead of using the dead project, Snowpack.

We've had this new version almost ready for quite awhile now but progress has been held up by our migration of all UI code from the keycloak/keycloak-ui repo to keycloak/keycloak.

So the main reason this PR has not gotten more traction is because it doesn't make sense to make big changes to account console V2 while we've been expecting V3 to come out soon. Unfortunately, soon has turned into much longer than anticipated.

Thanks for your incredible patience and again, I'm sorry I didn't explain this sooner. I do think it won't be long before the migration is complete. Then we can work to get fine grained permissions into V3.

ssilvert · 2024-05-02T19:16:52Z

@cgeorgilakis Just noticed this old PR. We have completed V3 of account console. So if you still want to get this done I suggest we start from scratch with a new PR. I'm going to close this one though.

DPicillo · 2025-03-18T16:22:35Z

😥

abstractj assigned ssilvert Mar 8, 2022

cgeorgilakis mentioned this pull request Sep 5, 2022

View groups from account console #7933

Merged

stianst reviewed Sep 6, 2022

View reviewed changes

stianst requested changes Sep 6, 2022

View reviewed changes

stianst assigned stianst and unassigned ssilvert Sep 6, 2022

cgeorgilakis force-pushed the 8734-account-console branch from 31e0a3a to 64e1d5d Compare September 6, 2022 07:36

cgeorgilakis force-pushed the 8734-account-console branch 3 times, most recently from 7d152ef to 697f579 Compare September 6, 2022 10:28

stianst requested a review from ssilvert September 6, 2022 10:53

cgeorgilakis force-pushed the 8734-account-console branch 2 times, most recently from beb7d8b to 17c4ef8 Compare September 20, 2022 11:30

jonkoops suggested changes Sep 26, 2022

View reviewed changes

cgeorgilakis force-pushed the 8734-account-console branch from a2c8f1d to dc9d5a6 Compare September 27, 2022 10:20

jonkoops suggested changes Sep 27, 2022

View reviewed changes

cgeorgilakis requested a review from ssilvert September 29, 2022 09:28

cgeorgilakis requested review from jonkoops and removed request for ssilvert September 30, 2022 07:43

jonkoops suggested changes Oct 5, 2022

View reviewed changes

cgeorgilakis force-pushed the 8734-account-console branch from a8cf426 to 141181f Compare October 5, 2022 09:39

Support for granular permissions in Account Console

5b2dcdd

Closes keycloak#8734

cgeorgilakis force-pushed the 8734-account-console branch from 96f5923 to 5b2dcdd Compare October 5, 2022 13:04

cgeorgilakis requested a review from jonkoops October 6, 2022 10:31

jonkoops approved these changes Oct 6, 2022

View reviewed changes

cgeorgilakis requested review from ssilvert and stianst and removed request for ssilvert October 6, 2022 11:18

cgeorgilakis requested review from ssilvert and stianst and removed request for ssilvert and stianst February 17, 2023 15:17

stianst removed their assignment Jul 26, 2023

stianst added team/core team/ui labels Jul 26, 2023

stianst added team/core-shared and removed team/core labels Feb 13, 2024

ssilvert self-assigned this Feb 13, 2024

ssilvert closed this May 2, 2024

		import static org.keycloak.models.AccountRoles.MANAGE_ACCOUNT_2FA;
		import static org.keycloak.models.AccountRoles.MANAGE_ACCOUNT_BASIC_AUTH;

-//     kc.hasAtLeastOneResourceRoles = function(resource, ...roles) {
-//            if (!kc.resourceAccess) {
-//                return false;
-//            }
-//
-//            var access = kc.resourceAccess[resource || kc.clientId];
-//            if (!!access) {
-//                return roles.some(function(role) {
-//                    return access.roles.indexOf(role) >= 0;
-//                });
-//            } else {
-//               return false;
-//            }
-//        }

-        if (!access) {
-            return false;
-        } else if (Array.isArray(role)) {
-            return role.some(function(r) {
-                return access.roles.indexOf(r) >= 0;
-            });
-        } else{
-            return !!access && access.roles.indexOf(role) >= 0;
-        }
+        if (!access) {
+            return false;
+        }
+        if (Array.isArray(role)) {
+            return role.some(function(r) {
+                return access.roles.indexOf(r) >= 0;
+            });
+        }
+        return access.roles.indexOf(role) >= 0;

Conversation

cgeorgilakis commented Mar 3, 2022

Uh oh!

cgeorgilakis commented Aug 31, 2022

Uh oh!

sschu commented Aug 31, 2022

Uh oh!

cgeorgilakis commented Aug 31, 2022

Uh oh!

sschu commented Aug 31, 2022

Uh oh!

cgeorgilakis commented Aug 31, 2022

Uh oh!

ssilvert commented Sep 3, 2022

Uh oh!

cgeorgilakis commented Sep 5, 2022

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!

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!

stianst left a comment

Choose a reason for hiding this comment

Uh oh!

cgeorgilakis commented Sep 6, 2022

Uh oh!

sschu commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

jonkoops left a comment

Choose a reason for hiding this comment

Uh oh!

cgeorgilakis commented Sep 28, 2022

Uh oh!

jonkoops commented Sep 29, 2022

Uh oh!

cgeorgilakis commented Sep 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgeorgilakis commented Oct 6, 2022

Uh oh!

jonkoops left a comment

Choose a reason for hiding this comment

Uh oh!

cgeorgilakis commented Oct 13, 2022

Uh oh!

ssilvert commented Feb 21, 2023

Uh oh!

ssilvert commented May 2, 2024

sschu commented Sep 21, 2022 •

edited

Loading