-
Notifications
You must be signed in to change notification settings - Fork 5
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
Organization member-management #1414
base: main
Are you sure you want to change the base?
Changes from all commits
c6cf298
d800ba4
5faef25
1d6e42b
aa5c147
ad93482
22a5b20
1eef979
de5d3d7
3733a79
ccd26d8
bb000da
008dfdb
188b95f
2cde7d2
30d6ad4
fe30038
6556ecf
1dcf3d8
211c9fe
aacf92b
1415d84
dfc97e5
6238bd4
2b43178
ca93d1a
1e2b663
8a16781
2f06296
3af4c03
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,12 +2,12 @@ | |||||
from django.utils.text import slugify | ||||||
from decorators import permission_required | ||||||
|
||||||
from apps.users.types import UserType | ||||||
from apps.permissions.models import ResponsibleGroup | ||||||
|
||||||
from apps.organizations import permissions as perms | ||||||
from apps.organizations.models import Membership, Organization | ||||||
from apps.organizations.types import MembershipType, OrganizationType | ||||||
from apps.users.models import User | ||||||
|
||||||
|
||||||
def get_organization_from_data(*_, membership_data, **kwargs) -> Organization: | ||||||
|
@@ -47,7 +47,7 @@ | |||||
id = graphene.ID(required=True) | ||||||
organization_data = OrganizationInput(required=False) | ||||||
|
||||||
@permission_required("organizations.manage_organization") | ||||||
@permission_required("organizations.change_organization") | ||||||
def mutate(self, info, id, organization_data=None): | ||||||
organization = Organization.objects.get(pk=id) | ||||||
user = info.context.user | ||||||
|
@@ -111,12 +111,77 @@ | |||||
return AssignMembership(membership=membership, ok=True) | ||||||
|
||||||
|
||||||
class RemoveMembership(graphene.Mutation): | ||||||
removed_member = graphene.Field(UserType) | ||||||
class MembershipInputWithUsername(graphene.InputObjectType): | ||||||
username = graphene.String() | ||||||
organization_id = graphene.ID() | ||||||
group_id = graphene.ID() | ||||||
|
||||||
|
||||||
class AssignMembershipWithUsername(graphene.Mutation): | ||||||
membership = graphene.Field(MembershipType) | ||||||
ok = graphene.Boolean() | ||||||
|
||||||
class Arguments: | ||||||
member_id = graphene.ID() | ||||||
membership_data = MembershipInputWithUsername(required=True) | ||||||
|
||||||
@permission_required("organizations.manage_organization", fn=get_organization_from_data) | ||||||
def mutate(self, _, membership_data): | ||||||
organization = Organization.objects.prefetch_related("permission_groups").get( | ||||||
pk=membership_data["organization_id"] | ||||||
) | ||||||
|
||||||
try: | ||||||
group = organization.permission_groups.get(pk=membership_data.get("group_id")) | ||||||
except ResponsibleGroup.DoesNotExist: | ||||||
return AssignMembershipWithUsername(membership=None, ok=False) | ||||||
|
||||||
try: | ||||||
user_id = User.objects.get(username=membership_data["username"]).id | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to the comment about moving this to the backend
Suggested change
|
||||||
except User.DoesNotExist: | ||||||
return AssignMembershipWithUsername(membership=None, ok=False) | ||||||
|
||||||
membership = Membership( | ||||||
organization_id=membership_data["organization_id"], | ||||||
user_id=user_id, | ||||||
group=group, | ||||||
) | ||||||
membership.save() | ||||||
return AssignMembershipWithUsername(membership=membership, ok=True) | ||||||
|
||||||
def mutate(self, info, member_id): | ||||||
raise NotImplementedError("Denne funksjonaliteten er ikke implementert.") | ||||||
|
||||||
class DeleteMembership(graphene.Mutation): | ||||||
membership = graphene.Field(MembershipType) | ||||||
ok = graphene.Boolean() | ||||||
|
||||||
class Arguments: | ||||||
membership_id = graphene.ID() | ||||||
|
||||||
@permission_required("organizations.manage_organization") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, some poor decision making on my part to add a lot of complexity with permissions comes back to bite us 🦷 This permission check must include the ID of the organization the membership belongs to. Otherwise, you can delete memberships from other organizations than your own. Look to |
||||||
def mutate(self, info, membership_id): | ||||||
membership = Membership.objects.get(pk=membership_id) | ||||||
membership.delete() | ||||||
|
||||||
ok = True | ||||||
return DeleteMembership(ok=ok) | ||||||
|
||||||
|
||||||
class ChangeMembershipInput(graphene.InputObjectType): | ||||||
membership_id = graphene.ID() | ||||||
group_id = graphene.ID() | ||||||
|
||||||
|
||||||
class ChangeMembership(graphene.Mutation): | ||||||
membership = graphene.Field(MembershipType) | ||||||
ok = graphene.Boolean() | ||||||
|
||||||
class Arguments: | ||||||
membership_data = ChangeMembershipInput(required=True) | ||||||
|
||||||
@permission_required("organizations.manage_organization") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, the permission check must include the organization ID |
||||||
def mutate(self, info, membership_data): | ||||||
membership = Membership.objects.get(pk=membership_data["membership_id"]) | ||||||
membership.group_id = membership_data["group_id"] | ||||||
membership.save() | ||||||
|
||||||
ok = True | ||||||
return ChangeMembership(membership=membership, ok=ok) | ||||||
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -7,19 +7,19 @@ | |||
|
||||
from apps.organizations.models import Membership, Organization | ||||
from apps.permissions.constants import ( | ||||
HR_GROUP_NAME, | ||||
HR_TYPE, | ||||
ADMIN_GROUP_NAME, | ||||
ADMIN_GROUP_TYPE, | ||||
ORGANIZATION, | ||||
PRIMARY_GROUP_NAME, | ||||
PRIMARY_TYPE, | ||||
MEMBER_GROUP_NAME, | ||||
MEMBER_GROUP_TYPE, | ||||
) | ||||
from apps.permissions.models import ResponsibleGroup | ||||
|
||||
|
||||
@receiver(post_save, sender=Membership) | ||||
def handle_new_member(instance: Membership, **kwargs): | ||||
optional_group: Optional[ResponsibleGroup] = instance.group | ||||
group: Group = instance.organization.primary_group.group | ||||
group: Group = instance.organization.member_group.group | ||||
org_group: Group = Group.objects.get(name=ORGANIZATION) | ||||
user = instance.user | ||||
user.groups.add(org_group) | ||||
|
@@ -31,7 +31,7 @@ | |||
|
||||
@receiver(pre_delete, sender=Membership) | ||||
def handle_removed_member(instance: Membership, **kwargs): | ||||
group: Group = instance.organization.primary_group.group | ||||
group: Group = instance.organization.member_group.group | ||||
org_group: Group = Group.objects.get(name=ORGANIZATION) | ||||
user = instance.user | ||||
if group: | ||||
|
@@ -44,19 +44,21 @@ | |||
@receiver(post_save, sender=Organization) | ||||
def create_default_groups(instance: Organization, created, **kwargs): | ||||
""" | ||||
Creates and assigns a primary group and HR group to members of the organization. | ||||
Creates and assigns a primary group and ADMIN group to members of the organization. | ||||
""" | ||||
if created: | ||||
ResponsibleGroup.objects.create( | ||||
name=PRIMARY_GROUP_NAME, | ||||
name=f"{instance.name}:{MEMBER_GROUP_NAME}", | ||||
description=f"Medlemmer av {instance.name}.", | ||||
organization=instance, | ||||
group_type=PRIMARY_TYPE, | ||||
group_type=MEMBER_GROUP_TYPE, | ||||
) | ||||
hr_group = ResponsibleGroup.objects.create( | ||||
name=HR_GROUP_NAME, | ||||
description=f"HR-gruppen til {instance.name}. Tillatelser for å se og behandle søknader.", | ||||
admin_group = ResponsibleGroup.objects.create( | ||||
name=f"{instance.name}:{ADMIN_GROUP_NAME}", | ||||
description=f"ADMIN-gruppen til {instance.name}. Tillatelser for å se og behandle søknader og medlemmer.", | ||||
organization=instance, | ||||
group_type=HR_TYPE, | ||||
group_type=ADMIN_GROUP_TYPE, | ||||
) | ||||
assign_perm("forms.add_form", hr_group.group) | ||||
print(admin_group.group, instance) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
assign_perm("forms.add_form", admin_group.group) | ||||
# assign_perm("organizations.manage_organization", admin_group.group, instance) |
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'm going to do a longer write up here with a general suggestion, so be warned: wall of text incoming.
Early on in the life span of this project, I got carried away with permissions, thinking it would be cool to have a super-flexible permission structure that could handle as many use cases as I could imagine. This resulted in us adding the complex permission structure of the project today, which are way too complex. In reality, we just need, at most, a way to tell if a user is and
ADMIN
or aMEMBER
in an organization.As a result, my suggestion here is to try to hide some of the complexity of our permission structure, to prevent it from leaking out through the API, and make it easier to work with in the future. It doesn't require a huge structural change, we can just change this API slightly.
Rather than forcing the users of the API (us) to have to figure out which
group_id
a user should be added to, we can hide that complexity behind a choice with two options: A user should either be added as anADMIN
or as aMEMBER
.Specifically, something like this:
The overall goal here is to reduce the complexity for the end-user of the API, placing more of the responsibility on the backend to handle the underlying (and in this case, unnecessary 😢 ) complexity.