rcpch / national-paediatric-diabetes-audit

A django application to audit the care of children and young people with diabetes in England and Wales.
0 stars 1 forks source link

Persistent permissions not correlating with those assigned #127

Open dc2007git opened 2 weeks ago

dc2007git commented 2 weeks ago

I'm currently on assigning permissions to different groups based on these roles:

image

Permissions are then explicitly defined (in the new permissions branch) like so, for each group above:

CLINICIAN_PERMISSIONS = [
        # patient-related permissions
        {"codename": "view_patient", "content_type": patientContentType},
        {"codename": "change_patient", "content_type": patientContentType},
        {"codename": "add_patient", "content_type": patientContentType},

        # visit-related permissions
        {"codename": "view_visit", "content_type": visitContentType},
        {"codename": "change_visit", "content_type": visitContentType},
        {"codename": "add_visit", "content_type": visitContentType},

        # site-related permissions = None
    ]

After this step, the function groups_seeder() in create_groups.py checks if the groups have already been created. If they have, then permissions are assigned like so, by checking the group name:

# TRUST_AUDIT_TEAM_EDIT_ACCESS = CLINICIAN
            elif group == TRUST_AUDIT_TEAM_EDIT_ACCESS:
                # basic permissions
                add_permissions_to_group(CLINICIAN_PERMISSIONS, newGroup)
                add_permissions_to_group(EDITOR_CUSTOM_PERMISSIONS, newGroup)

and then same code is ran after groups are newly created if they haven't already been.

Deleting all migrations and restarting the entire app logs the following to the terminal (focusing on the Clinician group):

national-paediatric-diabetes-audit-django-1   | ...creating group: trust_audit_team_edit_access
national-paediatric-diabetes-audit-django-1   | ...adding permissions to trust_audit_team_edit_access...
national-paediatric-diabetes-audit-django-1   | ...Adding view_patient
national-paediatric-diabetes-audit-django-1   | ...Adding change_patient
national-paediatric-diabetes-audit-django-1   | ...Adding add_patient
national-paediatric-diabetes-audit-django-1   | ...Adding view_visit
national-paediatric-diabetes-audit-django-1   | ...Adding change_visit
national-paediatric-diabetes-audit-django-1   | ...Adding add_visit
national-paediatric-diabetes-audit-django-1   | ...Adding can_opt_out_child_from_inclusion_in_audit
national-paediatric-diabetes-audit-django-1   | ...Adding can_lock_child_patient_data_from_editing
national-paediatric-diabetes-audit-django-1   | ...Adding can_unlock_child_patient_data_from_editing
national-paediatric-diabetes-audit-django-1   | ...Adding can_allocate_npda_lead_centre

IMPORTANT POINT - it is clear here that this group, called trust_audit_team_edit_access, aka the Clinician group, does NOT have delete_patient permissions. We can further test this by logging the current user's permissions (they are a Clinician I created) by accessing request.user.get_all_permissions():

{'npda.change_visit', 'npda.add_visit', 'npda.change_patient', 'npda.view_visit', 'npda.view_patient', 'npda.can_opt_out_child_from_inclusion_in_audit', 'npda.can_unlock_child_patient_data_from_editing', 'npda.can_allocate_npda_lead_centre', 'npda.add_patient', 'npda.can_lock_child_patient_data_from_editing'} and is superuser status: False

So this user should not be able to delete patient, as evident from their permissions logged above. In the PatientDeleteView I have integrated the PermissionRequiredMixin, and set permission_required = 'npda.delete_patient'. I expect this user to be returned to a PermissionDenied page at the moment - BUT they are in fact able to delete the user.

And upon further inspection in the django admin page for this user (again of type Clinician), they have this group assigned (which is correct):

image

And the following permissions (which are wrong, note also reference to Epilepsy12 - we need to tidy the codebase from refs to epilepsy in general) - I could only take a screenshot of the small window:

image

But it seems like they have complete a complete set of CRUD permissions even though this is not what their permissions should be.

Will continue to work on this but logging here on github in case someone knows where else permissions could be set? My gut feeling is that somewhere in the code users are out of the box given all permissions by accident.

dc2007git commented 2 weeks ago

UPDATE

Reopening this issue even after 'fixing' it. In the permissions branch I have managed to adequately check for permissions for each user, and if this Clinician tries to delete a patient, they will face a 403 error (still to be properly handled but functionally permissions are working).

However, in the admin console, it is still showing the user of type Clinician that I created as having delete patient permission:

image

even though they are not assigned this permission in the code, and running request.user.has_perm('npda.delete_patient') returns False.

Very strange indeed. Does anyone have any idea what could be causing this behaviour?