Api perm check 1281

[vanlummelhuizen]

Did some work to get rid of the can_view_dataset permission.

When running tests, only signbank.dictionary.tests.ManageDatasetTests.test_User_is_dataset_manager fails, but that seems a bug in that test because it is trying to change a Dataset but it expect a message about whether the user can view a dataset.

[vanlummelhuizen]

I hope this PR does not bite PR #1279 ...

[susanodd]

Did some work to get rid of the can_view_dataset permission.

When running tests, only signbank.dictionary.tests.ManageDatasetTests.test_User_is_dataset_manager fails, but that seems a bug in that test because it is trying to change a Dataset but it expect a message about whether the user can view a dataset.

I can look a the tests. They're mainly helpful for debugging code.
The tests look at the feedback that is put in messages, because there is no other way to find out if something actually worked.
So it could be the message that got changed then the test will fail. We started doing more things with translations. But the tests need to keep using English for the messages testing. (So a test would fail if you're running in NL.)

SPECIFIC TEST:

The test test_User_is_dataset_manager gives lots of log information.
The test in question is:
The dataset manager wants to grant "change" permission to a user that does not already have "view" permission (for the specific dataset being tested).
So the dataset manager should first give the user view permission before giving change permission.
(This is probably intended to avoid clicking on the wrong buttons in the Manage Dataset page. So the test is making sure that view permission is granted first.)

[susanodd]

So what is the procedure for deployment?
We can do it first in signbank-dev.

Get and Pull the branch (or just merge it first), then execute the new command in the venv, then create a new test db.
Then it's okay to run the deploy.sh and the tests.
The code needs to be updated on the server and the database modified and a new test database created before the normal deploy command will work. (Chicken or the egg going on.)

If necessary, we can try the procedure steps out on both test servers to make sure it works.
This makes me really nervous!

[Woseseltops]

Thanks for cleaning up, @vanlummelhuizen :) . Two thoughts after reviewing the code:

• The management command that you added is only temporary, right? If so, it might be good to add that to the docstring, so that we are not scared to remove it if we encounter it later

• Very good to use our unit tests to verify this works. I'd like to manually test this with Divya's specific usecase to ensure we're not breaking anything for her (given her deadline) before we put it live, though... Let me know if this is on signbank-dev. In case somebody else wants to test, this is what I would do:

  • Create a test user with Divya's roles
  • Add and remove viewing permissions for various datasets
  • See if the dictionary/info endpoint responds with the correct datasets

[susanodd]

At the moment signbank-dev has the branch batch_edit.
I set up signbank-test yeterday and put branch batch_edit there.
@Jetske has her own branch now, signbank-jetske. We set that up yesterday.

So signbank-dev can be used for this branch.

[susanodd]

@vanlummelhuizen is there a migration missing?
The meta of model class Dataset has been changed by removing the can_view_dataset.

I've put this branch on signbank-dev

I ran the remove_can_view_dataset command.
Worked without complaints (I saved the output log, just in case.)
Now doing a deploy.
It complains there are changes to the models but without a migration. I assume it is the above.

Running migrations:
  No migrations to apply.
  Your models in app(s): 'dictionary' have changes that are not yet reflected in a migration, and so won't be applied.
  Run 'manage.py makemigrations' to make new migrations, and then re-run 'manage.py migrate' to apply them.

[susanodd]

I'm working on this locally to add the migration and fix the test.

[susanodd]

I figured out what is wrong with the test.
The test dataset is set to NGT.
But the test in question is supposed to make sure the user has view permission before being granted change permission.
For NGT this won't work because NGT is a public dataset, so the user always has view permission.

The test used to make a new dataset and grant and revoke permissions to it. (NOT use NGT.)

For now, we probably need to remove or comment out this test.
It won't work if NGT is the test dataset.

If I remember correctly it was set to tstMH for the dataset tests as well as the corpus tests.
This is because the tests write things to the file system.
I can't find that code anywhere.

[susanodd]

Deze branch zit nu op signbank-dev.

Ooops.

This branch is on signbank-dev now.

[susanodd]

@vanlummelhuizen @Woseseltops you can inspect the permissions now in the Admin on signbank-dev.

It is this branch.
The command was run.

But if you look at the permissions for NGT, some users that have change permission do not have view permission anymore!
(Visible in Datasets -> NGT -> Object permissions)

For example, Casper can change the dataset but NOT view the dataset. (This is wrong.)

Divya can change AND view the dataset.

More comical, @uklomp can CHANGE dataset Oefen, but she can't view it! (She's the only one that can't view it.)

[susanodd]

Did some work to get rid of the can_view_dataset permission.

When running tests, only signbank.dictionary.tests.ManageDatasetTests.test_User_is_dataset_manager fails, but that seems a bug in that test because it is trying to change a Dataset but it expect a message about whether the user can view a dataset.

The tests for Dataset Manager ought to make use of this dataset:

settings.TEST_DATASET_ACRONYM

instead of NGT.
Because NGT is a public dataset so it isn't possible to remove the view dataset permission.

[susanodd]

@vanlummelhuizen @Woseseltops you can inspect the permissions now in the Admin on signbank-dev.

It is this branch. The command was run.

But if you look at the permissions for NGT, some users that have change permission do not have view permission anymore! (Visible in Datasets -> NGT -> Object permissions)

For example, Casper can change the dataset but NOT view the dataset. (This is wrong.)

Divya can change AND view the dataset.

More comical, @uklomp can CHANGE dataset Oefen, but she can't view it! (She's the only one that can't view it.)

Okay, for Casper and @uklomp the permissions were also that before the new code. Neither had view permission.

Probably we ought to make sure the original permissions are correct before applying the command.

Note: The command was applied to signbank-dev first, before the deploy (restart with the code updates that check the permissions), and before the migration. Because the test mentioned above failed, and because the deploy after the command was run (the deploy invokes the tests), then the test was modified to comment out the offender, and the migration was created (because the deploy complained). Then the migration was done and restarted.

This is kind of a chicken or the egg because removing the can_view_dataset from the Dataset model requires a migration. But the command requires that this exists.

@Woseseltops if you want to try this out yourself, get a fresh copy of the database and try running it again. But again, this is chicken or the egg.

I suggest to fix all the Object Permissions on the Dataset objects in the Admin to make the explicit can_view_dataset and view_dataset columns be the same. Plus if a particular user has change_dataset, the user should also have the two view permissions.
Then we can just remove the can_view_dataset. (It will become redundant. All the updates to the code check view_dataset.)
But again, this is chicken or the egg.
(The database permission objects need to be changed, but in order to run the command the new code is needed.)

[susanodd]

I updated the perms by hand for a couple of datasets in the admin on signbank-test.
But as this pull request branch does, it is less work to use a command.
(This branch is on signbank-dev.)

But I think there needs to be a second command to apply first before this branch is deployed.

• For all the datasets, for all the users:
  if the user has (exact match, not "any") permission can_view_dataset
  then grant the user also permission view_dataset
  if the user has (exact match, not "any") permission change_dataset
  then make sure the user also has view_dataset permission and grant this if the user does not have this

Then the newer view_dataset will be filled in in the Object Permissions table for the Dataset in the Admin.
And any user that has permission to change the dataset will also have permission to view the dataset

For this kind of command, a test can be made that confirms this,

• For all datasets, for all users, the user has permission view_dataset if either the user has permission can_view_dataset or the user has permission change_dataset (for the specific dataset in the loop)

Such a command can't be evaluated once the deploy is done.

It's important to use explicit match, not "any" match.
The idea of the dataset permissions is that the dataset manager has granted them.
Not that the user is somehow else getting them.
This should be visible in the Admin table.

The main use of "any" is that staff and superusers can have extra permissions that are not shown in the table.
For other permissions, the users get this via the Group, e.g., Editor, like for change_gloss, etc

I think we need this extra step before the other command to make sure the permissions are correct to start with.

It's kind of chicken or egg because a migration is needed to remove the can_view_dataset permission, but then the objects will or will not be removed so the command won't work, or the code won't deploy because of references to deleted code.

@vanlummelhuizen @Woseseltops @Jetske ?

[vanlummelhuizen]

• The management command that you added is only temporary, right? If so, it might be good to add that to the docstring, so that we are not scared to remove it if we encounter it later

The alternative is to change it from a command into a data migration. This makes sure it is run once. After that we could create a migration that removes the can_view_dataset.

About creating view permissions for users that have change permission: I think it does not matter if it is done before or after the command/data migration and the migration removing can_view_dataset, for the simple reason that the command/migrations do not touch upon change permissions.

[susanodd]

• The management command that you added is only temporary, right? If so, it might be good to add that to the docstring, so that we are not scared to remove it if we encounter it later

The alternative is to change it from a command into a data migration. This makes sure it is run once. After that we could create a migration that removes the can_view_dataset.

About creating view permissions for users that have change permission: I think it does not matter if it is done before or after the command/data migration and the migration removing can_view_dataset, for the simple reason that the command/migrations do not touch upon change permissions.

Okay, that sounds good.
Via a data migration. That is safer.

[Woseseltops]

Outcome of my experiment with the dictionary/info endpoint: it seems to work, as long as the user as write/change permission. Datasets for which the user has just read permissions do not show up. If this was intentional, we can go live.

[susanodd]

No the info endpoint is a read only access, so the user should not need change permission.

@vanlummelhuizen is going to make a migration to fix the permissions, so the table contents are changed at the same time as the model. Otherwise it doesn't work to deploy because once the model is changed, the code can't be run anymore because it references old preferences. So @vanlummelhuizen is going to write a migration that maps the values.

[susanodd]

@vanlummelhuizen I removed the migration that was needed but unwanted.

This branch has been merged with all the recent updates so there are no conflicts.

I put the "delete gloss" branch onto signbank-dev because the others were all busy.

When you want to you can use it again for this request.

I replaced the database with a new one that does not have that command applied.

[susanodd]

SAMENVATTING

signbank-dev is now showing branch master

It can be switched to this one in order to develop the migration (to replace the command)

The test that was broken for this branch was commented out. So it's probably still broken.
I'm not sure why it was broken, it has to do with permissions of the dataset manager and user being granted.

This should be made a draft until the migration is done.
A migration is needed to remove the wrong can_view_dataset permissions and move the permissions to the newer Django permission view_dataset. A migration is needed because the old permission is in the Dataset model.

[susanodd]

NEW UPDATE

I created an empty signbank (see #1316)

If you browse the changes to the code there are numerous changes to the permissions being tested or assigned.
The code passes all of the tests.

I did not delete the old permission can_view_dataset, but it is not used anymore.

We still need to migrate the master signbank database.

Because the permissions differ in #1316, I'm not sure what is going on, why there are different names for the permissions. To get the code to run, I kept putting print statements to inspect what permissions were being found and then adapt the code and make sure the tests match. (So both sides work.) (Maybe there is some context involved?)

I'll try the revised code on a non-empty database, too.

Then we still need a migration with data mapping from this branch.

[susanodd]

This branch should be completed before the empty database branch is delivered so the old permission is removed via migration. (#1316 #1270 )
That branch is already using the correct permission.
The empty database when used by the VGT'ers will still need to be able to pull from master in their setup.

[vanlummelhuizen]

This is now done as far as I am concerned. @Woseseltops @susanodd

[vanlummelhuizen]

See also #1316 (comment)

[susanodd]

Merged with master, all the tests succeed (locally)!

Copied the newest global signbank to signbank-dev.
Deployed this branch on signbank-dev.
All the tests succeed!

APPROVED!