Fix #1036 -- Add permission check to GeoODK submissions

[oliverroick]

Proposed changes in this pull request

• Adds a check whether the contributing user has permissions to add data to the project. The check is against user roles instead of permissions because we need to check against four different permissions here (spatial.add, tenure_rel.add, party.add, resource.add) as users upload everything at once. For our current setup, the proposed solution is sufficient; we need to revisit when we implement fine-grained permission settings. We could then either check if the user has all permissions, which means if one permission is missing they would not be able to add anything. Or we check if one permission is present, which means users could add information they are not supposed to. Both solutions obviously have flaws, and I have no idea how to solve this at this point.
• Adds a test whether the permissions are checked correctly and PermissionDenied is raised as expected
• Adds a test whether the view returns 403 in case the authenticated user is not permitted to upload data.

When should this PR be merged

ASAP

Risks

Low

Follow up actions

None

Checklist (for reviewing)

General

• Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
• Is the PR labeled correctly? It should have the migration label if a new migration is added.
  Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.

Functionality

• Are all requirements met? Compare implemented functionality with the requirements specification.
• Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

• Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
• Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
• Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

• Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
• If this is a bug fix, are tests for the issue in place There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.

Documentation

• Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes must be documented in the Cadasta Platform Documentation.
• Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented in the API docs.
• Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

[linzjax]

Given how messy the situation is, this at least works.

Unfortunately, users who are granted permission and then have it revoked (are set to Public User or removed from the organization) are still able to submit, but that's more tied to #1008 than this.

[seav]

I have a couple of minor requests. Also, I also have no good solution to the all-or-nothing permission problem.

I was thinking that if the user tried to add a specific entity type for which they have no permission, they either get denied outright or those entities are discarded while the rest get accepted. This is still messy.

[seav]

misspelling: "do" should be "to".

[seav]

Would it be better to send the actual status object instead of a string as shown below? This way, if you misspell the error code, you get a flake error instead of a runtime error.

return self._sendErrorResponse(request, e, status.HTTP_400_BAD_REQUEST)

[oliverroick]

You'd still get a runtime error. Flake doesn't check whether the object in an imported module actually exists.

Anyway, I'll change the code to use the status object since this is more clear.

[oliverroick]

I was thinking that if the user tried to add a specific entity type for which they have no permission, they either get denied outright or those entities are discarded while the rest get accepted. This is still messy.

This is exactly what is currently happening. We're granting permission to create objects based on user roles. Any of the user roles either create all of the entities or none. I discussed with @dpalomino, denying all entities is not an option.

Discarding those objects a user is not allowed to create requires more thinking. Through GeoODK users create sets of records, i.e. Location + Relationship + Parties. Some of these sets are 1:n, one location many relationships. How would we handle a case where the user can create a location and a party but no relationship. Do we discard the complete set or do we create both locations and parties without the relationship? How do we communicate what happened back to the user? Is either of the solutions what users would expect or does this contradict their use case?

Those questions would have to be answered first before we can implement a more complex solution. Since this is an urgent issue, I suggest to move on with the solution I suggested unless there are issues with the code itself or we find a better solution that can be implemented in a timely manner.

[seav]

This is OK.