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

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

Open
Woseseltops opened this issue Jun 28, 2024 · 6 comments
Open

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

Woseseltops opened this issue Jun 28, 2024 · 6 comments

Comments

@Woseseltops
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

susanodd commented Jul 2, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

susanodd commented Jul 3, 2024

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
Copy link
Collaborator

Seer PR #1282

@Jetske
Copy link
Collaborator

Jetske commented Jul 4, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants