Signbank / Global-signbank

An online sign dictionary and sign database management system for research purposes. Developed originally by Steve Cassidy/ This repo is a fork for the Dutch version, previously called 'NGT-Signbank'.
http://signbank.cls.ru.nl
BSD 3-Clause "New" or "Revised" License
19 stars 13 forks source link

Dictionary/info/ API endpoint: check permissions correctly #1281

Open Woseseltops opened 1 week ago

Woseseltops commented 1 week ago

Situation as I write this:

    user_datasets = guardian.shortcuts.get_objects_for_user(request.user, 'change_dataset', Dataset)

What we really want to know is whether the user can view the dataset. @susanodd changed it to view_dataset before, but that didn't work for Divya. There are two theories for solutions (both by @susanodd ), they might both be correct.

  1. The name of the permission is not view_dataset but can_view_dataset.
  2. Divya's user didn't have the correct roles

I'd like to use this issue to find out what is true and what isn't (perhaps using a test account). Once we know, we can change the permission name to something that actually makes sense.

Related issues:

vanlummelhuizen commented 1 week ago

There are two view-dataset permission:

Can view dataset: can_view_dataset
View dataset: view_dataset

Before Django 2.1 there was no built in view-permission (https://docs.djangoproject.com/en/5.0/releases/2.1/#what-s-new-in-django-2-1). We made view-permission ourselves but also let Django make the built in ones. This all led so this confusion. See #946 for more on this.

susanodd commented 1 week ago

I revised the permissions of the user that reported the error to make sure she has both of the view permissions.

The original code that was testing this was not specific for a particular dataset.

Not sure if related to this, she was not in group Editor, but was in group Researcher and Publisher.

Some of the permissions are from the group. (e.g., check the access to the specific pages in Admin. There it's per group.)

And many permissions are tested inconsistently.

vanlummelhuizen commented 6 days ago

My advise would be to remove the permission with codename can_view_dataset and leave the permission with codename view_dataset.

I think this involves the following steps:

  1. For the following relations, copy can_view_dataset permission to view_dataset permission if the latter does not exist:
    1. User-Permissions
    2. Group-Permissions
    3. User-Object-Permissions (Guardian, object permissions)
    4. Group-Object-Permissions (Guardian, object permissions)
  2. Remove all references to can_view_dataset in the code, e.g. in assign_perm and get_object_for_user function calls
  3. Delete the permission
  4. Rename view_dataset from View dataset' to 'Can view dataset'

Do you agree, @susanodd @Woseseltops @Jetske ?

susanodd commented 6 days ago

I agree with this, but I did not want to do this myself. We don't know how some users got the original permissions in the first place. And to "grab" the correct permission object is not necessarily the correct object.

Some of the "lookup" (internal, code that we did not write) searches on matches of "view dataset" (akin to "LIKE") which sometimes matches and sometimes does not. In the past I explicitly did this locally to modify said permissions, but the queries on the permissions tables did not necessarily work correctly. -- WRITING THIS FROM MEMORY) This was back when we were going from Django 1.11 to Django 4.2.

vanlummelhuizen commented 5 days ago

Seer PR #1282

Jetske commented 5 days ago

@vanlummelhuizen that sounds like a good solution. I remember changing everything to 'can view dataset' because 'view dataset' was not allowed anymore by Django probably due to the change in 2.1, so sorry for creating all this confusion. If Django allows this I approve to change it back.