-
Notifications
You must be signed in to change notification settings - Fork 7.4k
Allow managing members of an organization #28057
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
Conversation
7df4bd3
to
faabb75
Compare
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 thank you for preparing this PR. I've added few comments, please see inline.
I've also checked the description of #27934, specifically I mean - Create a user as a member of an organization
. I believe the PR allows to Associate an existing user to an organization
atm.
Do you plan to update the issue description or do that part as follow up?
|
||
entity.setId(KeycloakModelUtils.generateId()); | ||
entity.setId(group.getId()); |
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.
I think it should not be used the same ID for two different objects.
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.
Yeah, I was not sure too because a 1:1 makes things a bit simpler while still making more clear the relationship between the two.
I'll change to an FK instead.
GroupModel group = groupProvider.getGroupByName(realm, null, name); | ||
|
||
if (group != null) { | ||
throw new IllegalArgumentException("A group with the same already exist and it is bound to different organization"); |
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.
The existing group doesn't have to be necessarily bound to a different organization, right? It might be misleading message. I'd keep there only that "A group with the same already exists."
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.
Not really, if the group exists with that name is because it is already bound to a organization.
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.
Probably it would be the case in most cases, but I guess there is nothing what prevents administrator to create or already have a regular group with any name.
Do you plan to restrict it in a way that group with kc.org.
prefix cannot exist unless it's an organization?
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.
Yes, the idea is to reserve such names exclusively to orgs.
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.
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.
I believe the IllegalArgumentException
should be replaced by ModelException
in the file.
JpaConnectionProvider jpaProvider = session.getProvider(JpaConnectionProvider.class); | ||
this.em = jpaProvider.getEntityManager(); | ||
groupProvider = session.groups(); |
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.
I've noticed the groupProvider
is used within createdOrganizationGroup
method, while in removeOrganization
there is session.groups()
used. I'd suggest to either use one or another approach.
String getId(); | ||
|
||
void setName(String name); | ||
|
||
String getName(); | ||
|
||
boolean addMember(UserModel 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.
I've noticed all other methods which touches managing members in Organization are declared in the OrganizationProvider
. What is reasoning behind a decision to have it here and not in the provider?
I agree it's more convenient to use this method rather than something like session.organizations().addMemberToOrganization(realm, organization, user)
. On the other hand having the implementation in single place (JpaOrganizationProvider
) helps with maintanability, IMO.
We could probably leverage the same approach what is used already in the codebase: Thereis e.g. this method in ClientModel
:
void addClientScopes(Set<ClientScopeModel> clientScopes, boolean defaultScope); |
and the implementation then forwards the call to the provider:
keycloak/model/jpa/src/main/java/org/keycloak/models/jpa/ClientAdapter.java
Lines 355 to 357 in adf838f
public void addClientScopes(Set<ClientScopeModel> clientScopes, boolean defaultScope) { | |
session.clients().addClientScopes(getRealm(), this, clientScopes, defaultScope); | |
} |
void addClientScopes(RealmModel realm, ClientModel client, Set<ClientScopeModel> clientScopes, boolean defaultScope); |
and the implementation lives inside a provider implementation.
|
||
Stream<UserModel> getMembersStream(RealmModel realm, OrganizationModel model); | ||
|
||
UserModel getMemberById(RealmModel realm, OrganizationModel organization, String id); | ||
|
||
OrganizationModel getOrganizationByMember(RealmModel realm, UserModel model); |
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.
Do you want to add a javadoc for these methods now, or rather as follow-up?
public boolean addMember(UserModel member) { | ||
RealmModel realm = session.getContext().getRealm(); | ||
GroupModel group = session.groups().getGroupById(realm, entity.getId()); | ||
|
||
if (member.isMemberOf(group)) { | ||
return false; | ||
} | ||
|
||
member.joinGroup(group); | ||
member.setSingleAttribute(USER_ORGANIZATION_ATTRIBUTE, group.getId()); | ||
|
||
return true; | ||
} |
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.
I believe there is missing check that the user doesn't belong to some different organization already.
I don't have plans to update the description or have any follow-ups. From a user perspective, you are creating a user as an organization member. Internally, as we are leveraging the User API we are always adding an existing user to an organization. |
Alright, makes sense. I just wanted to be sure we are not forgetting something. Thank you! |
Closes keycloak#27934 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.
Thank you @pedroigor. As agreed for remaining comments I've either created a follow up issues or I'll refactor some code within #27993
<column name="GROUP_ID" type="VARCHAR(255)"> | ||
<constraints nullable="false"/> | ||
</column> |
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.
Now that it's changed to FK. Do we want to introduce a addForeignKeyConstraint
?
UPDATE: it's been decided to address this in follow-up issue: #28131
|
||
private final OrganizationEntity entity; | ||
private final KeycloakSession session; |
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.
Since the addMember
method was removed, it seems the session
is not used anywhere, would it make sense to keep it here?
UPDATE: As agreed on internal chat, I'll fix that within #27993
GroupModel group = groupProvider.getGroupByName(realm, null, name); | ||
|
||
if (group != null) { | ||
throw new IllegalArgumentException("A group with the same already exist and it is bound to different organization"); |
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.
I am trying to add a user in organization using rest admin api URL: Body:
but I am getting 400 error code with messgae
|
I managed to get this working in a .net web api
|
@zsafder did you solve this? |
Closes #27934