Rothamsted-Ecoinformatics / farm_rothamsted

Custom farmOS features for Rothamsted Research.
GNU General Public License v2.0
6 stars 1 forks source link

Experiment Module: Create a new user profile specifically for sponsors #76

Closed aislinnpearson closed 1 year ago

aislinnpearson commented 2 years ago

Updated for incorporation into Phase 2:

As part of Phase 2 of the experiment module we need to create a specific user profile for scientists (internally referred to as sponsors). The basic requirements are

This is a child issue of #295


Scientist User Profile Permissions:

Following a call with Mike and Paul we agreed the following permissions for the first implementation of this issue:

Can view all logs, assets and plans (the equivalent of a 'Viewer' which is their current level of permission) with the following additions:

  1. Create new experiment plans
  2. Edit Experiment Plans which explicitly references their user profile (see #322)
  3. Create observation logs and edit observation logs associated with their plans
  4. With the exception of observation logs, we agreed that for this initial implementation scientists would not be able to edit logs associated with a plan that explicitly references their user profile, partly because this is easier to code and we would like to get a minimum viable replacement for Field Net released by January, but also to be able to test this minimum model and see what is required by user testing.

Original issue As described in the document on https://github.com/Rothamsted-Ecoinformatics/farm_rothamsted/issues/70#issue-1093250757:

"This would give read access only for the experiments and allow them to edit certain aspects of the plan such as changing and uploading new versions of the experiment plan. Ideally this would alert the farm team, and the farm team would also be able to alert the scientists (which the scientists could opt out of) whenever a job is completed on their experiment."

paul121 commented 2 years ago

This would give read access only for the experiments and allow them to edit certain aspects of the plan such as changing and uploading new versions of the experiment plan.

It would be easy to implement this in a new "role" that only has access to view/edit experiment plans (and view only for assets + logs). But will the "scientists" ever need to create or edit assets and logs?

aislinnpearson commented 2 years ago

But will the "scientists" ever need to create or edit assets and logs?

I think this is a case in point. I'd need to gather some proper user specifications (and I think we'd need to recruit some test users amongst the scientists) before creating this user profile. I'm especially interested in how this question integrates with SurveyStack actually... a question I was also thinking about in today's Technical meeting. Definitely a Phase II requirement,

Although I wonder if we should create this role in phase I if it's simple, as a means of getting some of that feedback. What do you think @richardostler ?

aislinnpearson commented 2 years ago

I had a look at the list of permissions sent by Mike and I think for the time being we only need view access for the scientists.

In the longer term, I think we would actually want a different users: scientist (viewer) and scientist (editor).

The scientist (viewer) user would be able to view any plans (with their associated logs, areas and files) if they are associated with a plan (i.e. listed as a project member)

The scientist (editor) would be the same, only they would be able to edit, create and delete logs, areas and files *if** they are associated with a plan. They would not be able to delete or archive a plan (this would need higher access permissions as it relates to wider processes such as archiving and documenting experiment numbers).

This does make me wonder if there is a way of recovering deleted logs though, so that only people with higher access permissions can permanently delete a log/ remove it from the bin?

For now I think we will just stick with keeping the scientists as viewers, and move this to Phase II

aislinnpearson commented 2 years ago

Update: The scientist (viewer) and scientist (editor) would only be able to view/ edit experiments which they are associated with (i.e. they are assigned a role in the experiment module metadata).

aislinnpearson commented 2 years ago

Following our conversation yesterday evening we agreed that Sponsors should also be able to create and edit observation logs associated with their experiment. Ideally they would only be able to submit the log if it references an experiment plan that they are associated with, but I think that might be a bit complicated @paul121 ?

paul121 commented 2 years ago

Ideally they would only be able to submit the log if it references an experiment plan that they are associated with, but I think that might be a bit complicated @paul121 ?

That is correct. I'm curious if by "references an experiment plan" we actually mean the "log references a plot(s) associated with an experiment plan". If we can use the logic that their log references a plot in a plan, then this may be easier.

But perhaps an easier solution... could we create an observation quick form that they must use to create these observation logs? This quick form would make it easier for them to select the relevant plot(s). Once the observation log is created we could let them edit it with additional fields not included by the quick form

aislinnpearson commented 2 years ago

Hi @paul121,

I'm curious if by "references an experiment plan" we actually mean the "log references a plot(s) associated with an experiment plan". If we can use the logic that their log references a plot in a plan, then this may be easier.

I guess it could reference either the plot(s) associated with an experiment or the boundary associated with an experiment

But perhaps an easier solution... could we create an observation quick form that they must use to create these observation logs?

I agree that this is a good idea, and there is already an issue open for this (#73). I can add it to a follow up milestone (2.12) which I created specifically to catch and prioritise these kinds of follow up issues once the initial work we have already agreed is complete.

paul121 commented 2 years ago

So we had discussed not creating any "profile" or "role" for the sponsors. The thought being that anyone with view access to a plan could simply view the plan, but if a user is also included in the plan.contact field then they would get additional update access.

Well, I'm running into an issue where users cannot access the plant.edit page unless if they specifically have the update permission for their user. In other words, my custom logic to "allow update access if a user is included in plan.contact" is not triggered for the plan.edit page.... As mentioned here: https://www.drupal.org/docs/8/api/routing-system/access-checking-on-routes/route-access-checking-basics#s-multiple-access-checks and linking to this issue: https://www.drupal.org/project/drupal/issues/2991698 (@mstenta you may find this interesting)

So, in the end, I think it will be easiest if we have a "Sponsor" role that grants users the update permission for experiment plans. This way they can access the edit page, but my custom logic will deny users access if they are not a member of plan.contact.

Similarly, I think we should create a role + permission for the "Experiment admin" so that users like Aislinn can be an admin for all experiments without explicitly being a contact for each one.

mstenta commented 2 years ago

linking to this issue: https://www.drupal.org/project/drupal/issues/2991698 (@mstenta you may find this interesting)

I came across this recently myself.

I'm curious how you went about adding your access logic. By altering specific routes to add access checks? Or by overriding the access handler for the entity type itself?

The latter is heavy handed, but would give you more control over what access checks are run, so you might be able to achieve the original goal. Not sure it's worth it though.

aislinnpearson commented 2 years ago

HI @mstenta, @paul121

Haha. Oh dear. This is getting very complicated!

So, in the end, I think it will be easiest if we have a "Sponsor" role that grants users the update permission for experiment plans. This way they can access the edit page, but my custom logic will deny users access if they are not a member of plan.contact.

I think I misunderstood something somewhere - I had always assumed we would have to have a sponsor role. What I wasn't sure about was if we needed was an experiment administrator role, or if the current 'Manager' role would suffice. In my head, we'd have to make the scientists 'sponsors' and then any 'sponsor' added to the plan would have the right to edit. We'd then have a separate field for contacts - so you could have a sponsor who can edit the plan, but doesn't necessarily get e-mail updates. I may still be misunderstanding you Paul, but I was expecting we'd have plan.sponsor (with edit permissions) and plan.contact as one or more e-mail addresses that receive updates. What I hadn't thought through is if this e-mail address for the contact has to match the e-mail address for one of the sponsors... Could we add sponsors (plan.sponsor) and then give the user the option to select one or more of these sponsors to receive e-mails? Which I think is similar to oyur point here Paul:

So, in the end, I think it will be easiest if we have a "Sponsor" role that grants users the update permission for experiment plans. This way they can access the edit page, but my custom logic will deny users access if they are not a member of plan.contact.

Although not every sponsor would be a contact? Again, apologies if I am still misunderstanding the problem... I am aware I might be. Also aware I can't really comment on @mstenta comment above a that is out of my depth.

Similarly, I think we should create a role + permission for the "Experiment admin" so that users like Aislinn can be an admin for all experiments without explicitly being a contact for each one.

It seems to me like we do need an experiment admin role though - I've made an issue for it here (#341), so at least this issue can be resolve and we make sure to continue discussing it even once the Experiment Sponsor role is updated.

aislinnpearson commented 2 years ago

One more note to add after our conversation on Tuesday, so I remember to add it to the documentation:

Users who have left will be blocked not deleted. This is important as it means that person's association with logs and experiment plans will remain intact, even after they no longer have access to the system, ensuring they are still acknowledged in papers, etc after they have left. I wonder if we could maybe come up with a better word than blocked though @mstenta... like 'archived' maybe, although I can see that how 'archiving' people might also not work but for different reasons. :)

mstenta commented 2 years ago

@aislinnpearson Another option, instead of "blocking" users: just remove their roles. This will keep them in the system, but revoke the permissions that they have. They will still be able to log in (blocking them is what prevents this), but they will see "access denied" everywhere. So that's another option. Don't necessarily need to have a concept of "blocked" or "archived" at all.

paul121 commented 2 years ago

Done! With this I've added our first functional & automated test to the farm_rothamsted module :-) Since we are adding custom access checking this is fairly important to have automated tests for. It should be easier to expand and add additional tests now that this is in place.

The solution to this issue is basically 3 changes:

aislinnpearson commented 1 year ago

Hi @mstenta, @paul121,

I just noticed we can't use the experiment module at all. At first I was thrilled I couldn't edit anything - it does exactly what we want it to:

image

But then when I went to add myself and Jonah as an admin, I realised the new roles weren't there... so I now I am locked out. :)

image

I suspect I am supposed to switch something on somewhere but just don't know how? If you could take a look and let me know asap I'd be super grateful - we've got quite a bit of work to do this end. Still, great to know it works! :)

mstenta commented 1 year ago

Hi @aislinnpearson! I just took a look and realized why this is happening - just a small thing that was overlooked (not your or @paul121's fault). I added the role to your user manually, and can add it to others if you need until we fix this. I'll explain what's happening...

Farmier adds the Role Delegation module to farmOS to allow non-admin users (no one has full admin access in Farmier instances for security reasons) to assign roles to others. However, right now it only allows assigning "managed roles" (a concept that farmOS uses for roles that have their permissions automatically assigned).

The new role that was added isn't technically a "managed role" right now, so it is not appearing in the list. The "Operator" role is a managed role, so that one works. I think we can just convert the new roles to "managed" roles to fix this.

paul121 commented 1 year ago

I think we can just convert the new roles to "managed" roles to fix this.

Done!