Merge bug/oauth-scope-and-email: OAuth scope-clobber + admin-email fixes
Two user-reported signin bugs + a review-surfaced revoke fix: - A logout→login no longer wipes a user's YouTube grant: _store_token keeps the whole existing grant when a base-scope sign-in would narrow the stored YouTube scopes (the old refresh token stays valid on Google's side; overwriting it was the loss). - Admin 'new access request' email: correct menu path (Users → Access requests), the requester address spelled out + a deep-link to the approve view, and it now notifies the ACTUAL admins (active role=admin) unioned with env, not just the static env list. - purge_user's Google-revoke fallback decrypts the access token (was sending ciphertext).
This commit is contained in:
commit
c1a2ec63cc
3 changed files with 64 additions and 16 deletions
|
|
@ -73,6 +73,8 @@ READ_SCOPE = f"{WRITE_SCOPE}.readonly"
|
|||
BASE_SCOPES = "openid email profile"
|
||||
READ_SCOPES = f"{BASE_SCOPES} {READ_SCOPE}"
|
||||
WRITE_SCOPES = f"{BASE_SCOPES} {READ_SCOPE} {WRITE_SCOPE}"
|
||||
# The YouTube grants (read ⊂ write). A sign-in must never silently drop one of these — see _store_token.
|
||||
_YOUTUBE_SCOPES = frozenset({READ_SCOPE, WRITE_SCOPE})
|
||||
|
||||
log = logging.getLogger("siftlode.auth")
|
||||
|
||||
|
|
@ -132,6 +134,20 @@ def has_write_scope(user: User) -> bool:
|
|||
return WRITE_SCOPE in tok.scopes.split()
|
||||
|
||||
|
||||
def admin_notify_emails(db: Session) -> list[str]:
|
||||
"""Who to email about admin events (e.g. a new access request). The ACTUAL admins — every active,
|
||||
non-suspended `role=admin` user — UNIONED with the env ADMIN_EMAILS bootstrap list. Using the DB
|
||||
roles means an admin promoted through the UI is notified too (env alone would miss them, yet they
|
||||
CAN approve requests — the approve UI is role-gated, so the notify set must match); the env union
|
||||
keeps a freshly-seeded instance working before any DB admin exists."""
|
||||
db_admins = db.execute(
|
||||
select(User.email).where(
|
||||
User.role == "admin", User.is_active.is_(True), User.is_suspended.is_(False)
|
||||
)
|
||||
).scalars()
|
||||
return sorted({e.lower() for e in db_admins if e} | settings.admin_email_set)
|
||||
|
||||
|
||||
def is_allowed(db: Session, email: str) -> bool:
|
||||
"""May this email sign in? An approved Invite is the source of truth; the env
|
||||
ALLOWED_EMAILS/ADMIN_EMAILS remain a bootstrap fallback (e.g. a fresh DB)."""
|
||||
|
|
@ -282,12 +298,9 @@ async def callback(
|
|||
# land the user on a friendly "request received" screen instead of a raw 403.
|
||||
if email:
|
||||
inv = upsert_pending_invite(db, email)
|
||||
if inv is not None and settings.admin_email_set:
|
||||
background.add_task(
|
||||
email_mod.send_admin_new_request,
|
||||
sorted(settings.admin_email_set),
|
||||
email,
|
||||
)
|
||||
admins = admin_notify_emails(db)
|
||||
if inv is not None and admins:
|
||||
background.add_task(email_mod.send_admin_new_request, admins, email)
|
||||
return RedirectResponse(url="/?access=requested")
|
||||
return RedirectResponse(url="/?access=denied")
|
||||
|
||||
|
|
@ -376,6 +389,18 @@ def _store_token(db: Session, user: User, token: dict) -> None:
|
|||
include_granted_scopes=true means `scope` is the union of everything granted, so this
|
||||
reflects read/write upgrades correctly."""
|
||||
tok = user.token or OAuthToken(user=user)
|
||||
old = set((tok.scopes or "").split())
|
||||
new = set((token.get("scope") or "").split())
|
||||
# A plain re-sign-in requests only BASE_SCOPES, and Google can hand back a FRESH base-only
|
||||
# access+refresh token. Adopting it would DESTROY a previously-granted YouTube read/write grant —
|
||||
# can_read/can_write flip to false and the feed demands a reconnect (the exact bug users hit on a
|
||||
# logout→login). The OLD refresh token stays valid on Google's side, so when this exchange would
|
||||
# NARROW our YouTube scopes, keep the WHOLE existing grant (refresh token, access token, scopes)
|
||||
# untouched. The grant only shrinks on a full disconnect (purge_user deletes the token row); an
|
||||
# externally-revoked scope surfaces later as a YouTube API 403 → reconnect, not through this write.
|
||||
if (old & _YOUTUBE_SCOPES) - new:
|
||||
db.add(tok)
|
||||
return
|
||||
if token.get("refresh_token"):
|
||||
tok.refresh_token_enc = encrypt(token["refresh_token"])
|
||||
# Encrypt the access token at rest too (it's a live ~1h bearer credential), matching the
|
||||
|
|
@ -383,7 +408,8 @@ def _store_token(db: Session, user: User, token: dict) -> None:
|
|||
tok.access_token = encrypt(token.get("access_token"))
|
||||
expires_at = token.get("expires_at")
|
||||
tok.expiry = datetime.fromtimestamp(expires_at, tz=timezone.utc) if expires_at else None
|
||||
tok.scopes = token.get("scope") or BASE_SCOPES
|
||||
# Union (never narrow): a broader-or-equal exchange may still list only the newly-added scope.
|
||||
tok.scopes = " ".join(sorted(old | new)) or BASE_SCOPES
|
||||
db.add(tok)
|
||||
|
||||
|
||||
|
|
@ -414,7 +440,9 @@ def purge_user(db: Session, user: User, background: BackgroundTasks) -> None:
|
|||
email = user.email.lower()
|
||||
user_id = user.id
|
||||
tok = user.token
|
||||
google_token = (decrypt(tok.refresh_token_enc) or tok.access_token) if tok else None
|
||||
# Both are stored encrypted — decrypt for the revoke call (the access-token fallback was passing
|
||||
# ciphertext, so a token row with no refresh token silently failed to revoke).
|
||||
google_token = (decrypt(tok.refresh_token_enc) or decrypt(tok.access_token)) if tok else None
|
||||
# Raw delete so Postgres applies ON DELETE CASCADE / SET NULL on every dependent table.
|
||||
db.execute(delete(User).where(User.id == user_id))
|
||||
db.execute(delete(Invite).where(Invite.email == email))
|
||||
|
|
@ -495,10 +523,9 @@ async def request_access(
|
|||
if is_allowed(db, email):
|
||||
return {"status": "approved"} # already allowed — just sign in
|
||||
inv = upsert_pending_invite(db, email)
|
||||
if inv is not None and settings.admin_email_set:
|
||||
background.add_task(
|
||||
email_mod.send_admin_new_request, sorted(settings.admin_email_set), email
|
||||
)
|
||||
admins = admin_notify_emails(db)
|
||||
if inv is not None and admins:
|
||||
background.add_task(email_mod.send_admin_new_request, admins, email)
|
||||
return {"status": "pending"}
|
||||
|
||||
|
||||
|
|
@ -629,8 +656,9 @@ def _register_account(email: str, password: str) -> None:
|
|||
raw = _issue_token(db, user, "verify", VERIFY_TTL)
|
||||
# Fragment (#), not a query token: keeps the token out of proxy/access logs + Referer (SB3).
|
||||
email_mod.send_verify_email(email, f"{_app_base()}/#verify={raw}")
|
||||
if settings.admin_email_set:
|
||||
email_mod.send_admin_new_request(sorted(settings.admin_email_set), email)
|
||||
admins = admin_notify_emails(db)
|
||||
if admins:
|
||||
email_mod.send_admin_new_request(admins, email)
|
||||
|
||||
|
||||
@router.post("/verify")
|
||||
|
|
|
|||
|
|
@ -142,10 +142,17 @@ def send_account_suspended(to: str, operator: str | None) -> bool:
|
|||
|
||||
|
||||
def send_admin_new_request(admins: list[str], requester: str) -> bool:
|
||||
# The link deep-links straight to the admin Access-requests page (the SPA reads ?admin=…). The
|
||||
# requester address is spelled out (mail clients auto-link both the URL and the address), and the
|
||||
# reply note is accurate: _send sets Reply-To to the requester, so a reply reaches THEM — not the
|
||||
# sending mailbox. (The old copy said "Settings → Account", which was the wrong menu.)
|
||||
approve_url = f"{settings.app_base}/?admin=access-requests"
|
||||
body = (
|
||||
f"{requester} just requested access to Siftlode.\n\n"
|
||||
"Open Siftlode → Settings → Account → Access requests to approve or deny it.\n\n"
|
||||
"(Reply to this email to reach the requester directly.)\n"
|
||||
f"Approve or deny it here:\n{approve_url}\n"
|
||||
f"(or in Siftlode: open Users → Access requests)\n\n"
|
||||
f"Requester's email: {requester}\n"
|
||||
f"Replying to this message goes straight to the requester (it's the Reply-To address).\n"
|
||||
)
|
||||
return _send(admins, f"Siftlode access request from {requester}", body, reply_to=requester)
|
||||
|
||||
|
|
|
|||
|
|
@ -528,6 +528,19 @@ export default function App() {
|
|||
stripUrlParams();
|
||||
}, []); // eslint-disable-line react-hooks/exhaustive-deps
|
||||
|
||||
// Deep-link from the "new access request" admin email (?admin=access-requests) → jump straight to
|
||||
// the admin Users → Access requests view. Snapshot the param so the pre-login URL strip doesn't
|
||||
// drop it; act once `me` is known, and only for an admin (others just land on their default page).
|
||||
const [adminDeepLink] = useState(() => new URLSearchParams(window.location.search).get("admin"));
|
||||
useEffect(() => {
|
||||
if (adminDeepLink !== "access-requests" || !meQuery.data) return;
|
||||
if (meQuery.data.role === "admin") {
|
||||
focusAccessRequestsTab();
|
||||
setPage("users");
|
||||
}
|
||||
stripUrlParams();
|
||||
}, [adminDeepLink, meQuery.data?.id]); // eslint-disable-line react-hooks/exhaustive-deps
|
||||
|
||||
// First-login onboarding: prompt the user to connect YouTube (and resume the flow after
|
||||
// each consent redirect). Derived from granted scopes + storage flags so it's stable
|
||||
// across the full-page OAuth round-trip; dismissible and reopenable from Settings.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue