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

Fix Squonk access control #937

Open
phraenquex opened this issue Jul 26, 2022 · 30 comments
Open

Fix Squonk access control #937

phraenquex opened this issue Jul 26, 2022 · 30 comments

Comments

@phraenquex
Copy link
Collaborator

phraenquex commented Jul 26, 2022

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

Likely also include speccing out scenarios for squonk access control

@phraenquex phraenquex changed the title Fix behaviour of Project Details modal (defaults) Fix Project Details modal and Squonk access control Jul 26, 2022
@phraenquex
Copy link
Collaborator Author

phraenquex commented Aug 2, 2022

After discussion with @tdudgeon and @rsanchezgarc:

Project dialog is the mechanism for creating projects in squonk.

  • ask user if the new project also needs a new squonk project
  • if not, let them pick an existing one from a drop-down (squonk API supports query for list of projects)

Squonk-side:

  • the "Unit" is the Target's "Proposal"
    • get it from ISpyB
    • if it does not yet exist, create it
    • project ownership goes to everybody that has access to the data in Fragalysis.
    • all project owners must be able to trigger squonk jobs.
  • The Unit gets an overall quota. For now:
    • space: 30Gb
    • compute: equal allocation at any time. If N units have stuff in the queue, then each unit gets total/N nodes. reallocation should be instant (as opposed to gradually, as the queue empties out)
    • (more nuanced resource policy will be deferred to Epic14.)

Epic14 ideas:

  • include Squonk's logical, but unimplemented, Tiers (gold, silver, bronze))
  • space management - once people can delete projects, it's for them to clear data if they're out of space.

@phraenquex
Copy link
Collaborator Author

General project navigation issues to solve:

  • explict "new project" button on viewer page
  • Takes FOREVER to load the project list
  • Need a "Projects" drop-down, showing all projects for Target.
  • Top of the dropdown (or otherwise easily accessible) should be an "explore" item that opens a new (!!) tab (not step on my existing view!!), with same layout as landing page.

Landing page:

  • project list
    • must be sortable by the various headings
    • default sort order: by target (alphanumeric)
    • target is left-most column
    • tight line spacing - max 1.5x line height
  • target list:
    • same line spacing as project list
    • additional columns: created (date), last modified (date), last looked (date)
    • allow sorting by columns
    • default sort order: by target (alphanumeric)

@tdudgeon
Copy link
Collaborator

tdudgeon commented Aug 3, 2022

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

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

Mapping between Fragalysis and Squonk

image

Also:

  • from within Squonk, should not be able to create new projects - should only be able to join projects created by Fragalysis
  • all project creation logic will live in Fragalysis, and happen via the backend, using existing authorization logic already in Django - it almost certainly covers all use-cases.
    • only Fragalysis-priveleged users will be allowed to run jobs directly in Squonk jobs, only in projects that have been created for them by Fragalysis
    • check that users can only create projects for which they are explicitly authorised. i.e. they shouldn't be able to run stuff on any old public target. The Django logic almost certainly does this already.

@phraenquex
Copy link
Collaborator Author

phraenquex commented Aug 18, 2022

Out of scope for this ticket/release

  • block users (Units) from creating projects from within Squonk - would need squonk-level work, and doesn't solve a problem (says @phraenquex on reflection)
  • Quota/resource restrictions at the Unit level. Unlikely to cause problems this early in the game.
  • Squonk discovering user access permissions from the Target/visit hierarcy in Fragalysis (ultimately in ISpyB/CAS)

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

  • what component (b/e, f/e) generates the new Unit (Proposal) in squonk, at what point
  • @phraenquex requests: squonk-side things should be generated automatically - no dialogues, please.

@rsanchezgarc rsanchezgarc changed the title Fix Project Details modal and Squonk access control Fix Squonk access control Sep 6, 2022
@tdudgeon
Copy link
Collaborator

tdudgeon commented Sep 8, 2022

@alanbchristie
Copy link
Collaborator

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

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

@phraenquex
Copy link
Collaborator Author

phraenquex commented Nov 24, 2022

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_access_string, to which users permissions can be allocated by the user using UAS.
In any other service, the target_access_string might look different.
Squonk needs to be presented with that target_access_string 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,

  • the "Project1.2" on the left is "sessionproject:title") in the DB.
  • the "Proposal" is "target_access_string" (which is "visit" in Diamond).

@boriskovar-m2ms
Copy link
Collaborator

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

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

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

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

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

@phraenquex
Copy link
Collaborator Author

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. Create new project from scratch #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
Copy link
Collaborator

alanbchristie commented Feb 28, 2023

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

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

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

alanbchristie commented Mar 7, 2023

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

boriskovar-m2ms commented Mar 7, 2023

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

phraenquex commented Mar 7, 2023

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

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

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

@alanbchristie
Copy link
Collaborator

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

@mwinokan mwinokan moved this to In production (Done) in Fragalysis May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

5 participants