Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
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

If in LDAP mode, users not being assign a default role at login #8708

Merged
merged 8 commits into from
Nov 25, 2024

Conversation

rragundez
Copy link
Contributor

@rragundez rragundez commented Nov 15, 2024

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 maintainers in 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 made mantainers. 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

    • Enhanced LDAP user management by refining group assignment logic, ensuring default roles are only assigned to non-superuser and non-staff users.
  • Bug Fixes

    • Improved control flow for user group assignments to prevent unnecessary role assignments for elevated privilege users.

Copy link
Contributor

coderabbitai bot commented Nov 15, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes made in the cvat/apps/iam/signals.py file focus on the create_user function, specifically refining the LDAP user management logic. An additional check has been introduced to ensure that the default role group is appended to the user_groups list only for users who are neither superusers nor staff. This adjustment maintains the overall structure of the function while enhancing the control flow for group assignment, ensuring appropriate user privilege handling.

Changes

File Path Change Summary
cvat/apps/iam/signals.py Modified create_user function to include a check for superuser and staff status before appending the default role group to user_groups. Updated method signature to reflect new logic.

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
Loading

🐰 In a world of roles and groups so bright,
A rabbit hops in, with logic just right.
Superusers and staff, they take their stand,
While others join groups, as planned by the hand.
Hooray for the changes, let's dance and cheer,
For every new user, we hold dear! 🥳


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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:

  1. Consider adding logging to track role assignments for debugging purposes
  2. Plan for existing LDAP users who might need the default role retroactively
  3. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3eec9fe and 5f1dcdd.

📒 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:

  1. The implementation is consistent with BASIC auth behavior and properly handles group assignments
  2. 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
  3. 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

@azhavoro
Copy link
Contributor

@rragundez Hi, thanks for the contrubution.
I have a question: why not just set up mapping between ldap and cvat groups using AUTH_LDAP_USER_GROUPS ?https://docs.cvat.ai/docs/administration/advanced/ldap/

@rragundez
Copy link
Contributor Author

rragundez commented Nov 15, 2024

@azhavoro we do use that functionality to assign admins, but a group on the Active Directory that encompasses all users does not exist.
On the other hand the search LDAPSearch when providing access to the application using the _BASE_DN can be done at the company level, hence providing access to anyone without having to create a dedicated AD group and manage adding people, etc.

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 IAM_DEFAULT_ROLE in the settings.py file.

@rragundez
Copy link
Contributor Author

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.

@rragundez
Copy link
Contributor Author

Hi @azhavoro and @Marishka17 a quick ping to get your feedback. Thanks.

@Marishka17
Copy link
Contributor

@rragundez, @azhavoro, Generally, I guess it makes sense, but I would prefer to add the IAM_DEFAULT_ROLE group only if no groups have been added after processing DJANGO_AUTH_LDAP_GROUPS.

@rragundez
Copy link
Contributor Author

Hi @Marishka17 thanks for the feedback. I agree with you and have made the necessary adjustment to the if condition.

@rragundez
Copy link
Contributor Author

Hi @Marishka17 @azhavoro maybe you have time to look at the changes before the weekend? they are pretty straight forward.
If you think it will take longer can you let us know, is just that we are onboarding users as we speak and we need to make a decision if to continue to have this more manual addition to AD groups until this is merged, or fork the code and maintain the change. Thanks again and apologies for being so insistent but we are being currently affected by this.

rragundez and others added 2 commits November 22, 2024 21:29
Co-authored-by: Maria Khrustaleva <maria@cvat.ai>
Copy link
Contributor

@Marishka17 Marishka17 left a comment

Choose a reason for hiding this comment

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

LGTM,
@azhavoro, Could you please merge PR if you have no comments?

@azhavoro
Copy link
Contributor

@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-commenter
Copy link

codecov-commenter commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.13%. Comparing base (7070875) to head (145c5d8).
Report is 4 commits behind head on develop.

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     
Components Coverage Δ
cvat-ui 78.36% <ø> (-0.23%) ⬇️
cvat-server 70.48% <0.00%> (-0.01%) ⬇️
---- 🚨 Try these New Features:

@rragundez rragundez requested a review from nmanovic as a code owner November 25, 2024 11:39
@rragundez
Copy link
Contributor Author

Done @azhavoro, please check

Copy link

sonarcloud bot commented Nov 25, 2024

@azhavoro azhavoro merged commit e97ace2 into cvat-ai:develop Nov 25, 2024
2 checks passed
@rragundez
Copy link
Contributor Author

Awesome @azhavoro thanks! One last question, how often are the releases onto master? in order to get this change

@rragundez rragundez deleted the ldap-default-role branch November 27, 2024 01:46
@cvat-bot cvat-bot bot mentioned this pull request Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants