-
Notifications
You must be signed in to change notification settings - Fork 22
Add global default authorizers for core-internal workflows #1215
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
base: main
Are you sure you want to change the base?
Changes from all commits
1291dd9
c9e883a
a073b37
7b6be82
44ed3c4
dfedaa8
0a4ee3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,10 +17,13 @@ | |
| from typing import Literal | ||
|
|
||
| from pydantic import Field, NonNegativeInt, PostgresDsn, RedisDsn | ||
| from pydantic.main import BaseModel | ||
| from pydantic_settings import BaseSettings | ||
|
|
||
| from oauth2_lib.fastapi import OIDCUserModel | ||
| from oauth2_lib.settings import oauth2lib_settings | ||
| from orchestrator.services.settings_env_variables import expose_settings | ||
| from orchestrator.utils.auth import Authorizer | ||
| from orchestrator.utils.expose_settings import SecretStr as OrchSecretStr | ||
| from pydantic_forms.types import strEnum | ||
|
|
||
|
|
@@ -111,3 +114,48 @@ class AppSettings(BaseSettings): | |
| expose_settings("app_settings", app_settings) # type: ignore | ||
| if app_settings.EXPOSE_OAUTH_SETTINGS: | ||
| expose_settings("oauth2lib_settings", oauth2lib_settings) # type: ignore | ||
|
|
||
|
|
||
| class Authorizers(BaseModel): | ||
| # Callbacks specifically for orchestrator-core callbacks. | ||
| # Separate from defaults for user-defined workflows and steps. | ||
| internal_authorize_callback: Authorizer | None = None | ||
| internal_retry_auth_callback: Authorizer | None = None | ||
|
|
||
| async def authorize_callback(self, user: OIDCUserModel | None) -> bool: | ||
| """This is the authorize_callback to be registered for workflows defined within orchestrator-core. | ||
|
|
||
| If Authorizers.internal_authorize_callback is None, this function will return True. | ||
| i.e. any user will be authorized to start internal workflows. | ||
| """ | ||
| if self.internal_authorize_callback is None: | ||
| return True | ||
| return await self.internal_authorize_callback(user) | ||
|
|
||
| async def retry_auth_callback(self, user: OIDCUserModel | None) -> bool: | ||
| """This is the retry_auth_callback to be registered for workflows defined within orchestrator-core. | ||
|
|
||
| If Authorizers.internal_retry_auth_callback is None, this function will return True. | ||
| i.e. any user will be authorized to retry internal workflows on failure. | ||
| """ | ||
| if self.internal_retry_auth_callback is None: | ||
| return True | ||
| return await self.internal_retry_auth_callback(user) | ||
|
|
||
|
|
||
| _authorizers = Authorizers() | ||
|
|
||
|
|
||
| def get_authorizers() -> Authorizers: | ||
|
Contributor
Author
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. Using this to reduce footgun opportunities since this is security-focused. On the other hand, it doesn't match how we handle app_settings, so I'm open to not going this route if folks have strong opinions about it.
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. I think the this need to be registered from the app the same way we have
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. Agree that this doesn't belong to the settings class. What I do think may cause confusion for end users is the class AuthManager:
"""Manages the authentication and authorization mechanisms for the application.
This manager class orchestrates authentication and authorization, utilizing OpenID Connect (OIDC) and
Open Policy Agent (OPA) respectively. It serves as a central hub for user authentication states and
authorization policies. If defaults are insufficient, users can register alternatives or customize existing ones.
...."""An instance of that class is created in the app.register_authentication(surf_authn)
app.register_authorization(surf_authz)
app.register_graphql_authorization(surf_graphql_authz)The AuthManager's docstring suggests that it manages all authn/authz, but obviously this only holds for the API - the workflow authz is specifically for the Worker (which, in case of the threadpool executor, is running in the same process; but for the celery executor it will be separate). I'm not suggesting we should make the Authorizers part of the AuthManager, maybe we can just name/document things well enough to avoid confusion. And perhaps tweak the AuthManager's docstring a little bit to say it's for the API.
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. auth manager isn't specifically for the
Contributor
Author
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.
Just to clarify - the API endpoints for creating and resuming processes do check these callbacks, along with a couple other REST and GraphQL endpoints. I'm not actually aware of the worker running these checks; by checking in the request handler, we can prevent the workflow from ever making it to the queue if the process shouldn't be queued/re-queued. I originally considered adding Should I add it to
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. I said it wrong, yes the API is the only one that does auth checks and the worker shouldn't because of the prevention in the API. |
||
| """Acquire singleton of app authorizers to assign these callbacks at app setup. | ||
|
|
||
| Ensures downstream users can acquire singleton without being tempted to do | ||
| from orchestrator.settings import authorizers | ||
| authorizers = my_authorizers | ||
| or | ||
| from orchestrator import settings | ||
| settings.authorizers = my_authorizers | ||
|
|
||
| ...each of which goes wrong in its own way. | ||
| """ | ||
| return _authorizers | ||
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.
See also #1294