Multiple Provider Scenario (1+ providers) #4
Conversation
This reverts commit e1b4c3c
constantine2nd
changed the title
| if (!!routeId && needsAuthorization(routeId)) { | ||
| logger.debug('Checking authorization for user route:', event.url.pathname); | ||
| if (!oauth2ProviderManager.isReady()) { | ||
| logger.warn('OAuth2 providers not ready:', oauth2ProviderManager.getStatus().error); |
There was a problem hiding this comment.
I think we should keep the bubble up of the error from the provider manager here
| email: string; | ||
| username: string; | ||
| entitlements: { | ||
| entitlements: { |
There was a problem hiding this comment.
Various unnecessary formatting changes, can leave them in this time I guess
| } | ||
|
|
||
| // Always return a response, even when there's no session | ||
| return await resolve(event); |
There was a problem hiding this comment.
just a formatting change
| const existingProvider = this.status.providers.find((p) => p.provider === definedProvider); | ||
| if (existingProvider) { | ||
| existingProvider.status = 'unavailable'; | ||
| existingProvider.error = 'Provider not configured in OBP API well-known endpoints'; |
There was a problem hiding this comment.
I guess it would be good to have a log message for all providers that we have strategies for in the portal but aren,t in the well-known endpoints. Like a log.warn. Could help debugging
| } catch (error: any) { | ||
| logger.error('Opey Auth error:', error); | ||
| return json({ error: error.message || 'Internal Server Error' }, { status: 500 }); | ||
| } |
There was a problem hiding this comment.
only formatting changes here, ignore
| return json( | ||
| { success: true, authenticated: true }, | ||
| setCookieHeaders ? { headers: { 'Set-Cookie': setCookieHeaders } } : {} | ||
| ); |
| {/if} | ||
| </div> | ||
| </div> | ||
| </div> |
| logger.error(`Error during ${provider} OAuth login:`, err); | ||
| throw error(500, 'Internal Server Error'); | ||
| } | ||
| } |
|
|
||
| // Redirect to the generic provider route for backward compatibility | ||
| logger.debug(`Redirecting to generic provider route: /login/${provider}`); | ||
| throw redirect(302, `/login/${provider}`); |
There was a problem hiding this comment.
I guess this is kept for backwards compatibility? But we may not really need it
| logger.error('Invalid user data received from OBP - missing user_id or email'); | ||
| throw error(400, 'Authentication failed - invalid user data'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Again, not sure if it is actually nessecary to keep this at all
There was a problem hiding this comment.
Looks good, we don't need to undo the formatting changes necessarily, but good to keep them out for next time with an ai.md or similar.
I was wondering because I think there are some tests configured for the oauth stuff already in the portal. Maybe we need to write new tests now...
3 participants
No description provided.