From d60e4bc099ee01fe0dfab36e5713104fe2f29bde Mon Sep 17 00:00:00 2001 From: themylogin Date: Fri, 19 Jul 2024 17:03:51 +0200 Subject: [PATCH] NAS-130069 / 24.10 / Fix API key API (#14030) * Fix API key API * Address review --- .../middlewared/api/v25_04_0/api_key.py | 18 +++---- .../middlewared/plugins/api_key.py | 5 ++ tests/api2/test_api_key.py | 14 +++--- tests/api2/test_api_key_crud.py | 49 +++++++++++++++++++ 4 files changed, 69 insertions(+), 17 deletions(-) create mode 100644 tests/api2/test_api_key_crud.py diff --git a/src/middlewared/middlewared/api/v25_04_0/api_key.py b/src/middlewared/middlewared/api/v25_04_0/api_key.py index a0309f0a07d6c..c319065334dad 100644 --- a/src/middlewared/middlewared/api/v25_04_0/api_key.py +++ b/src/middlewared/middlewared/api/v25_04_0/api_key.py @@ -2,9 +2,9 @@ from typing import Literal, TypeAlias from typing_extensions import Annotated -from pydantic import ConfigDict, StringConstraints +from pydantic import StringConstraints -from middlewared.api.base import BaseModel, Excluded, excluded_field, NonEmptyString, Private +from middlewared.api.base import BaseModel, Excluded, excluded_field, ForUpdateMetaclass, NonEmptyString, Private HttpVerb: TypeAlias = Literal["GET", "POST", "PUT", "DELETE", "CALL", "SUBSCRIBE", "*"] @@ -16,18 +16,18 @@ class AllowListItem(BaseModel): class ApiKeyEntry(BaseModel): - """Represents a record in the account.api_key table.""" - id: int name: Annotated[NonEmptyString, StringConstraints(max_length=200)] - key: Private[str] created_at: datetime allowlist: list[AllowListItem] +class ApiKeyEntryWithKey(ApiKeyEntry): + key: Private[str] + + class ApiKeyCreate(ApiKeyEntry): id: Excluded = excluded_field() - key: Excluded = excluded_field() created_at: Excluded = excluded_field() @@ -36,10 +36,10 @@ class ApiKeyCreateArgs(BaseModel): class ApiKeyCreateResult(BaseModel): - result: ApiKeyEntry + result: ApiKeyEntryWithKey -class ApiKeyUpdate(ApiKeyCreate): +class ApiKeyUpdate(ApiKeyCreate, metaclass=ForUpdateMetaclass): reset: bool @@ -49,7 +49,7 @@ class ApiKeyUpdateArgs(BaseModel): class ApiKeyUpdateResult(BaseModel): - result: ApiKeyEntry + result: ApiKeyEntryWithKey class ApiKeyDeleteArgs(BaseModel): diff --git a/src/middlewared/middlewared/plugins/api_key.py b/src/middlewared/middlewared/plugins/api_key.py index 869b727837fb0..99f5729865f04 100644 --- a/src/middlewared/middlewared/plugins/api_key.py +++ b/src/middlewared/middlewared/plugins/api_key.py @@ -47,6 +47,11 @@ class Config: datastore_extend = "api_key.item_extend" cli_namespace = "auth.api_key" + @private + async def item_extend(self, item): + item.pop("key") + return item + @api_method(ApiKeyCreateArgs, ApiKeyCreateResult) async def do_create(self, data: dict) -> dict: """ diff --git a/tests/api2/test_api_key.py b/tests/api2/test_api_key.py index 4ed0b32e8819d..7fce2b058a05e 100644 --- a/tests/api2/test_api_key.py +++ b/tests/api2/test_api_key.py @@ -1,8 +1,8 @@ -#!/usr/bin/env python3 - import contextlib import os + import pytest + import sys sys.path.append(os.getcwd()) from functions import POST, GET, DELETE, SSH_TEST @@ -31,7 +31,7 @@ def user(): assert results.status_code == 200, results.text -def test_root_api_key_websocket(request): +def test_root_api_key_websocket(): """We should be able to call a method with root API key using Websocket.""" ip = truenas_server.ip with api_key([{"method": "*", "resource": "*"}]) as key: @@ -53,7 +53,7 @@ def test_root_api_key_websocket(request): c.call("service.update", "cifs", {"enable": False}) -def test_allowed_api_key_websocket(request): +def test_allowed_api_key_websocket(): """We should be able to call a method with API key that allows that call using Websocket.""" ip = truenas_server.ip with api_key([{"method": "CALL", "resource": "system.info"}]) as key: @@ -64,7 +64,7 @@ def test_allowed_api_key_websocket(request): assert 'uptime' in str(results['stdout']) -def test_denied_api_key_websocket(request): +def test_denied_api_key_websocket(): """We should not be able to call a method with API key that does not allow that call using Websocket.""" ip = truenas_server.ip with api_key([{"method": "CALL", "resource": "system.info_"}]) as key: @@ -74,10 +74,8 @@ def test_denied_api_key_websocket(request): assert results['result'] is False -def test_denied_api_key_noauthz(request): +def test_denied_api_key_noauthz(): with api_key([{"method": "CALL", "resource": "system.info"}]) as key: - auth_token = None - with client(auth=None) as c: assert c.call("auth.login_with_api_key", key) diff --git a/tests/api2/test_api_key_crud.py b/tests/api2/test_api_key_crud.py new file mode 100644 index 0000000000000..a79a41524b981 --- /dev/null +++ b/tests/api2/test_api_key_crud.py @@ -0,0 +1,49 @@ +import contextlib + +from middlewared.test.integration.utils import call, client + + +@contextlib.contextmanager +def api_key(allowlist): + key = call("api_key.create", {"name": "Test API Key", "allowlist": allowlist}) + try: + yield key + finally: + call("api_key.delete", key["id"]) + + +def test_has_key_after_creation_but_not_read(): + key = call("api_key.create", {"name": "Test", "allowlist": []}) + try: + assert "key" in key + + instance = call("api_key.get_instance", key["id"]) + assert "key" not in instance + + update = call("api_key.update", key["id"], {}) + assert "key" not in update + finally: + call("api_key.delete", key["id"]) + + +def test_api_key_reset(): + with api_key([]) as key: + with client(auth=None) as c: + assert c.call("auth.login_with_api_key", key["key"]) + + updated = call("api_key.update", key["id"], {"reset": True}) + + with client(auth=None) as c: + assert not c.call("auth.login_with_api_key", key["key"]) + + with client(auth=None) as c: + assert c.call("auth.login_with_api_key", updated["key"]) + + +def test_api_key_delete(): + with api_key([]) as key: + with client(auth=None) as c: + assert c.call("auth.login_with_api_key", key["key"]) + + with client(auth=None) as c: + assert not c.call("auth.login_with_api_key", key["key"])