diff --git a/examples/login-with-api.py b/examples/login-with-api.py index 268c2611a..f0bd3f6a4 100644 --- a/examples/login-with-api.py +++ b/examples/login-with-api.py @@ -21,7 +21,6 @@ - ARMADA_API_BASE - OIDC_DOMAIN - OIDC_CLIENT_ID -- OIDC_AUDIENCE These can either be set in a `.env` file or as environment variables. @@ -32,7 +31,6 @@ - OIDC_DOMAIN=http://keycloak.local:8080/realms/jobbergate-local - OIDC_CLIENT_ID=cli -- OIDC_AUDIENCE=https://local.omnivector.solutions - ARMADA_API_BASE=http://localhost:8000 Note: After logging in the first time, running this demo again will use the token saved @@ -52,7 +50,6 @@ domain = os.getenv("OIDC_DOMAIN") client_id = os.getenv("OIDC_CLIENT_ID") -audience = os.getenv("OIDC_AUDIENCE") base_api_url = os.getenv("ARMADA_API_BASE") access_token_file = pathlib.Path("./access.token") @@ -84,7 +81,6 @@ def login(): data=dict( client_id=client_id, grant_type="client_credentials", - audience=audience, ), ) device_code_data = response.json() @@ -122,7 +118,6 @@ def refresh(refresh_token): f"{domain}/protocol/openid-connect/token", data=dict( client_id=client_id, - audience=audience, grant_type="refresh_token", refresh_token=refresh_token, ), diff --git a/jobbergate-agent/CHANGELOG.md b/jobbergate-agent/CHANGELOG.md index 00104bbbc..a09002f23 100644 --- a/jobbergate-agent/CHANGELOG.md +++ b/jobbergate-agent/CHANGELOG.md @@ -5,6 +5,7 @@ This file keeps track of all notable changes to jobbergate-agent ## Unreleased - Changed auto-update task to reuse current scheduler instead of creating a new one - Fixed environment variables from the machine running the agent propagating to slurm jobs (notice `--export=ALL` is the default behavior for sbatch) +- Removed the `OIDC_AUDIENCE` setting ## 5.3.0 -- 2024-09-09 diff --git a/jobbergate-agent/README.md b/jobbergate-agent/README.md index af3f2aaa2..38d8f484b 100644 --- a/jobbergate-agent/README.md +++ b/jobbergate-agent/README.md @@ -21,7 +21,6 @@ To install the package from Pypi simply run `pip install jobbergate-agent`. JOBBERGATE_AGENT_X_SLURM_USER_NAME="" JOBBERGATE_AGENT_SENTRY_DSN="" JOBBERGATE_AGENT_OIDC_DOMAIN="" - JOBBERGATE_AGENT_OIDC_AUDIENCE="" JOBBERGATE_AGENT_OIDC_CLIENT_ID="" JOBBERGATE_AGENT_OIDC_CLIENT_SECRET="" ``` diff --git a/jobbergate-agent/jobbergate_agent/clients/cluster_api.py b/jobbergate-agent/jobbergate_agent/clients/cluster_api.py index e5e8756b0..c3acfd289 100644 --- a/jobbergate-agent/jobbergate_agent/clients/cluster_api.py +++ b/jobbergate-agent/jobbergate_agent/clients/cluster_api.py @@ -25,7 +25,6 @@ def acquire_token(token: Token) -> Token: logger.debug("Attempting to acquire token from OIDC") oidc_body = dict( - audience=SETTINGS.OIDC_AUDIENCE, client_id=SETTINGS.OIDC_CLIENT_ID, client_secret=SETTINGS.OIDC_CLIENT_SECRET, grant_type="client_credentials", diff --git a/jobbergate-agent/jobbergate_agent/settings.py b/jobbergate-agent/jobbergate_agent/settings.py index 239c80ec5..1bcb21688 100644 --- a/jobbergate-agent/jobbergate_agent/settings.py +++ b/jobbergate-agent/jobbergate_agent/settings.py @@ -39,7 +39,6 @@ class Settings(BaseSettings): # OIDC config for machine-to-machine security OIDC_DOMAIN: str - OIDC_AUDIENCE: str = "https://apis.omnivector.solutions" OIDC_CLIENT_ID: str OIDC_CLIENT_SECRET: str OIDC_USE_HTTPS: bool = True diff --git a/jobbergate-cli/CHANGELOG.md b/jobbergate-cli/CHANGELOG.md index b837f971c..7d6050537 100644 --- a/jobbergate-cli/CHANGELOG.md +++ b/jobbergate-cli/CHANGELOG.md @@ -5,6 +5,8 @@ This file keeps track of all notable changes to jobbergate-cli ## Unreleased - Enabled positional arguments to select entries using the cli [ASP-5649] +- Modified internal details to ensure commands that require no authentication face no configuration errors [ASP-5745] +- Removed the `OIDC_AUDIENCE` setting ## 5.3.0 -- 2024-09-09 diff --git a/jobbergate-cli/jobbergate_cli/config.py b/jobbergate-cli/jobbergate_cli/config.py index 30e73a2e3..0b692d232 100644 --- a/jobbergate-cli/jobbergate_cli/config.py +++ b/jobbergate-cli/jobbergate_cli/config.py @@ -64,9 +64,8 @@ class Settings(BaseSettings): JOBBERGATE_LEGACY_NAME_CONVENTION: Optional[bool] = False # Auth0 config for machine-to-machine security - OIDC_DOMAIN: str - OIDC_AUDIENCE: str - OIDC_CLIENT_ID: str + OIDC_DOMAIN: Optional[str] = None + OIDC_CLIENT_ID: Optional[str] = None OIDC_USE_HTTPS: bool = True OIDC_CLIENT_SECRET: Optional[str] = None diff --git a/jobbergate-cli/jobbergate_cli/context.py b/jobbergate-cli/jobbergate_cli/context.py new file mode 100644 index 000000000..2eea319d2 --- /dev/null +++ b/jobbergate-cli/jobbergate_cli/context.py @@ -0,0 +1,62 @@ +""" +Provides a data object for context passed from the main entry point. +""" + +from dataclasses import dataclass +from functools import cached_property + +from buzz import check_expressions +from httpx import Client +from jobbergate_core.auth.handler import JobbergateAuthHandler + +from jobbergate_cli.auth import show_login_message, track_login_progress +from jobbergate_cli.exceptions import Abort +from jobbergate_cli.config import settings +from jobbergate_cli.schemas import ContextProtocol + + +@dataclass +class JobbergateContext(ContextProtocol): + """ + A data object describing context passed from the main entry point. + """ + + raw_output: bool = False + full_output: bool = False + + @cached_property + def client(self) -> Client: + """ + Client for making requests to the Jobbergate API. + """ + return Client( + base_url=settings.ARMADA_API_BASE, + auth=self.authentication_handler, + timeout=settings.JOBBERGATE_REQUESTS_TIMEOUT, + ) + + @cached_property + def authentication_handler(self) -> JobbergateAuthHandler: + """ + The authentication handler for the context. + + This is a cached property to ensure that the handler is only created when needed, + so commands that require no authentication face no configuration errors. + """ + with check_expressions( + "The following settings are required to enable authenticated requests:", + raise_exc_class=Abort, + raise_kwargs=dict(support=True, subject="Configuration error"), + ) as check: + check(settings.OIDC_DOMAIN, "OIDC_DOMAIN") + check(settings.OIDC_CLIENT_ID, "OIDC_CLIENT_ID") + + protocol = "https" if settings.OIDC_USE_HTTPS else "http" + return JobbergateAuthHandler( + cache_directory=settings.JOBBERGATE_USER_TOKEN_DIR, + login_domain=f"{protocol}://{settings.OIDC_DOMAIN}", + login_client_id=settings.OIDC_CLIENT_ID, + login_client_secret=settings.OIDC_CLIENT_SECRET, + login_url_handler=show_login_message, + login_sequence_handler=track_login_progress, + ) diff --git a/jobbergate-cli/jobbergate_cli/main.py b/jobbergate-cli/jobbergate_cli/main.py index 3974b48a4..e658df559 100644 --- a/jobbergate-cli/jobbergate_cli/main.py +++ b/jobbergate-cli/jobbergate_cli/main.py @@ -5,17 +5,15 @@ import sys from typing import Any -import httpx import importlib_metadata import typer -from jobbergate_core.auth.handler import JobbergateAuthHandler -from jobbergate_cli.auth import show_login_message, track_login_progress from jobbergate_cli.config import settings +from jobbergate_cli.context import JobbergateContext from jobbergate_cli.exceptions import Abort, handle_abort, handle_authentication_error from jobbergate_cli.logging import init_logs, init_sentry from jobbergate_cli.render import render_demo, render_json, terminal_message -from jobbergate_cli.schemas import JobbergateContext +from jobbergate_cli.schemas import ContextProtocol from jobbergate_cli.subapps.applications.app import app as applications_app from jobbergate_cli.subapps.job_scripts.app import app as job_scripts_app from jobbergate_cli.subapps.job_submissions.app import app as job_submissions_app @@ -68,27 +66,10 @@ def main( init_logs(verbose=verbose) init_sentry() - protocol = "https" if settings.OIDC_USE_HTTPS else "http" - authentication_handler = JobbergateAuthHandler( - cache_directory=settings.JOBBERGATE_USER_TOKEN_DIR, - login_domain=f"{protocol}://{settings.OIDC_DOMAIN}", - login_audience=settings.OIDC_AUDIENCE, - login_client_id=settings.OIDC_CLIENT_ID, - login_client_secret=settings.OIDC_CLIENT_SECRET, - login_url_handler=show_login_message, - login_sequence_handler=track_login_progress, - ) - - ctx.obj = JobbergateContext( - client=httpx.Client( - base_url=settings.ARMADA_API_BASE, - auth=authentication_handler, - timeout=settings.JOBBERGATE_REQUESTS_TIMEOUT, - ), - authentication_handler=authentication_handler, - full_output=full, - raw_output=raw, - ) + # Stored first as a local variable to enable type checking and make mypy happy with the syntax + # Then stored in the context object to be passed to the subcommands + context: ContextProtocol = JobbergateContext(full_output=full, raw_output=raw) + ctx.obj = context @app.command(rich_help_panel="Authentication") diff --git a/jobbergate-cli/jobbergate_cli/render.py b/jobbergate-cli/jobbergate_cli/render.py index 33fde8c32..da3e8ffc8 100644 --- a/jobbergate-cli/jobbergate_cli/render.py +++ b/jobbergate-cli/jobbergate_cli/render.py @@ -12,7 +12,7 @@ from rich.table import Table from rich.markdown import Markdown -from jobbergate_cli.schemas import JobbergateContext, ListResponseEnvelope +from jobbergate_cli.schemas import ContextProtocol, ListResponseEnvelope from jobbergate_cli.text_tools import dedent from jobbergate_cli.text_tools import indent as indent_text @@ -105,7 +105,7 @@ def render_json(data: Any): def render_list_results( - ctx: JobbergateContext, + ctx: ContextProtocol, envelope: ListResponseEnvelope, style_mapper: StyleMapper | None = None, hidden_fields: list[str] | None = None, @@ -179,7 +179,7 @@ def render_dict( def render_single_result( - ctx: JobbergateContext, + ctx: ContextProtocol, result: dict[str, Any] | pydantic.BaseModel, hidden_fields: list[str] | None = None, title: str = "Result", @@ -212,7 +212,7 @@ def render_single_result( def render_paginated_list_results( - ctx: JobbergateContext, + ctx: ContextProtocol, envelope: ListResponseEnvelope, title: str = "Results List", style_mapper: Optional[StyleMapper] = None, diff --git a/jobbergate-cli/jobbergate_cli/schemas.py b/jobbergate-cli/jobbergate_cli/schemas.py index bb4659b1e..c28449554 100644 --- a/jobbergate-cli/jobbergate_cli/schemas.py +++ b/jobbergate-cli/jobbergate_cli/schemas.py @@ -4,14 +4,14 @@ from datetime import datetime from pathlib import Path -from typing import Any, Dict, Generic, List, Optional, TypeVar +from typing import Any, Dict, Generic, List, Optional, Protocol, TypeVar import httpx import pydantic import pydantic.generics +from jobbergate_core.auth.handler import JobbergateAuthHandler from jobbergate_cli.constants import FileType -from jobbergate_core.auth.handler import JobbergateAuthHandler class TokenSet(pydantic.BaseModel, extra="ignore"): @@ -53,15 +53,22 @@ class DeviceCodeData(pydantic.BaseModel, extra="ignore"): interval: int -class JobbergateContext(pydantic.BaseModel, arbitrary_types_allowed=True): +class ContextProtocol(Protocol): """ - A data object describing context passed from the main entry point. + A protocol describing context passed from the main entry point. + + It aims to help static type checkers at the same time that prevents + circular import issues on the actual implementation. """ - client: httpx.Client - authentication_handler: JobbergateAuthHandler - raw_output: bool = False - full_output: bool = False + raw_output: bool + full_output: bool + + @property + def client(self) -> httpx.Client: ... + + @property + def authentication_handler(self) -> JobbergateAuthHandler: ... class JobbergateConfig(pydantic.BaseModel, extra="allow"): diff --git a/jobbergate-cli/jobbergate_cli/subapps/applications/app.py b/jobbergate-cli/jobbergate_cli/subapps/applications/app.py index 95701ac67..18e1133e3 100644 --- a/jobbergate-cli/jobbergate_cli/subapps/applications/app.py +++ b/jobbergate-cli/jobbergate_cli/subapps/applications/app.py @@ -12,7 +12,7 @@ from jobbergate_cli.exceptions import Abort from jobbergate_cli.render import StyleMapper, render_single_result, terminal_message from jobbergate_cli.requests import make_request -from jobbergate_cli.schemas import ApplicationResponse, JobbergateContext +from jobbergate_cli.schemas import ApplicationResponse, ContextProtocol from jobbergate_cli.subapps.applications.tools import ( fetch_application_data, save_application_files, @@ -21,7 +21,6 @@ from jobbergate_cli.subapps.pagination import handle_pagination from jobbergate_cli.subapps.tools import resolve_application_selection - # TODO: move hidden field logic to the API HIDDEN_FIELDS = [ "cloned_from_id", @@ -74,11 +73,7 @@ def list_all( """ Show available applications """ - jg_ctx: JobbergateContext = ctx.obj - - # Make static type checkers happy - assert jg_ctx is not None - assert jg_ctx.client is not None + jg_ctx: ContextProtocol = ctx.obj params: Dict[str, Any] = dict( include_null_identifier=show_all, @@ -124,7 +119,7 @@ def get_one( """ Get a single application by id or identifier """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj result = fetch_application_data( jg_ctx, id_or_identifier=resolve_application_selection(id_or_identifier, id, identifier) ) @@ -171,10 +166,7 @@ def create( if application_desc: req_data["description"] = application_desc - jg_ctx: JobbergateContext = ctx.obj - - # Make static type checkers happy - assert jg_ctx.client is not None + jg_ctx: ContextProtocol = ctx.obj result = cast( Dict[str, Any], @@ -268,10 +260,7 @@ def update( if application_name: req_data["name"] = application_name - jg_ctx: JobbergateContext = ctx.obj - - # Make static type checkers happy - assert jg_ctx.client is not None + jg_ctx: ContextProtocol = ctx.obj if req_data: make_request( @@ -330,10 +319,7 @@ def delete( """ Delete an existing application. """ - jg_ctx: JobbergateContext = ctx.obj - - # Make static type checkers happy - assert jg_ctx.client is not None + jg_ctx: ContextProtocol = ctx.obj identification = resolve_application_selection(id_or_identifier, id, identifier) @@ -376,7 +362,7 @@ def download_files( """ Download the files from an application to the current working directory. """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj result = fetch_application_data(jg_ctx, resolve_application_selection(id_or_identifier, id, identifier)) saved_files = save_application_files( @@ -444,10 +430,7 @@ def clone( if application_name: req_data["name"] = application_name - jg_ctx: JobbergateContext = ctx.obj - - # Make static type checkers happy - assert jg_ctx.client is not None + jg_ctx: ContextProtocol = ctx.obj result = cast( ApplicationResponse, diff --git a/jobbergate-cli/jobbergate_cli/subapps/applications/tools.py b/jobbergate-cli/jobbergate_cli/subapps/applications/tools.py index d56ea5c0a..d851db9b4 100644 --- a/jobbergate-cli/jobbergate_cli/subapps/applications/tools.py +++ b/jobbergate-cli/jobbergate_cli/subapps/applications/tools.py @@ -26,7 +26,7 @@ ApplicationResponse, JobbergateApplicationConfig, JobbergateConfig, - JobbergateContext, + ContextProtocol, LocalApplication, LocalTemplateFile, LocalWorkflowFile, @@ -104,7 +104,7 @@ def fetch_application_data_locally( ) -def fetch_application_data(jg_ctx: JobbergateContext, id_or_identifier: int | str) -> ApplicationResponse: +def fetch_application_data(jg_ctx: ContextProtocol, id_or_identifier: int | str) -> ApplicationResponse: """ Retrieve an application from the API by id or identifier. @@ -116,9 +116,6 @@ def fetch_application_data(jg_ctx: JobbergateContext, id_or_identifier: int | st Returns: An instance of ApplicationResponse containing the application data. """ - # Make static type checkers happy - assert jg_ctx.client is not None - stub = f"id={id_or_identifier}" if isinstance(id_or_identifier, int) else f"identifier={id_or_identifier}" return cast( ApplicationResponse, @@ -213,7 +210,7 @@ def get_upload_files(application_path: pathlib.Path): def upload_application( - jg_ctx: JobbergateContext, + jg_ctx: ContextProtocol, application_path: pathlib.Path, id_or_identifier: int | str, ): @@ -226,12 +223,6 @@ def upload_application( application_id: The id of the application for which to upload data. application_identifier: The identifier of the application for which to upload data. """ - - # Make static type checkers happy - assert jg_ctx.client is not None - - identification = id_or_identifier - Abort.require_condition(application_path.is_dir(), f"Application directory {application_path} does not exist") config_file_path = application_path / JOBBERGATE_APPLICATION_CONFIG_FILE_NAME @@ -249,7 +240,7 @@ def upload_application( make_request( jg_ctx.client, - f"/jobbergate/job-script-templates/{identification}", + f"/jobbergate/job-script-templates/{id_or_identifier}", "PUT", expect_response=False, abort_message="Request to upload application configuration was not accepted by the API", @@ -272,7 +263,7 @@ def upload_application( make_request( jg_ctx.client, - f"/jobbergate/job-script-templates/{identification}/upload/template/{file_type.value}", + f"/jobbergate/job-script-templates/{id_or_identifier}/upload/template/{file_type.value}", "PUT", expect_response=False, abort_message="Request to upload application files was not accepted by the API", @@ -285,7 +276,7 @@ def upload_application( with open(module_file_path, "rb") as module_file: make_request( jg_ctx.client, - f"/jobbergate/job-script-templates/{identification}/upload/workflow", + f"/jobbergate/job-script-templates/{id_or_identifier}/upload/workflow", "PUT", expect_response=False, abort_message="Request to upload application module was not accepted by the API", @@ -299,15 +290,13 @@ def upload_application( def save_application_files( - jg_ctx: JobbergateContext, + jg_ctx: ContextProtocol, application_data: ApplicationResponse, destination_path: pathlib.Path, ) -> List[pathlib.Path]: """ Save the application files from the API response into a local destination. """ - # Make static type checkers happy - assert jg_ctx.client is not None logger.debug(f"Saving application files to {destination_path.as_posix()}") saved_files: List[pathlib.Path] = [] diff --git a/jobbergate-cli/jobbergate_cli/subapps/clusters/app.py b/jobbergate-cli/jobbergate_cli/subapps/clusters/app.py index 3d725409f..37a720738 100644 --- a/jobbergate-cli/jobbergate_cli/subapps/clusters/app.py +++ b/jobbergate-cli/jobbergate_cli/subapps/clusters/app.py @@ -5,7 +5,7 @@ import typer from jobbergate_cli.render import terminal_message -from jobbergate_cli.schemas import JobbergateContext +from jobbergate_cli.schemas import ContextProtocol from jobbergate_cli.subapps.clusters.tools import get_client_ids @@ -19,9 +19,6 @@ def list_all( """ Show available clusters """ - jg_ctx: JobbergateContext = ctx.obj - - # Make static type checkers happy - assert jg_ctx is not None + jg_ctx: ContextProtocol = ctx.obj terminal_message("\n".join(get_client_ids(jg_ctx)), subject="Cluster Names", color="yellow", indent=True) diff --git a/jobbergate-cli/jobbergate_cli/subapps/clusters/tools.py b/jobbergate-cli/jobbergate_cli/subapps/clusters/tools.py index 28a2479c8..4ac71c997 100644 --- a/jobbergate-cli/jobbergate_cli/subapps/clusters/tools.py +++ b/jobbergate-cli/jobbergate_cli/subapps/clusters/tools.py @@ -11,13 +11,11 @@ from jobbergate_cli.config import settings from jobbergate_cli.exceptions import Abort from jobbergate_cli.requests import make_request -from jobbergate_cli.schemas import ClusterCacheData, JobbergateContext +from jobbergate_cli.schemas import ClusterCacheData, ContextProtocol from jobbergate_cli.text_tools import conjoin -def pull_client_ids_from_api(ctx: JobbergateContext) -> List[str]: - assert ctx.client is not None - +def pull_client_ids_from_api(ctx: ContextProtocol) -> List[str]: response_data = cast( Dict, make_request( @@ -49,9 +47,6 @@ def pull_client_ids_from_api(ctx: JobbergateContext) -> List[str]: def save_clusters_to_cache(client_ids: List[str]): - # Make static type checkers happy - assert settings.JOBBERGATE_CLUSTER_LIST_PATH is not None - cache_data = ClusterCacheData( updated_at=datetime.utcnow(), client_ids=client_ids, @@ -62,9 +57,6 @@ def save_clusters_to_cache(client_ids: List[str]): def load_clusters_from_cache() -> Optional[List[str]]: - # Make static type checkers happy - assert settings.JOBBERGATE_CLUSTER_LIST_PATH is not None - try: cache_data = ClusterCacheData(**json.loads(settings.JOBBERGATE_CLUSTER_LIST_PATH.read_text())) except Exception as err: @@ -78,9 +70,7 @@ def load_clusters_from_cache() -> Optional[List[str]]: return cache_data.client_ids -def get_client_ids(ctx: JobbergateContext) -> List[str]: - assert ctx.client is not None - +def get_client_ids(ctx: ContextProtocol) -> List[str]: client_ids = load_clusters_from_cache() if client_ids is None: client_ids = pull_client_ids_from_api(ctx) @@ -89,7 +79,7 @@ def get_client_ids(ctx: JobbergateContext) -> List[str]: return client_ids -def validate_client_id(ctx: JobbergateContext, client_id: str): +def validate_client_id(ctx: ContextProtocol, client_id: str): client_ids = get_client_ids(ctx) Abort.require_condition( client_id in client_ids, diff --git a/jobbergate-cli/jobbergate_cli/subapps/job_scripts/app.py b/jobbergate-cli/jobbergate_cli/subapps/job_scripts/app.py index 9d19335e5..d7fa67886 100644 --- a/jobbergate-cli/jobbergate_cli/subapps/job_scripts/app.py +++ b/jobbergate-cli/jobbergate_cli/subapps/job_scripts/app.py @@ -13,7 +13,7 @@ from jobbergate_cli.exceptions import Abort from jobbergate_cli.render import StyleMapper, render_single_result, terminal_message from jobbergate_cli.requests import make_request -from jobbergate_cli.schemas import JobbergateContext, JobScriptCreateRequest, JobScriptResponse +from jobbergate_cli.schemas import ContextProtocol, JobScriptCreateRequest, JobScriptResponse from jobbergate_cli.subapps.job_scripts.tools import ( download_job_script_files, fetch_job_script_data, @@ -64,11 +64,7 @@ def list_all( """ Show available job scripts """ - jg_ctx: JobbergateContext = ctx.obj - - # Make static type checkers happy - assert jg_ctx is not None - assert jg_ctx.client is not None + jg_ctx: ContextProtocol = ctx.obj params: Dict[str, Any] = dict(user_only=not show_all) if search is not None: @@ -101,7 +97,7 @@ def get_one( """ Get a single job script by id. """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj id = resolve_selection(id, id_option) result = fetch_job_script_data(jg_ctx, id) @@ -142,12 +138,9 @@ def create_stand_alone( """ Create and upload files for a standalone job script (i.e., unrelated to any application). """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj job_script_path = resolve_selection(job_script_path, job_script_path_option, option_name="job_script_path") - # Make static type checkers happy - assert jg_ctx.client is not None - request_data = JobScriptCreateRequest( name=name, description=description, @@ -219,7 +212,7 @@ def create_locally( The templates will be overwritten with the rendered files. """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj render_job_script_locally( jg_ctx, @@ -315,7 +308,7 @@ def create( """ Create a new job script from an application. """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj selector = resolve_application_selection(id_or_identifier, application_id, application_identifier) job_script_result = render_job_script( @@ -428,12 +421,9 @@ def update( """ Update an existing job script. """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj id = resolve_selection(id, id_option) - # Make static type checkers happy - assert jg_ctx.client is not None - update_params: Dict[str, Any] = dict() if name is not None: update_params.update(name=name) @@ -475,12 +465,9 @@ def delete( """ Delete an existing job script. """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj id = resolve_selection(id, id_option) - # Make static type checkers happy - assert jg_ctx.client is not None - make_request( jg_ctx.client, f"/jobbergate/job-scripts/{id}", @@ -505,7 +492,7 @@ def show_files( """ Show the files for a single job script by id. """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj id = resolve_selection(id, id_option) with tempfile.TemporaryDirectory() as tmp_dir: @@ -538,7 +525,7 @@ def download_files( """ Download the files from a job script to the current working directory. """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj id = resolve_selection(id, id_option) downloaded_files = download_job_script_files(id, jg_ctx, pathlib.Path.cwd()) @@ -574,12 +561,9 @@ def clone( """ Clone an existing job script, so the user can own and modify a copy of it. """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj id = resolve_selection(id, id_option) - # Make static type checkers happy - assert jg_ctx.client is not None - update_params: Dict[str, Any] = dict() if name is not None: update_params.update(name=name) diff --git a/jobbergate-cli/jobbergate_cli/subapps/job_scripts/tools.py b/jobbergate-cli/jobbergate_cli/subapps/job_scripts/tools.py index abf2d5b94..97d8cc96f 100644 --- a/jobbergate-cli/jobbergate_cli/subapps/job_scripts/tools.py +++ b/jobbergate-cli/jobbergate_cli/subapps/job_scripts/tools.py @@ -20,7 +20,7 @@ from jobbergate_cli.requests import make_request from jobbergate_cli.schemas import ( JobbergateConfig, - JobbergateContext, + ContextProtocol, JobScriptCreateRequest, JobScriptFile, JobScriptRenderRequestData, @@ -73,15 +73,12 @@ def validate_parameter_file(parameter_path: pathlib.Path) -> Dict[str, Any]: def fetch_job_script_data( - jg_ctx: JobbergateContext, + jg_ctx: ContextProtocol, id: int, ) -> JobScriptResponse: """ Retrieve a job_script from the API by ``id`` """ - # Make static type checkers happy - assert jg_ctx.client is not None - return cast( JobScriptResponse, make_request( @@ -232,7 +229,7 @@ def render_template( def render_job_script_locally( - jg_ctx: JobbergateContext, + jg_ctx: ContextProtocol, job_script_name: str, application_path: pathlib.Path, output_path: pathlib.Path, @@ -252,9 +249,6 @@ def render_job_script_locally( param_file: Path to a parameters file. fast: Whether to use default answers (when available) instead of asking the user. """ - # Make static type checkers happy - assert jg_ctx.client is not None - app_data = fetch_application_data_locally(application_path) if not app_data.workflow_files: @@ -303,7 +297,7 @@ def render_job_script_locally( def render_job_script( - jg_ctx: JobbergateContext, + jg_ctx: ContextProtocol, id_or_identifier: int | str, name: Optional[str] = None, description: Optional[str] = None, @@ -327,9 +321,6 @@ def render_job_script( Returns: The new job script. """ - # Make static type checkers happy - assert jg_ctx.client is not None - app_data = fetch_application_data(jg_ctx, id_or_identifier) if not app_data.workflow_files: @@ -382,9 +373,6 @@ def render_job_script( ), ) - # Make static type checkers happy - assert jg_ctx.client is not None - job_script_result = cast( JobScriptResponse, make_request( @@ -403,7 +391,7 @@ def render_job_script( def upload_job_script_files( - jg_ctx: JobbergateContext, + jg_ctx: ContextProtocol, job_script_id: int, job_script_path: pathlib.Path, supporting_file_paths: list[pathlib.Path] | None = None, @@ -466,14 +454,11 @@ def upload_job_script_files( def save_job_script_file( - jg_ctx: JobbergateContext, destination_path: pathlib.Path, job_script_file: JobScriptFile + jg_ctx: ContextProtocol, destination_path: pathlib.Path, job_script_file: JobScriptFile ) -> pathlib.Path: """ Save a job script file from the API response to the destination path. """ - # Make static type checkers happy - assert jg_ctx.client is not None - filename = job_script_file.filename file_path = destination_path / filename file_path.parent.mkdir(parents=True, exist_ok=True) @@ -490,9 +475,7 @@ def save_job_script_file( return file_path -def download_job_script_files( - id: int, jg_ctx: JobbergateContext, destination_path: pathlib.Path -) -> List[JobScriptFile]: +def download_job_script_files(id: int, jg_ctx: ContextProtocol, destination_path: pathlib.Path) -> List[JobScriptFile]: """ Download all job script files from the API and save them to the destination path. """ diff --git a/jobbergate-cli/jobbergate_cli/subapps/job_submissions/app.py b/jobbergate-cli/jobbergate_cli/subapps/job_submissions/app.py index b16b57aec..ff647c60b 100644 --- a/jobbergate-cli/jobbergate_cli/subapps/job_submissions/app.py +++ b/jobbergate-cli/jobbergate_cli/subapps/job_submissions/app.py @@ -12,7 +12,7 @@ from jobbergate_cli.exceptions import Abort from jobbergate_cli.render import StyleMapper, render_single_result, terminal_message from jobbergate_cli.requests import make_request -from jobbergate_cli.schemas import JobbergateContext, JobSubmissionResponse +from jobbergate_cli.schemas import ContextProtocol, JobSubmissionResponse from jobbergate_cli.subapps.job_submissions.tools import fetch_job_submission_data, job_submissions_factory from jobbergate_cli.subapps.pagination import handle_pagination from jobbergate_cli.subapps.tools import resolve_selection @@ -93,7 +93,7 @@ def create( """ Create a new job submission. """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj job_script_id = resolve_selection(job_script_id, job_script_id_option) try: @@ -144,7 +144,7 @@ def list_all( """ Show available job submissions. """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj params: Dict[str, Any] = dict(user_only=not show_all) if search is not None: @@ -183,12 +183,9 @@ def get_one( """ Get a single job submission by id """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj id = resolve_selection(id, id_option) - # Make static type checkers happy - assert jg_ctx is not None, "JobbergateContext is uninitialized" - value_mappers = None organization_id = jg_ctx.authentication_handler.get_identity_data().organization_id if organization_id is not None: @@ -224,12 +221,9 @@ def delete( """ Delete an existing job submission. """ - jg_ctx: JobbergateContext = ctx.obj + jg_ctx: ContextProtocol = ctx.obj id = resolve_selection(id, id_option) - # Make static type checkers happy - assert jg_ctx.client is not None, "Client is uninitialized" - make_request( jg_ctx.client, f"/jobbergate/job-submissions/{id}", diff --git a/jobbergate-cli/jobbergate_cli/subapps/job_submissions/tools.py b/jobbergate-cli/jobbergate_cli/subapps/job_submissions/tools.py index 236da6e9b..6038935ae 100644 --- a/jobbergate-cli/jobbergate_cli/subapps/job_submissions/tools.py +++ b/jobbergate-cli/jobbergate_cli/subapps/job_submissions/tools.py @@ -15,12 +15,12 @@ from jobbergate_cli.constants import FileType from jobbergate_cli.exceptions import Abort from jobbergate_cli.requests import make_request -from jobbergate_cli.schemas import JobbergateContext, JobSubmissionCreateRequestData, JobSubmissionResponse +from jobbergate_cli.schemas import ContextProtocol, JobSubmissionCreateRequestData, JobSubmissionResponse from jobbergate_cli.subapps.job_scripts.tools import download_job_script_files def _map_cluster_name( - jg_ctx: JobbergateContext, + jg_ctx: ContextProtocol, base_cluster_name: str, ) -> str: """ @@ -56,7 +56,7 @@ class JobSubmissionABC(ABC): sbatch_arguments: An optional list of arguments to pass to inject into the job script. """ - jg_ctx: JobbergateContext + jg_ctx: ContextProtocol job_script_id: int name: str execution_directory: Path | None = None @@ -96,8 +96,6 @@ def get_request_data(self) -> JobSubmissionCreateRequestData: def make_post_request(self, job_submission_data: JobSubmissionCreateRequestData) -> JobSubmissionResponse: """Make the POST request to the API to create the job submission.""" - # Make static type checkers happy - assert self.jg_ctx.client is not None, "jg_ctx.client is uninitialized" return cast( JobSubmissionResponse, make_request( @@ -194,7 +192,7 @@ def get_request_data(self) -> JobSubmissionCreateRequestData: def job_submissions_factory( - jg_ctx: JobbergateContext, + jg_ctx: ContextProtocol, job_script_id: int, name: str, execution_directory: Path | None = None, @@ -228,14 +226,12 @@ def job_submissions_factory( def fetch_job_submission_data( - jg_ctx: JobbergateContext, + jg_ctx: ContextProtocol, job_submission_id: int, ) -> JobSubmissionResponse: """ Retrieve a job submission from the API by ``id`` """ - # Make static type checkers happy - assert jg_ctx.client is not None, "Client is uninitialized" return cast( JobSubmissionResponse, diff --git a/jobbergate-cli/jobbergate_cli/subapps/pagination.py b/jobbergate-cli/jobbergate_cli/subapps/pagination.py index faee463f0..f814a2b57 100644 --- a/jobbergate-cli/jobbergate_cli/subapps/pagination.py +++ b/jobbergate-cli/jobbergate_cli/subapps/pagination.py @@ -8,11 +8,11 @@ from jobbergate_cli.constants import PaginationChoices from jobbergate_cli.render import StyleMapper, render_paginated_list_results from jobbergate_cli.requests import make_request -from jobbergate_cli.schemas import JobbergateContext, ListResponseEnvelope +from jobbergate_cli.schemas import ContextProtocol, ListResponseEnvelope def handle_pagination( - jg_ctx: JobbergateContext, + jg_ctx: ContextProtocol, url_path: str, abort_message: str = "There was an error communicating with the API", params: Optional[Dict[str, Any]] = None, @@ -22,9 +22,6 @@ def handle_pagination( nested_response_model_cls: Type[pydantic.BaseModel] | None = None, value_mappers: Optional[Dict[str, Callable[[Any], Any]]] = None, ): - assert jg_ctx is not None - assert jg_ctx.client is not None - current_page = 1 while True: diff --git a/jobbergate-cli/pyproject.toml b/jobbergate-cli/pyproject.toml index 766246dea..68d23e084 100644 --- a/jobbergate-cli/pyproject.toml +++ b/jobbergate-cli/pyproject.toml @@ -75,7 +75,6 @@ env = [ "JOBBERGATE_DEBUG = false", "JOBBERGATE_CACHE_DIR = /tmp/jobbergate-cache", "OIDC_DOMAIN = dummy_auth_domain.com", - "OIDC_AUDIENCE = https://dummy_auth_audience.com", "OIDC_CLIENT_ID = dummy_client_id", ] diff --git a/jobbergate-cli/tests/subapps/conftest.py b/jobbergate-cli/tests/subapps/conftest.py index aeac5ef19..dc486343f 100644 --- a/jobbergate-cli/tests/subapps/conftest.py +++ b/jobbergate-cli/tests/subapps/conftest.py @@ -9,8 +9,9 @@ from typer.testing import CliRunner from jobbergate_cli.constants import JOBBERGATE_APPLICATION_CONFIG_FILE_NAME, JOBBERGATE_APPLICATION_MODULE_FILE_NAME +from jobbergate_cli.context import JobbergateContext from jobbergate_cli.exceptions import handle_abort, handle_authentication_error -from jobbergate_cli.schemas import IdentityData, JobbergateApplicationConfig, JobbergateContext +from jobbergate_cli.schemas import IdentityData, JobbergateApplicationConfig, ContextProtocol from jobbergate_cli.subapps.applications.tools import load_application_from_source from jobbergate_cli.text_tools import dedent @@ -41,20 +42,20 @@ def _helper(command_name: str, command_function: Callable): @pytest.fixture -def dummy_context(mocker, tmp_path, dummy_domain) -> Generator[JobbergateContext, None, None]: +def dummy_context(mocker, tmp_path, dummy_domain) -> Generator[ContextProtocol, None, None]: def dummy_auth(request: httpx.Request) -> httpx.Request: request.headers["Authorization"] = "Bearer XXXXXXXX" return request - authentication_handler = JobbergateAuthHandler( - cache_directory=Path(tmp_path), - login_domain="test-domain", - login_audience="test-audience", - ) + authentication_handler = JobbergateAuthHandler(cache_directory=Path(tmp_path), login_domain="test-domain") + + context = JobbergateContext() + with mocker.patch.object(authentication_handler, attribute="acquire_access", return_value=dummy_auth): - yield JobbergateContext( - client=httpx.Client(base_url=dummy_domain), authentication_handler=authentication_handler - ) + # This is all it takes to replace both cached properties + context.client = httpx.Client(base_url=dummy_domain) + context.authentication_handler = authentication_handler + yield context @pytest.fixture diff --git a/jobbergate-cli/tests/test_config.py b/jobbergate-cli/tests/test_config.py index 7b096a280..1eb9926ef 100644 --- a/jobbergate-cli/tests/test_config.py +++ b/jobbergate-cli/tests/test_config.py @@ -1,10 +1,9 @@ -import os from pathlib import Path import pydantic import pytest -from jobbergate_cli.config import Settings, build_settings +from jobbergate_cli.config import Settings def test_SENTRY_TRACE_RATE__requires_float_in_valid_range(): @@ -17,21 +16,6 @@ def test_SENTRY_TRACE_RATE__requires_float_in_valid_range(): Settings(SENTRY_TRACE_RATE=1.1) -def test_Validation_error__when_parameter_is_missing(): - original_value = os.environ.get("OIDC_DOMAIN") - try: - if "OIDC_DOMAIN" in os.environ: - del os.environ["OIDC_DOMAIN"] - - with pytest.raises(SystemExit) as e: - build_settings(_env_file=None) - - assert e.value.code == 1 - finally: - if original_value is not None: - os.environ["OIDC_DOMAIN"] = original_value - - def test_cache_dir__expands_user_and_resolves(): settings = Settings(JOBBERGATE_CACHE_DIR="~/.jobbergate-cli-prod") assert settings.JOBBERGATE_CACHE_DIR == Path.home() / ".jobbergate-cli-prod" diff --git a/jobbergate-cli/tests/test_context.py b/jobbergate-cli/tests/test_context.py new file mode 100644 index 000000000..01f2aef6a --- /dev/null +++ b/jobbergate-cli/tests/test_context.py @@ -0,0 +1,83 @@ +import re +from unittest import mock + +import pytest + +from jobbergate_cli.auth import show_login_message, track_login_progress +from jobbergate_cli.context import JobbergateContext +from jobbergate_cli.config import settings +from jobbergate_cli.exceptions import Abort + + +@mock.patch("jobbergate_cli.context.Client") +def test_client_is_lazy_on_context(mocked_client, tweak_settings): + ctx = JobbergateContext() + + mocked_client.assert_not_called() + + local_settings = dict(ARMADA_API_BASE="test.example.com/", JOBBERGATE_REQUESTS_TIMEOUT=42) + + with tweak_settings(**local_settings): + client = ctx.client + + assert client == mocked_client.return_value + + mocked_client.assert_called_once_with( + base_url=local_settings["ARMADA_API_BASE"], + auth=ctx.authentication_handler, + timeout=local_settings["JOBBERGATE_REQUESTS_TIMEOUT"], + ) + + +@mock.patch("jobbergate_cli.context.JobbergateAuthHandler") +def test_authentication_handler_is_lazy_on_context(mocked_auth_handler, tweak_settings): + ctx = JobbergateContext() + + mocked_auth_handler.assert_not_called() + + local_settings = dict( + OIDC_USE_HTTPS=True, + OIDC_DOMAIN="example.com", + OIDC_CLIENT_ID="client-id", + OIDC_CLIENT_SECRET="client-secret", + ) + + with tweak_settings(**local_settings): + auth_handler = ctx.authentication_handler + + assert auth_handler == mocked_auth_handler.return_value + + mocked_auth_handler.assert_called_once_with( + cache_directory=settings.JOBBERGATE_USER_TOKEN_DIR, + login_domain=f"https://{local_settings['OIDC_DOMAIN']}", + login_client_id=local_settings["OIDC_CLIENT_ID"], + login_client_secret=local_settings["OIDC_CLIENT_SECRET"], + login_url_handler=show_login_message, + login_sequence_handler=track_login_progress, + ) + + +@mock.patch("jobbergate_cli.context.JobbergateAuthHandler") +@pytest.mark.parametrize("missing_setting", ["OIDC_DOMAIN", "OIDC_CLIENT_ID"]) +def test_authentication_handler__fails_on_missing_setting(mocked_auth_handler, missing_setting, tweak_settings): + ctx = JobbergateContext() + + local_settings = dict( + OIDC_DOMAIN="example.com", + OIDC_CLIENT_ID="client-id", + OIDC_CLIENT_SECRET="client-secret", + ) + + local_settings[missing_setting] = None + + with tweak_settings(**local_settings): + with pytest.raises( + Abort, + match=re.compile( + f"The following settings are required to enable authenticated requests:.*{missing_setting}$", + flags=re.DOTALL, + ), + ): + ctx.authentication_handler + + mocked_auth_handler.assert_not_called() diff --git a/jobbergate-composed/docker-compose.yml b/jobbergate-composed/docker-compose.yml index 17ab3fcfb..09f6d6206 100644 --- a/jobbergate-composed/docker-compose.yml +++ b/jobbergate-composed/docker-compose.yml @@ -94,7 +94,6 @@ services: environment: - ARMADA_API_BASE=http://jobbergate-api:80 - OIDC_DOMAIN=keycloak.local:8080/realms/jobbergate-local - - OIDC_AUDIENCE=https://local.omnivector.solutions - OIDC_CLIENT_ID=cli - OIDC_USE_HTTPS=false - JOBBERGATE_CACHE_DIR=/cache @@ -263,7 +262,6 @@ services: - JOBBERGATE_AGENT_DEFAULT_SLURM_WORK_DIR=/slurm-work-dir - JOBBERGATE_AGENT_BASE_API_URL=http://jobbergate-api:80 - JOBBERGATE_AGENT_OIDC_DOMAIN=keycloak.local:8080/realms/jobbergate-local - - JOBBERGATE_AGENT_OIDC_AUDIENCE=https://local.omnivector.solutions - JOBBERGATE_AGENT_OIDC_CLIENT_ID=local-slurm - JOBBERGATE_AGENT_OIDC_CLIENT_SECRET=SVkaJ2f9xeYfOVzQPHXYyiwr12za4xGF - JOBBERGATE_AGENT_OIDC_USE_HTTPS=false diff --git a/jobbergate-core/CHANGELOG.md b/jobbergate-core/CHANGELOG.md index 1d7b54d21..773f1d379 100644 --- a/jobbergate-core/CHANGELOG.md +++ b/jobbergate-core/CHANGELOG.md @@ -4,6 +4,7 @@ This file keeps track of all notable changes to jobbergate-core ## Unreleased +- Removed the `OIDC_AUDIENCE` setting ## 5.3.0 -- 2024-09-09 diff --git a/jobbergate-core/jobbergate_core/auth/handler.py b/jobbergate-core/jobbergate_core/auth/handler.py index dc7198d95..dea99c543 100644 --- a/jobbergate-core/jobbergate_core/auth/handler.py +++ b/jobbergate-core/jobbergate_core/auth/handler.py @@ -93,7 +93,6 @@ class JobbergateAuthHandler: Arguments: cache_directory: Directory to be used for the caching tokens. login_domain: Domain used for the login. - login_audience: Audience of the login. login_client_id: Client ID used for login. Note: @@ -115,7 +114,6 @@ class to authenticate a request: >>> jobbergate_auth = JobbergateAuthHandler( ... cache_directory=Path("."), ... login_domain="http://keycloak.local:8080/realms/jobbergate-local", - ... login_audience="https://local.omnivector.solutions", ... login_client_id="cli", ... ) >>> jobbergate_base_url = "http://localhost:8000/jobbergate" @@ -130,7 +128,6 @@ class to authenticate a request: cache_directory: Path login_domain: str - login_audience: str login_client_id: str = "default" login_client_secret: str | None = None login_url_handler: Callable[[DeviceCodeData], None] = print_login_url @@ -212,7 +209,6 @@ def get_access_from_secret(self) -> None: method="POST", request_kwargs={ "data": { - "audience": self.login_audience, "client_id": self.login_client_id, "client_secret": self.login_client_secret, "grant_type": "client_credentials", @@ -316,7 +312,6 @@ def _get_device_code(self) -> DeviceCodeData: "data": { "client_id": self.login_client_id, "grant_type": "client_credentials", - "audience": self.login_audience, } }, sensitive_keys={"device_code"}, @@ -357,7 +352,6 @@ def _get_refresh_token(self) -> TokenInformation: request_kwargs={ "data": { "client_id": self.login_client_id, - "audience": self.login_audience, "grant_type": "refresh_token", "refresh_token": self._refresh_token.content, }, diff --git a/jobbergate-core/tests/auth/test_handler.py b/jobbergate-core/tests/auth/test_handler.py index 7f8cacfe7..4b4d49730 100644 --- a/jobbergate-core/tests/auth/test_handler.py +++ b/jobbergate-core/tests/auth/test_handler.py @@ -16,7 +16,6 @@ DUMMY_LOGIN_DOMAIN = "http://keycloak.local:8080/realms/jobbergate-local" -DUMMY_LOGIN_AUDIENCE = "https://local.omnivector.solutions" DUMMY_LOGIN_CLIENT_ID = "cli" @@ -28,7 +27,6 @@ def dummy_jobbergate_auth(tmp_path): return JobbergateAuthHandler( cache_directory=tmp_path, login_domain=DUMMY_LOGIN_DOMAIN, - login_audience=DUMMY_LOGIN_AUDIENCE, login_client_id=DUMMY_LOGIN_CLIENT_ID, ) @@ -39,7 +37,6 @@ def test_auth_base_case(tmp_path, dummy_jobbergate_auth): """ assert dummy_jobbergate_auth.cache_directory == tmp_path assert dummy_jobbergate_auth.login_domain == DUMMY_LOGIN_DOMAIN - assert dummy_jobbergate_auth.login_audience == DUMMY_LOGIN_AUDIENCE assert dummy_jobbergate_auth.login_client_id == DUMMY_LOGIN_CLIENT_ID assert dummy_jobbergate_auth._access_token.content == ""