Skip to content

Fix ADFS tests for id4slab1 lab migration.#5810

Open
RyAuld wants to merge 6 commits intomainfrom
ryauld/ADFS-ReEnable
Open

Fix ADFS tests for id4slab1 lab migration.#5810
RyAuld wants to merge 6 commits intomainfrom
ryauld/ADFS-ReEnable

Conversation

@RyAuld
Copy link
Copy Markdown
Contributor

@RyAuld RyAuld commented Mar 4, 2026

Re-enable ADFS tests for id4slab1 lab migration

Migrates all ADFS integration tests from the retired lab to fs.id4slab1.com and fixes every failure uncovered once they were un-skipped.


Background

ADFS tests have been gated by the IGNORE_FEDERATED compile symbol, which is controlled by the MSAL_SKIP_FEDERATED_TESTS MSBuild variable. That variable is set to "True" as a permanent pipeline-level default in ADO pipeline #905 (.NET MSAL PR (YAML)). Changing it would affect all concurrent PR builds, so it was not touched. Instead, the condition in tests/Directory.Build.props was temporarily commented out during development to force tests to run locally and in a validation build, then restored to match main for merge (see commit fd4eda5e5).


Commits

93bd19efc — Re-enable ADFS tests for id4slab1 migration

  • Updated ADFSAuthority constant to https://fs.id4slab1.com/adfs
  • Removed dead Adfs2019LabConstants class
  • Temporarily commented out the MSAL_SKIP_FEDERATED_TESTS prop in tests/Directory.Build.props to force ADFS tests to run in CI regardless of the pipeline variable (see validation note below)

b480f881f — Fix PopWhithAdfsUserAndBroker_Async cache environment

  • Fixed the cache environment reference for the PoP+ADFS+Broker test after the authority migration

8658c75a2 — Fix ADFS interactive Selenium test failures

  • UserInformationFieldIds.DetermineFieldIds: changed UserType string comparison to OrdinalIgnoreCase. KeyVault stores "federated" (lowercase) but LabConstants.UserTypeFederated is "Federated" — the case-sensitive == fell through to AAD field IDs on the ADFS page.
  • InteractiveFlowTests.RunTestForUserAsync: switched to FindFreeLocalhostRedirectUri() for the federated-path test to avoid an HttpListenerException port conflict with Interactive_Adfs_DirectAsync on the hardcoded localhost:52073.

45c22049e — Fix all ADFS test failures for id4slab1 lab migration

  • ClientCredentialsTests.NetFwk.cs: fixed JWT audience check using Contains("/adfs/") which fails when the authority has no trailing slash — changed to Contains("/adfs"). Regression introduced in a51b7f68a (Jan 2026), never caught because IGNORE_FEDERATED was still active. Also required server-side fixes on ADDC1 (cert added to LocalMachine\Root, JWTSigningKey configured).
  • UsernamePasswordIntegrationTests.NetFwk.cs and InteractiveFlowTests.NetFwk.cs: switched from AppPCAClient (a ServerApplication which requires client auth) to new AppAdfsNativeClient. ADFS public client flows (ROPC, interactive) require a NativeClientApplication registration — ServerApplications reject them with MSIS9622.
  • KeyVaultSecrets.cs: added AppAdfsNativeClient = "App-AdfsNativeClient-Config" constant.
  • Server-side on ADDC1 (permanent, outside this repo): registered MSAL-Lab-Client-Native (c697bd8e-16d8-4f73-97d8-262e446581c2) in the MSAL-Lab-Tests group; created KV secret App-AdfsNativeClient-Config in id4skeyvault.

fd4eda5e5 — Restore tests/Directory.Build.props to match main

  • Restored the MSAL_SKIP_FEDERATED_TESTS == 'True' condition in tests/Directory.Build.props to exactly match origin/main, so the pipeline variable behaviour is unchanged post-merge.

Testing

All 11 ADFS tests were validated locally (with the props condition temporarily disabled) and in ADO build #1601983 where they all passed:

Test Framework Result
Interactive_Adfs_FederatedAsync NetCore ✅ Passed
Interactive_Adfs_DirectAsync NetCore ✅ Passed
ROPC_ADFSv4Federated_Async NetCore ✅ Passed
AcquireTokenFromAdfsUsernamePasswordAsync NetCore ✅ Passed
WithCertificate_TestAsync (Adfs,NetFx) NetFx ✅ Passed
WithCertificate_TestAsync (Adfs,NetCore) NetCore ✅ Passed
WithClientAssertion_Manual_TestAsync (Adfs,NetCore) NetCore ✅ Passed
WithClientAssertion_Wilson_TestAsync (Adfs,NetFx) NetFx ✅ Passed
WithSecret_TestAsync (Adfs,NetFx) NetFx ✅ Passed
WithClientClaims_OverrideClaims_TestAsync (Adfs,NetCore) NetCore ✅ Passed
WithClientClaims_SendX5C_OverrideClaims_TestAsync (Adfs,NetCore) NetCore ✅ Passed

Build #1602009 (triggered by final commit fd4eda5e5) shows tests skipped — expected, because the props file now respects the pipeline-level MSAL_SKIP_FEDERATED_TESTS=True variable again.


Post-merge action required

To make ADFS tests run by default in all future PR builds, change the ADO pipeline variable after merging:

  1. Go to https://identitydivision.visualstudio.com/IDDP/_build?definitionId=905
  2. Click EditVariables
  3. Change MSAL_SKIP_FEDERATED_TESTS from TrueFalse
  4. Click Save

Individual builds can still override this at queue time since "Allow override at queue time" is enabled on the variable.

- Comment out MSAL_SKIP_FEDERATED_TESTS prop in Directory.Build.props so ADFS tests always run regardless of pipeline variable group
- Update ADFSAuthority constant to point to fs.id4slab1.com (new lab)
- Remove unused Adfs2019LabConstants class (no references in codebase)
@RyAuld RyAuld requested a review from a team as a code owner March 4, 2026 23:06
Copy link
Copy Markdown
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

Don't change the props

RyAuld added 3 commits March 5, 2026 13:44
- UserInformationFieldIds.DetermineFieldIds: use OrdinalIgnoreCase for
  UserType comparisons. KeyVault stores 'federated' (lowercase) but
  LabConstants.UserTypeFederated is 'Federated', causing the case-sensitive
  == to fall through to AAD field IDs on an ADFS page (root cause).

- InteractiveFlowTests.RunTestForUserAsync: use FindFreeLocalhostRedirectUri()
  for the non-direct path to avoid HttpListenerException port conflict with
  Interactive_Adfs_DirectAsync on localhost:52073. AAD public client apps
  accept any http://localhost:{port} so a dynamic port is valid here.
ClientCredentialsTests.NetFwk.cs:
- Fix audience check: Contains('/adfs/') fails for authority without trailing
  slash. Changed to Contains('/adfs'). Bug introduced in a51b7f6 (Avery-Dunn,
  Jan 15 2026), never caught because IGNORE_FEDERATED was still gating tests.
  Also required server-side fixes on ADDC1 (cert in Root store + JWTSigningKey).

UsernamePasswordIntegrationTests.NetFwk.cs:
- Use AppAdfsNativeClient (ADFS NativeClientApplication GUID) instead of
  AppPCAClient (ServerApplication GUID). ADFS ServerApplications require client
  auth; public client ROPC flows need a NativeClientApplication registration.

InteractiveFlowTests.NetFwk.cs:
- Same AppAdfsNativeClient fix for Interactive_Adfs_DirectAsync.

KeyVaultSecrets.cs:
- Add AppAdfsNativeClient = 'App-AdfsNativeClient-Config' constant pointing to
  NativeClientApplication (c697bd8e-16d8-4f73-97d8-262e446581c2) registered
  in MSAL-Lab-Tests group on ADDC1.

SeleniumExtensions.cs / UserInformationFieldIds.cs:
- Simplify EnterPassword: remove redundant ADFS fallback logic (now handled
  upstream by DetermineFieldIds with OrdinalIgnoreCase comparison).

Server-side changes on ADDC1 (permanent, not code):
- NativeClientApplication 'MSAL-Lab-Client-Native' registered in MSAL-Lab-Tests
- KV secret 'App-AdfsNativeClient-Config' created in id4skeyvault

All 11 ADFS tests now pass locally (8 on NetCore, 3 on NetFx).
Copilot AI review requested due to automatic review settings March 6, 2026 21:40
Copy link
Copy Markdown
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 re-enables ADFS integration tests that were being silently skipped due to a pipeline variable (MSAL_SKIP_FEDERATED_TESTS=True). It migrates all ADFS lab references from the old msidlab8 lab to the new id4slab1 lab, updates app configurations to use the correct ADFS-specific native client secret, and applies several bug fixes to ensure tests work correctly in the new lab environment.

Changes:

  • Migrated ADFS authority/environment references from fs.msidlab8.com to fs.id4slab1.com across test constants and test files, and removed the unused Adfs2019LabConstants class.
  • Changed Directory.Build.props to prevent IGNORE_FEDERATED from being defined when the pipeline variable is True, re-enabling all ADFS tests.
  • Fixed test infrastructure: added AppAdfsNativeClient key vault secret, switched ADFS-direct tests to the correct app config, used dynamic ports to avoid parallel test conflicts, fixed string comparisons to case-insensitive, and corrected /adfs//adfs Contains checks to match the new authority format (no trailing slash).

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/Directory.Build.props Changed condition from == 'True' to == 'False' to prevent IGNORE_FEDERATED from being defined when the pipeline variable is True
tests/Microsoft.Identity.Test.Common/TestConstants.cs Updated ADFSAuthority to id4slab1; removed unused Adfs2019LabConstants class
tests/Microsoft.Identity.Test.Unit/pop/PoPTests.cs Updated cache environment from fs.msidlab8.com to fs.id4slab1.com
tests/Microsoft.Identity.Test.LabInfrastructure/KeyVaultSecrets.cs Added AppAdfsNativeClient key vault secret constant
tests/Microsoft.Identity.Test.Integration.netcore/SeleniumTests/InteractiveFlowTests.NetFwk.cs Switched ADFS direct test to AppAdfsNativeClient; used dynamic port for AAD path to avoid parallel test conflicts
tests/Microsoft.Identity.Test.Integration.netcore/Infrastructure/UserInformationFieldIds.cs Changed == to string.Equals with OrdinalIgnoreCase for user type comparison
tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/UsernamePasswordIntegrationTests.NetFwk.cs Switched ADFS username/password test to AppAdfsNativeClient
tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/ClientCredentialsTests.NetFwk.cs Fixed .Contains("/adfs/") to .Contains("/adfs") to match new authority format without trailing slash

You can also share your feedback on Copilot code review. Take the survey.

Copilot AI review requested due to automatic review settings March 6, 2026 21:57
@RyAuld RyAuld force-pushed the ryauld/ADFS-ReEnable branch from cb497a6 to 043895c Compare March 6, 2026 22:01
Copy link
Copy Markdown
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

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.


You can also share your feedback on Copilot code review. Take the survey.

@RyAuld RyAuld force-pushed the ryauld/ADFS-ReEnable branch from 043895c to fd4eda5 Compare March 6, 2026 22:30
@RyAuld RyAuld changed the title Re-enable ADFS tests for id4slab1 lab migration Fix ADFS tests for id4slab1 lab migration. Mar 6, 2026
@RyAuld
Copy link
Copy Markdown
Contributor Author

RyAuld commented Mar 9, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@RyAuld RyAuld requested a review from bgavrilMS March 9, 2026 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants