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

Adds organization models, serializer and views #64

Merged
merged 1 commit into from
Feb 17, 2016

Conversation

oliverroick
Copy link
Member

Adds

  • Update to Django 1.9 for JSONField support + update RandomIDModel tests
  • Organization model
  • Organization serializer
  • Add permissions
  • API versioning
  • Basic List and Create API view
  • All basic API endpoints and views (no permissions yet)
  • Permissions
  • Org users permissions + clean up
  • Contact validation
  • Archiving flow
  • Add django-tutelary to requirements
  • Makes one of email/phone required for contacts

Some specifics

Adding method-specific permissions

One API endpoint can be used for different actions, e.g. listing exisiting and creating new organisations. These actions require different permissions that should be linked to the corresponding HTTP method.

In the implementation a view’s permissions_required is a dict that uses the HTTP method as the key. Permissions can either be a string if only one permission is required or a tuple with many required permissions.

class OrganisationList(APIVIew):
    permission_required = {
        ‘GET’: ‘org.list’,
        ‘POST: ‘org.create’
    }

Adding addition permissions based on HTTP body

Changing attributes of an entity can require additional permissions, for instance for archiving and unarchiving. These permissions need to be set before permissions are evaluated.

In the current approach the method initial is overwritten; the POST body is evaluated and additional permissions are added to self.add_permission_required. This attribute is then evaluated in get_permissions. (Example)

PermissionRequiredMixin

Views work slighly different in Django Rest Framework, so I implemented a specific PermissionRequiredMixin to cover for that.

I had to mostly copy the functionality of django-tutelary’s has_permission and add to check_permissions, which is called by DRF’s APIView. Instead of return False if permissions are not satisfied, it raises PermissionDenied, which is handled during DRF’s flow.

Further, I added get_permission_required, which mostly copies Django’s default implementation and adds a few tweaks to handle permission settings described before.

Issues that should be discussed

  • We should think of a way to combine certain permissions, for instance, *.list and *.view. Say I have two organisations and a user is allowed to only access one of them. When that user requests the list of organisations, the response should only contain one organisation.
  • Is there a way to differentiate between access actions and change actions? When a user can’t access a resource then they shouldn’t be able to update the ressource. Instead of a 403, because the user can’t update the resource, the service should return 404 if we want to hide inaccessble resources.
  • Is there a way to add custom “Permission denied” messages? Right now, whenever permissions for an action are not satisfactory, a simple “Permission denied” message is returned. I’m not sure if there is a requirement for adding more explanatory errors, such as “Not allowed to create a new organisation” or if this is something the client-side should take care of.
  • Should archiving permissions be different from updating permissions? Do you need updating permissions to have archiving permission?
  • Adding and removing user: What implications does it have? Do users get admin permissions on the organization?

- Update to Django 1.9 for JSONField support + update RandomIDModel tests
- Organization model
- Organization serializer
- Add permissions
- API versioning
- Basic List and Create API view
- All basic API endpoints and views (no permissions yet)
- Permissions first
- Permissions depending on request method
- Org users permissions + clean up
- Contact validation first try
- Contact validation
- Archiving flow
- Add django-tutelary to requirements
- Makes one of email/phone required for contacts
@ian-ross
Copy link
Contributor

I'll merge this right away, but I want to move something like most of the permissions functionality you've added into django-tutelary itself. For one thing, it seems as though it ought to be possible to make a PermissionsRequiredMixin that works with both normal Django and DRF without django-tutelary having an explicit dependence on DRF. I've also started doing something a bit like your dictionary approach for permissions_required as part of dealing with "collective actions" (list, create, etc.). (I've updated django-tutelary with this this morning, and so things may not quite work right now, since this is all a bit of a moving target.)

I think I'd also like to make it possible to provide a callable for permissions_required to deal with your "variable permissions based on HTTP body" case. Some further iteration of django-tutelary will allow you to specify object attribute conditions in policies (so you could have different permissions for archived and non-archived objects), but I don't want to do that right now, since it will just involve even more work on non-platform code.

To address your other points:

We should think of a way to combine certain permissions, for instance, _.listand_.view. Say I have two organisations and a user is allowed to only access one of them. When that user requests the list of organisations, the response should only contain one organisation.

This isn't a permissions issue as such, is it? It's a question of the semantics you impose for collective actions on collections of objects for which the action is allowed for only a subset of the objects. To implement this sort of thing within the permissions system, you'd need the PermissionsRequiredMixin to modify the queryset for the view to filter out objects for which the user doesn't have the relevant permissions. (I don't even know if it's allowed in Django to modify the queryset at that point.)

The default semantics in django-tutelary for collective actions is that you need to have the relevant permissions for all combinations of action and object that you try to do. Maybe the best way to deal with this case is to write a custom view that filters its queryset based on permissions? I don't quite know how that would work.

Is there a way to differentiate between access actions and change actions? When a user can’t access a resource then they shouldn’t be able to update the ressource. Instead of a 403, because the user can’t update the resource, the service should return 404 if we want to hide inaccessble resources.

It sounds like you're thinking about some sort of ordering on actions, i.e. in order to do "blah.edit", you also need "blah.view"? Again, I'm not sure about this one: it seems like it's quite application-specific and maybe should be handled by catching the "permission denied" exceptions somewhere to modify them if required. How were you imagining it might work?

Is there a way to add custom “Permission denied” messages? Right now, whenever permissions for an action are not satisfactory, a simple “Permission denied” message is returned. I’m not sure if there is a requirement for adding more explanatory errors, such as “Not allowed to create a new organisation” or if this is something the client-side should take care of.

My initial thought was to say that this is a client-side issue, but we probably want better error returns from the API as well. I'll add a django-tutelary issue for this. It should probably work by being able to specify an error message in the actions dictionary (either as a string or a callable, to deal with translation?).

Should archiving permissions be different from updating permissions? Do you need updating permissions to have archiving permission?

Totally separate, I'd say. This is what I'd characterise as a "policy question", and policy questions are dealt with when you write policies! If you want to make it so that archiving and updating permissions always go together, just write policies where that's the case. At the django-tutelary level, there's no meaning attached to particular action names: it's up to the person writing the policy to deal with that.

Adding and removing user: What implications does it have? Do users get admin permissions on the organization?

I didn't think so: most members of organisations won't be admins.

ian-ross added a commit that referenced this pull request Feb 17, 2016
Adds organization models, serializer and views
@ian-ross ian-ross merged commit c7bcea8 into master Feb 17, 2016
@ian-ross ian-ross deleted the feature/organizations branch February 17, 2016 09:10
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.

2 participants