Review *view* permissions

[vanlummelhuizen]

At some point between version 1.11 and 4.x, Django added view permissions. We also had our own view permission before Django had them and now they may conflict or may be confusing, not working properly, etc. See e.g. #940. Please review the use of those view permissions and try to conform to one system, I suggest the Django system. This may involve adding Django view permissions to users or groups where there is already a view permission that we made.

[vanlummelhuizen]

I think https://github.com/Signbank/Global-signbank/issues/940#issuecomment-1497113069 is also caused by this.

[susanodd]

I wrote up a lot of comments in #940 about what is visible in the Admin.

It's not clear to me what the can_view_dataset is actually doing. It's only assigned in 4 places. (See other issue) It's the first View Dataset column of the Dataset Object Permissions table. (But nobody has permission can_view_dataset at the moment.)

[susanodd]

Okay, here is something weird. The test-user I made, I tried granting the user View permission for tstMH (local server).

The second error shown is the result of not being able to send an email to the user!!!

[susanodd]

It seems it's the send_mail after assigning permission. (The send mail to the user. Although PyCharm doesn't report anything about this. The user does have permission. It's just the notification that failed.

Except, the test user can see the newly granted dataset in the select datasets list, but not in the header. Okay, after selecting it shows.

[susanodd]

Here's what that users new permissions look like among all the dataset permissions:

So now the test-user has the can_view_dataset permission (assigned via Dataset Manager) but not the view_dataset permission. So granting a user View permission via the Dataset Manager page does not grant any other kind of View dataset permission, only the can_view_dataset.

[susanodd]

This fixes it:

 import guardian
 from django.contrib.auth.models import Permission
 view_perm = Permission.objects.get(codename='can_view_dataset')
 view_perm.name = 'Can view dataset'
 view_perm.save()

[susanodd]

[susanodd]

Maybe the Django permissions have evolved enough that the Groups idea can be used? Once upon a time, group NGT was created with this in mind.

[susanodd]

The page Dataset Manager is visible to users in group Editor as well as group Dataset_Manager. That's why it shows up to users that are not able to use it. (See permissions in Admin for the page.)

[susanodd]

@ocrasborn wrote:

Sorry all @susanodd @vanlummelhuizen , not quite solved yet: when selecting one of the items in the 'Recently added signs', the person who added the items gets the message 'The gloss you are trying to view is not in a dataset you can view.' Confirmed by me by logging in as the dataset test user. I imagine this must be related, but feel free to open a new issue :-)

So this is related to selected datasets. (That's what I changed in the pull request, so all places that look up the view permission check both permissions.)

[susanodd]

Can we try out using groups to grant permissions for (gloss viewing) of (the glosses in) the dataset?

Suppose if Django now has its own view_dataset then actually we don't need to declare it anymore, right? (The new declaration of can_view_dataset could just be removed, because view_dataset exists now without needing a declaration?) But we are actually using it to mean something else, not just view dataset (object).

[Jetske]

Another case which I assume I changed in the Django upgrade is in interpreter.html line 6: perms.feedback.can_view_interpreterfeedback. But is that template even used anywhere?

[susanodd]

COMPLETED (Still need to check on obsolete feedback interpreter permissions.)

Yes, those were all removed. There is no feedback on the interpreter. But the permissions are stuck existing. It doesn't seem Django automatically removes its own automatically generated permissions.

I deleted lots of obsolete feedback code. But to my surprise that template file is there.

YES, I did remove them:

https://github.com/Signbank/Global-signbank/commit/7f8fe5ec53553310a4e536a46a270e1f308114df

This is slightly worrying. I suspect the Django merge puts stuff back sometimes.

This is a reason NOT to remove branches. (@vanlummelhuizen )

[vanlummelhuizen]

Just talked to @susanodd . I think it is a good idea to investigate the whole permission system, because it looks pretty esoteric in some cases, e.g. checking multiple (object) permissions and/or groups for a single action.

I will try to shed some light on this next week.

[ocrasborn]

Hi @vanlummelhuizen and all, please make this top priority, as it really seems to prevent basic data entry for us.

[susanodd]

@vanlummelhuizen Import CSV Update Lemmas requires change_dataset permission.

[vanlummelhuizen]

Hi @vanlummelhuizen and all, please make this top priority, as it really seems to prevent basic data entry for us.

@ocrasborn This issue is mainly for reviewing how permissions are used now in Signbank and how they should be changed for easier understanding and maintaining. And hopefully make it more clear for the user.

If you are referring to the problem you had earlier, I think we should discuss that in #940. @susanodd made a workaround for that issue: https://github.com/Signbank/Global-signbank/pull/948. I take it, that did not solve it. I suggest commenting in #940 or making a new issue and describing the problem in detail so that we can consider an ad hoc solution.

[susanodd]

@ocrasborn are you able to create new users again (#947)?

That is somewhat related as when a user is created, they also need to be granted permission to do things. At the moment, they are given view access (can_view_dataset) for public datasets they have selected when requesting a new account (namely NGT).

To be able to modify glosses, the user needs to be granted this in the Manage Datasets page. (change_dataset permission). This also has the effect of putting the user in group Editor. (e.g., granting change permission for NGT to a user that already has view permission for NGT has the effect of granting change_dataset for NGT plus putting the user in group Editor. (Being in group Editor allows the user to change glosses.)

At the moment, there are numerous places in the code that test for various combinations of permissions. This is mainly because of how Datasets contain sets of Glosses, etc. So changing a gloss in a dataset needs a combination of change_dataset and change_gloss permissions.

The Group membership is also used to determine whether the page is visible in the pull-down menu bar. (These are set in the Pages Admin as additional properties of Pages in the menu bar.) This is a bit weird because most pages use templates, which also test permissions. (You can see that the Manage Datasets page is visible to users in group Editor, but they can't grant permissions unless they are in group Dataset Manager. This is unexpected. Probably the export hyperlinks on the Manage page needs to be elsewhere.)

Some of the problems with the selected datasets is that these are used everywhere, as well as in the site banner. These are also being used to guard some of the commands, that the user needs to select the necessary dataset. Old permissions remain in the database. Django doesn't remove them. We want to make it more deliberate what the combinations of permissions are doing by say making a single permission for a combination.

[susanodd]

@vanlummelhuizen @ocrasborn any idea who removed group NGT ?

[susanodd]

This issue has become more important because of senses.

[ocrasborn]

The problem @susanodd noted above persists: editing permissions via the Dataset Management affects the old 'Can view dataset' column, not the new 'View dataset' column. See screenshot (object permissions for dataset IS_WFD1975), the result of revoking view permissions for users like lottev, and adding view permissions for lori and ari.price. The result was that lori could not view the dataset. (I've since changed permissions in Django so that these two can actually view the dataset.)

[susanodd]

@ocrasborn there was a problem with these in admin (as well) because it was not possible to tell which one was being selected in the list format for the permissions. (The match is something analogous to "like" on the name.)

The permissions in the code ought to be testing for both "view" permission names. I'll check if that code has been modified somehow.

[susanodd]

I made some permission changes to hopefully resolve this. In the Dataset List View, where the user sees if they have view permission, it was only showing Yes for the view_dataset and not the can_view_dataset. And the datasets the user is allowed to view (in the various templates) was only accepting explicitly granted view permission, not via any groups. (There used to be a group NGT that had view permission for all users in this group. This group seems to have disappeared. It was an easy way to grant view permission for NGT for all new users. This might be causing some bugs because of how it used to work. I changed it so it also accepts view permission via a group. So if the user is in group Guest, they should get this then.) (This is not live yet.)

I need to update this on Global BY HAND since the master branch has been upgraded to Django 4.2.7 and Python 3.12.1. (The master branch is deployed on signbank-dev but that isn't meant for actual use since it's missing videos, etc.)