Fix/token api hardening

CHANGELOG.md

@@ -170,6 +170,63 @@ and the app is installable as a PWA.
   within half a second of the title replaced the title's pending write and it
   never reached the server. The title stayed on screen until the next sync
   quietly restored the old one. Each field now has its own timer.
+- **Environment variables can set a setting to `null`.** Before, `null` only
+  worked in `config.yml`: `API_TOKEN_MANAGEMENT_OIDC_LOGIN_MAX_AGE_DAYS=null` and
+  other number settings stopped the server at start, and
+  `NEW_USER_DEFAULT_LABELS=null` silently kept the default labels. Now `VAR=null`
+  means null. `EMAIL_TRANSPORT=null` still selects the transport that discards
+  mail, and an unquoted `EMAIL_TRANSPORT: null` in `config.yml` now does the same
+  instead of failing validation. The configuration reference marks nullable
+  settings (`int or null`).
+
+### Security
+
+- **Secrets no longer end up in DEBUG logs.** With `LOG_LEVEL=DEBUG` the server
+  wrote the full API key of every key-authenticated request, the OIDC token
+  response (access, refresh and id token), the OIDC refresh token on every
+  refresh, all request headers of `/api/auth/list` (session cookie and
+  `Authorization` header included), and the plain password when an admin set a
+  user's first password. These lines are gone, and a freshly generated API key is
+  kept wrapped so it no longer shows up in a repr or traceback. **If your instance
+  ever ran with `LOG_LEVEL=DEBUG`, treat the API keys, OIDC refresh tokens and
+  passwords in those logs as leaked** (see `docs/UPGRADING.md`).
+- **API keys can no longer manage API keys.** Listing, creating and deleting keys
+  under `/api/user/me/api-keys` now needs a browser session. A request with an
+  `Authorization: Bearer` header (an API key or a login token) gets `403`. Before,
+  a leaked key could mint new keys that outlived its own revocation. The web app
+  is unaffected. Scripts that managed keys with a token must use a session login
+  instead. The admin endpoints under `/api/user/{id}/api-keys` are unchanged.
+- **A login token whose source login is gone now gets `401`** instead of a `500`.
+- **API keys of OIDC users follow the user's provider.** The
+  `RESTRICT_USER_SEARCH_TO_OWN_GROUPS` rule of an OIDC provider is now decided by
+  the user, not by how the request authenticates. Before, a key created in the
+  token manager bypassed it in user search and group sharing.
+- **API keys of OIDC users pause after a long time without OIDC login.** Groups
+  and roles are only synced at OIDC login, so a user removed at the provider kept
+  their old access through a key. A key from the token manager now gets `401` when
+  its user's last OIDC login is older than the new setting
+  `API_TOKEN_MANAGEMENT_OIDC_LOGIN_MAX_AGE_DAYS` (default `30`, `null` disables).
+  The next OIDC login reactivates the key. Local users are not affected. The user
+  records the provider and time of the last OIDC login (migration `0019`).
+- **Stricter policy for API keys from the token manager.** Keys now have their
+  own settings, separate from the lifetime of login tokens
+  (`API_TOKEN_DEFAULT_EXPIRY_TIME_MINUTES` now only applies to those):
+  `API_TOKEN_MANAGEMENT_DEFAULT_EXPIRY_DAYS` (default `30`),
+  `API_TOKEN_MANAGEMENT_MAX_EXPIRY_DAYS` (default `365`, at most `3650`; a longer
+  lifetime is refused with `422`) and `API_TOKEN_MANAGEMENT_MAX_TOKENS_PER_USER`
+  (default `20` unexpired keys, `409` beyond it, `null` for no limit).
+  **`API_TOKEN_ALLOW_NEVER_EXPIRE` now defaults to `false`.** Existing keys keep
+  working, including never-expiring ones and ones above the new maximum. The token
+  manager offers no lifetime above the maximum and shows the server's reason when
+  the limit is reached. `/api/public-config` reports the new values
+  (`api_token_max_expiry_days`, `api_token_max_keys_per_user`).
+- **API key housekeeping.** `user_auth.api_token_id` gets a unique index
+  (migration `0020`), so looking up a key no longer scans the table. A key's
+  `last_used_at` is now written at most once per minute instead of on every
+  request, so the shown value can lag by up to a minute. The unused
+  `include_revoked` query parameter is gone from `GET /api/user/me/api-keys` and
+  `GET /api/user/{user_id}/api-keys` (nothing ever marked a key as revoked). Admins
+  can now list the keys of a deactivated user instead of getting `404`.
 
 ### Upgrade notes
 

CheckCheck/backend/checkcheckserver/api/auth/security.py

@@ -3,6 +3,7 @@
 
 from typing import List, Literal, Annotated, NoReturn
 import asyncio
+import datetime
 import uuid
 
 from fastapi import (
@@ -38,6 +39,7 @@
 )
 from checkcheckserver.db.user_session import UserSessionCRUD, UserSession
 from checkcheckserver.config import Config
+from checkcheckserver.model._base_model import naive_utc_now
 from checkcheckserver.log import get_logger
 from checkcheckserver.api.auth.utils import (
     oidc_refresh_access_token,
@@ -56,30 +58,58 @@
 SESSION_COOKIE_NAME = f"session_{slugify_string(config.APP_NAME,'_')}"
 
 
-def caller_restricted_to_own_groups(current_user_auth: "UserAuth") -> bool:
-    """True when the caller logged in via an OIDC provider configured with
-    ``RESTRICT_USER_SEARCH_TO_OWN_GROUPS`` — meaning they may only see / target
+def caller_restricted_to_own_groups(user: "User") -> bool:
+    """True when the caller's OIDC provider is configured with
+    ``RESTRICT_USER_SEARCH_TO_OWN_GROUPS``, meaning they may only see / target
     other users (and OIDC groups) they share at least one group with.
 
-    Local users and callers from an unrestricted provider return ``False``
-    (unrestricted). Shared by user-search (routes_user) and group-share
-    (routes_checklist_share) so both apply the exact same scoping rule."""
+    Decided by the user (the provider of their last OIDC login), not by the
+    credential of the request, so an API key of a restricted user is restricted
+    as well. Local users and users of an unrestricted provider return ``False``.
+    Shared by user-search (routes_user) and group-share (routes_checklist_share)
+    so both apply the exact same scoping rule."""
+    if user is None or not user.oidc_provider_slug:
+        return False
+    provider = next(
+        (
+            p
+            for p in config.AUTH_OIDC_PROVIDERS
+            if p.get_provider_name_slug() == user.oidc_provider_slug
+        ),
+        None,
+    )
+    return provider is not None and provider.RESTRICT_USER_SEARCH_TO_OWN_GROUPS
+
+
+def reject_paused_api_key(user_auth: "UserAuth", user: "User") -> None:
+    """Raise a 401 when `user_auth` is a key from the token manager and its OIDC
+    user has not signed in via their provider for longer than
+    ``API_TOKEN_MANAGEMENT_OIDC_LOGIN_MAX_AGE_DAYS``.
+
+    Groups and roles are only synced at OIDC login, so without this a user removed
+    at the provider would keep their old access through the key. The key is only
+    paused: the next OIDC login moves `last_oidc_login_at` and it works again.
+    Login tokens are not affected (they resolve to their source login, which is
+    checked against the provider on refresh), and neither are users without an
+    OIDC login time (local users, OIDC users not seen since the upgrade)."""
+    max_age_days = config.API_TOKEN_MANAGEMENT_OIDC_LOGIN_MAX_AGE_DAYS
     if (
-        current_user_auth is not None
-        and current_user_auth.auth_source_type == AllowedAuthSchemeType.oidc
-        and current_user_auth.oidc_provider_slug
+        max_age_days is None
+        or user_auth.auth_source_type != AllowedAuthSchemeType.api_token
+        or user.last_oidc_login_at is None
+    ):
+        return
+    if naive_utc_now() - user.last_oidc_login_at > datetime.timedelta(
+        days=max_age_days
     ):
-        provider = next(
-            (
-                p
-                for p in config.AUTH_OIDC_PROVIDERS
-                if p.get_provider_name_slug() == current_user_auth.oidc_provider_slug
+        raise HTTPException(
+            status_code=status.HTTP_401_UNAUTHORIZED,
+            detail=(
+                f"API key paused: your last sign-in via your login provider was more "
+                f"than {max_age_days} days ago. Sign in to CheckCheck in the browser "
+                f"once to reactivate it."
             ),
-            None,
         )
-        if provider is not None and provider.RESTRICT_USER_SEARCH_TO_OWN_GROUPS:
-            return True
-    return False
 
 
 NEEDS_ADMIN_API_INFO = "Needs Admin role"
@@ -134,6 +164,9 @@ async def get_current_user_auth(
             user_auth = await user_auth_crud.get(
                 user_auth_token.api_token_source_user_auth_id
             )
+            if user_auth is None:
+                # The login this token was minted from is gone (logout, cleanup).
+                raise not_authenticated_exception
 
     else:
         # if not token based auth then it must be a session
@@ -232,9 +265,33 @@ async def get_current_user(
     user = await user_crud.get(user_auth.user_id)
     if user is None:
         raise not_authenticated_exception
+    reject_paused_api_key(user_auth, user)
     return user
 
 
+async def get_current_user_by_session(
+    request: Request,
+    user_session_crud: UserSessionCRUD = Depends(UserSessionCRUD.get_crud),
+    user_auth_crud: UserAuthCRUD = Depends(UserAuthCRUD.get_crud),
+    user_crud: UserCRUD = Depends(UserCRUD.get_crud),
+    api_token: Optional[HTTPAuthorizationCredentials] = Depends(api_token_security),
+) -> User:
+    """Like `get_current_user`, but refuses API tokens. Used for API key management,
+    so a leaked key can not mint new keys that outlive its own revocation."""
+    if api_token:
+        raise HTTPException(
+            status_code=status.HTTP_403_FORBIDDEN,
+            detail="API keys can only be managed from a browser session, not with an API token.",
+        )
+    return await get_current_user(
+        request=request,
+        user_session_crud=user_session_crud,
+        user_auth_crud=user_auth_crud,
+        user_crud=user_crud,
+        api_token=None,
+    )
+
+
 async def user_is_admin(
     user: Annotated[User, Security(get_current_user)],
 ) -> bool:

CheckCheck/backend/checkcheckserver/api/auth/utils.py

@@ -100,7 +100,6 @@ async def oidc_refresh_access_token(
         oidc_server_metadata = await oauth_client.client.load_server_metadata()
         token_endpoint = oidc_server_metadata.get("token_endpoint", None)
         refresh_token = old_token.get("refresh_token")
-        log.debug(f"refresh_token: {refresh_token}")
         new_access_token = await oauth_client.client.fetch_access_token(
             token_endpoint,
             refresh_token=refresh_token,
@@ -211,7 +210,6 @@ async def wipe_expired_user_session_or_user_auth(
 
 
 def get_access_token_expires_at_value_from_token(token: dict) -> int:
-    log.debug(f"get_access_token_expires_at_value_from_token token {token}")
     if "userinfo" in token and "exp" in token["userinfo"]:
         return token["userinfo"]["exp"]
     if "expires_at" in token:
@@ -252,7 +250,6 @@ async def get_userinfo_from_token_or_endpoint(
             raw_userinfo = resp.json()
     if raw_userinfo is None:
         raise ValueError("Could not extract userinfo.")
-    log.debug(f"raw_userinfo {raw_userinfo}")
 
     return UserInfoOidc.from_raw_userinfo(raw_userinfo, oidc_config)
 

CheckCheck/backend/checkcheckserver/api/routes/routes_auth.py

@@ -37,7 +37,12 @@
 #
 
 from checkcheckserver.db.user import UserCRUD, UserCreate
-from checkcheckserver.model.user import UserCreate, UserRegisterAPI
+from checkcheckserver.model.user import (
+    UserCreate,
+    UserRegisterAPI,
+    UserUpdateOidcLogin,
+)
+from checkcheckserver.model._base_model import naive_utc_now
 from checkcheckserver.db.user_auth import UserAuthCRUD
 from checkcheckserver.db.user_session import UserSessionCRUD
 from checkcheckserver.model.user_auth import (
@@ -86,7 +91,6 @@ class APIToken(BaseModel):
 
 @fast_api_auth_base_router.get("/auth/list", response_model=List[AuthSchemeInfo])
 async def list_available_login_schemes(request: Request):
-    log.debug(f"request.headers: {request.headers}")
     schemes: List[AuthSchemeInfo] = []
     if config.AUTH_BASIC_LOGIN_IS_ENABLED:
         schemes.append(
@@ -373,7 +377,6 @@ async def auth_oidc_callback(
             status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
             detail=f"Auth error can not fetch access token from {oauth_client.access_token_url}. Error: {e}",
         )
-    log.debug(f"Token {token}")
     userinfo = await get_userinfo_from_token_or_endpoint(
         token, oauth_client, oauth_config
     )
@@ -389,22 +392,31 @@ async def auth_oidc_callback(
                 detail=f"Can not validate user data: {e}",
             )
         user: User = await user_crud.create(user_create)
+        user = await user_crud.update(
+            user_update=UserUpdateOidcLogin(
+                oidc_provider_slug=oauth_config.get_provider_name_slug(),
+                last_oidc_login_at=naive_utc_now(),
+            ),
+            user_id=user.id,
+        )
         # A brand-new account: any group they arrived with is "new", so reconcile
         # so cards already shared with those groups appear on their first login.
         groups_changed = bool(user.oidc_groups)
     elif user is None:
         raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED)
     else:
-        # Sync display_name, email and roles from the OIDC provider on every login
-        from checkcheckserver.db.user import UserUpdateByAdmin
-
+        # Sync display_name, email and roles from the OIDC provider on every login.
+        # Provider and login time let API keys of this user follow the provider
+        # (group restriction, paused keys after a long time without OIDC login).
         old_groups = set(user.oidc_groups or [])
         synced = UserCreate.from_oidc_userinfo(userinfo)
-        update = UserUpdateByAdmin(
+        update = UserUpdateOidcLogin(
             display_name=synced.display_name,
             email=synced.email,
             roles=synced.roles,
             oidc_groups=synced.oidc_groups,
+            oidc_provider_slug=oauth_config.get_provider_name_slug(),
+            last_oidc_login_at=naive_utc_now(),
         )
         user = await user_crud.update(user_update=update, user_id=user.id)
         # Living group shares: only reconcile when the OIDC group set actually
@@ -507,7 +519,6 @@ async def logout(
             if user_session:
                 await user_session_crud.delete(user_session.id)
         response.delete_cookie(key=SESSION_COOKIE_NAME)
-        log.debug(f"current_user_auth: {current_user_auth}")
         if current_user_auth.auth_source_type == AllowedAuthSchemeType.oidc:
             refresh_token = current_user_auth.get_decrypted_oidc_token().get(
                 "refresh_token"

CheckCheck/backend/checkcheckserver/api/routes/routes_checklist_share.py

@@ -29,10 +29,8 @@
 )
 
 from checkcheckserver.db.user import User, UserCRUD
-from checkcheckserver.db.user_auth import UserAuth
 from checkcheckserver.api.auth.security import (
     get_current_user,
-    get_current_user_auth,
     caller_restricted_to_own_groups,
 )
 from checkcheckserver.api.access import (
@@ -461,12 +459,11 @@ async def list_my_groups(
 def _require_group_in_scope(
     group: str,
     checklist_access: UserChecklistAccess,
-    current_user_auth: UserAuth,
 ) -> None:
     """Group scoping: a caller from an OIDC provider configured to restrict search
     to own groups may only target a group they themselves belong to (the same rule
     user-search applies). Local / unrestricted callers may target any group."""
-    if caller_restricted_to_own_groups(current_user_auth):
+    if caller_restricted_to_own_groups(checklist_access.user):
         if group not in (checklist_access.user.oidc_groups or []):
             raise HTTPException(
                 status_code=status.HTTP_403_FORBIDDEN,
@@ -517,7 +514,6 @@ async def share_with_group(
     checklist_access: UserChecklistAccess = Security(
         require_checklist_permission(ChecklistAccessLevel.owner)
     ),
-    current_user_auth: UserAuth = Depends(get_current_user_auth),
     user_crud: UserCRUD = Depends(UserCRUD.get_crud),
     checklist_group_share_crud: CheckListGroupShareCRUD = Depends(
         CheckListGroupShareCRUD.get_crud
@@ -531,7 +527,7 @@ async def share_with_group(
     sync_crud: SyncNotifiationCRUD = Depends(SyncNotifiationCRUD.get_crud),
     notification_crud: NotificationCRUD = Depends(NotificationCRUD.get_crud),
 ) -> GroupShareResult:
-    _require_group_in_scope(group, checklist_access, current_user_auth)
+    _require_group_in_scope(group, checklist_access)
 
     # Persist the group→level intent (source of truth), then materialize access for
     # its current members. The reconciler reads the card's group shares back, so the
@@ -577,7 +573,6 @@ async def revoke_group_share(
     checklist_access: UserChecklistAccess = Security(
         require_checklist_permission(ChecklistAccessLevel.owner)
     ),
-    current_user_auth: UserAuth = Depends(get_current_user_auth),
     user_crud: UserCRUD = Depends(UserCRUD.get_crud),
     checklist_group_share_crud: CheckListGroupShareCRUD = Depends(
         CheckListGroupShareCRUD.get_crud
@@ -591,7 +586,7 @@ async def revoke_group_share(
     sync_crud: SyncNotifiationCRUD = Depends(SyncNotifiationCRUD.get_crud),
     notification_crud: NotificationCRUD = Depends(NotificationCRUD.get_crud),
 ):
-    _require_group_in_scope(group, checklist_access, current_user_auth)
+    _require_group_in_scope(group, checklist_access)
     # Delete the source row first, then reconcile: the reconciler recomputes each
     # former member's level from the *remaining* group shares, so a member still in
     # another shared group keeps (or drops to) that group's level, while one who