AuthHandshakeMessageHandlerTests are order-dependent through a process-wide static credential cache
- Dominant language
- C#
- Stars
- 3.2k
- Forks
- 1.3k
- PR merge metrics
- PR metrics pending
Description
## Summary
`AuthHandshakeMessageHandlerTests.Authenticate` in `test/Microsoft.NET.Build.Containers.UnitTests/AuthHandshakeMessageHandlerTests.cs` is order-dependent, and as a result most of its `[DynamicData]` rows never exercise the code path they were written for.
## Details
`AuthHandshakeMessageHandler` caches the negotiated `Authorization` header in a process-wide static, keyed by registry name:
```csharp
// src/Containers/Microsoft.NET.Build.Containers/AuthHandshakeMessageHandler.cs
private static ConcurrentDictionary _authenticationHeaders = new();
```
`SendAsync` consults it before issuing the request, and only performs the auth handshake when the server answers `401`.
Every `Authenticate` data row uses the same `TestRegistryName` (`registry.test`). The first row that completes a handshake populates the cache; every later row is then sent with that cached header, the fake server accepts it immediately, and the row never reads `REGISTRY_AUTH_FILE`, never parses the credential JSON, and never reaches the token endpoint it was written to verify.
This was found while parallelizing the assembly: removing the class-level `[DoNotParallelize]` reorders the rows, and the identity-token row then really did parse its config — and failed, because `ConfigAuthWithIdentityToken` was emitting invalid JSON (the `identitytoken` value was unquoted). The JSON is fixed in #TBD, but the row still does not reach the parse in practice, so the fix is unverified by execution.
## Impact
- Several `Authenticate` rows are effectively vacuous — they assert `HttpStatusCode.OK` for a request that was authorized by a previous row's cached header.
- The class must keep `[DoNotParallelize]`, so it is deferred to the serial tail of `Microsoft.NET.Build.Containers.UnitTests`, which runs at `MSTestParallelizeScope=MethodLevel`. A `[ResourceLock]` cannot replace it: a lock serializes tests but cannot reset the cache between them, and the cache is `private` with no way to clear it.
## Suggested fix
Give each data row its own registry name so each gets its own cache entry:
- have `GetAuthenticateTestData` yield the registry name alongside the auth config and the fake server,
- derive `ConfigAuthWith*`, `RequestUrl` and `BearerRealmUrl` from that per-row name instead of the `TestRegistryName` / `RequestUrl` / `BearerRealmUrl` consts the server helpers currently close over,
- assert that the challenge/token endpoint was actually reached, so a row that silently skips the handshake fails instead of passing.
With rows isolated, re-evaluate whether the class still needs `[DoNotParallelize]` or whether `[ResourceLock(WellKnownResources.EnvironmentVariables)]` on the two tests that mutate environment variables is sufficient.
## Notes
Alternatively (or additionally), consider whether `AuthHandshakeMessageHandler`'s credential cache should be instance state or an injectable dependency rather than a process-wide static — that is what makes it untestable in isolation.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with test/Microsoft.NET.Build.Containers.UnitTests/AuthHandshakeMessageHandlerTests.cs, especially Authenticate and GetAuthenticateTestData, then read src/Containers/Microsoft.NET.Build.Containers/AuthHandshakeMessageHandler.cs to understand the static cache. Isolate each data row's registry name and verify that its challenge and token endpoint are reached; then run the affected unit tests and reassess parallelization.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- authentication, testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 65/100