Skip to content
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

[frontend/backend] New user shouldn't be assigned default groups if other groups are specified (#9234) #9274

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Archidoit
Copy link
Member

Proposed changes

  • In User creation form, in Group list field, groups should be ordered alphabetically
  • [backend] When creating a user, if groups are specified, don't add also the user to the default groups (only add the user to the default groups if no groups are specified)
  • In the front, in user creation form, the groups initial values are the default groups

Related issues

#9273
#9234

@Archidoit Archidoit added the filigran team use to identify PR from the Filigran team label Dec 10, 2024
@Archidoit Archidoit self-assigned this Dec 10, 2024
@Archidoit Archidoit changed the title [frontend/backend] New user shouldn't be assigned default group if groups are specified [frontend/backend] New user shouldn't be assigned default group if groups are specified (#9234) & (#9273) Dec 10, 2024
@Archidoit Archidoit changed the title [frontend/backend] New user shouldn't be assigned default group if groups are specified (#9234) & (#9273) [frontend/backend] New user shouldn't be assigned default group if groups are specified (#9234) Dec 10, 2024
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.38%. Comparing base (0a4409d) to head (321f63f).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9274      +/-   ##
==========================================
+ Coverage   65.37%   65.38%   +0.01%     
==========================================
  Files         624      624              
  Lines       59660    59663       +3     
  Branches     6686     6687       +1     
==========================================
+ Hits        39000    39010      +10     
+ Misses      20660    20653       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Archidoit Archidoit changed the title [frontend/backend] New user shouldn't be assigned default group if groups are specified (#9234) [frontend/backend] New user shouldn't be assigned default groups if other groups are specified (#9234) Dec 10, 2024
@lndrtrbn lndrtrbn self-requested a review December 11, 2024 08:17
Copy link
Member

@lndrtrbn lndrtrbn left a comment

Choose a reason for hiding this comment

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

Now that default groups are set in the form in the UI I don't think we should check on the backend if nothing is set then set the defaults. Because, in my opinion, as a user if I clear the field with the defaults, it's not to see the user created with defaults anyway after all.

Would be nice to have product opinion

opencti-platform/opencti-graphql/src/domain/user.js Outdated Show resolved Hide resolved
@delemaf delemaf self-requested a review December 11, 2024 08:57
@Archidoit
Copy link
Member Author

Now that default groups are set in the form in the UI I don't think we should check on the backend if nothing is set then set the defaults. Because, in my opinion, as a user if I clear the field with the defaults, it's not to see the user created with defaults anyway after all.

Would be nice to have product opinion

It was in case users are created from the API... If no groups are specified, the user should be added to the default groups (we don't create users with no groups). @nino-filigran @romain-filigran What do you think?

@delemaf delemaf removed their request for review December 11, 2024 10:27
@nino-filigran
Copy link

I agree that we need to also have a check at BE side in case you create through API only. Your comment makes sense @Archidoit

@SouadHadjiat
Copy link
Member

could you update user-test to test these changes ? There is at least the part "User has no capability query behavior" that removes the default_assignation, which is not necessary anymore. And maybe add a test to make sure when we create a user with a specific group, we don't assign the default group to the user.

@Archidoit
Copy link
Member Author

Archidoit commented Dec 18, 2024

could you update user-test to test these changes ? There is at least the part "User has no capability query behavior" that removes the default_assignation, which is not necessary anymore. And maybe add a test to make sure when we create a user with a specific group, we don't assign the default group to the user.

@SouadHadjiat I don't understand why you want we to remove the default_assignation of 'User has no capability query behavior'. If so, the user will be affected to the default group since there are no groups specified in the user to create input.

@Archidoit
Copy link
Member Author

@SouadHadjiat tests written :)

@Archidoit Archidoit marked this pull request as draft December 23, 2024 09:07
@Archidoit
Copy link
Member Author

Archidoit commented Dec 23, 2024

Waiting for Product decision about the functional breaking change (@romain-filigran )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filigran team use to identify PR from the Filigran team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants