Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive authorization system (AuthZ) for the CHURRO application, building on top of the existing SAML authentication. The implementation adds a two-tier access control mechanism using eduPersonEntitlement values for global access and SUNet ID mappings for per-application access.
Key Changes:
- Added authorization enforcement in middleware for dashboard and application routes
- Implemented API-level authorization helpers and wrappers for protecting API endpoints
- Created authorization utility functions for access control checks based on user entitlements and mappings
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| middleware.ts | Added authorization checks for dashboard and application routes, enforcing access rules based on user permissions |
| lib/auth-utils.ts | New authorization utilities including global access checks, per-application access validation, and environment variable parsing |
| lib/api-auth.ts | New API authorization helpers with withApiAuthorization wrapper for protecting API routes |
| app/applications/[uuid]/page.tsx | Added authorization error handling for API responses with user-friendly error displays |
| app/api/acquia/visits/route.ts | Wrapped API handler with withApiAuthorization for authentication enforcement |
| app/api/acquia/views/route.ts | Wrapped API handler with withApiAuthorization for authentication enforcement |
| app/api/acquia/applications/route.ts | Wrapped API handler with withApiAuthorization for authentication enforcement |
| app/api/auth/logout/route.ts | Added support for redirect parameter to allow redirecting to specific pages after logout |
| docs/SAML.md | Added comprehensive documentation for the authorization system including configuration examples and access control flow |
| .github/copilot-instructions.md | Updated with authorization system details and usage patterns |
| README.md | Added authorization environment variables documentation |
| .env.example | Added example authorization configuration variables |
| .gitignore | Added .cache directory to gitignore |
| app/auth/test/page.tsx | Updated logout link to redirect back to test page |
Comments suppressed due to low confidence (2)
lib/auth-utils.ts:89
- Missing newline between function declarations. This appears to be a formatting issue where the closing brace and JSDoc comment are merged together without proper spacing.
const response = await fetch('/api/auth/status')
lib/auth-utils.ts:127
- Missing newline between function declarations. This appears to be a formatting issue where the closing brace and JSDoc comment are merged together without proper spacing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { withApiAuthorization } from '@/lib/api-auth'; | ||
|
|
||
| export async function GET(request: NextRequest) { | ||
| return withApiAuthorization(async (request: NextRequest, context: { user: any }) => { |
There was a problem hiding this comment.
Inconsistent indentation. The code inside the withApiAuthorization callback is not properly indented. The block from line 7 onwards should be indented to match the callback function nesting level for better code readability.
| return { | ||
| authorized: false, | ||
| user, | ||
| error: `Access denied. You do not have permission to access application ${applicationUuid}.` |
There was a problem hiding this comment.
Exposing the application UUID in the error message could be a security concern. Users who don't have access to an application shouldn't necessarily see its internal UUID. Consider using a more generic error message like "Access denied. You do not have permission to access this application."
| error: `Access denied. You do not have permission to access application ${applicationUuid}.` | |
| error: 'Access denied. You do not have permission to access this application.' |
| import { withApiAuthorization } from '@/lib/api-auth'; | ||
|
|
||
| export async function GET(request: NextRequest) { | ||
| return withApiAuthorization(async (request: NextRequest, context: { user: any }) => { |
There was a problem hiding this comment.
The API is protected with withApiAuthorization but doesn't verify that the user has access to the specific application identified by subscriptionUuid. This means any authenticated user can access data for any application. Consider using requireApplicationUuid: true option or manually checking application access using the subscriptionUuid parameter.
| return withApiAuthorization(async (request: NextRequest, context: { user: any }) => { | ||
| const searchParams = request.nextUrl.searchParams; | ||
| const subscriptionUuid = searchParams.get('subscriptionUuid'); | ||
| const from = searchParams.get('from'); | ||
| const to = searchParams.get('to'); | ||
| const resolution = searchParams.get('resolution'); // Get granularity for daily data | ||
| /** | ||
| console.log('🚀 Views by Application API Route called with params:', { | ||
| subscriptionUuid, | ||
| from, | ||
| to, | ||
| resolution | ||
| }); | ||
| */ | ||
| if (!subscriptionUuid) { | ||
| console.error('❌ Missing required parameter: subscriptionUuid'); | ||
| return NextResponse.json( | ||
| { error: 'subscriptionUuid is required' }, | ||
| { status: 400 } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation. The code inside the withApiAuthorization callback is not properly indented, making it harder to read and maintain. The entire block from line 7-86 should be indented to match the callback function nesting level.
| - Have full access to all API endpoints | ||
|
|
||
| #### Per-Application Access Users | ||
| - Can access dashboard (shows only their authorized applications) |
There was a problem hiding this comment.
The documentation states per-application users "Can access dashboard (shows only their authorized applications)", but the implementation in hasDashboardAccess function only grants access to users with global access. This means per-app users will receive a 403 error when trying to access the dashboard, contradicting what's documented here.
| - Can access dashboard (shows only their authorized applications) | |
| - Cannot access dashboard (will receive 403 Forbidden) |
| import { withApiAuthorization } from '@/lib/api-auth'; | ||
|
|
||
| export async function GET(request: NextRequest) { | ||
| return withApiAuthorization(async (request: NextRequest, context: { user: any }) => { |
There was a problem hiding this comment.
The applications API returns all applications for any authenticated user without filtering based on their permissions. Users with per-application access should only see applications they're authorized to access. The user context from withApiAuthorization is available but not being used to filter results. Consider filtering the applications list based on the user's permissions using getUserApplicationAccess() or hasApplicationAccess().
| // Validate that redirect is to a safe path (starts with /) | ||
| if (redirectTo.startsWith('/')) { | ||
| return NextResponse.redirect(new URL(redirectTo, baseUrl)) |
There was a problem hiding this comment.
The redirect validation only checks if the path starts with '/', but this is insufficient to prevent open redirect vulnerabilities. An attacker could use URLs like '//evil.com' or '/\evil.com' which start with '/' but redirect to external sites. Use URL parsing and validate that the hostname matches the expected domain, or use a whitelist of allowed redirect paths.
| // Validate that redirect is to a safe path (starts with /) | |
| if (redirectTo.startsWith('/')) { | |
| return NextResponse.redirect(new URL(redirectTo, baseUrl)) | |
| // Validate that redirectTo is a safe, relative path (no protocol, no hostname, no double slashes, no backslashes) | |
| try { | |
| // Only allow relative paths that start with a single slash and do not contain suspicious patterns | |
| if ( | |
| redirectTo.startsWith('/') && | |
| !redirectTo.startsWith('//') && | |
| !redirectTo.startsWith('/\\') && | |
| !redirectTo.includes('\\') && | |
| !redirectTo.includes('..') // Prevent directory traversal | |
| ) { | |
| const url = new URL(redirectTo, baseUrl) | |
| // Ensure the redirect stays on the same origin | |
| if (url.origin === baseUrl) { | |
| return NextResponse.redirect(url) | |
| } | |
| } | |
| } catch (e) { | |
| // Ignore invalid URLs and fall through to default redirect |
| **Key Components** (`lib/auth-utils.ts`): | ||
| - `hasGlobalAccess(user)` - Check if user has global entitlement | ||
| - `hasApplicationAccess(user, uuid)` - Check specific app access | ||
| - `hasDashboardAccess(user)` - Check if can access dashboard (global OR any app) |
There was a problem hiding this comment.
The documentation states that dashboard access requires "global OR any app" access, but the implementation in hasDashboardAccess only grants access to users with global access. This inconsistency means per-application users will be denied dashboard access despite documentation stating they should have it.
Either update the implementation to allow per-app users to access the dashboard (showing only their authorized applications), or update the documentation to accurately reflect that only global access users can access the dashboard.
| - `hasDashboardAccess(user)` - Check if can access dashboard (global OR any app) | |
| - `hasDashboardAccess(user)` - Returns `true` if the user has global access **or** access to at least one application. Per-application users will see only their authorized applications on the dashboard. |
| requestHeaders.set('x-user-id', payload.id); | ||
| if (payload.sunetId) { | ||
| requestHeaders.set('x-user-sunetid', payload.sunetId); | ||
| requestHeaders.set('x-user-id', user.id); |
There was a problem hiding this comment.
The error message says "access this application" but this is the dashboard route, not an application. The error message should be "You do not have permission to access the dashboard." to be more accurate and helpful to users.
| * @param request NextRequest object | ||
| * @param applicationUuid Application UUID to check access for | ||
| * @returns Promise<{authorized: boolean, user: SamlUser | null, error?: string}> | ||
| */ | ||
| export async function checkApplicationApiAuthorization( | ||
| request: NextRequest, |
There was a problem hiding this comment.
The request parameter is unused in this function. Since this is an authorization check based on user identity from cookies, the request parameter is not needed and can be removed from the function signature.
| * @param request NextRequest object | |
| * @param applicationUuid Application UUID to check access for | |
| * @returns Promise<{authorized: boolean, user: SamlUser | null, error?: string}> | |
| */ | |
| export async function checkApplicationApiAuthorization( | |
| request: NextRequest, | |
| * @param applicationUuid Application UUID to check access for | |
| * @returns Promise<{authorized: boolean, user: SamlUser | null, error?: string}> | |
| */ | |
| export async function checkApplicationApiAuthorization( |
|
Closing this because I created it before necessary changes were made and the Copilot automatic review is just cluttering things up. |
1 participant
NOT READY FOR REVIEW
Summary
Review By (Date)
Criticality
Review Tasks
Setup tasks and/or behavior to test
Front End Validation
Backend / Functional Validation
Code
Code security
General
Affected Projects or Products
Associated Issues and/or People
@mentionthem here)Resources