From 89c9d7222811da5cd8711f6f8f02c050e29f72e1 Mon Sep 17 00:00:00 2001 From: Teo Koon Peng Date: Fri, 6 Sep 2024 15:15:18 +0800 Subject: [PATCH] api-server: remove stub authenticator (#1008) * remove stub auth Signed-off-by: Teo Koon Peng * comments Signed-off-by: Teo Koon Peng * fix lint Signed-off-by: Teo Koon Peng * fix lint Signed-off-by: Teo Koon Peng * fix tests Signed-off-by: Teo Koon Peng * add user token; add FIXME to stop using preferred_username to id users Signed-off-by: Teo Koon Peng --------- Signed-off-by: Teo Koon Peng --- packages/api-server/api_server/app_config.py | 3 +- .../api-server/api_server/authenticator.py | 78 +++++++------------ .../api-server/api_server/default_config.py | 5 +- packages/api-server/api_server/models/user.py | 3 + packages/api-server/scripts/test_config.py | 1 + .../dashboard/src/components/appbar.test.tsx | 2 +- .../src/services/stub-authenticator.ts | 33 +++++++- pnpm-lock.yaml | 29 +++---- 8 files changed, 84 insertions(+), 70 deletions(-) diff --git a/packages/api-server/api_server/app_config.py b/packages/api-server/api_server/app_config.py index 6a6618465..40841997f 100644 --- a/packages/api-server/api_server/app_config.py +++ b/packages/api-server/api_server/app_config.py @@ -17,9 +17,10 @@ class AppConfig: log_level: str builtin_admin: str jwt_public_key: str | None + jwt_secret: str | None oidc_url: str | None aud: str - iss: str | None + iss: str ros_args: list[str] timezone: str diff --git a/packages/api-server/api_server/authenticator.py b/packages/api-server/api_server/authenticator.py index 536beaf6e..16fff7802 100644 --- a/packages/api-server/api_server/authenticator.py +++ b/packages/api-server/api_server/authenticator.py @@ -1,10 +1,8 @@ -import base64 -import json -import logging from typing import Any, Callable, Coroutine, Protocol import jwt -from fastapi import Depends, Header, HTTPException +import jwt.algorithms +from fastapi import Depends, HTTPException from fastapi.security import OpenIdConnect from .app_config import app_config @@ -22,9 +20,12 @@ def fastapi_dep(self) -> Callable[..., Coroutine[Any, Any, User] | User]: ... class JwtAuthenticator: + _algorithms = jwt.algorithms.get_default_algorithms() + del _algorithms["none"] + def __init__( self, - pem_file: str, + key_or_secret: "jwt.algorithms.AllowedPublicKeys | str | bytes", aud: str, iss: str, *, @@ -38,8 +39,7 @@ def __init__( self.aud = aud self.iss = iss self.oidc_url = oidc_url - with open(pem_file, "r", encoding="utf8") as f: - self._public_key = f.read() + self._key_or_secret = key_or_secret async def _get_user(self, claims: dict) -> User: if not "preferred_username" in claims: @@ -48,18 +48,10 @@ async def _get_user(self, claims: dict) -> User: ) username = claims["preferred_username"] + # FIXME(koonpeng): We should use the "userId" as the identifier. Some idP may allow + # duplicated usernames. user = await User.load_or_create_from_db(username) - is_admin = False - if "realm_access" in claims: - if "roles" in claims["realm_access"]: - roles = claims["realm_access"]["roles"] - if "superuser" in roles: - is_admin = True - - if user.is_admin != is_admin: - await user.update_admin(is_admin) - return user async def verify_token(self, token: str | None) -> User: @@ -68,8 +60,8 @@ async def verify_token(self, token: str | None) -> User: try: claims = jwt.decode( token, - self._public_key, - algorithms=["RS256"], + self._key_or_secret, + algorithms=list(self._algorithms), audience=self.aud, issuer=self.iss, ) @@ -77,6 +69,7 @@ async def verify_token(self, token: str | None) -> User: return user except jwt.InvalidTokenError as e: + print(e) raise AuthenticationError(str(e)) from e def fastapi_dep(self) -> Callable[..., Coroutine[Any, Any, User] | User]: @@ -94,45 +87,30 @@ async def dep( return dep -class StubAuthenticator(Authenticator): - """ - StubAuthenticator will authenticate as an admin user called "stub" if no tokens are - present. If there is a bearer token in the `Authorization` header, then it decodes the jwt - WITHOUT verifying the signature and authenticated as the user given. - """ - - async def verify_token(self, token: str | None): - if not token: - return User(username="stub", is_admin=True) - # decode the jwt without verifying signature - parts = token.split(".") - # add padding to ignore incorrect padding errors - payload = base64.b64decode(parts[1] + "==") - username = json.loads(payload)["preferred_username"] - return await User.load_or_create_from_db(username) - - def fastapi_dep(self): - async def dep(authorization: str | None = Header(None)): - if not authorization: - return await self.verify_token(None) - token = authorization.split(" ")[1] - return await self.verify_token(token) - - return dep - +if app_config.jwt_public_key and app_config.jwt_secret: + raise ValueError("only one of jwt_public_key or jwt_secret must be set") +if not app_config.iss: + raise ValueError("iss is required") +if not app_config.aud: + raise ValueError("aud is required") if app_config.jwt_public_key: - if app_config.iss is None: - raise ValueError("iss is required") + with open(app_config.jwt_public_key, "br") as f: + authenticator = JwtAuthenticator( + f.read(), + app_config.aud, + app_config.iss, + oidc_url=app_config.oidc_url or "", + ) +elif app_config.jwt_secret: authenticator = JwtAuthenticator( - app_config.jwt_public_key, + app_config.jwt_secret, app_config.aud, app_config.iss, oidc_url=app_config.oidc_url or "", ) else: - authenticator = StubAuthenticator() - logging.warning("authentication is disabled") + raise ValueError("either jwt_public_key or jwt_secret is required") user_dep = authenticator.fastapi_dep() diff --git a/packages/api-server/api_server/default_config.py b/packages/api-server/api_server/default_config.py index 76780d80d..f7ea6e466 100644 --- a/packages/api-server/api_server/default_config.py +++ b/packages/api-server/api_server/default_config.py @@ -16,6 +16,8 @@ "builtin_admin": "admin", # path to a PEM encoded RSA public key which is used to verify JWT tokens, if the path is relative, it is based on the working dir. "jwt_public_key": None, + # jwt secret, this is mutually exclusive with `jwt_public_key`. + "jwt_secret": "rmfisawesome", # url to the oidc endpoint, used to authenticate rest requests, it should point to the well known endpoint, e.g. # http://localhost:8080/auth/realms/rmf-web/.well-known/openid-configuration. # NOTE: This is ONLY used for documentation purposes, the "jwt_public_key" will be the @@ -26,8 +28,7 @@ "aud": "rmf_api_server", # url or string that identifies the entity that issued the jwt token # Used to verify the "iss" claim - # If iss is set to None, it means that authentication should be disabled - "iss": None, + "iss": "stub", # list of arguments passed to the ros node, "--ros-args" is automatically prepended to the list. # e.g. # Run with sim time: ["-p", "use_sim_time:=true"] diff --git a/packages/api-server/api_server/models/user.py b/packages/api-server/api_server/models/user.py index 59d7d17b2..b5c630fa8 100644 --- a/packages/api-server/api_server/models/user.py +++ b/packages/api-server/api_server/models/user.py @@ -5,6 +5,9 @@ class User(PydanticModel): + # FIXME(koonpeng): We should use the "userId" as the identifier. Some idP may allow + # duplicated usernames. + # userId: str username: str is_admin: bool = False roles: list[str] = [] diff --git a/packages/api-server/scripts/test_config.py b/packages/api-server/scripts/test_config.py index ada8fd2c3..ece999981 100644 --- a/packages/api-server/scripts/test_config.py +++ b/packages/api-server/scripts/test_config.py @@ -11,6 +11,7 @@ "port": int(test_port), "log_level": "ERROR", "jwt_public_key": f"{here}/test.pub", + "jwt_secret": None, "iss": "test", "db_url": os.environ.get("RMF_API_SERVER_TEST_DB_URL", "sqlite://:memory:"), "timezone": "Asia/Singapore", diff --git a/packages/dashboard/src/components/appbar.test.tsx b/packages/dashboard/src/components/appbar.test.tsx index ab5212a91..c18015df5 100644 --- a/packages/dashboard/src/components/appbar.test.tsx +++ b/packages/dashboard/src/components/appbar.test.tsx @@ -59,7 +59,7 @@ describe('AppBar', () => { }); it('logout is triggered when logout button is clicked', async () => { - const authenticator = new StubAuthenticator('test'); + const authenticator = new StubAuthenticator(); const spy = vi.spyOn(authenticator, 'logout').mockImplementation(() => undefined as any); const root = render( diff --git a/packages/dashboard/src/services/stub-authenticator.ts b/packages/dashboard/src/services/stub-authenticator.ts index 30e6898db..f787dedc2 100644 --- a/packages/dashboard/src/services/stub-authenticator.ts +++ b/packages/dashboard/src/services/stub-authenticator.ts @@ -2,6 +2,33 @@ import EventEmitter from 'eventemitter3'; import { Authenticator, AuthenticatorEventType } from './authenticator'; +/** + * Hardcoded token using the secret 'rmfisawesome', expires in 2035-01-01. + * To update the token, use https://jwt.io and paste in the payload, also remember + * to set the secret to `rmfisawesome`. + * + * header: + * { + * "alg": "HS256", + * "typ": "JWT" + * } + * payload: + * { + * "sub": "stub", + * "preferred_username": "admin", + * "iat": 1516239022, + * "aud": "rmf_api_server", + * "iss": "stub", + * "exp": 2051222400 + * } + */ +const ADMIN_TOKEN = + 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzdHViIiwicHJlZmVycmVkX3VzZXJuYW1lIjoiYWRtaW4iLCJpYXQiOjE1MTYyMzkwMjIsImF1ZCI6InJtZl9hcGlfc2VydmVyIiwiaXNzIjoic3R1YiIsImV4cCI6MjA1MTIyMjQwMH0.zzX3zXp467ldkzmLVIadQ_AHr8M5uWVV43n4wEB0OhE'; + +// same as the admin token, except the `preferred_username` is "user". +const USER_TOKEN = + 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzdHViIiwicHJlZmVycmVkX3VzZXJuYW1lIjoidXNlciIsImlhdCI6MTUxNjIzOTAyMiwiYXVkIjoicm1mX2FwaV9zZXJ2ZXIiLCJpc3MiOiJzdHViIiwiZXhwIjoyMDUxMjIyNDAwfQ.vK3n4FbshCykQ9BW49w_7AfqKgbN9j2R3-Qh-rIOt_g'; + export class StubAuthenticator extends EventEmitter implements Authenticator @@ -10,10 +37,10 @@ export class StubAuthenticator readonly token?: string; - constructor(user = 'stub', token: string | undefined = undefined) { + constructor(isAdmin = true) { super(); - this.user = user; - this.token = token; + this.user = isAdmin ? 'admin' : 'user'; + this.token = isAdmin ? ADMIN_TOKEN : USER_TOKEN; } init(): Promise { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9568a5a43..d550aa5b4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -230,7 +230,7 @@ importers: version: 2.0.4(vitest@2.0.4(@types/node@20.14.12)(jsdom@24.1.1(canvas@2.11.2))(terser@5.31.6)) api-server: specifier: file:../api-server - version: link:../api-server + version: file:packages/api-server concurrently: specifier: ^8.2.2 version: 8.2.2 @@ -3226,6 +3226,9 @@ packages: resolution: {integrity: sha512-KMReFUr0B4t+D+OBkjR3KYqvocp2XaSzO55UcB6mgQMd3KbcE+mWTyvVV7D/zsdEbNnV6acZUutkiHQXvTr1Rw==} engines: {node: '>= 8'} + api-server@file:packages/api-server: + resolution: {directory: packages/api-server, type: directory} + aproba@2.0.0: resolution: {integrity: sha512-lYe4Gx7QT+MKGbDsA+Z+he/Wtef0BiwDOlK/XkBrdfsh9J/jPPXbX0tE9x9cl27Tmu5gg3QUbUrQYa/y+KOHPQ==} @@ -8218,7 +8221,6 @@ snapshots: '@babel/parser@7.25.3': dependencies: '@babel/types': 7.25.2 - optional: true '@babel/plugin-bugfix-firefox-class-in-computed-class-key@7.24.7(@babel/core@7.24.8)': dependencies: @@ -8987,7 +8989,6 @@ snapshots: '@babel/helper-string-parser': 7.24.8 '@babel/helper-validator-identifier': 7.24.7 to-fast-properties: 2.0.0 - optional: true '@base2/pretty-print-object@1.0.1': {} @@ -9461,7 +9462,7 @@ snapshots: '@jest/schemas': 28.1.3 '@types/istanbul-lib-coverage': 2.0.6 '@types/istanbul-reports': 3.0.4 - '@types/node': 20.14.10 + '@types/node': 22.2.0 '@types/yargs': 17.0.32 chalk: 4.1.2 @@ -10741,24 +10742,24 @@ snapshots: '@types/babel__core@7.20.5': dependencies: - '@babel/parser': 7.24.8 - '@babel/types': 7.24.9 + '@babel/parser': 7.25.3 + '@babel/types': 7.25.2 '@types/babel__generator': 7.6.8 '@types/babel__template': 7.4.4 '@types/babel__traverse': 7.20.6 '@types/babel__generator@7.6.8': dependencies: - '@babel/types': 7.24.9 + '@babel/types': 7.25.2 '@types/babel__template@7.4.4': dependencies: - '@babel/parser': 7.24.8 - '@babel/types': 7.24.9 + '@babel/parser': 7.25.3 + '@babel/types': 7.25.2 '@types/babel__traverse@7.20.6': dependencies: - '@babel/types': 7.24.9 + '@babel/types': 7.25.2 '@types/body-parser@1.19.5': dependencies: @@ -10774,7 +10775,7 @@ snapshots: '@types/connect@3.4.38': dependencies: - '@types/node': 20.14.10 + '@types/node': 22.2.0 '@types/crc@3.8.3': dependencies: @@ -10995,7 +10996,7 @@ snapshots: '@types/send@0.17.4': dependencies: '@types/mime': 1.3.5 - '@types/node': 20.14.10 + '@types/node': 22.2.0 '@types/serve-static@1.15.7': dependencies: @@ -11618,6 +11619,8 @@ snapshots: normalize-path: 3.0.0 picomatch: 2.3.1 + api-server@file:packages/api-server: {} + aproba@2.0.0: {} arch@2.2.0: {} @@ -12183,7 +12186,7 @@ snapshots: chrome-launcher@0.14.2: dependencies: - '@types/node': 15.14.9 + '@types/node': 22.2.0 escape-string-regexp: 4.0.0 is-wsl: 2.2.0 lighthouse-logger: 1.4.2