-
Notifications
You must be signed in to change notification settings - Fork 14
feat: add authz dependency injections #1539
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
Draft
shree-iyengar-dls
wants to merge
22
commits into
tiled-token-validation
Choose a base branch
from
add_depedency_injection
base: tiled-token-validation
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
27b97db
Add opa dependency function to create OpaUserClient
tpoliaw 96da85c
test opa dependency function
tpoliaw 4e3ea09
Add can_submit_task auth check method and config
tpoliaw 53c85a2
feat: add authz dependency injection
shree-iyengar-dls 01a7486
feat: add auth check dependency injections to task endpoints
shree-iyengar-dls f68ba83
feat: create new access task permission fns and add as dependencies
shree-iyengar-dls 0f1476f
refactor: update rest api version
shree-iyengar-dls 38c02dc
comment out dependency addition in set_state
shree-iyengar-dls 225644e
refactor: add admin check and check to set state function
2b34ff9
Update dependency names
tpoliaw 8a2eef1
Add missing admin check
tpoliaw 22e09bd
Handle missing opa and fix tests
tpoliaw 9d8c30b
Remove old admin method
tpoliaw 31ad93c
Use starlette statuses directly
tpoliaw 204a49e
test task submission authz
tpoliaw 004d28d
Use _config instead of _conf
tpoliaw 4fa20de
Re-use instrument session regex
tpoliaw 9ce866f
remove task access check
tpoliaw c3c8966
Add match to raises check
tpoliaw 2c84b6b
Add exception detail
tpoliaw 418380f
Let admin see all tasks
tpoliaw 1168a23
Start of api authz tests
tpoliaw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,7 +40,13 @@ | |
| from blueapi.worker import TrackableTask, WorkerState | ||
| from blueapi.worker.event import TaskStatusEnum | ||
|
|
||
| from .authorization import OpaClient, validate_tiled_config | ||
| from .authorization import ( | ||
| OpaClient, | ||
| OpaUserClient, | ||
| opa, | ||
| submit_permission, | ||
| validate_tiled_config, | ||
| ) | ||
| from .model import ( | ||
| DeviceModel, | ||
| DeviceResponse, | ||
|
|
@@ -146,6 +152,33 @@ def get_app(config: ApplicationConfig): | |
| return app | ||
|
|
||
|
|
||
| def access_task_permission( | ||
| opa: Annotated[OpaUserClient | None, Depends(opa)], | ||
| task_id: str, | ||
| fedid: Fedid, | ||
| runner: Annotated[WorkerDispatcher, Depends(_runner)], | ||
| ): | ||
| task = runner.run(interface.get_task_by_id, task_id) | ||
|
|
||
| if opa and not opa.admin() and (task and fedid != task.task.metadata.get("user")): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This only checks task access if OPA is configured. Do we want to restrict access based on user name when authn is enabled but authz is not? |
||
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND) | ||
|
|
||
|
|
||
| # start_task_permission is used when there is WorkerTask | ||
| def start_task_permission( | ||
| task: WorkerTask, | ||
| opa: Annotated[OpaUserClient, Depends(opa)], | ||
| fedid: Fedid, | ||
| runner: Annotated[WorkerDispatcher, Depends(_runner)], | ||
| ): | ||
| if not task.task_id: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, | ||
| detail="No task id provided", | ||
| ) | ||
| access_task_permission(opa, task.task_id, fedid, runner) | ||
|
|
||
|
|
||
| async def on_key_error_404(_: Request, __: Exception): | ||
| return JSONResponse( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
|
|
@@ -271,13 +304,13 @@ def submit_task( | |
| request: Request, | ||
| response: Response, | ||
| task_request: Annotated[TaskRequest, Body(..., examples=[example_task_request])], | ||
| _: Annotated[None, Depends(submit_permission)], | ||
| runner: Annotated[WorkerDispatcher, Depends(_runner)], | ||
| user: Fedid, | ||
| fedid: Fedid, | ||
| ) -> TaskResponse: | ||
| """Submit a task to the worker.""" | ||
| try: | ||
| user = user or "Unknown" | ||
| task_id: str = runner.run(interface.submit_task, task_request, {"user": user}) | ||
| task_id: str = runner.run(interface.submit_task, task_request, {"user": fedid}) | ||
| response.headers["Location"] = f"{request.url}/{task_id}" | ||
| return TaskResponse(task_id=task_id) | ||
| except ValidationError as e: | ||
|
|
@@ -309,6 +342,7 @@ def submit_task( | |
| @start_as_current_span(TRACER, "task_id") | ||
| def delete_submitted_task( | ||
| task_id: str, | ||
| _: Annotated[None, Depends(access_task_permission)], | ||
| runner: Annotated[WorkerDispatcher, Depends(_runner)], | ||
| ) -> TaskResponse: | ||
| return TaskResponse(task_id=runner.run(interface.clear_task, task_id)) | ||
|
|
@@ -326,7 +360,9 @@ def validate_task_status(v: str) -> TaskStatusEnum: | |
| @secure_router.get("/tasks", status_code=status.HTTP_200_OK, tags=[Tag.TASK]) | ||
| @start_as_current_span(TRACER) | ||
| def get_tasks( | ||
| fedid: Fedid, | ||
| runner: Annotated[WorkerDispatcher, Depends(_runner)], | ||
| opa: Annotated[OpaUserClient, Depends(opa)], | ||
| task_status: str | SkipJsonSchema[None] = None, | ||
| ) -> TasksListResponse: | ||
| """ | ||
|
|
@@ -346,6 +382,10 @@ def get_tasks( | |
| tasks = runner.run(interface.get_tasks_by_status, desired_status) | ||
| else: | ||
| tasks = runner.run(interface.get_tasks) | ||
|
|
||
| if opa and not opa.admin(): | ||
| tasks = [t for t in tasks if t.task.metadata.get("user") == fedid] | ||
|
|
||
| return TasksListResponse(tasks=tasks) | ||
|
|
||
|
|
||
|
|
@@ -363,6 +403,7 @@ def get_tasks( | |
| def set_active_task( | ||
| request: Request, | ||
| task: WorkerTask, | ||
| _: Annotated[None, Depends(start_task_permission)], | ||
| runner: Annotated[WorkerDispatcher, Depends(_runner)], | ||
| ) -> WorkerTask: | ||
| """Set a task to active status, the worker should begin it as soon as possible. | ||
|
|
@@ -393,6 +434,7 @@ def get_passthrough_headers(request: Request) -> dict[str, str]: | |
| @start_as_current_span(TRACER, "task_id") | ||
| def get_task( | ||
| task_id: str, | ||
| _: Annotated[None, Depends(access_task_permission)], | ||
| runner: Annotated[WorkerDispatcher, Depends(_runner)], | ||
| ) -> TrackableTask: | ||
| """Retrieve a task""" | ||
|
|
@@ -470,6 +512,8 @@ def get_state(runner: Annotated[WorkerDispatcher, Depends(_runner)]) -> WorkerSt | |
| def set_state( | ||
| state_change_request: StateChangeRequest, | ||
| response: Response, | ||
| fedid: Fedid, | ||
| opa: Annotated[OpaUserClient, Depends(opa)], | ||
| runner: Annotated[WorkerDispatcher, Depends(_runner)], | ||
| ) -> WorkerState: | ||
| """ | ||
|
|
@@ -496,6 +540,19 @@ def set_state( | |
| current_state in _ALLOWED_TRANSITIONS | ||
| and new_state in _ALLOWED_TRANSITIONS[current_state] | ||
| ): | ||
| active = runner.run(interface.get_active_task) | ||
|
|
||
| if ( | ||
| opa | ||
| and not opa.admin() | ||
| and active | ||
| and active.task.metadata.get("user") != fedid | ||
| ): | ||
| raise HTTPException( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail="Not authorized to set worker state", | ||
| ) | ||
|
|
||
| if new_state == WorkerState.PAUSED: | ||
| runner.run(interface.pause_worker, state_change_request.defer) | ||
| elif new_state == WorkerState.RUNNING: | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.