-
Notifications
You must be signed in to change notification settings - Fork 8
feat: Token exchange for tiled insertion #1342
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
c1b373e
82337c5
e4ac3a6
92157dc
11f8d8c
b12204c
8780c9d
c8f22ff
dd175d6
9cd9ffe
41b614d
5479b4a
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 |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| tests/system_tests/services/blueapi-oauth2-proxy/oauth2-proxy.cfg:generic-api-key:16 | ||
| tests/system_tests/services/tiled-oauth2-proxy/oauth2-proxy.cfg:generic-api-key:14 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,7 +112,13 @@ class TiledConfig(BlueapiBaseModel): | |
| default=False, | ||
| ) | ||
| url: HttpUrl = HttpUrl("http://localhost:8407") | ||
| api_key: str | None = os.environ.get("TILED_SINGLE_USER_API_KEY", None) | ||
| token_exchange_secret: str = Field( | ||
|
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. Are these three fields always going to be all present or all absent? Could we make it into a nested |
||
| description="Token exchange client secret", default="" | ||
| ) | ||
| token_url: str = Field(default="") | ||
| token_exchange_client_id: str = Field( | ||
|
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 is the client_id of the tiled service? The keycloak docs use the terms |
||
| description="Token exchange Client ID", default="" | ||
| ) | ||
|
|
||
|
|
||
| class WorkerEventConfig(BlueapiBaseModel): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,6 +181,17 @@ def _update_scan_num(md: dict[str, Any]) -> int: | |
| "Tiled has been configured but `instrument` metadata is not set - " | ||
| "this field is required to make authorization decisions." | ||
| ) | ||
| if configuration.oidc is None: | ||
| raise InvalidConfigError( | ||
| "Tiled has been configured but oidc configuration is missing " | ||
| "this field is required to make authorization decisions." | ||
| ) | ||
| if tiled_conf.token_exchange_secret == "": | ||
| raise InvalidConfigError( | ||
| "Tiled has been enabled but Token exchange secret has not been set " | ||
| "this field is required to enable tiled insertion." | ||
| ) | ||
| tiled_conf.token_url = configuration.oidc.token_endpoint | ||
|
Comment on lines
+184
to
+194
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. Does this mean we can't run blueapi against unauthenticated local tiled instances for testing? |
||
| self.tiled_conf = tiled_conf | ||
|
|
||
| def find_device(self, addr: str | list[str]) -> Device | None: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,24 +2,27 @@ | |
|
|
||
| import base64 | ||
| import os | ||
| import threading | ||
| import time | ||
| import webbrowser | ||
| from abc import ABC, abstractmethod | ||
| from enum import Enum | ||
| from functools import cached_property | ||
| from http import HTTPStatus | ||
| from pathlib import Path | ||
| from typing import Any, cast | ||
|
|
||
| import httpx | ||
| import jwt | ||
| import requests | ||
| from pydantic import TypeAdapter | ||
| from pydantic import BaseModel, TypeAdapter, computed_field | ||
| from requests.auth import AuthBase | ||
|
|
||
| from blueapi.config import OIDCConfig | ||
| from blueapi.config import OIDCConfig, TiledConfig | ||
| from blueapi.service.model import Cache | ||
|
|
||
| DEFAULT_CACHE_DIR = "~/.cache/" | ||
| SCOPES = "openid offline_access" | ||
| SCOPES = "openid" | ||
|
|
||
|
|
||
| class CacheManager(ABC): | ||
|
|
@@ -239,3 +242,104 @@ def __call__(self, request): | |
| if self.token: | ||
| request.headers["Authorization"] = f"Bearer {self.token}" | ||
| return request | ||
|
|
||
|
|
||
| class TokenType(str, Enum): | ||
| refresh_token = "refresh_token" | ||
| access_token = "access_token" | ||
|
|
||
|
|
||
| class Token(BaseModel): | ||
| token: str | ||
| expires_at: float | None | ||
|
|
||
| @computed_field | ||
| @property | ||
| def expired(self) -> bool: | ||
| if self.expires_at is None: | ||
| # Assume token is valid | ||
| return False | ||
| return time.time() > self.expires_at | ||
|
|
||
| def _get_token_expires_at( | ||
| self, token_dict: dict[str, Any], token_type: TokenType | ||
| ) -> int | None: | ||
| expires_at = None | ||
| if token_type == TokenType.access_token: | ||
| if "expires_in" in token_dict: | ||
| expires_at = int(time.time()) + int(token_dict["expires_in"]) | ||
| elif token_type == TokenType.refresh_token: | ||
| if "refresh_expires_in" in token_dict: | ||
| expires_at = int(time.time()) + int(token_dict["refresh_expires_in"]) | ||
| return expires_at | ||
|
|
||
| def __init__(self, token_dict: dict[str, Any], token_type: TokenType): | ||
| token = token_dict.get(token_type) | ||
| if token is None: | ||
| raise ValueError(f"Not able to find {token_type} in response") | ||
| super().__init__( | ||
| token=token, expires_at=self._get_token_expires_at(token_dict, token_type) | ||
| ) | ||
|
|
||
| def __str__(self) -> str: | ||
| return str(self.token) | ||
|
|
||
|
|
||
| class TiledAuth(httpx.Auth): | ||
| def __init__(self, tiled_config: TiledConfig, blueapi_jwt_token: str): | ||
| self._tiled_config = tiled_config | ||
| self._blueapi_jwt_token = blueapi_jwt_token | ||
| self._sync_lock = threading.RLock() | ||
| self._access_token: Token | None = None | ||
| self._refresh_token: Token | None = None | ||
|
|
||
| def exchange_access_token(self): | ||
| request_data = { | ||
| "client_id": self._tiled_config.token_exchange_client_id, | ||
| "client_secret": self._tiled_config.token_exchange_secret, | ||
| "subject_token": self._blueapi_jwt_token, | ||
| "subject_token_type": "urn:ietf:params:oauth:token-type:access_token", | ||
| "grant_type": "urn:ietf:params:oauth:grant-type:token-exchange", | ||
| "requested_token_type": "urn:ietf:params:oauth:token-type:refresh_token", | ||
| } | ||
| with self._sync_lock: | ||
| response = httpx.post( | ||
| self._tiled_config.token_url, | ||
| data=request_data, | ||
| ) | ||
| response.raise_for_status() | ||
| self.sync_tokens(response.json()) | ||
|
|
||
| def refresh_token(self): | ||
| if self._refresh_token is None: | ||
| raise Exception("Cannot refresh session as no refresh token available") | ||
| with self._sync_lock: | ||
| response = httpx.post( | ||
|
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. Do we have to use httpx? I think we're using requests for all other blueapi http stuff (including in this module) and keeping it to a single library would be good if we can. There's an issue to look at moving everything to |
||
| self._tiled_config.token_url, | ||
| data={ | ||
| "client_id": self._tiled_config.token_exchange_client_id, | ||
| "client_secret": self._tiled_config.token_exchange_secret, | ||
| "grant_type": "refresh_token", | ||
| "refresh_token": self._refresh_token, | ||
| }, | ||
| headers={"Content-Type": "application/x-www-form-urlencoded"}, | ||
| ) | ||
| response.raise_for_status() | ||
| self.sync_tokens(response.json()) | ||
|
|
||
| def sync_tokens(self, response): | ||
| self._access_token = Token(response, TokenType.access_token) | ||
| self._refresh_token = Token(response, TokenType.refresh_token) | ||
|
|
||
| def sync_auth_flow(self, request): | ||
| if self._access_token is not None and self._access_token.expired is not True: | ||
| request.headers["Authorization"] = f"Bearer {self._access_token}" | ||
| yield request | ||
| elif self._access_token is None: | ||
| self.exchange_access_token() | ||
| request.headers["Authorization"] = f"Bearer {self._access_token}" | ||
| yield request | ||
| else: | ||
| self.refresh_token() | ||
| request.headers["Authorization"] = f"Bearer {self._access_token}" | ||
| yield request | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,4 @@ | ||||||
| CONTEXT_HEADER = "traceparent" | ||||||
| VENDOR_CONTEXT_HEADER = "tracestate" | ||||||
| AUTHORIZAITON_HEADER = "authorization" | ||||||
|
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.
Suggested change
|
||||||
| PROPAGATED_HEADERS = {CONTEXT_HEADER, VENDOR_CONTEXT_HEADER, AUTHORIZAITON_HEADER} | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,8 @@ | |||||||||||||||||||||||
| from blueapi.core.context import BlueskyContext | ||||||||||||||||||||||||
| from blueapi.core.event import EventStream | ||||||||||||||||||||||||
| from blueapi.log import set_up_logging | ||||||||||||||||||||||||
| from blueapi.service.authentication import TiledAuth | ||||||||||||||||||||||||
| from blueapi.service.constants import AUTHORIZAITON_HEADER | ||||||||||||||||||||||||
| from blueapi.service.model import ( | ||||||||||||||||||||||||
| DeviceModel, | ||||||||||||||||||||||||
| PlanModel, | ||||||||||||||||||||||||
|
|
@@ -184,10 +186,27 @@ def begin_task( | |||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if tiled_config := active_context.tiled_conf: | ||||||||||||||||||||||||
| # Tiled queries the root node, so must create an authorized client | ||||||||||||||||||||||||
| blueapi_jwt_token = "" | ||||||||||||||||||||||||
| if pass_through_headers is None: | ||||||||||||||||||||||||
| raise ValueError( | ||||||||||||||||||||||||
| "Tiled config is enabled but no " | ||||||||||||||||||||||||
| f"{AUTHORIZAITON_HEADER} header in request" | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| authorization_header_value = pass_through_headers.get(AUTHORIZAITON_HEADER) | ||||||||||||||||||||||||
|
Comment on lines
+190
to
+195
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 doesn't check that the pass through headers contains the auth header. Might as well try and get it and fail if it's not there. Might need to default pass_through_headers if it's None (or make it required?) - numtracker is already needing to do it above. It would also be good if we could still run blueapi in testing using an unauthenticated instance of tiled. Could we make auth optional if it's not configured then make the error useful if we get a 401 back from tiled?
Suggested change
|
||||||||||||||||||||||||
| from fastapi.security.utils import get_authorization_scheme_param | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| _, blueapi_jwt_token = get_authorization_scheme_param( | ||||||||||||||||||||||||
| authorization_header_value | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if blueapi_jwt_token == "": | ||||||||||||||||||||||||
| raise KeyError("Tiled config is enabled but no Bearer Token in request") | ||||||||||||||||||||||||
| tiled_client = from_uri( | ||||||||||||||||||||||||
| str(tiled_config.url), | ||||||||||||||||||||||||
| api_key=tiled_config.api_key, | ||||||||||||||||||||||||
| headers=pass_through_headers, | ||||||||||||||||||||||||
| auth=TiledAuth( | ||||||||||||||||||||||||
| tiled_config, | ||||||||||||||||||||||||
| blueapi_jwt_token=blueapi_jwt_token, | ||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| tiled_writer_token = active_context.run_engine.subscribe( | ||||||||||||||||||||||||
| TiledWriter(tiled_client, batch_size=1) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
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.
Maybe protect the client secret in case the config ever makes its way into logging etc