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

feat: admin role will be scoped to workspaces #3115

Conversation

frascuchon
Copy link
Member

@frascuchon frascuchon commented Jun 7, 2023

Description

This PR brings changed to support 3 kinds of roles:

  • owner: full access (old admin role)
  • admin: workspace-level operations (create/delete datasets, add records,...)
  • annotator: same as before. Just for annotation purposes.

Most of the tests have been revisited regarding the mocked_client fixture. I've also added variations o tests to support most roles as possible for the tested functionality. Since a huge number of changes have been included here, I will add more details in future PRs.

Refs #3094

Type of change

(Please delete options that are not relevant. Remember to title the PR according to the type of change)

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (change restructuring the codebase without changing functionality)
  • Improvement (change adding some improvement to an existing functionality)

How Has This Been Tested

(Please describe the tests that you ran to verify your changes. And ideally, reference tests)

Tests have been updated

Checklist

  • I have merged the original branch into my forked branch
  • I added relevant documentation
  • follows the style guidelines of this project
  • I did a self-review of my code
  • I made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

@frascuchon frascuchon requested a review from gabrielmbmb June 7, 2023 14:25
@frascuchon frascuchon force-pushed the feat/admin-role-will-be-scoped-to-workspaces branch from 79d785d to acd03d8 Compare June 7, 2023 14:30
@frascuchon frascuchon marked this pull request as ready for review June 13, 2023 14:25
Copy link
Member

@gabrielmbmb gabrielmbmb left a comment

Choose a reason for hiding this comment

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

LGTM! just some small suggestions that can be applied

src/argilla/server/policies.py Outdated Show resolved Hide resolved
src/argilla/server/policies.py Outdated Show resolved Hide resolved
@@ -63,7 +63,7 @@ def create_dataset(self, user: User, dataset: CreateDatasetRequest) -> BaseDatas
if not accounts.get_workspace_by_name(self._db, workspace_name=dataset.workspace):
raise EntityNotFoundError(name=dataset.workspace, type=Workspace)

if not is_authorized(user, DatasetPolicy.create):
if not is_authorized(user, DatasetPolicy.create(workspace_name=dataset.workspace)):
raise ForbiddenOperationError(
"You don't have the necessary permissions to create datasets. Only administrators can create datasets"
Copy link
Member

Choose a reason for hiding this comment

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

When we write administrators, we mean both the role of owner and admin, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we can see the owner role as an extended role from admin

src/argilla/server/services/datasets.py Outdated Show resolved Hide resolved
frascuchon and others added 2 commits June 14, 2023 13:05
Co-authored-by: Gabriel Martín Blázquez <gmartinbdev@gmail.com>
@frascuchon frascuchon merged commit 576bbc7 into 3094-support-for-owner-and-workspace-admin-roles Jun 14, 2023
@frascuchon frascuchon deleted the feat/admin-role-will-be-scoped-to-workspaces branch June 14, 2023 11:06
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.

3 participants