-
Notifications
You must be signed in to change notification settings - Fork 439
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
Add admin users notion #2494
Add admin users notion #2494
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe recent updates focus on enhancing user management and role-based access control (RBAC) within the application. Key features include the introduction of an Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't expect the new admin user concept to permeate the RBAC code. I thought you might stop at the user endpoints without actually touching the RBAC utility code, given that they are mutually exclusive. I guess there's some beauty to having it part of RBAC though, because it kind of grew on me as I was reviewing the PR.
It all looks good with the exception of the re-factoring of the clean_client
concept. I strongly suggest you do that in a separate PR, because it's an entire story in and of itself. For this PR, it should be sufficient to write tests using the same approach that the existing user zen store integration tests use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some improvement suggestions for the unit tests, otherwise looks good.
Thanks for the effort @stefannica ! I will use your suggestions. |
Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com> Co-authored-by: Stefan Nica <stefan@zenml.io>
Quickstart template updates in |
Let's blow a bomb of CodeRabbit 🙂 @coderabbitai review |
@avishniakov, I will review the changes in the PR. 🚀 Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 10
Configuration used: .coderabbit.yaml
Files selected for processing (18)
- examples/quickstart/README.md (2 hunks)
- src/zenml/cli/user_management.py (4 hunks)
- src/zenml/client.py (4 hunks)
- src/zenml/models/v2/core/service_account.py (1 hunks)
- src/zenml/models/v2/core/user.py (6 hunks)
- src/zenml/models/v2/misc/external_user.py (1 hunks)
- src/zenml/zen_server/auth.py (2 hunks)
- src/zenml/zen_server/rbac/endpoint_utils.py (1 hunks)
- src/zenml/zen_server/rbac/utils.py (1 hunks)
- src/zenml/zen_server/routers/users_endpoints.py (11 hunks)
- src/zenml/zen_server/utils.py (2 hunks)
- src/zenml/zen_stores/migrations/versions/1a9a9d2a836d_admin_users.py (1 hunks)
- src/zenml/zen_stores/schemas/user_schemas.py (4 hunks)
- src/zenml/zen_stores/sql_zen_store.py (2 hunks)
- tests/integration/functional/zen_stores/test_zen_store.py (9 hunks)
- tests/integration/functional/zen_stores/utils.py (3 hunks)
- tests/unit/conftest.py (1 hunks)
- tests/unit/models/test_user_models.py (1 hunks)
Additional comments: 37
src/zenml/models/v2/misc/external_user.py (1)
- 28-28: The addition of the
is_admin
field with a default value ofFalse
to theExternalUserModel
class is implemented correctly and follows best practices. This change effectively introduces the notion of admin users, enhancing the system's security and user management capabilities.tests/unit/models/test_user_models.py (2)
- 29-29: The addition of the
is_admin=False
parameter to theUserRequest
constructor call in thetest_user_request_model_fails_with_long_password
test case is correctly implemented and ensures that the test reflects the updated model structure.- 36-36: Similarly, the addition of the
is_admin=False
parameter to theUserRequest
constructor call in thetest_user_request_model_fails_with_long_activation_token
test case is correctly implemented. This change maintains the consistency and accuracy of the test suite in light of the model updates.src/zenml/zen_server/rbac/endpoint_utils.py (1)
- 62-65: The reformatting of the
verify_permission
call in theverify_permissions_and_create_entity
function improves code readability without affecting functionality. This change aligns with Python best practices for formatting long function calls.src/zenml/models/v2/core/service_account.py (1)
- 150-150: The addition of the
is_admin
field with a default value ofFalse
in theto_user_model
method is correctly implemented. This change ensures that service accounts are accurately represented in contexts where they are treated as user models, aligning with the system's security model.examples/quickstart/README.md (2)
- 27-27: The update to the Google Colab badge URL is correctly implemented, ensuring users are directed to the correct notebook. This change enhances the user experience by providing accurate and helpful resources.
- 211-211: The adjustment to the Slack community link is a minor change that likely improves alignment or clarity. This update ensures that users have easy access to the ZenML Slack community, fostering engagement and support.
src/zenml/cli/user_management.py (2)
- 123-132: The addition of the
--is_admin
flag with a default value ofFalse
is a good practice, ensuring that users are not inadvertently granted admin privileges.- 204-281: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-429]
The integration of the new
is_admin
functionality into the CLI commands for user management is well-implemented, with appropriate error handling and user feedback mechanisms in place.src/zenml/zen_stores/schemas/user_schemas.py (3)
- 80-80: The addition of the
is_admin
field with a default value ofFalse
is a prudent choice, ensuring that users are not inadvertently granted admin privileges by default.- 171-171: Including the
is_admin
field in thefrom_user_request
method ensures that the admin status is correctly mapped from the request model to the schema.- 194-194: Explicitly setting
is_admin
toFalse
for service accounts in thefrom_service_account_request
method is a good practice, ensuring clear separation between user types.src/zenml/zen_server/utils.py (1)
- 401-428: The addition of the
verify_admin_status_if_no_rbac
function is a valuable security measure, ensuring that only admin users can perform sensitive actions when RBAC is disabled. The implementation and error handling are appropriate.src/zenml/models/v2/core/user.py (3)
- 148-195: The updates to the
UserRequest
class, including the addition of theis_admin
field and the root validator for setting default values forfull_name
andactive
, are well-implemented and align with the PR objectives.- 200-212: The updates to the
UserUpdate
class, including the optionalis_admin
field and the root validator for email updates, are well-implemented and provide flexibility for updating user information.- 381-395: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [272-392]
The updates to the
UserResponse
class, including the addition of theis_admin
field and getter methods foris_admin
andsrc/zenml/zen_server/rbac/utils.py (1)
- 236-239: The change to the
batch_verify_permissions
function signature, where parameters are now on separate lines, improves readability and adheres to Python's style guidelines. This is a good practice for maintaining clean and readable code.src/zenml/zen_server/routers/users_endpoints.py (9)
- 62-62: The addition of
verify_admin_status_if_no_rbac
is a critical security measure ensuring that only admin users or users with appropriate RBAC permissions can perform sensitive operations. It's important to ensure this function is thoroughly tested, especially in scenarios where RBAC is disabled, to prevent unauthorized access.- 116-118: The logic to ensure that users can see themselves even when RBAC restrictions apply is a good practice for user experience. However, it's crucial to ensure that this does not inadvertently expose sensitive user information to unauthorized users. Consider adding tests to cover various scenarios, including users with and without admin privileges and with RBAC enabled and disabled.
- 171-173: The use of
verify_admin_status_if_no_rbac
in thecreate_user
endpoint is appropriate for enforcing admin-only creation of users. This is a critical security measure. Ensure that the error messages provided to unauthorized users do not disclose too much information about the system's state or configuration.- 214-220: Applying
verify_admin_status_if_no_rbac
andverify_permission_for_model
in theget_user
endpoint ensures that user information is only accessible to authorized users. This is crucial for protecting user privacy. It's recommended to also verify the behavior when a user tries to access information about another user without sufficient permissions.- 260-266: In the
update_user
endpoint, the use ofverify_admin_status_if_no_rbac
andverify_permission_for_model
before allowing updates to user information is a good security practice. Additionally, the logic to prevent users from changing their own admin status unless they are admins themselves is crucial. Ensure that these checks are robust against potential bypass techniques.- 318-318: Preserving the admin status of a user during activation in the
activate_user
endpoint is important for maintaining the integrity of user roles. Ensure that this logic does not allow for unintended privilege escalation, especially in scenarios where a user's admin status might be changed before activation.- 351-359: The check to prevent self-deactivation in the
deactivate_user
endpoint is a necessary safeguard to prevent users from accidentally losing access. Coupled with the admin status verification, this ensures that only authorized users can deactivate others. It's important to test this functionality to ensure that users cannot bypass these restrictions.- 405-411: The security checks in the
delete_user
endpoint, including the prevention of self-deletion and the requirement for admin status, are essential for maintaining system integrity and preventing unauthorized user deletions. Ensure comprehensive testing around these checks to prevent potential security vulnerabilities.- 516-520: In the
update_myself
endpoint, ensuring that users cannot alter their ownactive
,is_admin
, andactivation_token
fields is crucial for security. This prevents users from granting themselves admin privileges or reactivating their accounts without proper authorization. It's recommended to add explicit tests to verify these constraints.tests/unit/conftest.py (1)
- 375-375: The addition of the
is_admin=True
attribute to thesample_user_model
fixture aligns with the PR's objectives of introducing admin users. However, it would be beneficial to add a comment explaining why this attribute is set toTrue
here, to clarify its purpose within the tests.src/zenml/zen_server/auth.py (1)
- 591-591: The changes to set the
is_admin
field based on theexternal_user.is_admin
value in theauthenticate_external_user
function are correctly implemented and align with the PR's objectives. However, it's crucial to ensure that this logic is secure and does not allow unauthorized elevation of privileges. Consider conducting a security review of this logic.Also applies to: 607-607
tests/integration/functional/zen_stores/utils.py (3)
- 211-211: The assignment of the
is_admin
attribute within theUserContext
class is correctly implemented. This ensures that theis_admin
flag is configurable upon instantiation of the context, allowing for flexible testing scenarios.- 216-219: The inclusion of the
is_admin
attribute in theUserRequest
object creation within the__enter__
method of theUserContext
class aligns with the PR's goal to differentiate between admin and non-admin users. This change correctly passes theis_admin
flag to the user creation process, ensuring that the system can create both admin and non-admin users as needed.- 1019-1019: In the
user_crud_test_config
, thecreate_model
forUserRequest
now includes theis_admin
parameter set toTrue
. This change is necessary for testing the new admin user functionality. However, similar to theUserContext
class, consider the implications of defaulting toTrue
for testing configurations. It might be beneficial to test both admin and non-admin user creation explicitly.Consider adding explicit tests for both admin and non-admin user creation to ensure comprehensive coverage of the new functionality.
tests/integration/functional/zen_stores/test_zen_store.py (2)
- 1041-1043: The
TestEventSource
class is commented out. If this class is not intended to be used or is awaiting future implementation, consider removing it or adding a TODO/FIXME comment to clarify its purpose.- 1053-1059: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1045-1073]
The
TestModel
class provides tests for model-related operations. It's well-structured and covers various scenarios, including fetching the latest version, updating names, and listing by tags. However, it's important to ensure that all test methods adhere to best practices for naming and documentation. Each test method should have a descriptive name that clearly indicates what scenario or functionality it's testing, and a docstring that briefly describes the test's purpose.src/zenml/client.py (2)
- 694-709: The method
create_user
correctly adds a new parameteris_admin
to distinguish between admin and non-admin users. This change aligns with the PR objectives to enhance security and user management capabilities. The logic to determine theactive
status of the user based on the presence of a password and the store type is maintained, ensuring backward compatibility.- 817-824: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [808-844]
The method
update_user
has been updated to include new parametersupdated_password
andupdated_is_admin
, allowing for the modification of a user's admin status and password. This enhancement supports the PR objectives of improving user account management. The conditional checks forNone
before updating attributes ensure that only specified fields are modified, preserving existing data integrity.src/zenml/zen_stores/sql_zen_store.py (2)
- 7517-7524: The addition of a check to prevent removing admin status from the default user enhances the system's security by ensuring there's always at least one admin. However, it's crucial to ensure that
self._default_user_name
is securely managed and not susceptible to manipulation.- 7615-7615: Setting the
is_admin
flag toTrue
for the default user upon creation is a good practice, ensuring consistency with the security measure introduced in the first hunk. This ensures the system always has at least one admin user.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks great. Let's land this beast !
Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com>
https://github.com/zenml-io/zenml into bugfix/OSSK-452-fix-privilege-escalation-bug-behavior
Ok, let's go! |
* add admin users to OSS * skip missing methods * increase readability * doc string * lint * lint * missing arg * add some edge-cases * wip commit to carve out clean_client changes * revert irrelevant changes * revert irrelevant changes * rework tests to run on rest * Apply suggestions from code review Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com> Co-authored-by: Stefan Nica <stefan@zenml.io> * polish test cases * fix branching * admin user mgmt CLI/Client * close activation vulnerability * revert rbac changes * verify admin permissions in endpoints * add `is_admin` to external users * only reg users will be migrated as admins * default is always admin * extend tests * lint * default `is_admin` None * Auto-update of Starter template * review suggestions * review suggestions * calm down linter * Update src/zenml/cli/user_management.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com> * review suggestion --------- Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com> Co-authored-by: Stefan Nica <stefan@zenml.io> Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* add admin users to OSS * skip missing methods * increase readability * doc string * lint * lint * missing arg * add some edge-cases * wip commit to carve out clean_client changes * revert irrelevant changes * revert irrelevant changes * rework tests to run on rest * Apply suggestions from code review Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com> Co-authored-by: Stefan Nica <stefan@zenml.io> * polish test cases * fix branching * admin user mgmt CLI/Client * close activation vulnerability * revert rbac changes * verify admin permissions in endpoints * add `is_admin` to external users * only reg users will be migrated as admins * default is always admin * extend tests * lint * default `is_admin` None * Auto-update of Starter template * review suggestions * review suggestions * calm down linter * Update src/zenml/cli/user_management.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com> * review suggestion --------- Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com> Co-authored-by: Stefan Nica <stefan@zenml.io> Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* Interact with feature gate * Properly handle entitlement violations * Apply suggestions from code review Co-authored-by: Barış Can Durak <36421093+bcdurak@users.noreply.github.com> Co-authored-by: Safoine El Khabich <34200873+safoinme@users.noreply.github.com> * Auto-update of Starter template * Applied code reviews * reformatted * Reformatted * Disable feature_gate when no source specified. * Auto-update of Starter template * Auto-update of E2E template * Auto-update of NLP template * Handle corrupted or empty global configuration file (#2508) * Handle corrupted or empty global configuration file * Auto-update of Starter template --------- Co-authored-by: GitHub Actions <actions@github.com> * Linted * Add admin users notion (#2494) * add admin users to OSS * skip missing methods * increase readability * doc string * lint * lint * missing arg * add some edge-cases * wip commit to carve out clean_client changes * revert irrelevant changes * revert irrelevant changes * rework tests to run on rest * Apply suggestions from code review Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com> Co-authored-by: Stefan Nica <stefan@zenml.io> * polish test cases * fix branching * admin user mgmt CLI/Client * close activation vulnerability * revert rbac changes * verify admin permissions in endpoints * add `is_admin` to external users * only reg users will be migrated as admins * default is always admin * extend tests * lint * default `is_admin` None * Auto-update of Starter template * review suggestions * review suggestions * calm down linter * Update src/zenml/cli/user_management.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com> * review suggestion --------- Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com> Co-authored-by: Stefan Nica <stefan@zenml.io> Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * remove dashboard from gitignore (#2517) * Colima / Homebrew fix (#2512) * attempt fix * Auto-update of Starter template * colima qemu fix trial * remove qemu * logs * logs better * testing brew workaround * try second possible fix for python gha * actually apply the fix * try the second possible solution for unbreaking python * make the CI whole again * linting * fix python 3.11 on mac (test) * one more attempt * formatting * different fix * restore the CI to full glory (fixed now!) --------- Co-authored-by: GitHub Actions <actions@github.com> * remove extra env var assignment (#2518) * Allow installing packages using UV (#2510) * Allow installing packages using UV * Auto-update of Starter template * actually make it work * Auto-update of Starter template --------- Co-authored-by: GitHub Actions <actions@github.com> * Additional fields for track events (#2507) * additional fields for track events * formatting * Auto-update of Starter template * adding a few recommendations * formatting * Auto-update of Starter template --------- Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com> * Auto-update of Starter template * Auto-update of NLP template * Auto-update of E2E template * Update src/zenml/zen_server/exceptions.py Co-authored-by: Stefan Nica <stefan@zenml.io> * Update src/zenml/zen_server/cloud_utils.py Co-authored-by: Stefan Nica <stefan@zenml.io> * Applied code review. * Properly reformatted * Reformatted * Fixed test * Fixed docstring * Model deletion works now, fixed error message * Show correct error message when creating models that exceed subscription limit * Send resource id * Auto-update of LLM Finetuning template * Fix error * Limit pipeline namespaces * Remove billing url * Linted * Potential fix --------- Co-authored-by: Barış Can Durak <36421093+bcdurak@users.noreply.github.com> Co-authored-by: Safoine El Khabich <34200873+safoinme@users.noreply.github.com> Co-authored-by: GitHub Actions <actions@github.com> Co-authored-by: Stefan Nica <stefan@zenml.io> Co-authored-by: Alex Strick van Linschoten <strickvl@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Jayesh Sharma <wjayesh@outlook.com> Co-authored-by: Michael Schuster <schustmi@users.noreply.github.com> Co-authored-by: Michael Schuster <michael.schuster.ffb@googlemail.com>
Describe changes
I implemented the
is_admin
flag for the user accounts and added protection to certain operations performed via the REST interface to ADMIN-allowed only.Only admins can: list all users, create users, get another user, update another user, deactivate another user, and delete a user.
All users can: list self, get self, update self, and activate a user.
These checks are only performed if no RBAC is in place, so ZenML Cloud RBAC functionality is not affected.
Pre-requisites
Please ensure you have done the following:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes
Summary by CodeRabbit