Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions helm/blueapi/config_schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -340,8 +340,20 @@
"minLength": 1,
"title": "Root",
"type": "string"
},
"audience": {
"default": "account",
"title": "Audience",
"type": "string"
},
"tiled_service_account_check": {
"title": "Tiled Service Account Check",
"type": "string"
}
},
"required": [
"tiled_service_account_check"
],
"title": "OpaConfig",
"type": "object",
"$id": "OpaConfig"
Expand Down
12 changes: 12 additions & 0 deletions helm/blueapi/values.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -755,14 +755,26 @@
"$id": "OpaConfig",
"title": "OpaConfig",
"type": "object",
"required": [
"tiled_service_account_check"
],
"properties": {
"audience": {
"title": "Audience",
"default": "account",
"type": "string"
},
"root": {
"title": "Root",
"default": "http://localhost:8181/",
"type": "string",
"format": "uri",
"maxLength": 2083,
"minLength": 1
},
"tiled_service_account_check": {
"title": "Tiled Service Account Check",
"type": "string"
}
},
"additionalProperties": false
Expand Down
1 change: 1 addition & 0 deletions src/blueapi/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ class Tag(StrEnum):
class OpaConfig(BlueapiBaseModel):
root: HttpUrl = HttpUrl("http://localhost:8181")
audience: str = "account"
tiled_service_account_check: str
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This changes only when the policy changes so we can have a default value for it here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends if we want to include diamond specific configuration as defaults? As far as I can see so far, there is nothing included so far. Would the services copier template be a better place to put the default values?



class ApplicationConfig(BlueapiBaseModel):
Expand Down
29 changes: 28 additions & 1 deletion src/blueapi/service/authorization.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@

from aiohttp import ClientSession

from blueapi.config import OpaConfig
from blueapi.config import OIDCConfig, OpaConfig, ServiceAccount
from blueapi.service.authentication import TiledAuth

LOGGER = logging.getLogger(__name__)

Expand Down Expand Up @@ -45,3 +46,29 @@ def for_config(
return aclosing(cls(instrument, config))
LOGGER.info("No OPA config provided - not creating OpaClient")
return nullcontext()

async def require_tiled_service_account(self, token: str):
if not await self._call_opa(
self._config.tiled_service_account_check,
{"token": token, "beamline": self._instrument},
):
raise ValueError(
f"Tiled service account is not valid for '{self._instrument}'"
)


async def validate_tiled_config(
tiled: ServiceAccount | str | None, oidc: OIDCConfig | None, opa: OpaClient | None
):
if not isinstance(tiled, ServiceAccount):
# can't validate an API key
return

if not opa or not oidc:
LOGGER.info("Missing OPA or OIDC configuration required to validate tiled auth")
return

LOGGER.info("Validating tiled configuration")
tiled.token_url = oidc.token_endpoint
auth = TiledAuth(tiled)
await opa.require_tiled_service_account(auth.get_access_token())
3 changes: 2 additions & 1 deletion src/blueapi/service/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
from blueapi.worker import TrackableTask, WorkerState
from blueapi.worker.event import TaskStatusEnum

from .authorization import OpaClient
from .authorization import OpaClient, validate_tiled_config
from .model import (
DeviceModel,
DeviceResponse,
Expand Down Expand Up @@ -98,6 +98,7 @@ async def inner(app: FastAPI):
setup_runner(config)
async with OpaClient.for_config(meta and meta.instrument, config.opa) as opa:
app.state.authz = opa
await validate_tiled_config(config.tiled.authentication, config.oidc, opa)
yield
teardown_runner()

Expand Down
93 changes: 91 additions & 2 deletions tests/unit_tests/service/test_authorization.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
from unittest.mock import AsyncMock, MagicMock, patch
from contextlib import AbstractContextManager, nullcontext
from unittest.mock import AsyncMock, MagicMock, Mock, patch

import pytest
from pydantic import HttpUrl

from blueapi.config import OpaConfig
from blueapi.config import OIDCConfig, OpaConfig, ServiceAccount
from blueapi.service.authorization import (
OpaClient,
validate_tiled_config,
)

# Reusable client patch decorator
Expand All @@ -20,9 +22,50 @@
def opa_config() -> OpaConfig:
return OpaConfig(
root=HttpUrl("http://auth.example.com"),
tiled_service_account_check="/auth/tiled",
)


@patch_client_session
@pytest.mark.parametrize(
"result,context",
[
(False, pytest.raises(ValueError, match="Tiled service account is not valid ")),
(True, nullcontext()),
],
)
async def test_tiled_service_account(
session: MagicMock,
opa_config: OpaConfig,
result: bool,
context: AbstractContextManager,
):
session.return_value.post = AsyncMock(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm writing this comment only once,But it applies to all the tests.

I personally think that pytest-aiohttp provide a better way of do this type of test. Creating so many mocks looks unnecessary/ clutters the code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how pytest-aiohttp would be used here? It appears to be targetted at testing webservers not mocking out responses.

return_value=MagicMock(json=AsyncMock(return_value={"result": result}))
)

client = OpaClient(instrument="p99", config=opa_config)

session.assert_called_once_with(base_url="http://auth.example.com/")
with context:
await client.require_tiled_service_account(token="foo_bar")
session().post.assert_called_once_with(
"/auth/tiled",
json={"input": {"token": "foo_bar", "beamline": "p99", "audience": "account"}},
)


@patch_client_session
async def test_exception_raised_when_opa_fails(
session: MagicMock, opa_config: OpaConfig
):
session.return_value.post = AsyncMock(side_effect=RuntimeError("Connection failed"))
async with OpaClient.for_config("p45", opa_config) as client:
assert client is not None
with pytest.raises(RuntimeError, match="Connection failed"):
await client.require_tiled_service_account(token="foo_bar")


@patch_client_session
async def test_session_closed(session: MagicMock, opa_config: OpaConfig):
async with OpaClient.for_config("p45", opa_config):
Expand Down Expand Up @@ -60,3 +103,49 @@ async def test_opa_adds_input_fields(session: MagicMock, opa_config: OpaConfig):
"foo/bar",
json={"input": {"beamline": "p45", "audience": "account", "foo": "bar"}},
)


async def test_validate_tiled_config():
opa = MagicMock(spec=OpaClient)
tiled = ServiceAccount()
oidc = Mock(spec=OIDCConfig)
oidc.token_endpoint = "token-endpoint"
with patch("blueapi.service.authorization.TiledAuth") as auth:
auth.return_value.get_access_token.return_value = "tiled-token"
await validate_tiled_config(tiled, oidc, opa)

auth.assert_called_once_with(tiled)
opa.require_tiled_service_account.assert_called_once_with("tiled-token")


@pytest.mark.parametrize(
"tiled_auth,oidc,opa_client",
[
(None, None, MagicMock(spec=OpaClient)),
(
None,
OIDCConfig(well_known_url="http://example.com", client_id="test-client"),
MagicMock(spec=OpaClient),
),
("api_key", None, MagicMock(spec=OpaClient)),
(
"api_key",
OIDCConfig(well_known_url="http://example.com", client_id="test-client"),
MagicMock(spec=OpaClient),
),
(ServiceAccount(), None, MagicMock(spec=OpaClient)),
(
ServiceAccount(),
OIDCConfig(well_known_url="http://example.com", client_id="test-client"),
None,
),
],
)
async def test_validate_tiled_config_with_missing_config(
tiled_auth: ServiceAccount | str | None,
oidc: OIDCConfig | None,
opa_client: MagicMock | None,
):
assert await validate_tiled_config(tiled_auth, oidc, opa_client) is None
if opa_client is not None:
opa_client.require_tiled_service_account.assert_not_called()
1 change: 1 addition & 0 deletions tests/unit_tests/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ def test_config_yaml_parsed(temp_yaml_config_file):
"opa": {
"root": "http://opa.example.com/",
"audience": "account",
"tiled_service_account_check": "v1/tiled_service_account",
},
},
{
Expand Down
Loading