feat: add native groups to identity #18465

maryarm · 2024-05-13T14:43:23Z

Description

Crud on native groups is added. Assign a user to a group or removing of membership is also supported.

Related issues

closes #18463

maryarm · 2024-05-13T15:20:14Z

The PR is ready for review, but I have to move some classes to new packages after the PR from Ben is merged.

Ben-Sheppard · 2024-05-16T14:29:32Z

...kend/src/main/java/io/camunda/identity/usermanagement/service/CamundaUserDetailsManager.java

+  public static final String DEF_USERS_GROUPS_QUERY =
+      "select g.id, g.group_name"
+          + " from groups g, group_members gm"
+          + " where gm.username = ? and g.id = gm.group_id";


🔧 - I'm not sure it matters too much now as our data is small but I wonder about the performance of multiple tables in the from vs a JOIN i.e

SELECT g.id, g.group_name FROM groups g JOIN group_members gm ON g.id = gm.group_id WHERE gm.username = ?

🔧 - I don't think we use the id column here, we can simplify by just selecting the group_name and then the loadUserGroups method can use the same pattern as the loadUsers method (by using queryForList)

Ben-Sheppard · 2024-05-16T14:44:38Z

identity/backend/src/main/java/io/camunda/identity/usermanagement/service/GroupService.java

+
+  public List<Group> findAllGroups() {
+    return userDetailsManager.findAllGroups().stream()
+        .map(g -> new Group(userDetailsManager.findGroupId(g), g))


❓ is there any reason we make n calls to the database for this information? We could have a single repo method that returns a list of group objects

A valid point, I did it because I wanted to reuse the existing methods but as It's a findAll method it may downgrade performance a lot, I try to change it as you suggested.

Ben-Sheppard · 2024-05-16T14:53:14Z

identity/backend/src/main/java/io/camunda/identity/usermanagement/service/GroupService.java

+  }
+
+  public Optional<Group> findGroupByName(final String group) {
+    final int groupId = userDetailsManager.findGroupId(group);


❓ What does the findGroupId method return if there is no group? Is it a Integer with null? I think based on https://github.com/camunda/zeebe/pull/18465/files#diff-12171547b1d5162f24b1cced4f6ba4b53d8229dfdb4b68519597db91f3e0eed2R64 you'll get an exception so the if block is never used.

Instead of having to compose the objects in the service layer here can we just use a Group return method from the repository? It can still be optional because the SQL can return null for no results

I also think have the repository return a proper object makes the code in this class simpler

I prefer to keep the UserDetailManager class as much as possible independent from identity models and more like its parent class.

Ben-Sheppard · 2024-05-16T14:55:13Z

...tity/backend/src/main/java/io/camunda/identity/usermanagement/service/MembershipService.java

+    userDetailsManager.removeUserFromGroup(user.username(), group.name());
+  }
+
+  public List<CamundaUser> geMembers(final Group group) {


Suggested change

public List<CamundaUser> geMembers(final Group group) {

public List<CamundaUser> getMembers(final Group group) {

Ben-Sheppard · 2024-05-16T14:57:25Z

...tity/backend/src/main/java/io/camunda/identity/usermanagement/service/MembershipService.java

+import org.springframework.stereotype.Service;
+
+@Service
+public class MembershipService {


❓ I'm wondering if we need to have a separate service for this, are these methods really applicable to the object they are applied to i.e. these are group methods "addUserToGroup" or "findUsersInGroup" whereas "getUserGroups" is likely a userService level thing.

I did the separation because I preferred to have a central place to look for all membership related stuff and avoid adding dependency to groups in UserService and vice versa. For example "addUserToGroup" can be in both UserService or GroupService but with this specific MembershipService it's easier to find.

Thats fair :) - I agree, some methods can easily be in multiple places so moving it out like this creates a nice intentional place for it

Ben-Sheppard · 2024-05-16T15:04:35Z

...tity/backend/src/main/java/io/camunda/identity/usermanagement/service/MembershipService.java

+            g ->
+                groupService
+                    .findGroupByName(g)
+                    .orElseThrow(() -> new RuntimeException("group.notFound")))


🔧 Do we use this exception for anything? If we don't I think we can remove it as it doesn't add value, I think we can change this method to just filter those out, for example

return userDetailsManager.loadUserGroups(user.username()).stream() .map(groupService::findGroupByName) .filter(Optional::isPresent) .map(Optional::get) .toList();

In my idea, this exception should never happens unless we have data inconsistency so it's better to keep it so later in log we can be better noticed about problem.

Ben-Sheppard · 2024-05-20T12:48:12Z

identity/backend/src/main/java/io/camunda/identity/usermanagement/Group.java

+ */
+package io.camunda.identity.usermanagement;
+
+public record Group(Integer id, String name) {


❓ Just wondering, should we use Integer here or UUID? I personally prefer UUIDs but not sure on the general feeling :)

In my idea we can have int for internal/table level relations and use the uuid (as a uk column) whenever we want to make a service available for public or other modules. however here I just used the default ddl of spring security for groups.

Ben-Sheppard · 2024-05-20T12:57:20Z

...ackend/src/main/java/io/camunda/identity/usermanagement/repository/MembershipRepository.java

+          + " from groups g, group_members gm"
+          + " where gm.username = ? and g.id = gm.group_id";
+
+  private final JdbcTemplate jdbcTemplate;


💭 NABD but I've just remembered that I think we could use a NamedParameterJdbcTemplate here which would be nicer to work with (providing a map of params) but its not required of course :)

I prefer to keep it as it is in this PR because maybe with next task (customised fields for User) I change these repository classes to use spring data jpa instead of jdbc.

Ben-Sheppard · 2024-05-20T12:58:12Z

...tity/backend/src/main/java/io/camunda/identity/usermanagement/service/MembershipService.java

+public class MembershipService {
+  private final CamundaUserDetailsManager userDetailsManager;
+
+  private final UserService usersService;


Suggested change

private final UserService usersService;

private final UserService userService;

Ben-Sheppard · 2024-05-20T12:58:27Z

...tity/backend/src/main/java/io/camunda/identity/usermanagement/service/MembershipService.java

+      final GroupService groupService,
+      final MembershipRepository membershipRepository) {
+    this.userDetailsManager = userDetailsManager;
+    usersService = userService;


Suggested change

usersService = userService;

this.userService = userService;

Ben-Sheppard

Just a couple of tweaks from my side but other than that looks good :)

Ben-Sheppard

Thank you for addressing my comments :) - lets get it in and carry on the progress!

maryarm added 4 commits April 30, 2024 09:12

Merge remote-tracking branch 'origin/main'

2226e51

Merge remote-tracking branch 'origin/main'

3834b03

Merge remote-tracking branch 'origin/main'

2827ac5

feat: add native groups to identity

878a244

github-actions bot added the component/identity Related to the Identity component/team label May 13, 2024

maryarm requested review from Ben-Sheppard and steff46 May 13, 2024 15:20

Ben-Sheppard reviewed May 16, 2024

View reviewed changes

maryarm added 2 commits May 17, 2024 15:26

fix: create separate repositories

a984a33

Merge remote-tracking branch 'origin/main' into 2798-native-groups-cruds

c9aa226

maryarm requested a review from Ben-Sheppard May 17, 2024 14:49

Ben-Sheppard reviewed May 20, 2024

View reviewed changes

fix: rename the attribute to match the convention

d770a53

Ben-Sheppard self-requested a review May 21, 2024 08:32

Ben-Sheppard approved these changes May 21, 2024

View reviewed changes

maryarm added this pull request to the merge queue May 21, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 21, 2024

Merge remote-tracking branch 'origin/main' into 2798-native-groups-cruds

0519506

maryarm added this pull request to the merge queue May 21, 2024

Merged via the queue into main with commit 8d818a1 May 21, 2024
17 checks passed

maryarm deleted the 2798-native-groups-cruds branch May 21, 2024 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add native groups to identity #18465

feat: add native groups to identity #18465

maryarm commented May 13, 2024

maryarm commented May 13, 2024

Ben-Sheppard May 16, 2024

Ben-Sheppard May 16, 2024

Ben-Sheppard May 16, 2024

maryarm May 17, 2024

Ben-Sheppard May 16, 2024

Ben-Sheppard May 16, 2024

maryarm May 17, 2024

Ben-Sheppard May 16, 2024

Ben-Sheppard May 16, 2024

maryarm May 17, 2024

Ben-Sheppard May 20, 2024

Ben-Sheppard May 16, 2024

maryarm May 17, 2024

Ben-Sheppard May 20, 2024

maryarm May 21, 2024

Ben-Sheppard May 20, 2024

maryarm May 21, 2024

Ben-Sheppard May 20, 2024

Ben-Sheppard May 20, 2024

Ben-Sheppard left a comment

Ben-Sheppard left a comment

	public List<CamundaUser> geMembers(final Group group) {
	public List<CamundaUser> getMembers(final Group group) {

	private final UserService usersService;
	private final UserService userService;

feat: add native groups to identity #18465

feat: add native groups to identity #18465

Conversation

maryarm commented May 13, 2024

Description

Related issues

maryarm commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ben-Sheppard left a comment

Choose a reason for hiding this comment

Ben-Sheppard left a comment

Choose a reason for hiding this comment