If in LDAP mode, users not being assign a default role at login #8708
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes made in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LDAP
participant Signals
User->>Signals: Trigger create_user
Signals->>LDAP: Check user role
alt User is neither superuser nor staff
LDAP->>Signals: Append default role group
else User is superuser or staff
LDAP->>Signals: Do not append default role group
end
Signals->>User: User created with appropriate groups
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
cvat/apps/iam/signals.py (2)
45-47: Add a comment explaining the default role assignment logic.Consider adding a comment to explain why default roles are assigned only to non-privileged users, making it easier for future maintainers to understand the intent.
+ # Assign default role to non-privileged users, similar to BASIC auth behavior if not (user.is_superuser or user.is_staff): user_groups.append(Group.objects.get(name=settings.IAM_DEFAULT_ROLE))
45-47: Consider adding monitoring and migration plan.Since this change affects authentication and permissions:
- Consider adding logging to track role assignments for debugging purposes
- Plan for existing LDAP users who might need the default role retroactively
- Monitor authentication/authorization errors after deployment to catch any issues early
Example logging addition:
if not (user.is_superuser or user.is_staff): + import logging + logger = logging.getLogger(__name__) + logger.info(f"Assigning default role to LDAP user {user.username}") user_groups.append(Group.objects.get(name=settings.IAM_DEFAULT_ROLE))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
cvat/apps/iam/signals.py(1 hunks)
🔇 Additional comments (1)
cvat/apps/iam/signals.py (1)
45-47: LGTM! The change correctly implements default role assignment for LDAP users.
The implementation properly checks for non-privileged users and assigns the default role, matching the behavior of BASIC authentication. This directly addresses the reported issue of LDAP users not receiving default roles at login.
Let's verify the impact on existing LDAP users:
✅ Verification successful
The change is safe and correctly implements default role assignment for LDAP users
After analyzing the codebase:
- The implementation is consistent with BASIC auth behavior and properly handles group assignments
- The change is safe for existing LDAP users because:
- LDAP group mappings are processed first, preserving any existing role assignments
- The default role is only added for non-privileged users (not superuser/staff)
- The
user.groups.set()call happens after all groups are collected, ensuring atomic group assignment
- The code follows the documented LDAP integration pattern as per the comment regarding user saving before group assignment
The implementation correctly fixes the issue of LDAP users not getting default roles while maintaining all existing LDAP group mapping functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing LDAP user role assignments that might be affected
# Search for LDAP-related configuration and user handling
rg -A 5 "LDAP" --type python
# Look for any custom role assignment logic
ast-grep --pattern 'user.groups.set($$$)'
ast-grep --pattern 'user.groups.add($$$)'
Length of output: 212
Script:
#!/bin/bash
# Let's get more context about LDAP user handling and role assignments
# Search for LDAP-related code with proper file type
rg -t py "LDAP" -A 5
# Check the entire signal handler implementation
cat cvat/apps/iam/signals.py
# Look for any other group assignments in the codebase
rg -t py "groups\.(add|set|remove)" -A 3
Length of output: 8067
|
@rragundez Hi, thanks for the contrubution. |
|
@azhavoro we do use that functionality to assign admins, but a group on the Active Directory that encompasses all users does not exist. This change will enable having the admins do the initial setup but once that is done, owners and maintainers of organizations can manage user access themselves without us, the admins, having to deal with any extra LDAP group membership or assigning roles via the admin panel, creating tickets, etc. Hope that helps with the explanation, also the flow of sending invites via email doesn't really fit without default permissions. Non-admins, maintainers invite people, they can login but can't do anything because they don't have a role assigned. I think is quite nice to provide this functionality and then the default role can simply be manage via the variable |
|
Hi @azhavoro, what do you think? does it makes sense? Sorry to push it but we have this bug now and we would prefer to give back to the source code and not having to fork the repo to add this change. |
|
Hi @azhavoro and @Marishka17 a quick ping to get your feedback. Thanks. |
|
@rragundez, @azhavoro, Generally, I guess it makes sense, but I would prefer to add the |
|
Hi @Marishka17 thanks for the feedback. I agree with you and have made the necessary adjustment to the |
|
Hi @Marishka17 @azhavoro maybe you have time to look at the changes before the weekend? they are pretty straight forward. |
Co-authored-by: Maria Khrustaleva <maria@cvat.ai>
Co-authored-by: Maria Khrustaleva <maria@cvat.ai>
There was a problem hiding this comment.
LGTM,
@azhavoro, Could you please merge PR if you have no comments?
|
@rragundez LGTM, but could you please add a changelog entry? The instrcutions can be found here https://github.com/cvat-ai/cvat/blob/develop/CHANGELOG.md?plain=1#L9-L14 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8708 +/- ##
===========================================
- Coverage 74.22% 74.13% -0.09%
===========================================
Files 401 406 +5
Lines 43513 43715 +202
Branches 3953 3984 +31
===========================================
+ Hits 32298 32409 +111
- Misses 11215 11306 +91
|
|
Done @azhavoro, please check |
|
|
Awesome @azhavoro thanks! One last question, how often are the releases onto master? in order to get this change |
Currently if using LDAP auth IAM_TYPE, a user might have access to the application to login but no role is assigned to the user. This creates confusion to users as they are not able to perform any actions in CVAT even though they can login.
In comparison, this behaviour is not encountered if using the BASIC auth IAM_TYPE, as the
IAM_DEFAULT_ROLE, is assigned to the user when logs in for the first time.To explain a bit further the flow that we encountered, maintainers of an organization use the email service to invite users to their org, then users click on the email link, login to the application and accept the invite to the organization. Even though they might be even
maintainersin the organization they cannot perform any action as the rego rules for organization need users to at least have the "worker" role in the application. The same goes for sending invitation to the organization if they have been mademantainers. Here the links to the rego files:How has this been tested?
Yes, I have a local deployment with LDAP enabled. WIth these changes, if I log in then I get assigned the default role and I can perform actions in the application.
Summary by CodeRabbit
New Features
Bug Fixes