Skip to content

Add 3-mode auth support to User model + ports - #334

Merged
andyne13 merged 2 commits into
refactor/hexagonalfrom
refactor/auth-models-update
Apr 22, 2026
Merged

Add 3-mode auth support to User model + ports#334
andyne13 merged 2 commits into
refactor/hexagonalfrom
refactor/auth-models-update

Conversation

@andyne13

@andyne13 andyne13 commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Extends User with password_hash, adds ApiKey, TokenPayload, OIDCSessionRepository.

Summary by CodeRabbit

  • New Features
    • Added API key support for programmatic access to the platform.
    • Enhanced authentication with JWT-based password support.
    • Implemented OIDC session management for secure session handling.
    • Extended user account capabilities with status tracking and timestamp management.

User now supports three auth modes:
- OIDC/SSO (external_user_id)
- API token (legacy or- tokens)
- Password + JWT (email + password_hash)

Added is_active, updated_at fields.
ApiKey model for programmatic access (prefix + hash + expiry).
TokenPayload for JWT claims.
…pository

UserRepository: added get_user_by_email(), count_users(), API key CRUD
(create_api_key, get_api_keys_by_prefix, list_api_keys_for_user,
delete_api_key), count_partition_users().

OIDCSessionRepository: new port for OIDC session persistence (create,
get_by_token_hash, get_by_sid, revoke, delete_expired).

CatalogStore: added oidc_session_repo property.
@coderabbitai

coderabbitai Bot commented Apr 22, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

The PR extends domain models and repository interfaces to support API key-based and OIDC session-based authentication. It introduces new ApiKey and TokenPayload models, expands the User model with authentication-related fields, and defines a new OIDCSessionRepository interface alongside API key management methods in the UserRepository interface.

Changes

Cohort / File(s) Summary
Model Exports
openrag/core/models/__init__.py
Added re-exports for ApiKey and TokenPayload domain models to the module's public API.
Domain Models
openrag/core/models/user.py
Extended User model with password_hash, is_active, and updated_at fields. Added new ApiKey model for programmatic access with UUID id, user linkage, prefix metadata, activation/expiry timestamps, and hashed representation. Added TokenPayload model to represent JWT claim fields (sub, type, role, exp).
Repository Interfaces
openrag/core/ports/__init__.py, openrag/core/ports/user_repo.py
Expanded UserRepository interface with email-based lookup, user counting, and API key management methods (create_api_key, get_api_keys_by_prefix, list_api_keys_for_user, delete_api_key). Added export of OIDCSessionRepository to public API.
OIDC Session Repository
openrag/core/ports/oidc_session_repo.py
New repository interface defining 8 abstract async methods for OIDC session lifecycle management: session creation, retrieval by token hash or session ID, updates, single and bulk revocation, and expiration cleanup.
Aggregate Repository
openrag/core/ports/catalog_store.py
Added abstract property oidc_session_repo to CatalogStore aggregate to expose the new OIDCSessionRepository port.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • Phase 2: domain models #330: Introduces initial OIDC domain models (OIDCSession) that this PR extends with session repository interface and related authentication infrastructure.

Suggested labels

feat

Poem

🐰 Hops with glee through auth refrain,
API keys now join the chain,
OIDC sessions fresh and new,
Password hashes, tokens too!
Whiskers twitch with code delight,
Security done just right! 🔐

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 13.64% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Add 3-mode auth support to User model + ports' clearly and specifically summarizes the main changes: extending the User model with three authentication modes and updating related ports.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/auth-models-update

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot added the feat Add a new feature label Apr 22, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@openrag/core/models/user.py`:
- Around line 35-41: The Pydantic User fields password_hash, is_active, and
updated_at are not persisted because the SQLAlchemy ORM User model lacks
matching columns; add SQLAlchemy Column definitions for password_hash (String,
nullable), is_active (Boolean, default True), and updated_at (DateTime, default
now, onupdate now) to the ORM User class, update the ORM-to-Pydantic mapping to
include these attributes (respecting password_hash exclusion/representation),
and create an Alembic migration that adds these columns with safe
defaults/nullable handling so existing rows upgrade cleanly; ensure updated_at
is updated automatically on update and that any repository code saving users
correctly sets/reads these fields.
- Around line 61-68: Update the key_hash Field so it cannot be serialized or
printed and must be non-empty: replace the current declaration of key_hash with
a required Field(...) that sets exclude=True and repr=False (e.g., key_hash: str
= Field(..., exclude=True, repr=False, min_length=1)) so model_dump() and
repr/logging won't expose it and empty/default "" is no longer allowed; keep the
same type name key_hash and update any constructors or tests that relied on a
default value.

In `@openrag/core/ports/oidc_session_repo.py`:
- Line 23: The update_session signature currently accepts arbitrary **fields
which permits updating immutable/sensitive session data; change
update_session(session_id: int, *, expires_at: Optional[datetime]=None,
is_active: Optional[bool]=None, last_used_at: Optional[datetime]=None) ->
OIDCSession | None (or accept a typed DTO UpdateOIDCSession) so only explicit
mutable fields can be changed, and ensure the implementation in OIDCSessionRepo
(and any callers) uses that contract; additionally, before persisting any token
fields (id_token, access_token, refresh_token) in methods that create or update
OIDCSession records, encrypt them with Fernet (use a centralized encrypt/decrypt
helper) so the oidc_sessions table never stores plaintext tokens.
- Line 17: Add a docstring to get_by_token_hash(self, token_hash: str) stating
it only returns active sessions (revoked_at IS NULL and session_expires_at >=
now()) and returns None for revoked or expired sessions so implementors preserve
the same filtering; and change the update_session(self, session_id: int,
**fields) signature to remove the arbitrary **fields — either replace it with an
explicit allow-list of safe updatable fields (e.g., "last_seen",
"user_metadata", etc.) or split into dedicated methods and rely on the existing
update_oidc_session_tokens(...) for token updates; update the interface method
name/signature accordingly and ensure callers are adapted to use the new
explicit fields or dedicated token rotation method.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ac65c4ab-5bee-458b-ac12-17bec3dceaca

📥 Commits

Reviewing files that changed from the base of the PR and between 335979d and ace8759.

📒 Files selected for processing (6)
  • openrag/core/models/__init__.py
  • openrag/core/models/user.py
  • openrag/core/ports/__init__.py
  • openrag/core/ports/catalog_store.py
  • openrag/core/ports/oidc_session_repo.py
  • openrag/core/ports/user_repo.py

Comment on lines +35 to +41
password_hash: str | None = Field(None, exclude=True, repr=False)
is_admin: bool = False
is_active: bool = True
file_quota: int | None = None
file_count: int = 0
created_at: datetime = Field(default_factory=lambda: datetime.now(UTC))
updated_at: datetime = Field(default_factory=lambda: datetime.now(UTC))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify that the backing users table, migrations, and mappings include the new auth fields.
# Expected: password_hash, is_active, and updated_at appear in the SQLAlchemy User model
# and/or migration files that alter the users table.
rg -nP -C4 '\b(password_hash|is_active|updated_at)\b|class\s+User\b|__tablename__\s*=\s*["'\''"]users["'\''"]' \
  --glob '*.py' \
  --glob '!**/.venv/**' \
  --glob '!**/node_modules/**'

Repository: linagora/openrag

Length of output: 4596


🏁 Script executed:

cat -n openrag/components/indexer/vectordb/models.py | sed -n '100,130p'

Repository: linagora/openrag

Length of output: 1714


🏁 Script executed:

# Search for migration files and users table definitions
fd -e py -path '*migrations*' -o -path '*alembic*' | head -20

Repository: linagora/openrag

Length of output: 228


🏁 Script executed:

# Look for any references to password_hash, is_active, updated_at in SQLAlchemy/migration context
rg -n 'password_hash|is_active.*Column|updated_at.*Column' openrag/components/indexer/vectordb/models.py

Repository: linagora/openrag

Length of output: 42


🏁 Script executed:

find . -type f -name "*.py" | grep -E '(migrations|alembic|versions)' | head -20

Repository: linagora/openrag

Length of output: 1069


🏁 Script executed:

# Search for any migration or schema definition files that mention users table
rg -l 'users' . --glob '*.py' | grep -iE '(migration|alembic|schema)' | head -20

Repository: linagora/openrag

Length of output: 398


🏁 Script executed:

cat -n openrag/scripts/migrations/alembic/versions/f5b6c918f741_add_oidc_auth.py

Repository: linagora/openrag

Length of output: 4238


🏁 Script executed:

cat -n openrag/scripts/migrations/alembic/versions/cd642e4502d8_create_users_memberships_tables.py

Repository: linagora/openrag

Length of output: 5525


🏁 Script executed:

cat -n openrag/scripts/migrations/alembic/versions/c224d4befe71_add_file_count_and_file_quota.py

Repository: linagora/openrag

Length of output: 2432


🏁 Script executed:

# Search all migration files for password_hash, is_active, updated_at
rg 'password_hash|is_active.*Column|updated_at.*Column' openrag/scripts/migrations/alembic/versions/

Repository: linagora/openrag

Length of output: 42


Persist the new auth fields in the backing user table.

Lines 35-41 add password_hash, is_active, and updated_at to the Pydantic User model, but the SQLAlchemy User table in openrag/components/indexer/vectordb/models.py:100-114 does not define corresponding columns. No existing migrations add these columns either. Password/JWT auth and user activation state will not survive database round-trips unless the ORM model, migrations, and any related mappings are updated to persist these fields.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openrag/core/models/user.py` around lines 35 - 41, The Pydantic User fields
password_hash, is_active, and updated_at are not persisted because the
SQLAlchemy ORM User model lacks matching columns; add SQLAlchemy Column
definitions for password_hash (String, nullable), is_active (Boolean, default
True), and updated_at (DateTime, default now, onupdate now) to the ORM User
class, update the ORM-to-Pydantic mapping to include these attributes
(respecting password_hash exclusion/representation), and create an Alembic
migration that adds these columns with safe defaults/nullable handling so
existing rows upgrade cleanly; ensure updated_at is updated automatically on
update and that any repository code saving users correctly sets/reads these
fields.

Comment on lines +61 to +68
id: str = Field(default_factory=lambda: str(uuid.uuid4()))
user_id: int = 0
key_hash: str = ""
key_prefix: str = ""
name: str = ""
is_active: bool = True
created_at: datetime = Field(default_factory=lambda: datetime.now(UTC))
expires_at: datetime | None = None

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether ApiKey instances are serialized or returned by API paths.
# Expected: no route/response/log path exposes key_hash.
rg -nP -C4 '\b(ApiKey\b|list_api_keys_for_user\s*\(|create_api_key\s*\(|model_dump\s*\(|dict\s*\()' \
  --glob '*.py' \
  --glob '!**/.venv/**' \
  --glob '!**/node_modules/**'

Repository: linagora/openrag

Length of output: 26004


🏁 Script executed:

# First, examine the actual ApiKey class definition
cat -n openrag/core/models/user.py | head -100 | tail -50

Repository: linagora/openrag

Length of output: 1862


🏁 Script executed:

# Search for API key routes and endpoints
rg -nP '(create_api_key|list_api_keys|delete_api_key|get_api_keys)' --glob '*.py' | head -30

Repository: linagora/openrag

Length of output: 478


🏁 Script executed:

# Look for any response serialization of ApiKey
rg -nP 'ApiKey.*model_dump|ApiKey.*dict' --glob '*.py' -B2 -A2

Repository: linagora/openrag

Length of output: 42


🏁 Script executed:

# Search for API routes that expose API key functionality
rg -nP '(create_api_key|list_api_keys|delete_api_key|get_api_keys|api.*key)' --glob '*.py' openrag/routers/ -A 3

Repository: linagora/openrag

Length of output: 1729


🏁 Script executed:

# Check for response models or serialization in main/auth routers
find openrag/routers -name "*.py" -type f -exec grep -l "apikey\|api_key\|ApiKey" {} \;

Repository: linagora/openrag

Length of output: 110


🏁 Script executed:

# Search for adapter implementations of UserRepository to see how ApiKey is handled
rg -nP 'class.*UserRepository|def create_api_key|def list_api_keys' --glob '*.py' -A 10

Repository: linagora/openrag

Length of output: 1960


🏁 Script executed:

# Find the concrete implementation of UserRepository
rg -nP 'class.*UserRepository.*:' --glob '*.py' -A 2 | grep -v 'class UserRepository(ABC)'

Repository: linagora/openrag

Length of output: 201


🏁 Script executed:

# Search for REST endpoints related to API keys
rg -nP '(@.*\.post|@.*\.get|def.*create.*api.*key|def.*list.*api.*key)' --glob '*.py' -B 2 -A 5

Repository: linagora/openrag

Length of output: 26504


🏁 Script executed:

# Check if ApiKey is ever returned directly in responses
rg -nP '-> ApiKey|response_model.*ApiKey|list\[ApiKey\]' --glob '*.py' -B 3 -A 3

Repository: linagora/openrag

Length of output: 464


🏁 Script executed:

# Search for response models returning ApiKey or lists
rg -nP 'response_model.*ApiKey|ApiKey.*response' --glob '*.py' -B 3 -A 3

Repository: linagora/openrag

Length of output: 42


🏁 Script executed:

# Find concrete implementation of UserRepository
fd -name "*.py" -type f | xargs grep -l "class.*UserRepository" | grep -v "ports"

Repository: linagora/openrag

Length of output: 290


🏁 Script executed:

# Check if ApiKey models are exposed in any serialization or API response contexts
rg -nP 'ApiKey\(|ApiKey\s*\[|list\s*\[\s*ApiKey' --glob '*.py' -B 2 -A 2

Repository: linagora/openrag

Length of output: 786


🏁 Script executed:

# Find which file implements UserRepository
rg -l 'class.*UserRepository.*:' --glob '*.py' | grep -v ports

Repository: linagora/openrag

Length of output: 42


🏁 Script executed:

# Search for ApiKey instantiation and usage in vectordb components
rg -nP 'ApiKey\(' --glob '*.py' -B 2 -A 5

Repository: linagora/openrag

Length of output: 490


🏁 Script executed:

# Check if there are REST endpoints that return api keys
rg -nP 'def.*(api_key|apikey).*return|@router\.(get|post).*api' --glob 'routers/*.py' -i -B 2 -A 10

Repository: linagora/openrag

Length of output: 208


🏁 Script executed:

# Find all implementations of UserRepository interface
find openrag -name "*.py" -type f -exec grep -l "UserRepository" {} \;

Repository: linagora/openrag

Length of output: 158


🏁 Script executed:

# Search in components/indexer/vectordb for UserRepository implementation
cat openrag/components/indexer/vectordb/utils.py | head -600 | tail -100

Repository: linagora/openrag

Length of output: 3832


🏁 Script executed:

# Look for all routes in routers/users.py to see if ApiKey is exposed
cat openrag/routers/users.py

Repository: linagora/openrag

Length of output: 8671


🏁 Script executed:

# Search for concrete implementations of create_api_key method
rg -nP 'async def create_api_key|async def list_api_keys' --glob '*.py' -B 2 -A 10

Repository: linagora/openrag

Length of output: 1289


🏁 Script executed:

# Search for any API key related routes
rg -nP 'api.key|apikey|api_key' --glob 'routers/*.py' -i

Repository: linagora/openrag

Length of output: 208


🏁 Script executed:

# Look for all REST endpoints mounted in the API
cat openrag/api.py | head -300

Repository: linagora/openrag

Length of output: 11216


🏁 Script executed:

# Check if ApiKey is used in any logging statements
rg -nP 'logger\.(debug|info|warning|error).*ApiKey|log.*ApiKey' --glob '*.py' -B 2 -A 2

Repository: linagora/openrag

Length of output: 42


🏁 Script executed:

# Check if ApiKey.model_dump() is called anywhere
rg -nP 'ApiKey.*\.model_dump|model_dump.*ApiKey' --glob '*.py' -B 2 -A 2

Repository: linagora/openrag

Length of output: 42


🏁 Script executed:

# Search for any serialization of ApiKey instances
rg -nP 'json\.dumps|jsonify|model_dump\(\)' --glob '*.py' -B 5 -A 5 | grep -A 5 -B 5 'ApiKey\|key_hash'

Repository: linagora/openrag

Length of output: 42


🏁 Script executed:

# Search more broadly for all files containing "ApiKey"
rg -l 'ApiKey' --glob '*.py'

Repository: linagora/openrag

Length of output: 151


🏁 Script executed:

# Check the full openrag/core/models/user.py file to see all ApiKey-related code
wc -l openrag/core/models/user.py

Repository: linagora/openrag

Length of output: 90


🏁 Script executed:

# Read the entire user.py file
cat openrag/core/models/user.py

Repository: linagora/openrag

Length of output: 2960


🏁 Script executed:

# Search for implementations of UserRepository outside of ports/
find openrag -name "*.py" -type f | xargs grep -l "class.*UserRepository.*:" | grep -v ports

Repository: linagora/openrag

Length of output: 42


🏁 Script executed:

# Look for where ApiKey is instantiated or used
rg -nP 'ApiKey\s*\(' --glob '*.py' -B 5 -A 5

Repository: linagora/openrag

Length of output: 723


🏁 Script executed:

# Check if there are any tests that use ApiKey
find . -name "*test*.py" -type f | xargs grep -l "ApiKey" 2>/dev/null

Repository: linagora/openrag

Length of output: 44


Fix key_hash to exclude from serialization and require non-empty values.

The key_hash field stores secret verifier material that should never be exposed via model_dump(), responses, or logs. Currently it defaults to an empty string and lacks exclude=True and repr=False constraints, creating a data leakage and validation risk.

Proposed fix
 id: str = Field(default_factory=lambda: str(uuid.uuid4()))
 user_id: int = 0
-key_hash: str = ""
-key_prefix: str = ""
-name: str = ""
+key_hash: str = Field(..., min_length=1, exclude=True, repr=False)
+key_prefix: str = Field(..., min_length=1)
+name: str = Field(..., min_length=1)
 is_active: bool = True
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openrag/core/models/user.py` around lines 61 - 68, Update the key_hash Field
so it cannot be serialized or printed and must be non-empty: replace the current
declaration of key_hash with a required Field(...) that sets exclude=True and
repr=False (e.g., key_hash: str = Field(..., exclude=True, repr=False,
min_length=1)) so model_dump() and repr/logging won't expose it and
empty/default "" is no longer allowed; keep the same type name key_hash and
update any constructors or tests that relied on a default value.

async def create_session(self, session: OIDCSession) -> OIDCSession: ...

@abstractmethod
async def get_by_token_hash(self, token_hash: str) -> OIDCSession | None: ...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Inspect callers of get_by_token_hash to confirm they reject revoked or expired sessions.
# Expected result: each auth-path caller checks revoked_at/session_expires_at, or the repository implementation filters active sessions.

rg -nP -C6 '\bget_by_token_hash\s*\(' --type=py

Repository: linagora/openrag

Length of output: 1062


🏁 Script executed:

#!/bin/bash
# Find the concrete implementation of OIDCSessionRepository
find . -type f -name "*.py" | xargs grep -l "class.*OIDCSessionRepository" | head -5

Repository: linagora/openrag

Length of output: 101


🏁 Script executed:

#!/bin/bash
# Search for all callers of get_by_token_hash to see how they validate sessions
rg -B3 -A8 'get_by_token_hash' --type=py | head -80

Repository: linagora/openrag

Length of output: 938


🏁 Script executed:

#!/bin/bash
# Look at the update_session implementation to understand what fields are allowed
rg -B2 -A10 'def update_session|async def update_session' --type=py

Repository: linagora/openrag

Length of output: 967


🏁 Script executed:

#!/bin/bash
# Find OIDCSession model definition
rg 'class OIDCSession' --type=py -A15

Repository: linagora/openrag

Length of output: 3698


🏁 Script executed:

#!/bin/bash
# Search for any imports or usage of OIDCSessionRepository
rg 'OIDCSessionRepository|from.*oidc_session_repo' --type=py

Repository: linagora/openrag

Length of output: 464


🏁 Script executed:

#!/bin/bash
# Look at the complete oidc_session_repo.py file to understand the full context
cat openrag/core/ports/oidc_session_repo.py

Repository: linagora/openrag

Length of output: 1248


🏁 Script executed:

#!/bin/bash
# Get complete OIDCSession model from user.py
rg 'class OIDCSession' -A30 openrag/core/models/user.py

Repository: linagora/openrag

Length of output: 722


🏁 Script executed:

#!/bin/bash
# Search for implementations of OIDCSessionRepository
find . -type f -name "*.py" -exec grep -l "OIDCSessionRepository" {} \; | xargs grep -l "class.*OIDCSessionRepository" | xargs cat

Repository: linagora/openrag

Length of output: 1248


🏁 Script executed:

#!/bin/bash
# Search for all usages of get_by_token_hash to see how sessions are validated
rg 'get_by_token_hash' --type=py -B5 -A10

Repository: linagora/openrag

Length of output: 1185


🏁 Script executed:

#!/bin/bash
# Search for usages of update_session to see what fields are updated
rg 'update_session' --type=py -B3 -A8

Repository: linagora/openrag

Length of output: 973


🏁 Script executed:

#!/bin/bash
# Search for implementations or references in adapters or repositories directories
find . -type d -name "*adapter*" -o -type d -name "*repository" -o -type d -name "*repositories" | head -10

Repository: linagora/openrag

Length of output: 42


🏁 Script executed:

#!/an/bash
# Look for any SQLAlchemy repository implementations
rg 'OIDCSessionRepository|oidc_session' --type=py -l | grep -E '(adapter|repository|impl)' | head -10

Repository: linagora/openrag

Length of output: 42


🏁 Script executed:

#!/bin/bash
# Check if there are any authentication or login handlers that might call these methods
rg 'get_by_token_hash|openrag_session' --type=py | head -20

Repository: linagora/openrag

Length of output: 2106


🏁 Script executed:

#!/bin/bash
# Look at the catalog_store to understand the injection pattern
rg -A5 'def oidc_session_repo' openrag/core/ports/catalog_store.py

Repository: linagora/openrag

Length of output: 121


🏁 Script executed:

#!/bin/bash
# Check the auth router to see how the session token is handled after retrieval
rg -A20 'SESSION_COOKIE_NAME|openrag_session' openrag/routers/auth.py | head -50

Repository: linagora/openrag

Length of output: 1637


🏁 Script executed:

#!/bin/bash
# Check the middleware to see how session validation is done
cat openrag/components/auth/middleware.py

Repository: linagora/openrag

Length of output: 9186


🏁 Script executed:

#!/bin/bash
# Check the app_front to see session validation
rg -B5 -A15 'openrag_session' openrag/app_front.py | head -60

Repository: linagora/openrag

Length of output: 1280


🏁 Script executed:

#!/bin/bash
# Find the refresh_session_if_needed function
rg -A30 'def refresh_session_if_needed|async def refresh_session_if_needed' --type=py

Repository: linagora/openrag

Length of output: 2540


🏁 Script executed:

#!/bin/bash
# Search for any concrete implementations of OIDCSessionRepository (may be in adapters or elsewhere)
find . -type f -name "*.py" | xargs grep -l "class.*OIDCSessionRepository" | xargs cat

Repository: linagora/openrag

Length of output: 1248


🏁 Script executed:

#!/bin/bash
# Check if get_oidc_session_by_token is a Ray actor or wrapper around the repository
rg 'get_oidc_session_by_token' --type=py -B3 -A10

Repository: linagora/openrag

Length of output: 29065


Add docstring to get_by_token_hash() clarifying that it returns only active, non-revoked, non-expired sessions.

The abstract interface lacks documentation: get_by_token_hash() should document that it returns None if the session is revoked (revoked_at IS NOT NULL) or expired (session_expires_at < now()). The implementation in PartitionFileManager correctly filters on both conditions, and all callers (middleware, routers) assume this filtering is built-in, but the abstract contract is silent. Add a docstring to prevent future implementations from accidentally returning invalid sessions.

Replace the arbitrary **fields parameter in update_session() with an explicit allow-list or dedicate it to non-sensitive fields only.

The method signature async def update_session(self, session_id: int, **fields) is too permissive for an auth-session repository. Per the OIDC guidelines, encrypted IdP tokens (id_token_encrypted, access_token_encrypted, refresh_token_encrypted) must be protected from arbitrary updates. Either enumerate the allowed fields explicitly, or use dedicated typed methods (like the existing update_oidc_session_tokens() for token rotation). The **fields pattern bypasses safe access control and should not exist on a security-sensitive repository interface.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openrag/core/ports/oidc_session_repo.py` at line 17, Add a docstring to
get_by_token_hash(self, token_hash: str) stating it only returns active sessions
(revoked_at IS NULL and session_expires_at >= now()) and returns None for
revoked or expired sessions so implementors preserve the same filtering; and
change the update_session(self, session_id: int, **fields) signature to remove
the arbitrary **fields — either replace it with an explicit allow-list of safe
updatable fields (e.g., "last_seen", "user_metadata", etc.) or split into
dedicated methods and rely on the existing update_oidc_session_tokens(...) for
token updates; update the interface method name/signature accordingly and ensure
callers are adapted to use the new explicit fields or dedicated token rotation
method.

async def get_by_sid(self, sid: str) -> list[OIDCSession]: ...

@abstractmethod
async def update_session(self, session_id: int, **fields) -> OIDCSession | None: ...

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Constrain update_session to an explicit update contract.

Arbitrary **fields makes it easy to update immutable or sensitive auth-session fields without a central allow-list/encryption boundary. Prefer a typed update DTO or named parameters for only the mutable fields. As per coding guidelines, In OIDC mode, encrypt IdP tokens (id_token, access_token, refresh_token) using Fernet before storing in the oidc_sessions table.

🔒 Proposed direction
+from datetime import datetime
+from typing import TypedDict
+
 from openrag.core.models.user import OIDCSession
 
 
+class OIDCSessionUpdate(TypedDict, total=False):
+    access_token_expires_at: datetime
+    session_expires_at: datetime
+    last_refresh_at: datetime | None
+    revoked_at: datetime | None
+
+
 class OIDCSessionRepository(ABC):
@@
-    async def update_session(self, session_id: int, **fields) -> OIDCSession | None: ...
+    async def update_session(
+        self,
+        session_id: int,
+        fields: OIDCSessionUpdate,
+    ) -> OIDCSession | None: ...
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openrag/core/ports/oidc_session_repo.py` at line 23, The update_session
signature currently accepts arbitrary **fields which permits updating
immutable/sensitive session data; change update_session(session_id: int, *,
expires_at: Optional[datetime]=None, is_active: Optional[bool]=None,
last_used_at: Optional[datetime]=None) -> OIDCSession | None (or accept a typed
DTO UpdateOIDCSession) so only explicit mutable fields can be changed, and
ensure the implementation in OIDCSessionRepo (and any callers) uses that
contract; additionally, before persisting any token fields (id_token,
access_token, refresh_token) in methods that create or update OIDCSession
records, encrypt them with Fernet (use a centralized encrypt/decrypt helper) so
the oidc_sessions table never stores plaintext tokens.

@andyne13
andyne13 merged commit b0fdaac into refactor/hexagonal Apr 22, 2026
4 of 5 checks passed
@andyne13
andyne13 deleted the refactor/auth-models-update branch April 22, 2026 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat Add a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant