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.
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
Allow adding realm users as an organization member #29029
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @pedroigor for the PR. LGTM, I've just two minor comments.
model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProvider.java
Outdated
Show resolved
Hide resolved
...est/java/org/keycloak/testsuite/organization/admin/OrganizationMemberAuthenticationTest.java
Outdated
Show resolved
Hide resolved
auth.realm().requireManageRealm(); | ||
if (rep == null || !Objects.equals(rep.getUsername(), rep.getEmail())) { | ||
throw ErrorResponse.error("To add a member to the organization it is expected the username and the email is the same.", Status.BAD_REQUEST); | ||
UserModel user = session.users().getUserById(realm, id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the user is not a local user i.e user imported from LDAP or a custom user federation? (id in f:uuid:uuid format) Do we want to allow the user to become a member?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this handled by UserStorageManager
? I don't see any reason why don't allow any realm user to become a member.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martin-kanis please correct me if I am wrong ... I believe the question or concern might come from a fact that there is no control over those users who are not local keycloak users, so any update to those users (such as messing up with kc.org
attribute) might be done outside of Keycloak.
Would there be needed some mechanism to prevent/detect this kind of outside updates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For LDAP, we can do that as attributes like this one are very likely to be managed through a mapper. For custom providers, it will depend on how they are managing the user attributes.
Federated users (from custom providers) have their attributes stored in a specific table in Keycloak. I think we can handle such cases as well.
We can discuss this in a follow-up if you guys want as this PR is blocking others to go in. It is a very valid point and we need to see how to deal with custom user storages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, a mapper would probably be in place for cases like this if we are to propagate the persistence of the org attribute to LDAP. But the question still remains - if an admin changes the org in LDAP, how are we going to handle that? Will we need to validate these changes when re-importing/validating the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't allow creating a mapper to kc.org
we don't need to worry about this, I would say.
The real problem, I would say, is with custom user providers because they have access to internals and can easily bypass user profile or any other validation we enforce elsewhere.
outside of existing comments this all LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedroigor For me the PR looks good, thank you. The only remaining thing is resolving @martin-kanis's comment. As far as I'm concerned we can address it as a follow-up (which I would prefer to be created prior merging this one), but leaving it for @martin-kanis as he started the discussion.
@vramik @martin-kanis Anything else blocking you from finishing reviewing and approving? |
Follow-up created #29080 |
Thanks. Please, let me know if you want to change anything else so we can move forward. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Closes keycloak#29023 Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving based on reviews from @vramik @martin-kanis and @sguilhen
Closes #29023
kc.org
attribute set to users can not change if not managed through the Organization API