-
Notifications
You must be signed in to change notification settings - Fork 53
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
Added create service method #619
Conversation
To summarize: Tiled has had a hidden feature of "service principles", Principals which:
Up until this PR, there has been no way to actually create a Service Principal, unless you log into the SQL database and hand-code queries. This PR:
The intended flow that is:
The usual AccessControlPolicy mechanism may be applied, same as with normal user accounts, to restrict which nodes the service can access. |
This is going to get rushed into deployment to test with beam on a dummy experiment, but review is appreciate at any point, before or after merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't really go line by line, but looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good.
- I added a few non-critical comments that you could commit now or consider later.
- I added two suggestions regarding the new scope that I will address now.
tiled/authn_database/migrations/versions/769180ce732e_add_write_principals_scope_to_admin.py
Outdated
Show resolved
Hide resolved
request: Request, | ||
principal=Security(get_current_principal, scopes=["read:principals"]), | ||
db=Depends(get_database_session), | ||
role: str = Query(...), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a future PR, should we consider accepting this parameter from Query
or Body
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was:
- The role name will never be large (i.e. it will fit in the URL always).
- The role name is not sensitive (i.e. not a credential).
- Query params are simple.
I guess JSON bodies are also simple, though. Is there a particular driving use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking only for flexibility, but I agree that there is not a strong case for changing at the moment.
@danielballan I leave the merge for you. All looks good except the scopes permission on the new endpoint, which I committed so that you can review. |
One more thing to consider (for a future PR?): Should the "write:principals" scope be automatically stripped from "service" accounts? I.e., do we want to rule out at an early stage the possible perpetual generation of service accounts/keys? Should only a living admin should do this, or are there envisioned workflows where an automation chain would be useful? |
Co-authored-by: Padraic Shafer <76011594+padraic-shafer@users.noreply.github.com>
Thanks for the critical fix and the suggestions. I've addressed most, and spun out one into a follow-up GH issue.
It's a good point. I almost went ahead and implemented this---simple enough---but I think discussion about use cases for services is needed first. I can imagine use cases for services that manage Principals, such as cleaning up disused user Principals, or even spawning large numbers of Service Principals that are tightly scoped in access (e.g. to a specific proposal). Exactly how will implement this will matter: we may not want a complete ban, but perhaps distinct service Roles so that most Services Principals cannot spawn other Services Principals. #620 is a good place to explore this. |
This PR partially addresses #618 as part of the NSLS2 tier 2 data security project