feat: implement ORCiD OAuth authentication#50
Conversation
Backend: - Add auth routes (login, callback, refresh, logout) with OAuth 2.0 flow - Add domain models for User, Identity, and RefreshToken - Add AuthService and TokenService with JWT token generation - Add ORCiD identity provider with sandbox support - Add signed state tokens for CSRF protection (HMAC-SHA256) - Add database tables and Alembic migration for auth entities - Add unit tests for auth service, token service, and state signing Frontend: - Add AuthProvider context with SDK integration - Add LoginButton and UserMenu components - Add auth SDK (client, storage, types) for token management - Add auth callback page for OAuth redirect handling Infrastructure: - Simplify Justfile log commands (just logs <service>) - Fix docker-compose.dev.yml for hot-reload development - Add curl to Dockerfile builder stage for healthchecks - Expose server port 8000 in dev mode for OAuth redirects
|
Greptile OverviewGreptile SummaryImplements OAuth 2.0 authentication with ORCiD as identity provider, including JWT-based sessions, refresh token rotation, and full frontend integration. Backend follows clean DDD architecture with proper separation of concerns. Key Changes
Critical Issues Found
Security Strengths
Architecture QualityBackend implementation is solid with clean domain-driven design, proper error handling, and comprehensive test coverage. The token service correctly implements RFC 6749 OAuth 2.0 with additional security measures. Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| web/src/lib/sdk/auth.ts | Added OAuth client with token management - critical bugs: missing provider parameter in login URL, parameter name mismatch (orcid_id vs external_id) breaks callback parsing |
| web/src/lib/sdk/types.ts | Added TypeScript types for auth SDK - field naming inconsistency (orcidId should be externalId) |
| server/osa/config.py | Added auth config with JWT and ORCiD settings - JWT secret validation can be bypassed with empty string default |
| server/osa/application/api/v1/routes/auth.py | Implemented OAuth routes with proper state validation and token issuance - well structured with good error handling |
| server/osa/domain/auth/service/auth.py | Implemented auth service with user management and token rotation - solid implementation with proper refresh token family revocation on theft detection |
| server/osa/domain/auth/service/token.py | Implemented JWT and OAuth state token services with HMAC-SHA256 signing - proper CSRF protection and constant-time comparison |
Sequence Diagram
sequenceDiagram
participant User
participant Frontend
participant Backend
participant ORCiD
User->>Frontend: Click "Sign in with ORCiD"
Frontend->>Backend: GET /auth/login?provider=orcid&redirect_uri={url}
Backend->>Backend: Create signed state token (HMAC-SHA256)
Backend->>Backend: Generate authorization URL
Backend-->>Frontend: 302 Redirect to ORCiD
Frontend->>ORCiD: Redirect to authorization page
User->>ORCiD: Authorize application
ORCiD-->>Backend: GET /auth/callback?code={code}&state={state}
Backend->>Backend: Verify signed state token
Backend->>ORCiD: POST /oauth/token (exchange code)
ORCiD-->>Backend: Return access_token + user info
Backend->>Backend: Find or create User + Identity
Backend->>Backend: Create JWT access token (HS256)
Backend->>Backend: Create refresh token (hash & store)
Backend-->>Frontend: 302 Redirect with tokens in URL hash
Frontend->>Frontend: Parse tokens from hash
Frontend->>Frontend: Store tokens in localStorage
Frontend->>Frontend: Setup auto-refresh timer
Frontend->>User: Show authenticated UI
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${stored.tokens.accessToken}`, |
There was a problem hiding this comment.
The Authorization header is included in the logout request but the backend doesn't validate it - the endpoint only requires the refresh_token in the body. The header is unnecessary here.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/lib/sdk/auth.ts
Line: 157:157
Comment:
The `Authorization` header is included in the logout request but the backend doesn't validate it - the endpoint only requires the `refresh_token` in the body. The header is unnecessary here.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| // Clear hash from URL | ||
| window.history.replaceState(null, '', window.location.pathname + window.location.search); |
There was a problem hiding this comment.
The window.history.replaceState approach works, but setting window.location.hash = '' would be simpler and more direct for clearing the hash.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/components/auth/AuthProvider.tsx
Line: 47:48
Comment:
The `window.history.replaceState` approach works, but setting `window.location.hash = ''` would be simpler and more direct for clearing the hash.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| # ============================================================================= | ||
|
|
||
| # JWT signing secret (generate with: openssl rand -hex 32) | ||
| # IMPORTANT: Use a strong, unique secret in production! |
There was a problem hiding this comment.
Consider recommending a minimum length (e.g., 256 bits / 32 bytes) for the JWT secret in the comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/.env.example
Line: 36:36
Comment:
Consider recommending a minimum length (e.g., 256 bits / 32 bytes) for the JWT secret in the comment.
How can I resolve this? If you propose a fix, please make it concise.- callback/page: use useSearchParams hook and Next.js Link component - AuthProvider: simplify to standard useEffect pattern with eslint-disable - auth.ts: remove unused LocalTokenStorage import - pre-commit: add web-lint hook to catch frontend issues locally The auth UI shows a brief loading state on page load because tokens are stored in localStorage (not accessible during SSR). This is tracked in issue #51 for a proper cookie-based solution.
- Make JWT secret a required field with no default - Add model_validator ensuring minimum 32 character length - Remove default values from JwtConfig.secret, AuthConfig.jwt, and Config.auth - Add type: ignore[call-arg] at Config() call sites where Pydantic Settings populates from environment variables at runtime - Update tests to use secrets with 32+ characters - Set test secret in conftest.py before module imports - Sanitize error messages in OAuth callback (don't expose exception details)
Code reviewFound 6 issues:
server/server/osa/application/api/v1/routes/auth.py Lines 32 to 86 in ac28b85
server/server/osa/domain/auth/service/auth.py Lines 106 to 132 in ac28b85
server/server/osa/domain/auth/deps.py Lines 1 to 67 in ac28b85
server/server/osa/application/api/v1/routes/auth.py Lines 231 to 236 in ac28b85
server/server/osa/domain/auth/service/auth.py Lines 55 to 87 in ac28b85
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
- Add Suspense boundary around AuthCallbackContent component to fix Next.js static generation error with useSearchParams() - Add web-build pre-commit hook to catch build failures locally
- Add CurrentUser dataclass to auth domain model (value.py) - Add Dishka provider for CurrentUser that extracts JWT from Authorization header - Simplify /me endpoint to use FromDishka[CurrentUser] - Delete unused deps.py (FastAPI Depends implementation) - Remove unnecessary Authorization header from logout request in frontend
Move create_oauth_state and verify_oauth_state methods from routes to TokenService where other token-related cryptographic operations live. Routers should only handle HTTP translation, not business logic.
- Update InitiateLoginHandler to use TokenService for signed state - Update CompleteOAuthHandler to emit UserAuthenticated event - Add RefreshTokensHandler and LogoutHandler with UserLoggedOut event - Register all handlers in DI provider - Update routes to use handlers instead of calling services directly - Add unit tests for all command handlers This establishes the Route → Handler → Service → Repository pattern for auth operations, enabling future cross-cutting concerns.
Remove hardcoded ORCID references to support multiple identity providers: - Add ProviderIdentity value object encapsulating provider + external_id - Add ProviderRegistry port and InMemoryProviderRegistry implementation - Update CurrentUser to use ProviderIdentity instead of orcid_id - Update TokenService JWT claims to use provider/external_id - Update OAuth state to include provider for callback routing - Update command handlers to look up providers from registry - Update API routes to validate against configured providers - Rename UserAuthenticated.orcid_id to external_id - Rename AuthService.get_orcid_identity() to get_primary_identity()
|
@greptile |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Update user property naming from orcidId to externalId in UserMenu component, auth client, API client, and type definitions to use more generic terminology for external identity providers
Add row-level locking (SELECT FOR UPDATE) when fetching refresh tokens during the refresh flow. This prevents concurrent requests from both passing the is_revoked check before either revokes the token, which would defeat the theft detection mechanism.
Summary
Backend Changes
/auth/login,/auth/callback,/auth/refresh,/auth/logoutFrontend Changes
Infrastructure
Test plan
Related issues