Skip to content

Conversation

@ccastrotrejo
Copy link
Contributor

@ccastrotrejo ccastrotrejo commented Dec 8, 2025

Commit Type

  • feature - New functionality
  • fix - Bug fix
  • refactor - Code restructuring without behavior change
  • perf - Performance improvement
  • docs - Documentation update
  • test - Test-related changes
  • chore - Maintenance/tooling

Risk Level

  • Low - Minor changes, limited scope
  • Medium - Moderate changes, some user impact
  • High - Major changes, significant user/system impact

What & Why

This PR adds support for dynamic authentication providers in the chat iframe application. Previously, the authentication was hardcoded to use Azure AD (Microsoft Entra ID). Now, the application can dynamically display and support multiple identity providers configured via Azure App Service Easy Auth (e.g., Microsoft, Google, Facebook, GitHub).

Key changes:

  • Dynamic identity provider configuration via window.IDENTITY_PROVIDERS or fallback to default providers
  • Authentication status checking on app initialization to skip login prompt for already authenticated users
  • Multiple sign-in buttons in the login prompt, one for each configured provider
  • Improved UX with loading states per provider button

Impact of Change

  • Users:

    • Users will see multiple sign-in options if multiple identity providers are configured
    • Better UX with authentication status check - no unnecessary login prompts for authenticated users
    • Clear feedback about which provider they're signing in with
  • Developers:

    • New IdentityProvider interface for defining authentication providers
    • LoginPrompt component now accepts identityProviders prop
    • openLoginPopup now requires signInEndpoint parameter
    • New checkAuthStatus function for checking authentication state
  • System:

    • Authentication flow now supports any Easy Auth provider (Microsoft, Google, Facebook, GitHub, custom)
    • App checks /.auth/me endpoint on initialization to determine authentication status
    • Configuration can be injected via window.IDENTITY_PROVIDERS or falls back to default providers

Test Plan

  • Unit tests added/updated
    • Updated LoginPrompt component tests to handle multiple providers
    • Updated authHandler tests to include signInEndpoint parameter
    • Updated IframeWrapper tests to mock checkAuthStatus
    • Updated config-parser tests to verify identity provider configuration
  • E2E tests added/updated
    • Updated all E2E authentication tests to use provider-specific button names
    • Tests cover login prompt display, popup flow, and authentication success
  • Manual testing completed
  • Tested in: Local development environment with mock authentication

Contributors

@ccastrotrejo

Screenshots/Videos

Multiple identity providers

CleanShot 2025-12-08 at 17 48 12@2x

No identity providers in logic app setup

CleanShot 2025-12-09 at 15 19 22@2x

@github-actions
Copy link

github-actions bot commented Dec 8, 2025

🤖 AI PR Validation Report

PR Review Results

Thank you for your submission! Here's detailed feedback on your PR title and body compliance:

PR Title

  • Current: feat(iframe-app): Add dynamic authentication provider support for chat iframe app
  • Issue: None — title is clear, follows conventional commit style, and describes the change area and intent.
  • Recommendation: Keep as-is.

Commit Type

  • Properly selected (feature).
  • Only one option selected which is correct.

Risk Level

  • The PR body selects Medium and there is a label present Risk:Medium on the PR — these match.
  • Assessment: Correct selection and label present.

What & Why

  • Current: This PR adds dynamic authentication provider support to the chat iframe app and explains the key changes.
  • Issue: None functional in the description itself.
  • Recommendation: Consider adding a short explicit "Breaking / Migration" subsection summarizing the API signature and types that changed (see below). Example recommendation text to add:
    • "Breaking / Migration: openLoginPopup signature now accepts signInEndpoint; checkAuthStatus return type changed to { isAuthenticated, error }; ChatWidgetProps and libs export new IdentityProvider type. Consumers must update call-sites and imports accordingly."

⚠️ Impact of Change

  • The PR includes Users / Developers / System sections. Good coverage.
  • Issue: The Developer impact describes new interfaces but does not explicitly call out the migration steps that consumers of the library will need to take.
  • Recommendation: Add a short migration guide to the PR body and CHANGELOG that lists the concrete API changes and the code examples to update:
    • Users: No additional action required beyond the improved sign-in UX.
    • Developers: Document these items explicitly in the PR body and CHANGELOG:
      • openLoginPopup signature now accepts signInEndpoint?: string (and callers should pass signInEndpoint when using providers other than the default AAD). Provide example call sites before/after.
      • checkAuthStatus now returns { isAuthenticated: boolean; error: Error | null } instead of boolean — update consumers accordingly.
      • ChatWidgetProps / component props now include identityProviders?: Record<string, IdentityProvider> — update any consumers of the ChatWidget or wrapper components.
      • New exported type IdentityProvider was added to libs — update imports where necessary.
    • System: Consider calling out that authentication flows and tests were updated; mention any necessary environment configuration (Easy Auth providers, window.IDENTITY_PROVIDERS usage) and where to configure it for CI/e2e.

Test Plan

  • Test plan is comprehensive: unit, E2E, and manual testing boxes checked and numerous tests updated/added across unit and e2e suites.
  • Recommendation: Ensure the PR body (or a linked testing plan) states which CI jobs should be run and if any flaky tests are expected during migration.

⚠️ Contributors

  • The PR lists @ccastrotrejo. Good.
  • Recommendation: If PMs, Designers, or additional reviewers contributed, optionally mention them for credits. Not required to pass, but recommended for visibility.

Screenshots/Videos

  • Provided — relevant screenshots included for the new UI states.

Summary Table

Section Status Recommendation
Title Keep as-is.
Commit Type No change needed.
Risk Level Label matches body.
What & Why Add a brief "Breaking / Migration" note.
Impact of Change ⚠️ Add explicit migration steps for developers.
Test Plan Ensure CI job guidance included if needed.
Contributors ⚠️ Optionally credit others (PM/Design/Reviewers).
Screenshots/Videos Good — no change.

Final notes and required actions

  • Overall: PASS — the PR body and title comply with the template and are well-documented. The label and risk selection match the change footprint.
  • Important recommendation (please include in PR body and CHANGELOG): this change modifies authentication APIs and types used by other parts of the codebase and consumers (notably openLoginPopup signature, checkAuthStatus return type, and ChatWidgetProps adding identityProviders). Please add a short "Breaking / Migration" section that lists the exact code changes consumers must make. Example items to include in that section:
    • openLoginPopup: previously called as openLoginPopup({ baseUrl, ... }) — update to pass signInEndpoint when using non-AAD providers: openLoginPopup({ baseUrl, signInEndpoint: '/.auth/login/google', ... }).
    • checkAuthStatus return: update code that awaited a boolean to now destructure { isAuthenticated, error }.
    • ChatWidgetProps / exported types: update imports to include the new IdentityProvider type and to pass identityProviders where applicable.
  • Security & review: Because this changes authentication flow and e2e mocks, please request a quick security review and run the auth-related end-to-end CI pipelines. Call out any environment variables/config required for Easy Auth provider testing in the PR or repository docs.

Please update the PR body with the migration note and any guidance for consumers, then re-submit or add a follow-up commit. Thank you for the thorough tests and clear description — this looks well-developed and ready pending the small documentation additions.


Last updated: Tue, 09 Dec 2025 20:25:52 GMT

@ccastrotrejo ccastrotrejo changed the title Ccastrotrejo/dynamic auth feat: Add dynamic authentication provider support for chat iframe app Dec 8, 2025
@ccastrotrejo ccastrotrejo changed the title feat: Add dynamic authentication provider support for chat iframe app feat(chat): Add dynamic authentication provider support for chat iframe app Dec 8, 2025
@ccastrotrejo ccastrotrejo added the risk:medium Medium risk change with potential impact label Dec 8, 2025
@ccastrotrejo ccastrotrejo marked this pull request as ready for review December 9, 2025 19:15
Copilot AI review requested due to automatic review settings December 9, 2025 19:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds dynamic authentication provider support to the chat iframe application, replacing the hardcoded Azure AD authentication with a configurable system that supports multiple identity providers (Microsoft, Google, Facebook, GitHub) via Azure App Service Easy Auth. The changes include a new authentication status check on app initialization to skip login prompts for already-authenticated users, and a redesigned LoginPrompt component that displays multiple sign-in buttons when multiple providers are configured.

Key changes:

  • New IdentityProvider interface and type exports across libs/a2a-core for provider configuration
  • Enhanced authentication flow with dynamic provider endpoints and auth status checking
  • Updated LoginPrompt component with per-provider loading states and configuration messaging

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
libs/a2a-core/src/react/types/index.ts Added IdentityProvider to type exports and ChatWidgetProps interface
libs/a2a-core/src/react/index.ts Exported IdentityProvider type from react module
libs/a2a-core/src/client/types.ts Defined IdentityProvider interface with signInEndpoint and name properties
libs/a2a-core/src/client/index.ts Exported IdentityProvider type from client module
apps/iframe-app/src/lib/utils/config-parser.ts Added default identity providers configuration and window.IDENTITY_PROVIDERS support
apps/iframe-app/src/lib/utils/tests/config-parser.test.ts Added tests for identity provider configuration parsing
apps/iframe-app/src/lib/utils/tests/config-parser.contextId.test.ts Updated test to verify removed console.log statements
apps/iframe-app/src/lib/authHandler.ts Added signInEndpoint parameter to openLoginPopup, updated login URL construction, removed verbose logging
apps/iframe-app/src/lib/tests/authHandler.test.ts Updated tests to include signInEndpoint parameter and test multiple providers
apps/iframe-app/src/components/LoginPrompt/LoginPrompt.tsx Redesigned to support multiple providers with individual loading states and configuration messaging
apps/iframe-app/src/components/LoginPrompt/LoginPromptStyles.ts Updated styles for multi-provider layout with renamed messageBar class
apps/iframe-app/src/components/tests/LoginPrompt.spec.tsx Comprehensive test updates for multi-provider support and edge cases
apps/iframe-app/src/components/IframeWrapper.tsx Added authentication check on mount with loading state, updated login handler to accept provider
apps/iframe-app/src/components/tests/IframeWrapper.test.tsx Mocked checkAuthStatus and updated tests to await auth check completion
apps/iframe-app/src/components/tests/IframeWrapper.contextId.test.tsx Updated tests to mock auth check and handle async rendering
e2e/chatClient/tests/smoke/page-load.spec.ts Added .auth/me mock to return authenticated user
e2e/chatClient/tests/smoke/basic-chat.spec.ts Added .auth/me mock across all test suites
e2e/chatClient/tests/features/ui/edge-cases.spec.ts Added .auth/me mock to bypass login prompts in tests
e2e/chatClient/tests/features/ui/accessibility.spec.ts Added .auth/me mock for authenticated test scenarios
e2e/chatClient/tests/features/sessions/session-management.spec.ts Added .auth/me mock to test authenticated session flows
e2e/chatClient/tests/features/sessions/multi-session.spec.ts Added .auth/me mock across multi-session test suites
e2e/chatClient/tests/features/reliability/network-connectivity.spec.ts Added .auth/me mock to focus tests on network reliability
e2e/chatClient/tests/features/reliability/error-handling.spec.ts Added .auth/me mock to test error scenarios with authenticated users
e2e/chatClient/tests/features/messaging/input-validation.spec.ts Added .auth/me mock for authenticated messaging tests
e2e/chatClient/tests/features/messaging/complete-flow.spec.ts Added .auth/me mock to test complete chat flows
e2e/chatClient/tests/features/authentication/login-prompt.spec.ts Updated all tests to use provider-specific button names (Microsoft account)
e2e/chatClient/fixtures/sse-fixtures.ts Added .auth/me mock to SSE test fixtures for consistent authentication state

@ccastrotrejo ccastrotrejo changed the title feat(chat): Add dynamic authentication provider support for chat iframe app feat(iframe-app): Add dynamic authentication provider support for chat iframe app Dec 9, 2025
Copy link
Collaborator

@hartra344 hartra344 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed. The PR is well-structured with excellent test coverage.

Highlights:

  • Good separation of concerns with auth logic in authHandler.ts
  • Comprehensive unit tests covering auth flows and edge cases
  • Clean TypeScript interfaces for IdentityProvider

Minor suggestions for future consideration:

  • Consider clearing loadingProviderKeyRef when login completes to prevent stale UI
  • Add cleanup/abort mechanism in auth check useEffect for race condition safety

Overall, solid implementation. ✅

@ccastrotrejo ccastrotrejo merged commit aed9250 into main Dec 9, 2025
14 checks passed
@ccastrotrejo ccastrotrejo deleted the ccastrotrejo/dynamicAuth branch December 9, 2025 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-validated risk:medium Medium risk change with potential impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants