Permissions issue: user changed from DC to PU in a project is still able to add/edit/delete records

[dpalomino]

Steps to reproduce the error

(tested in demo)

1. Have a regular member of the organization with public user permissions to several projects. Some of these projects have been created before the release of Sprint 11 and others after the release of Sprint 11.
2. Access to a project created before release of Sprint 11. Apart from the issue Permissions issue: Public user role within an organization #961 already known, otherwise the behaviour is "correct"
3. Access to a project created after release of Sprint 11, where that user has only Public User permissions
4. You will see that for that project user is able to:

• see all data in the project (same as Permissions issue: Public user role within an organization #961)
• create a new location/party/resources etc through GUI
• download a questionnaire from ODK and submit new data
• delete locations, parties, resources, etc

Actual behaviour

See above

Expected behaviour

User is not able to submit, edit or delete any record

[amplifi]

Note: This occurs after a user has been made Data Collector for a project, then changed back to a Public User.

[bjohare]

Affects permissions for form downloads to ODK also.

[dpalomino]

Thanks really a lot @amplifi and @bjohare! Now that we know that is indeed related to changing user profile and not happening with all PU, we can move this to medium priority for now I think.

[seav]

Note: PU = Project User role. This is not the same as a Public User role which uses the code Pb. A Project User has access to the project's records but cannot manipulate it, while a Public User only has access to the project's overview page, and not any of the records.

I've investigated this for a bit and I think this bug is because the method that reassigns a user's project policy is never called when the user becomes a public user of the project. This method is called whenever the ProjectRole object relating the user to the project is saved (due to the models.signals.post_save signal). But because Public User is not a real role (it is not in ROLE_CHOICES), the user's ProjectRole object for the project is deleted instead. Since deletion is not equal to saving it, the policy reassigning method is not called because there is no post_save signal. (There will be pre_delete and post_delete signals instead.) So when a user becomes a project Public User, the user still retains the previous policy they had for the project.

Our permissioning and roles functionality is quite a mess to be honest.

I guess the quickest way to fix this bug is to call the policy reassigning method with the pre_delete signal too and adding code to check that the ProjectRole is being deleted and delete the user's project policies accordingly.

Alternatively, we can make Public User be a real role at the expense of having N×M ProjectRole instances for N projects and M users per organization. This seems cleaner to me.

Any thoughts from the other developers?

[wonderchook]

@seav could you expand a bit on which parts are making it a mess?

[seav]

@wonderchook: I feel that how we have implemented our permissioning, policies, and roles is not consistent or as clean as it can be. The Public User pseudo-role, for instance, was bolted on when before we set that all org members are Project Users by default.

As another example of inconsistency, we have code that checks a user's role in the organization or project before allowing some features when we should be really checking whether the user has the specific tutelary permission. I have already filed #815 and #816 before for this.

If we try to check tutelary permissions all the time, then if ever we decide to, say, allow data collectors to download the project's data instead of just project managers, then we only need to update the data collector policy file instead of refactoring our code to provide an additional exception to data collectors. Right now, we check that the user is a project manager before showing the link to download the project's data.

[wonderchook]

@seav is it worth writing out a decision record for this so we could plan to make things work better?

[seav]

I guess that can help. I was also thinking we can approach this in the same manner that Oliver approached cleaning up the API (#880).

[linzjax]

Adding to this: Removing a user from an organization isn't changing their permissions. I deleted a data collector and they can still add/edit/delete records.