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

Beamline policy rules #182

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tpoliaw
Copy link
Collaborator

@tpoliaw tpoliaw commented Oct 4, 2024

Setting as a draft for now as I'm sure there must be a better way of doing it. The aim was to make the rules only defined if the required fields are present in the input but still be present (but false) if the conditions weren't met. eg

input = {} => result = {}
input = {"user": "name"} => result = {"admin": true} or result = {"admin": false}.

Setting the default to false would probably be ok but means you could get misleading results, eg if you pass user, proposal and visit, getting beamline_admin = false back suggests the user is not the admin for the beamline for the visit which may or may not be correct.

@tpoliaw
Copy link
Collaborator Author

tpoliaw commented Oct 4, 2024

RE failing lints. The suggested changes change the behaviour so I 'm not sure if I'm trying to work against the way OPA expects rules to be written by making them undefined when the input is not present.

@tpoliaw tpoliaw marked this pull request as ready for review October 4, 2024 11:14
@tpoliaw tpoliaw force-pushed the beamline_rules branch 2 times, most recently from 48908c7 to a241bbb Compare October 11, 2024 08:06
Comment on lines 7 to 11
is_beamline_admin[subject] contains beamline if {
some subject
some role in data.diamond.data.subjects[subject].permissions
some beamline in data.diamond.data.admin[role]
}
Copy link
Member

@garryod garryod Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this name really makes sense given it maps to a list of beamline names. We could rename it or use a double mapping like:

Suggested change
is_beamline_admin[subject] contains beamline if {
some subject
some role in data.diamond.data.subjects[subject].permissions
some beamline in data.diamond.data.admin[role]
}
is_beamline_admin[subject][beamline] := true if {
some permission in data.diamond.data.subjects[subject].permissions
some beamline in data.diamond.data.admin[permissions]
}

Though this evaluates to undefined if they're not an admin, so not exactly what we want 🤔

Copy link
Member

@garryod garryod Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, after a bit of messing around:

Suggested change
is_beamline_admin[subject] contains beamline if {
some subject
some role in data.diamond.data.subjects[subject].permissions
some beamline in data.diamond.data.admin[role]
}
is_beamline_admin[subject][beamline] := true if {
some permission in data.diamond.data.subjects[subject].permissions
some beamline in data.diamond.data.admin[permissions]
}
is_beamline_admin[subject][beamline] := false if {
data.diamond.data.subjects[subject]
data.diamond.data.beamlines[beamline]
some permission in data.diamond.data.subjects[subject].permissions
some _beamline in data.diamond.data.admin[permissions]
beamline != _beamline
}

Feels like a more concise solution should exist tho

Copy link
Collaborator Author

@tpoliaw tpoliaw Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That (the first one) only includes beamlines where the user is an admin so the value is always true at which point we might as well use a set again. Rename it to something like beamlines_for_user? I can't think of a better name so open to suggestions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, feel like it would be better to go with something like the latter. beamlines_for_subject or subject_beamlines would make sense if we were to keep it as a list.

On another note, we should probably avoid calling them beamlines and prefer instruments instead as it includes microscopes etc

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've renamed it. I'll start using instrument when the data bundle stops listing beamline for each session and the beamlines subtree is renamed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it should be beamline_admin_for_subject instead. It's a bit unclear otherwise whether it's a list of beamlines a subjects is on sessions for.

@tpoliaw tpoliaw force-pushed the beamline_rules branch 2 times, most recently from f486150 to 3420bc4 Compare October 11, 2024 13:56
* session.access: user can access session
* session.named_user: user is a named member of the visit
* session.matches_beamline: visit is on the given beamline
* session.session_beamline: beamline for the given visit

* proposal.access: user can access proposal
* proposal.named_user: user is a named user on the proposal

* admin.admin: user is super admin
* admin.beamline_admin: user is admin for the given beamline

Rules are only defined if the required fields are included in the input.
`admin.beamline_admin` refers to the beamline passed as `input.beamline`
not as the beamline for the session defined by `proposal`+`visit`.

Previous function `session.beamline` has been renamed to `beamline_for`
to distinguish it from the `session.beamline` rule.
Copy link
Member

@garryod garryod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Though think I'll probably do a re-write using maps at some point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants