m2ms / fragalysis-frontend

The React, Redux frontend built by webpack
Other
1 stars 2 forks source link

Fix Squonk access control #937

Open phraenquex opened 2 years ago

phraenquex commented 2 years ago

(Pre-populate with sensible defaults Remove 20 character limit for "Description" field.)

Likely also include speccing out scenarios for squonk access control

phraenquex commented 2 years ago

After discussion with @tdudgeon and @rsanchezgarc:

Project dialog is the mechanism for creating projects in squonk.

Squonk-side:

Epic14 ideas:

phraenquex commented 2 years ago

General project navigation issues to solve:

Landing page:

tdudgeon commented 2 years ago

The first thing to decide on is the policy for which Squonk project is to be used by an individual user in Fragalysis. There are a number of options:

  1. each user has a single 'personal' squonk project in which they do their work for all targets
  2. each user has a a single 'personal' squonk project for each target
  3. each user has a a single 'personal' squonk project for each fragalysis project in each target
  4. each target has a dedicated squonk project belonging to a 'unit' corresponding to that target (or target's proposal) where all users do their work
  5. each user of a target has their own squonk project belonging to a 'unit' corresponding to that target (or target's proposal). Usage by all users of that target get's accrued to that unit, but quotas are applied at the project level
  6. same as 5, but squonk project for each fragalysis project in that target

Probably 2 or 5 make the most sense, but all have some merit and we can probably handle multiple options if needed. There is no real problem with having lots of squonk projects (resource usage for an empty project is minimal), but it will make accounting a bit more complex.

The complications are:

  1. users do not see a squonk project until they have been given access to it. So with option 4 fragalysis will need to be able to automatically add the user to the project. Do we just say that if the user can access the fragalysis target then fragalysis should be able to add the user to the squonk project without any further question?
  2. With options 4-6 who owns the unit, and for option 4 who owns the squonk project? Only the owner of the unit or someone who has been added as user of a unit can create projects in it (or maybe an admin user but I need to check this). Options 1-3 don't have problems here as the projects belong to the user and live within that user's unit.
  3. there is a bootstrapping problem here. Users don't exist in squonk (e.g. in the db) until they have logged in to squonk. They will be unable to do any of the necessary operations without this. Maybe we can just tell the user to log in to squonk before they do anything job related in fragalysis, or maybe we can add an API endpoint to bootstrap the user from fragalysis.
  4. All of the above mean some quite complicated logic in fragalysis, and some of this might need to be done as a squonk admin user.
  5. Some of these operations, such as allowing user to create or select a project seem like they reside in the fragalysis front-end, but currently all interactions with squonk are mediated by the fragalysis back-end and it would be cleaner to keep it that way.

Sorry this is all quite detailed, but the devil really is in the detail!

rsanchezgarc commented 2 years ago

I think that, al least for the first iteration, option 4 should be enough. It is more in line with what Frank posted above. Obviously, options 5 and 6 look more interesting, but also more complicated, so I prefer to start simple and check how it works

phraenquex commented 2 years ago

Mapping between Fragalysis and Squonk

image

Also:

phraenquex commented 2 years ago

Out of scope for this ticket/release

In scope for this ticket, needing final design (@tdudgeon @alanbchristie @boriskovar-m2ms)

tdudgeon commented 2 years ago

High level spec: https://docs.google.com/document/d/1nyMMENONXY-hqXcPfxzLcGj6YWM6tvyvwedkkV3vGog/edit?usp=sharing

phraenquex commented 2 years ago

Conversation today: how to ensure that squonk-side, advanced users can use data from multiple Fragalysis-project entities.
@phraenquex reduces the requirement: squonk-side, the Fraglysis-provenance needs to be maintained, no matter what the actual mechanism for getting data between projects. Also: ideally, advanced users should not have to guess where their Fragalysis-projects are, when they go squonk-side.

alanbchristie commented 2 years ago

It's not clear how the current API identifies the Proposal given a Target, Snapshot and a User. The internal ispyb query (in security.py) returns a list of proposals for a user (a string of the form "\<proposal>-\<session>"). To avoid "guessing" can someone identify the table relationships that allow the Job execution code to identify the one Proposal number (lb?????) from a Target/Snapshot reference?

If there is no reliable way to identify the Proposal, does the F/E need to (can it) provide the proposal ID/title in the originating request?


Also ... it's clear from the design that a user will only be able to run Squonk Jobs when they are associated with a Proposal. Is this new restriction correct?

alanbchristie commented 2 years ago

A tentative Low-Level Design (wip) can be found on Google docs...

https://docs.google.com/document/d/1lFpN29dK1luz80lwSGi0Rnj1Rqula2_CTRuyWDUBu14/edit#

alanbchristie commented 2 years ago

From a discussion with Frank & Warren it's clear that the Project table title contains the Proposal or, at the very least, a context under which the Squonk Job can be executed.

The title is a confusing property and should probably have been called proposal or token along with the addition of another column to record the origin of the token (i.e. with values like 'ispyb')

Nevertheless, the current API request content for transferring files to Squonk, and running Squonk jobs, lacks information about the Proposal. To respond to actions the B/E, which has the Target, will need the Proposal in the request, as a Target can be associated with more than one Project (Proposal).

So there is a need for F/E changes in this design.

alanbchristie commented 2 years ago

@phraenquex ... referring to the 4th-Aug diagram the Unit aligns with a Proposal. To be clear, does the Proposal in the diagram intend to include all visits or should there be a separate Unit for each visit?

alanbchristie commented 2 years ago

wrt the comment on Oct 21 ... there will be a separate Unit for each Visit, so each Proposal-Visit reference results in an independent Squonk Unit

phraenquex commented 2 years ago

@boriskovar-m2ms reckons not a lot of F/E work.

phraenquex commented 2 years ago

We need an additional database constraint fix: Target name can be duplicate across Proposals (Target name + project:title) should be unique. (project:title is "visit" in Diamond parlance.)

What we clarified: The "proposal" in the diagram above is actually a "visit", takes the form LB12323-5, which is Diamond-wide unique The visit is Diamond's _target_accessstring, to which users permissions can be allocated by the user using UAS. In any other service, the _target_accessstring might look different. Squonk needs to be presented with that _target_accessstring for authentication/authorisation.

In the F/E landing page, column header should be "visit", and make it a deployment variable, so that different deployments (i.e. not Diamond) can reconfigure this as required, to correspond to whatever authentication mechanism they have. In the database, call it target_access_string. (Change name in schema if at all possible.)

In F/E, ideally, hover over "Project" (sessionproject:id) should ideally pop up that cute dropdown in #1010 . If feasible, please scope out. Else launch a modal or something quicker.

In the diagram above,

boriskovar-m2ms commented 2 years ago

Target has assigned projects. Project has list of users. I have to display tuple of target - project (proposal-visit string) in the target list. Do I display them all or only those tuples where logged in user is part of the project? What to do when user is not logged in? In the document (https://docs.google.com/document/d/1lFpN29dK1luz80lwSGi0Rnj1Rqula2_CTRuyWDUBu14) it's mentioned that open proposals have no users and only closed proposals have list of users associated with them => this means that we are going to ignore open proposals?

phraenquex commented 2 years ago

Yes, show only target-target_access_string tuples for for the user is authorised. ALSO show if they are public projects. For that: introduce a column "open_to_public", which is boolean, if TRUE, pass that to front-end, so it can annotate the projects.

For current targets: provide a list of targets (and dates if available) - stick in this ticket - and we (Frank, Daren) will flush out the target_access_string from ISpyB.

phraenquex commented 1 year ago

@Waztom to point @alanbchristie to the bit of the code that creates the projects, so that it can be adjusted to force the code to be added. It's the old loader, and should be fixed there.

phraenquex commented 1 year ago

@boriskovar-m2ms reports that ISpyB is returning random subset of proposal/visits - not obvious what. @Waztom to link @alanbchristie up with Karl Levik to troubleshoot. @boriskovar-m2ms to generate a series of request/results in short succession, to pass on to Karl.

phraenquex commented 1 year ago

BE is losing the Celery processes that FE is opening. Fact-finding...

phraenquex commented 1 year ago

Remaining bugs:

  1. All open projects are to be attached to LB18453-1 (@phraenquex to confirm)
  2. Nobody should be able to execute any job if they are not authenticated against its target_access_string, even "Open" projects.
  3. 2 should also be true for who can access job content on Squonk. (What does this mean squonk-side?) (For instance, who can see this job?)

  4. If any project-authenticated user navigates to Squonk job from Fragalysis, system should gracefully grant them access squonk-side (configure keycloak etc), the same way someone gets added squonk-side the first time a job is executed on a project.

@alanbchristie to confirm if this is a Big Job.

alanbchristie commented 1 year ago
  1. Does not impact 937
  2. Will require a small change to the access logic
  3. Squonk projects need to be "private" (if not this is a bug). But only those with project membership can see project content (more investigation required)
  4. For this to work the F/E will need a new API endpoint in fragalysis so that the URL is generated in the B/E and not in the F/E. The B/E will need to check that the user has access to the Project/visit and then adjust the Squonk project to allow access, passing back an appropriate URL if successful.

About Project Membership ... Once a user is given access to a Project can they be removed? If they can then any generated URL, if kept by the user, can potentially be used in the future to access the project (member or not). The only workable solution in such a case is for Squonk to call into Fragalysis in order to check the user's membership of the associated project. And it will need to do this for every API call. This is not practical. <- This has now been deferred to ALC3 issue 1038.

boriskovar-m2ms commented 1 year ago

@phraenquex @alanbchristie For the point 4 there are two option how frontend can handle this issue. First option is way harder than second one.

First option:

URL will be generated by frontend and this URL will be used to access job page in squonk2. URL will look like https://fragalysis.diamond.ac.uk/viewer/react/job/12 where number at the end is the ID of the job in fragalysis (or is it an id of the job_request?). There will be a button in the job launcher dialog where user will be able to copy this URL to their clipboard. Same function will be available from job popup in the project history/tree component and in the jobs table as well. User can give this URL to a user who is also part of the project and they can use this URL to access the job in squonk. When someone uses this URL the frontend will check if user is logged in and if not a user is asked to log in (currently how log in works is that user is returned to landing page so it might be necessary for the user to use the URL again). Logged in user will be checked against the back end using endpoint squonk2-job-accessible which takes user-id and and job-request-id (jobid?) as two parameters. This endpoint will return {accessible: true/false, error: ''} and execute all necessary steps to provide access to actual squonk job. If user doesn't have access an error message is provided and if user can access the job then they will be redirected to the squonk2 page for given job.

Second option:

User will log in into the fragalysis and there will be a button which will show a dialog where user will be able to insert a url to squonk job page (something like https://data-manager-ui.xchem-dev.diamond.ac.uk/data-manager-ui/results/instance/instance-8d9eabf6-18ec-4550-837d-3ae8e246e56f?project=project-2a5a5c58-23c7-4f38-a32a-1b46b6f319e9). Front end will call api endpoint squonk2-job-accessible which will take two parameters where first one is the userid and second one is the url provided by the user. This endpoint will return {accessible: true/false, error: ''} and execute all necessary steps to provide access to actual squonk job. User will be notified about the result and if they were granted access they can use the squonk url to access given job.

phraenquex commented 1 year ago

Thanks @boriskovar-m2ms - I don't like option 2 much, because it makes the squonk URL the key entity. The route to Squonk should be via the relevant Fragalysis snapshot. So if the project views are rendered properly by the snapshot, then (a) the user has the necessary easy access to the Squonk job along with job context, and (b) the FE/BE has the opportunity to authenticate/authorise the user. I think this is what you're describing in option 1.
@alanbchristie might want to remind us what we discussed last week.

alanbchristie commented 1 year ago

Option 1 is my preferred route. The Job is the thing that we want to allow other users to access. The B/E will need sufficient information in the request to determine the Squonk Project (where the data files and Job were executed). The Fragalysis Job ID is the simplest piece of information.

A new endpoint, presented with the Job identity (12 in the example), will check the corresponding Access String associated with the Job ID and, if the user requesting access is permitted they will be granted access to Squonk at the same time as receiving a successful response from the endpoint.

The Squonk URL, currently and still manufactured by the F/E, can then be used to see the Job, and its events (log).

Also - the user_id need not be passed in as a parameter, it is the user wanting access that is making the call, so the user is implicit in the request. The user has to be logged in in order to gain access

boriskovar-m2ms commented 1 year ago

@phraenquex OK this is actually option 3 which is the easiest one of the bunch. Basically I will add button 'Request access' to job popup and to each row of the job table which logged in user can press and will be notified about the result. If access is granted then they can press 'Open in Squonk' and they should successfully access the job page in squonk.

phraenquex commented 1 year ago

Looks right - but just make both those two actions happen under the single button "Open in squonk". Boris asserts "it sounds easy", famous last words.

alanbchristie commented 1 year ago

This has been merged and made available in staging.

@phraenquex is is correct to assume that all open projects will be attached to LB18453-1

phraenquex commented 1 year ago

@alanbchristie that looks wrong. Make it LB18145-1. (@Waztom squawk if you think that's not right either.)

alanbchristie commented 1 year ago

On deployment, the Project table needs some manual alteration (automation of this step is extremely difficult due to the existing inconsistent content of the record titles).

Once deployed, in a shell in the stack Pod, inspect the existing titles from a django shell with something like...

python manage.py shell

And...

from django.conf import settings
from viewer.models import Project
projects = Project.objects.all()
for project in projects:
  print(project.title)

And then inspect the output to see how you can prepend the 'lb' sequence. Some project titles may already have an 'lb' prefix so they need to be skipped. When you've got that, run the following (here we're using what was found on the prevailing production DB)...

projects = Project.objects.all()
for project in projects:
  if project.title not in ['27156', '22722', 'OPEN', 'private_dummy_project'] and not project.title.startswith('lb'):
    project.title = 'lb' + project.title
    print(project.title)
    project.save()

Now mark the public projects (there is only one). Here we find and change the OPEN project to lb18145-1: -

for project in projects:
  if project.title == 'OPEN':
    project.title = 'lb18145-1'
    project.open_to_public = True
    project.save()

Production projects BEFORE

22717
lb22722
22722
private_dummy_project
19400
19400-1
13385
13385-87
20179
20179-1
20289
20289-1
19990
19990-1
18145
18145-44
16974
16974-1
16949
16949-4
16949-12
19572
19572-1
18944
18944-1
18954
18954-1
lb27156
27156
lb22717-79-error
lb22717-79
OPEN
27001
27001-2

Production projects AFTER

lb20289-1
lb19990
lb19990-1
lb18145
lb18145-44
lb16974
lb16974-1
lb16949
lb16949-4
lb16949-12
lb19572
lb19572-1
lb18944
lb18944-1
lb18954
lb18954-1
lb27001
lb27001-2
lb22722
22722
private_dummy_project
lb27156
27156
lb22717-79-error
lb22717-79
lb18145-1
lb22717
lb19400
lb19400-1
lb13385
lb13385-87
lb20179
lb20179-1
lb20289