added support admins pluginconfig managemnent #1470#1837
Conversation
federicofantini
requested review from
0ssigeno and
mlodic
and removed request for
mlodic
|
I leave the review to @0ssigeno only. Thanks for the contribution :) |
api_app/queryset.py
Outdated
| if membership.is_admin: | ||
| return self.filter(Q(for_organization=True) | Q(owner__isnull=True)) | ||
| else: | ||
| return self.filter( | ||
| ( | ||
| Q(for_organization=True) | ||
| & Q(owner=membership.organization.owner) | ||
| ) | ||
| | Q(owner=user) | ||
| | Q(owner__isnull=True) | ||
| ) |
There was a problem hiding this comment.
Unfortunately, with this condition, if you are an admin you can see the configuration of every other organization user values. This is obviously not intended.
1 - Can we add a test to verify or refute this claim?
2 - If the claim is valid, I would suggest something like
if membership.is_admin:
Q(for_organization=True, owner__membership__organization=membership.organization) |
Q(owner=user) | Q(owner_isnull=True)The test that you will make should fail before and pass after this change
There was a problem hiding this comment.
I was reasoning that in this case being admin/owner/user is indifferent to the level of visibility of the configs in the org...
I think the following code should be enough:
# If you are member of an organization you should see the configs.
return self.filter(
Q(for_organization=True, owner__membership__organization=membership.organization)
| Q(owner=user)
| Q(owner__isnull=True)
)
api_app/permissions.py
Outdated
| def has_object_permission(request, view, obj): | ||
| if request.user.has_membership(): | ||
| return request.user.membership.is_admin | ||
| return False |
There was a problem hiding this comment.
If you are admin you can retrieve the value of another organization.
The check should be something like
obj_owner = getattr(obj, "owner", None):
# if the object was not made for an organization, we return false
if not obj_owner or not obj_owner.has_membership():
return False
else:
# if we are admin we can e have to check that is our org
return request.user.has_membership() and request.user.membership.is_admin and obj_owner.membership.organization == request.user.membership.organization
api_app/views.py
Outdated
| if not request.user.has_membership() or not request.user.membership.is_owner: | ||
| if request.user.has_membership(): | ||
| if not ( | ||
| request.user.membership.is_owner or request.user.membership.is_admin |
There was a problem hiding this comment.
You can remove the is_owner chec, since the owner is always admin
| org = attrs.pop("organization") | ||
| if not ( | ||
| org.owner == attrs["owner"] | ||
| or ( | ||
| self.context["request"].user.has_membership() | ||
| and self.context["request"].user.membership.organization.pk | ||
| == org.pk | ||
| and self.context["request"].user.membership.is_admin | ||
| ) | ||
| ): |
There was a problem hiding this comment.
Ok we starts to need some sort of comment in this boolean expression because it is breaking my mind.
This should mean
we raise ValidationError() when
1 - we are not owner OR
2 - we are not admin of the same org
If I did not fucked up the boolean expression, the check seems valid to me, I would just suggest to add a comment because it took me 5 minutes to understand what was going on
| and not ( | ||
| self.context["request"].user.pk == instance.owner.pk | ||
| or ( | ||
| self.context["request"].user.has_membership() | ||
| and self.context["request"].user.membership.organization.pk | ||
| == instance.owner.membership.organization.pk | ||
| and self.context["request"].user.membership.is_admin | ||
| ) | ||
| ) |
There was a problem hiding this comment.
We need a comment here too, this is fucking me up.
We return `redacted` when
1) is a secret AND
2) is a value for the organization AND
3) we are not its owner OR
4) we are not an admin of the same organization
if I did not fuck up the boolean expression, it seems good to me, just add a comment for future us
fixed admin queryset query and added tests fixed admin permissions checks added comments in tests source code
Codecov Report
@@ Coverage Diff @@
## develop #1837 +/- ##
===========================================
+ Coverage 76.20% 76.22% +0.02%
===========================================
Files 378 381 +3
Lines 12380 12475 +95
Branches 1303 1315 +12
===========================================
+ Hits 9434 9509 +75
- Misses 2408 2426 +18
- Partials 538 540 +2
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
LGTM. The only thing missing is to allow the org owner to add/remove admins from the org panel in the frontend. |
3 participants
Description
Added support to admin permission management plugin config: now you don't need to be an owner but just be an admin of an organization.
NOTE: this update do not cover the frontend.
Type of change
Checklist
develop_monkeypatch()was used in its class to apply the necessary decorators.test_files.zipand you added the default tests for that mimetype in test_classes.py.FREE_TO_USE_ANALYZERSplaybook inplaybook_config.json.Black,Flake,Isort) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.testsfolder). All the tests (new and old ones) gave 0 errors.