Allow managing members of an organization #28057

pedroigor · 2024-03-19T15:46:36Z

vramik

@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?

vramik · 2024-03-20T09:42:27Z

model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProvider.java


-        entity.setId(KeycloakModelUtils.generateId());
+        entity.setId(group.getId());


I think it should not be used the same ID for two different objects.

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.

vramik · 2024-03-20T09:46:08Z

model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProvider.java

+        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");


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

Not really, if the group exists with that name is because it is already bound to a organization.

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?

Yes, the idea is to reserve such names exclusively to orgs.

vramik · 2024-03-20T09:47:23Z

model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProvider.java

I believe the IllegalArgumentException should be replaced by ModelException in the file.

vramik · 2024-03-20T18:55:49Z

model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProvider.java

        JpaConnectionProvider jpaProvider = session.getProvider(JpaConnectionProvider.class);
        this.em = jpaProvider.getEntityManager();
+        groupProvider = session.groups();


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.

vramik · 2024-03-21T08:01:20Z

server-spi/src/main/java/org/keycloak/models/OrganizationModel.java

    String getId();

    void setName(String name);

    String getName();

+    boolean addMember(UserModel member);


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:

keycloak/server-spi/src/main/java/org/keycloak/models/ClientModel.java

Line 230 in adf838f

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);

}

keycloak/server-spi/src/main/java/org/keycloak/models/ClientProvider.java

Line 116 in adf838f

void addClientScopes(RealmModel realm, ClientModel client, Set<ClientScopeModel> clientScopes, boolean defaultScope);

and the implementation lives inside a provider implementation.

vramik · 2024-03-21T08:24:59Z

server-spi-private/src/main/java/org/keycloak/organization/OrganizationProvider.java

+
+    Stream<UserModel> getMembersStream(RealmModel realm, OrganizationModel model);
+
+    UserModel getMemberById(RealmModel realm, OrganizationModel organization, String id);
+
+    OrganizationModel getOrganizationByMember(RealmModel realm, UserModel model);


Do you want to add a javadoc for these methods now, or rather as follow-up?

vramik · 2024-03-21T09:10:07Z

model/jpa/src/main/java/org/keycloak/organization/jpa/OrganizationAdapter.java

+    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;
+    }


I believe there is missing check that the user doesn't belong to some different organization already.

pedroigor · 2024-03-21T10:20:13Z

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 · 2024-03-21T10:24:54Z

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>

vramik

Thank you @pedroigor. As agreed for remaining comments I've either created a follow up issues or I'll refactor some code within #27993

vramik · 2024-03-21T11:44:15Z

model/jpa/src/main/resources/META-INF/jpa-changelog-25.0.0.xml

+            <column name="GROUP_ID" type="VARCHAR(255)">
+                <constraints nullable="false"/>
+            </column>


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

vramik · 2024-03-21T11:45:59Z

model/jpa/src/main/java/org/keycloak/organization/jpa/OrganizationAdapter.java


    private final OrganizationEntity entity;
+    private final KeycloakSession session;


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

vramik · 2024-03-21T12:58:55Z

model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProvider.java

+        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");


zsafder · 2024-10-09T09:22:02Z

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 · 2024-10-16T11:07:34Z

@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 · 2025-02-27T15:00:09Z

@zsafder did you solve this?

pedroigor force-pushed the issue-27934 branch 4 times, most recently from 7df4bd3 to faabb75 Compare March 19, 2024 20:01

pedroigor marked this pull request as ready for review March 19, 2024 20:01

pedroigor requested review from a team as code owners March 19, 2024 20:01

keycloak-github-bot bot added team/cloud-native team/core-clients team/core-iam flaky-test labels Mar 19, 2024

keycloak-github-bot bot mentioned this pull request Mar 19, 2024

Flaky test: org.keycloak.testsuite.model.DBLockTest#testTwoLocksCurrently #25794

Closed

pedroigor changed the title ~~Allow managing members for an organization~~ Allow managing members of an organization Mar 20, 2024

pedroigor force-pushed the issue-27934 branch from faabb75 to c190b04 Compare March 20, 2024 00:15

vramik self-requested a review March 20, 2024 09:31

vramik suggested changes Mar 21, 2024

View reviewed changes

vramik reviewed Mar 21, 2024

View reviewed changes

Allow managing members for an organization

736fed7

Closes keycloak#27934 Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>

pedroigor force-pushed the issue-27934 branch from c190b04 to 736fed7 Compare March 21, 2024 10:42

vramik mentioned this pull request Mar 21, 2024

Make sure organization-related data can only be managed from the Organization API #28133

Closed

vramik approved these changes Mar 21, 2024

View reviewed changes

stianst approved these changes Mar 21, 2024

View reviewed changes

pedroigor merged commit 32541f1 into keycloak:main Mar 21, 2024

pedroigor deleted the issue-27934 branch March 21, 2024 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow managing members of an organization #28057

Allow managing members of an organization #28057

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!


		entity.setId(KeycloakModelUtils.generateId());
		entity.setId(group.getId());

	public void addClientScopes(Set<ClientScopeModel> clientScopes, boolean defaultScope) {
	session.clients().addClientScopes(getRealm(), this, clientScopes, defaultScope);
	}


		private final OrganizationEntity entity;
		private final KeycloakSession session;

Allow managing members of an organization #28057

Allow managing members of an organization #28057

Uh oh!

Conversation

Uh oh!

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!