Update authentication for AIForOrcas Client UI - #544
Conversation
Moderators were reporting problems with the deployed instance. Trying to deploy a new one just resulted in changes not taking, so this PR makes a bunch of fixes/improvements. New Features * Added circuit-aware, server-side token handling and authentication state updates. * Added named authenticated/unauthenticated API client support. * Added request/response HTTP logging. * Improved identity-based username/display-name resolution. Bug Fixes * Safer email/name extraction when input is empty or missing. * Detection update failures now surface detailed HTTP error information. * Logout/sign-out now reliably clears circuit-scoped authentication state. Refactor * Updated services to use HTTP client factory and token injection instead of persisted browser tokens Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
This was a configuration issue in Azure which has now been fixed. |
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
There was a problem hiding this comment.
Reviewed the auth changes and built the client locally (0 errors). The core direction is a clear improvement: tokens are no longer persisted in browser localStorage, and the circuit-scoped server-side store with cleanup on circuit close is a sound design for Blazor Server. The IHttpClientFactory migration looks correct, and I found no leftover bare HttpClient injections. The pagination back-up fix in Candidates and the null guard in EmailHelper both look good.
A few non-blocking observations, none of which should hold up the merge given the staging testing already done:
-
ServerSideTokenStore.GetTokenrefreshes the 30-minute TTL on every read, so the store entry never expires while the session is active. However, the underlying AAD access token still expires (typically 60-75 minutes) and there is no mid-session refresh, so a long-lived session will eventually get 401s on authenticated calls until a page refresh opens a new circuit and silent re-auth runs. Is that a known limitation for now? -
In
ApiAuthenticationStateProvider,ParseClaimsFromJwtcatches internally and returns an empty list, so the outer catch that removes the bad token never fires. Sincenew ClaimsIdentity(claims, "jwt")sets an authentication type, a malformed token would produce a user withIsAuthenticated == trueand zero claims. Low risk since tokens come from MSAL, but worth tightening later by letting the parse failure propagate to the outer handler. -
AuthenticatedAPIandUnauthenticatedAPInamed clients currently have identical configuration, with the token applied manually per request viaApplyToken. ADelegatingHandlerregistered onAuthenticatedAPIwould centralize that and remove the per-callsite calls. Fine as is, just a possible follow-up. -
OnCircuitOpenedAsyncawaits the MSAL silent acquisition, which adds a JS interop and network round trip to every circuit connect. Since staging testing shows it working, no concern, just noting the latency cost.
Also, the PR description mentions request/response HTTP logging, but I did not spot it in the diff; the summary text may be from an earlier revision.
Approving. Happy to file follow-up issues for items 1-3 if useful.
Review performed with assistance from Claude Code (claude-fable-5); analysis and conclusions verified manually.
Moderators were reporting problems with the deployed instance. Trying to deploy a new one just resulted in changes not taking, so this PR makes a bunch of fixes/improvements.
New Features
Bug Fixes
Refactor