8000 Allow managing members of an organization by pedroigor · Pull Request #28057 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

pedroigor
Copy link
Contributor
@pedroigor pedroigor commented Mar 19, 2024

Closes #27934

@pedroigor pedroigor force-pushed the issue-27934 branch 4 times, most recently from 7df4bd3 to faabb75 Compare March 19, 2024 20:01
@pedroigor pedroigor marked this pull request as ready for review March 19, 2024 20:01
@pedroigor pedroigor requested review from a team as code owners March 19, 2024 20:01
@pedroigor pedroigor changed the title Allow managing members for an organization Allow managing members of an organization Mar 20, 2024
@vramik vramik self-requested a review March 20, 2024 09:31
Copy link
Contributor
@vramik vramik left a 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());
Copy link
Contributor

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.

Copy link
Contributor Author

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");
Copy link
Contributor
@vramik vramik Mar 20, 2024

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."

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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();
Copy link
Contributor

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);
Copy link
Contributor

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:

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.

Comment on lines 62 to 67

Stream<UserModel> getMembersStream(RealmModel realm, OrganizationModel model);

UserModel getMemberById(RealmModel realm, OrganizationModel organization, String id);

OrganizationModel getOrganizationByMember(RealmModel realm, UserModel model);
Copy link
Contributor

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?

Comment on lines 62 to 74
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;
}
Copy link
Contributor

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.

@pedroigor
Copy link
Contributor Author

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?

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.

@vramik
Copy link
Contributor
vramik commented Mar 21, 2024

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>
Copy link
Contributor
@vramik vramik left a 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

Comment on lines +28 to +30
<column name="GROUP_ID" type="VARCHAR(255)">
<constraints nullable="false"/>
</column>
Copy link
Contributor

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;
Copy link
Contributor

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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroigor pedroigor merged commit 32541f1 into keycloak:main Mar 21, 2024
@pedroigor pedroigor deleted the issue-27934 branch March 21, 2024 14:05
@zsafder
Copy link
zsafder commented Oct 9, 2024

I am trying to add a user in organization using rest admin api

URL:
<host>/admin/realms/<realm>/organizations/<organizations>/members

Body:

{ "id" : "<user_id>" }

but I am getting 400 error code with messgae

{ "errorMessage": "User does not exist" }

@SeanPWLynch
Copy link
SeanPWLynch commented Oct 16, 2024

@zsafder

I am trying to add a user in organization using rest admin api

URL: <host>/admin/realms/<realm>/organizations/<organizations>/members

Body:

{ "id" : "<user_id>" }

but I am getting 400 error code with messgae

{ "errorMessage": "User does not exist" }

I managed to get this working in a .net web api

var builder = new UriBuilder($"http://localhost:8080/admin/realms/{_keyCloakOptions.Realm}/organizations/{organisationId}/members/{userId}");

var response = await _httpClient.GetAsync(builder.Uri);
var stringResponse = await response.Content.ReadAsStringAsync(); _logger.LogInformation($"Get organisation member response: {stringResponse}");

if (response.IsSuccessStatusCode)
{
var organisationMember = await response.Content.ReadFromJsonAsync<KeyCloakOrganisationMember>(); return organisationMember;
}
else
{
_logger.LogInformation($"Failed to get organisation member {userId}. Status code: {response.StatusCode} {stringResponse}");
return null;
}

@kandakji
Copy link

@zsafder did you solve this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manage members of an organization through the REST Admin API
6 participants
0