fix: pricing page toggle and user auto-creation on OAuth login
- Fix monthly/annual price toggle by moving x-data scope to outer div - Auto-create UserProfile in DB on first Authentik OAuth login - Update and expand tests for oauth_callback and _ensure_user_profile Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com>
This commit is contained in:
copilot-swe-agent[bot]
+40
-2
@@ -5,11 +5,13 @@ import pathlib
|
||||
from functools import wraps
|
||||
|
||||
from authlib.integrations.starlette_client import OAuth
|
||||
from fastapi import APIRouter, Request, status
|
||||
from fastapi import APIRouter, Depends, Request, status
|
||||
from fastapi.templating import Jinja2Templates
|
||||
from sqlalchemy.orm import Session
|
||||
from starlette.responses import RedirectResponse
|
||||
|
||||
from app.config import settings
|
||||
from app.database import get_db
|
||||
|
||||
oauth = OAuth()
|
||||
|
||||
@@ -94,7 +96,40 @@ async def oauth_login(request: Request):
|
||||
return await oauth.authentik.authorize_redirect(request, redirect_uri)
|
||||
|
||||
|
||||
async def oauth_callback(request: Request):
|
||||
def _ensure_user_profile(db: Session, user_data: dict) -> None:
|
||||
"""Create a UserProfile row for *user_data* if one does not yet exist.
|
||||
|
||||
Uses the same identifier priority as ``get_current_owner_id`` (sub →
|
||||
preferred_username → email → id) so that the profile's ``user_id`` matches
|
||||
``FileRecord.owner_id`` for every document the user uploads.
|
||||
|
||||
If a profile already exists it is left unchanged; only missing profiles
|
||||
are created so that admin-managed settings (tier, limits, etc.) are
|
||||
preserved across logins.
|
||||
"""
|
||||
from app.models import UserProfile
|
||||
|
||||
user_id = (
|
||||
user_data.get("sub") or user_data.get("preferred_username") or user_data.get("email") or user_data.get("id")
|
||||
)
|
||||
if not user_id:
|
||||
logger.warning("Cannot create UserProfile: no stable user identifier in OAuth userinfo")
|
||||
return
|
||||
|
||||
try:
|
||||
existing = db.query(UserProfile).filter(UserProfile.user_id == user_id).first()
|
||||
if existing is None:
|
||||
display_name = user_data.get("name") or user_data.get("preferred_username") or user_data.get("email")
|
||||
profile = UserProfile(user_id=user_id, display_name=display_name)
|
||||
db.add(profile)
|
||||
db.commit()
|
||||
logger.info("Auto-created UserProfile for user_id=%s", user_id)
|
||||
except Exception:
|
||||
db.rollback()
|
||||
logger.exception("Failed to auto-create UserProfile for user_id=%s", user_id)
|
||||
|
||||
|
||||
async def oauth_callback(request: Request, db: Session = Depends(get_db)):
|
||||
"""Handle OAuth callback from provider"""
|
||||
try:
|
||||
token = await oauth.authentik.authorize_access_token(request)
|
||||
@@ -126,6 +161,9 @@ async def oauth_callback(request: Request):
|
||||
|
||||
request.session["user"] = user_data
|
||||
|
||||
# Auto-create or update UserProfile so the user appears in admin user management
|
||||
_ensure_user_profile(db, user_data)
|
||||
|
||||
# Log the successful authentication
|
||||
logger.info(f"[SECURITY] OAUTH_LOGIN_SUCCESS user={user_data.get('email', 'unknown')} admin={is_admin}")
|
||||
|
||||
|
||||
@@ -2,7 +2,7 @@
|
||||
{% block title %}Pricing & Plans – DocuElevate{% endblock %}
|
||||
|
||||
{% block content %}
|
||||
<div class="bg-gray-50 min-h-screen">
|
||||
<div class="bg-gray-50 min-h-screen" x-data="{ annual: false }">
|
||||
|
||||
<!-- ── Hero ──────────────────────────────────────────────────────────────── -->
|
||||
<div class="bg-gradient-to-br from-blue-700 via-indigo-700 to-purple-700 text-white py-20 px-4">
|
||||
@@ -18,7 +18,7 @@
|
||||
</p>
|
||||
|
||||
<!-- Annual / Monthly toggle (cosmetic — actual billing handled separately) -->
|
||||
<div class="mt-8 inline-flex items-center bg-white/10 rounded-full p-1 gap-1" x-data="{ annual: false }">
|
||||
<div class="mt-8 inline-flex items-center bg-white/10 rounded-full p-1 gap-1">
|
||||
<button
|
||||
@click="annual = false"
|
||||
:class="!annual ? 'bg-white text-indigo-700 shadow' : 'text-white'"
|
||||
@@ -36,8 +36,7 @@
|
||||
</div>
|
||||
|
||||
<!-- ── Tier cards ────────────────────────────────────────────────────────── -->
|
||||
<div class="max-w-7xl mx-auto px-4 -mt-10 pb-20" x-data="{ annual: false }">
|
||||
<!-- Recreate the toggle state here so cards react to the hero toggle too -->
|
||||
<div class="max-w-7xl mx-auto px-4 -mt-10 pb-20">
|
||||
<div class="grid grid-cols-1 sm:grid-cols-2 lg:grid-cols-4 gap-6 items-stretch">
|
||||
|
||||
{% for tier in tiers %}
|
||||
|
||||
+154
-8
@@ -375,6 +375,7 @@ class TestOAuthCallback:
|
||||
|
||||
mock_request = MagicMock(spec=Request)
|
||||
mock_request.session = {}
|
||||
mock_db = MagicMock()
|
||||
|
||||
userinfo = {
|
||||
"email": "test@example.com",
|
||||
@@ -388,11 +389,12 @@ class TestOAuthCallback:
|
||||
with (
|
||||
patch("app.auth.oauth") as mock_oauth,
|
||||
patch("app.auth.settings") as mock_settings,
|
||||
patch("app.auth._ensure_user_profile"),
|
||||
):
|
||||
mock_oauth.authentik = mock_authentik
|
||||
mock_settings.admin_group_name = "admin"
|
||||
|
||||
result = await oauth_callback(mock_request)
|
||||
result = await oauth_callback(mock_request, db=mock_db)
|
||||
|
||||
assert isinstance(result, RedirectResponse)
|
||||
assert result.status_code == status.HTTP_302_FOUND
|
||||
@@ -407,6 +409,7 @@ class TestOAuthCallback:
|
||||
|
||||
mock_request = MagicMock(spec=Request)
|
||||
mock_request.session = {}
|
||||
mock_db = MagicMock()
|
||||
|
||||
userinfo = {"email": "test@example.com", "name": "Test User"}
|
||||
|
||||
@@ -416,11 +419,12 @@ class TestOAuthCallback:
|
||||
with (
|
||||
patch("app.auth.oauth") as mock_oauth,
|
||||
patch("app.auth.settings") as mock_settings,
|
||||
patch("app.auth._ensure_user_profile"),
|
||||
):
|
||||
mock_oauth.authentik = mock_authentik
|
||||
mock_settings.admin_group_name = "admin"
|
||||
|
||||
result = await oauth_callback(mock_request)
|
||||
result = await oauth_callback(mock_request, db=mock_db)
|
||||
|
||||
# Gravatar should be added
|
||||
assert "picture" in mock_request.session["user"]
|
||||
@@ -433,6 +437,7 @@ class TestOAuthCallback:
|
||||
|
||||
mock_request = MagicMock(spec=Request)
|
||||
mock_request.session = {}
|
||||
mock_db = MagicMock()
|
||||
|
||||
userinfo = {
|
||||
"email": "test@example.com",
|
||||
@@ -446,11 +451,12 @@ class TestOAuthCallback:
|
||||
with (
|
||||
patch("app.auth.oauth") as mock_oauth,
|
||||
patch("app.auth.settings") as mock_settings,
|
||||
patch("app.auth._ensure_user_profile"),
|
||||
):
|
||||
mock_oauth.authentik = mock_authentik
|
||||
mock_settings.admin_group_name = "admin"
|
||||
|
||||
result = await oauth_callback(mock_request)
|
||||
result = await oauth_callback(mock_request, db=mock_db)
|
||||
|
||||
# Custom picture should be preserved, not replaced with Gravatar
|
||||
assert mock_request.session["user"]["picture"] == "https://example.com/custom-pic.jpg"
|
||||
@@ -462,6 +468,7 @@ class TestOAuthCallback:
|
||||
|
||||
mock_request = MagicMock(spec=Request)
|
||||
mock_request.session = {}
|
||||
mock_db = MagicMock()
|
||||
|
||||
userinfo = {
|
||||
"email": "admin@example.com",
|
||||
@@ -475,11 +482,12 @@ class TestOAuthCallback:
|
||||
with (
|
||||
patch("app.auth.oauth") as mock_oauth,
|
||||
patch("app.auth.settings") as mock_settings,
|
||||
patch("app.auth._ensure_user_profile"),
|
||||
):
|
||||
mock_oauth.authentik = mock_authentik
|
||||
mock_settings.admin_group_name = "admin"
|
||||
|
||||
result = await oauth_callback(mock_request)
|
||||
result = await oauth_callback(mock_request, db=mock_db)
|
||||
|
||||
# User should be marked as admin
|
||||
assert mock_request.session["user"]["is_admin"] is True
|
||||
@@ -491,6 +499,7 @@ class TestOAuthCallback:
|
||||
|
||||
mock_request = MagicMock(spec=Request)
|
||||
mock_request.session = {}
|
||||
mock_db = MagicMock()
|
||||
|
||||
userinfo = {
|
||||
"email": "user@example.com",
|
||||
@@ -504,11 +513,12 @@ class TestOAuthCallback:
|
||||
with (
|
||||
patch("app.auth.oauth") as mock_oauth,
|
||||
patch("app.auth.settings") as mock_settings,
|
||||
patch("app.auth._ensure_user_profile"),
|
||||
):
|
||||
mock_oauth.authentik = mock_authentik
|
||||
mock_settings.admin_group_name = "admin"
|
||||
|
||||
result = await oauth_callback(mock_request)
|
||||
result = await oauth_callback(mock_request, db=mock_db)
|
||||
|
||||
# User should not be marked as admin
|
||||
assert mock_request.session["user"]["is_admin"] is False
|
||||
@@ -520,6 +530,7 @@ class TestOAuthCallback:
|
||||
|
||||
mock_request = MagicMock(spec=Request)
|
||||
mock_request.session = {}
|
||||
mock_db = MagicMock()
|
||||
|
||||
mock_authentik = MagicMock()
|
||||
mock_authentik.authorize_access_token = AsyncMock(return_value={})
|
||||
@@ -527,7 +538,7 @@ class TestOAuthCallback:
|
||||
with patch("app.auth.oauth") as mock_oauth:
|
||||
mock_oauth.authentik = mock_authentik
|
||||
|
||||
result = await oauth_callback(mock_request)
|
||||
result = await oauth_callback(mock_request, db=mock_db)
|
||||
|
||||
assert isinstance(result, RedirectResponse)
|
||||
assert "/login?error=Failed+to+retrieve+user+information" in result.headers["location"]
|
||||
@@ -539,6 +550,7 @@ class TestOAuthCallback:
|
||||
|
||||
mock_request = MagicMock(spec=Request)
|
||||
mock_request.session = {"redirect_after_login": "/protected/page"}
|
||||
mock_db = MagicMock()
|
||||
|
||||
userinfo = {"email": "test@example.com", "name": "Test User"}
|
||||
|
||||
@@ -548,11 +560,12 @@ class TestOAuthCallback:
|
||||
with (
|
||||
patch("app.auth.oauth") as mock_oauth,
|
||||
patch("app.auth.settings") as mock_settings,
|
||||
patch("app.auth._ensure_user_profile"),
|
||||
):
|
||||
mock_oauth.authentik = mock_authentik
|
||||
mock_settings.admin_group_name = "admin"
|
||||
|
||||
result = await oauth_callback(mock_request)
|
||||
result = await oauth_callback(mock_request, db=mock_db)
|
||||
|
||||
assert isinstance(result, RedirectResponse)
|
||||
assert result.headers["location"] == "/protected/page"
|
||||
@@ -566,6 +579,7 @@ class TestOAuthCallback:
|
||||
|
||||
mock_request = MagicMock(spec=Request)
|
||||
mock_request.session = {}
|
||||
mock_db = MagicMock()
|
||||
|
||||
mock_authentik = MagicMock()
|
||||
mock_authentik.authorize_access_token = AsyncMock(side_effect=Exception("OAuth error"))
|
||||
@@ -573,11 +587,143 @@ class TestOAuthCallback:
|
||||
with patch("app.auth.oauth") as mock_oauth:
|
||||
mock_oauth.authentik = mock_authentik
|
||||
|
||||
result = await oauth_callback(mock_request)
|
||||
result = await oauth_callback(mock_request, db=mock_db)
|
||||
|
||||
assert isinstance(result, RedirectResponse)
|
||||
assert "/login?error=Authentication+failed" in result.headers["location"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_oauth_callback_creates_user_profile(self):
|
||||
"""Test OAuth callback auto-creates a UserProfile for the authenticated user."""
|
||||
from app.auth import oauth_callback
|
||||
|
||||
mock_request = MagicMock(spec=Request)
|
||||
mock_request.session = {}
|
||||
mock_db = MagicMock()
|
||||
|
||||
userinfo = {
|
||||
"sub": "oauth-sub-abc123",
|
||||
"email": "new@example.com",
|
||||
"name": "New User",
|
||||
"preferred_username": "newuser",
|
||||
}
|
||||
|
||||
mock_authentik = MagicMock()
|
||||
mock_authentik.authorize_access_token = AsyncMock(return_value={"userinfo": userinfo})
|
||||
|
||||
with (
|
||||
patch("app.auth.oauth") as mock_oauth,
|
||||
patch("app.auth.settings") as mock_settings,
|
||||
patch("app.auth._ensure_user_profile") as mock_ensure,
|
||||
):
|
||||
mock_oauth.authentik = mock_authentik
|
||||
mock_settings.admin_group_name = "admin"
|
||||
|
||||
await oauth_callback(mock_request, db=mock_db)
|
||||
|
||||
# _ensure_user_profile should be called with the db and user_data
|
||||
mock_ensure.assert_called_once()
|
||||
call_args = mock_ensure.call_args
|
||||
assert call_args[0][0] is mock_db
|
||||
assert call_args[0][1]["sub"] == "oauth-sub-abc123"
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestEnsureUserProfile:
|
||||
"""Tests for _ensure_user_profile() helper."""
|
||||
|
||||
def test_creates_profile_for_new_user(self):
|
||||
"""New user_id should insert a UserProfile row."""
|
||||
from app.auth import _ensure_user_profile
|
||||
from app.models import UserProfile
|
||||
|
||||
mock_db = MagicMock()
|
||||
mock_db.query.return_value.filter.return_value.first.return_value = None
|
||||
|
||||
user_data = {
|
||||
"sub": "sub-xyz",
|
||||
"email": "alice@example.com",
|
||||
"name": "Alice",
|
||||
"preferred_username": "alice",
|
||||
}
|
||||
|
||||
_ensure_user_profile(mock_db, user_data)
|
||||
|
||||
mock_db.add.assert_called_once()
|
||||
added_profile = mock_db.add.call_args[0][0]
|
||||
assert isinstance(added_profile, UserProfile)
|
||||
assert added_profile.user_id == "sub-xyz"
|
||||
assert added_profile.display_name == "Alice"
|
||||
mock_db.commit.assert_called_once()
|
||||
|
||||
def test_skips_existing_profile(self):
|
||||
"""Existing profile should not be overwritten."""
|
||||
from app.auth import _ensure_user_profile
|
||||
from app.models import UserProfile
|
||||
|
||||
mock_db = MagicMock()
|
||||
existing = UserProfile(user_id="sub-xyz", display_name="Old Name")
|
||||
mock_db.query.return_value.filter.return_value.first.return_value = existing
|
||||
|
||||
user_data = {"sub": "sub-xyz", "name": "New Name"}
|
||||
|
||||
_ensure_user_profile(mock_db, user_data)
|
||||
|
||||
mock_db.add.assert_not_called()
|
||||
mock_db.commit.assert_not_called()
|
||||
|
||||
def test_uses_preferred_username_fallback(self):
|
||||
"""Falls back to preferred_username when sub is absent."""
|
||||
from app.auth import _ensure_user_profile
|
||||
|
||||
mock_db = MagicMock()
|
||||
mock_db.query.return_value.filter.return_value.first.return_value = None
|
||||
|
||||
user_data = {"preferred_username": "bob", "name": "Bob"}
|
||||
|
||||
_ensure_user_profile(mock_db, user_data)
|
||||
|
||||
added_profile = mock_db.add.call_args[0][0]
|
||||
assert added_profile.user_id == "bob"
|
||||
|
||||
def test_uses_email_fallback(self):
|
||||
"""Falls back to email when sub and preferred_username are absent."""
|
||||
from app.auth import _ensure_user_profile
|
||||
|
||||
mock_db = MagicMock()
|
||||
mock_db.query.return_value.filter.return_value.first.return_value = None
|
||||
|
||||
user_data = {"email": "carol@example.com", "name": "Carol"}
|
||||
|
||||
_ensure_user_profile(mock_db, user_data)
|
||||
|
||||
added_profile = mock_db.add.call_args[0][0]
|
||||
assert added_profile.user_id == "carol@example.com"
|
||||
|
||||
def test_no_op_when_no_identifier(self):
|
||||
"""Does nothing and logs a warning when no identifier is found."""
|
||||
from app.auth import _ensure_user_profile
|
||||
|
||||
mock_db = MagicMock()
|
||||
|
||||
_ensure_user_profile(mock_db, {})
|
||||
|
||||
mock_db.add.assert_not_called()
|
||||
mock_db.commit.assert_not_called()
|
||||
|
||||
def test_handles_db_exception_gracefully(self):
|
||||
"""DB errors are caught; a rollback is issued and no exception propagates."""
|
||||
from app.auth import _ensure_user_profile
|
||||
|
||||
mock_db = MagicMock()
|
||||
mock_db.query.return_value.filter.return_value.first.return_value = None
|
||||
mock_db.commit.side_effect = Exception("DB error")
|
||||
|
||||
# Should not raise
|
||||
_ensure_user_profile(mock_db, {"sub": "sub-error-test"})
|
||||
|
||||
mock_db.rollback.assert_called_once()
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestAuthFunction:
|
||||
|
||||
Reference in New Issue
Block a user