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

Remove django guardian #1057

Merged

Conversation

newswangerd
Copy link
Member

No description provided.


from rest_framework import serializers
from rest_framework.exceptions import ValidationError

from pulpcore.app.models.role import Role

from pulpcore.app.role_util import get_perms_for_model

Choose a reason for hiding this comment

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

@bmbouter do you agree, we should have these in the plugin api?

Choose a reason for hiding this comment

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

I think so; if a plugin needs to use it and it's the right tool for the job then yes.

Comment on lines 34 to 32
# TODO(newswangerd): Figure out how to make this one SQL query instead of
# performing N queries for each permission

Choose a reason for hiding this comment

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

How about

missing_roles = set(roles) - set(Role.objects.filter(name__in=roles))

?

@bmclaughlin bmclaughlin force-pushed the fix/AAH-1093-remove-django-guardian branch from 645d08c to f487da3 Compare January 14, 2022 15:27
@bmclaughlin bmclaughlin force-pushed the fix/AAH-1093-remove-django-guardian branch from 7f66c47 to ced1f82 Compare February 14, 2022 23:47
@bmclaughlin bmclaughlin force-pushed the fix/AAH-1093-remove-django-guardian branch 5 times, most recently from 9c0df23 to 294afcd Compare March 18, 2022 18:27
@bmclaughlin bmclaughlin marked this pull request as ready for review March 18, 2022 18:46
@bmclaughlin bmclaughlin changed the title [WIP] Remove django guardian Remove django guardian Mar 22, 2022
Copy link
Contributor

@awcrosby awcrosby left a comment

Choose a reason for hiding this comment

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

We do have recently added guardian piece that will need to get updated, it is run on each deploy of insights mode: https://github.com/ansible/galaxy_ng/blob/master/galaxy_ng/app/management/commands/maintain-pe-group.py

@bmclaughlin bmclaughlin force-pushed the fix/AAH-1093-remove-django-guardian branch from 294afcd to 19eb37c Compare March 29, 2022 15:30
@netlify
Copy link

netlify bot commented Mar 29, 2022

Deploy Preview for galaxyng ready!

Name Link
🔨 Latest commit a307740
🔍 Latest deploy log https://app.netlify.com/sites/galaxyng/deploys/6245f6087f24670008a5e61e
😎 Deploy Preview https://deploy-preview-1057--galaxyng.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

newswangerd and others added 3 commits March 29, 2022 11:32
- Replace get_users_with_perms
- Replace get_objects_for_user
Issue: AAH-1093
- Rename object_roles back to object_permissions for compatibility
- Updated tests
- Import Roles model from the plugin api
- Remove unused settings import
- subclass core.Group for Group model
- Viewset shim required by pulpcore
- Fix gettext CI error
- Add locked roles  galaxy.group_admin, galaxy.user_admin and updated galaxy.synclist_owner
Issue: AAH-1093
@bmclaughlin bmclaughlin force-pushed the fix/AAH-1093-remove-django-guardian branch from 19eb37c to d123723 Compare March 29, 2022 15:33
@bmclaughlin
Copy link
Contributor

/retest

@bmclaughlin
Copy link
Contributor

/retest

galaxy_ng/app/management/commands/maintain-pe-group.py Outdated Show resolved Hide resolved
galaxy_ng/tests/unit/api/base.py Show resolved Hide resolved
@bmclaughlin bmclaughlin force-pushed the fix/AAH-1093-remove-django-guardian branch from 526c89d to f853794 Compare March 30, 2022 20:15
- Added galaxy.content_admin locked role
- Updated integration test
Issue: AAH-1093
@bmclaughlin bmclaughlin force-pushed the fix/AAH-1093-remove-django-guardian branch from f853794 to 2cb3e6c Compare March 30, 2022 20:21
…e-group pe_group

- updated tests as needed
Issue: AAH-1093
Copy link
Contributor

@awcrosby awcrosby left a comment

Choose a reason for hiding this comment

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

Updates look good, I want to do a quick check in an ephemeral env of the partner-engineering group (when the env is available). But the Jira issue should be able to proceed into QE Review.

Issue: AAH-1093
@awcrosby
Copy link
Contributor

awcrosby commented Apr 1, 2022

/retest

@awcrosby
Copy link
Contributor

awcrosby commented Apr 1, 2022

@bmclaughlin spinning up an ephemeral env and trying to create a namespace in the UI, I added a group to "Namespace owners" but the POST hit an error:

"status":"400",
"title":"Invalid input.",
"detail":"object_roles field is required",
"source":{"parameter":"groups"}

@newswangerd
Copy link
Member Author

@bmclaughlin spinning up an ephemeral env and trying to create a namespace in the UI, I added a group to "Namespace owners" but the POST hit an error:

"status":"400",
"title":"Invalid input.",
"detail":"object_roles field is required",
"source":{"parameter":"groups"}

@awcrosby the UI still needs to be updated to work with these changes. We've had to make some breaking changes to this api by removing the ability to add permissions to group/object combinations, since that's no longer supported by pulpcore.

@newswangerd newswangerd dismissed awcrosby’s stale review April 7, 2022 12:40

Brian fixed andrews changes and andrew is out sick and can't approve the pr

@newswangerd newswangerd merged commit d84dee6 into ansible:master Apr 7, 2022
himdel added a commit to himdel/ansible-hub-ui that referenced this pull request Apr 8, 2022
himdel added a commit to himdel/ansible-hub-ui that referenced this pull request Apr 8, 2022
newswangerd added a commit to newswangerd/galaxy_ng that referenced this pull request Apr 11, 2022
newswangerd added a commit that referenced this pull request Apr 11, 2022
* Revert "Remove django guardian (#1057)". This reverts commit d84dee6.
* Revert "Create galaxy_ng specific Roles (#1058)". This reverts commit 3fefab0.

No-Issue
himdel added a commit to himdel/ansible-hub-ui that referenced this pull request Apr 25, 2022
himdel added a commit to himdel/ansible-hub-ui that referenced this pull request Apr 26, 2022
himdel added a commit to himdel/ansible-hub-ui that referenced this pull request Apr 26, 2022
himdel added a commit to himdel/ansible-hub-ui that referenced this pull request Apr 26, 2022
himdel added a commit to himdel/ansible-hub-ui that referenced this pull request Apr 26, 2022
himdel added a commit to himdel/ansible-hub-ui that referenced this pull request Apr 26, 2022
himdel added a commit to ansible/galaxykit that referenced this pull request Aug 17, 2022
…ect_roles, not object_permissions (#39)

* gitignore vim swapfiles

* namespaces.addgroup - use object_roles, not object_permissions

no object_permissions since ansible/galaxy_ng#1057

* galaxykit groups & roles - add roles, move perms to roles

```diff
+galaxykit group role *
+galaxykit role *
-galaxykit group perm *
+galaxykit role perm *
```

* remove client.set_permissions

 (not sure if unused, or if we just need to change it to call roles.set_permissions instead of groups.set_permissions)

* fix regex typo

* galaxykit role create: add -p alias for --permissions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants