api-server: remove stub authenticator (#1008)

* remove stub auth Signed-off-by: Teo Koon Peng <[email protected]> * comments Signed-off-by: Teo Koon Peng <[email protected]> * fix lint Signed-off-by: Teo Koon Peng <[email protected]> * fix lint Signed-off-by: Teo Koon Peng <[email protected]> * fix tests Signed-off-by: Teo Koon Peng <[email protected]> * add user token; add FIXME to stop using preferred_username to id users Signed-off-by: Teo Koon Peng <[email protected]> --------- Signed-off-by: Teo Koon Peng <[email protected]>
open-rmf · Sep 6, 2024 · 89c9d72 · 89c9d72
1 parent d9361c1
commit 89c9d72
Show file tree

Hide file tree

Showing 8 changed files with 84 additions and 70 deletions.
diff --git 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
@@ -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,15 +60,16 @@ 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,
             )
             user = await self._get_user(claims)
 
             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
@@ -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
@@ -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
@@ -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
@@ -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(
       <Base>

diff --git 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<AuthenticatorEventType>
   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<void> {

diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml