refactor and v1.4.0#61
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the privacyIDEA ADFS provider’s client + adapter flow and modernizes the test suite. It introduces a per-request context object, reworks HTTP/JWT handling, updates the presentation HTML/JS for enrollment/passkey flows, and replaces the previous monolithic tests with fixture-driven parser tests plus WireMock integration tests.
Changes:
- Refactor
PrivacyIDEAclient: switch request plumbing, add JWT caching, introducePIRequestContext, add cancel-enrollment call, and make realm mapping case-insensitive. - Update adapter + UI: new tokenized HTML templating, add “Not now” for optional enroll-via-multichallenge, and streamline challenge state handling.
- Modernize tests: move to .NET 8 test project with updated packages, add fixtures + parser/integration coverage for OTP/push/WebAuthn/passkey/JWT caching/realm mapping.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/Tests.csproj | Bumps test TFMs/packages to .NET 8 and updates test dependencies. |
| Tests/TestHelpers.cs | Adds shared helpers for WireMock client setup and JWT/auth response generation. |
| Tests/PrivacyIDEAClientTests.cs | Removes old combined test class (replaced by focused suites). |
| Tests/ParserTests/WebAuthnParsingTests.cs | Adds parser-level coverage for WebAuthn parsing + merge behavior. |
| Tests/ParserTests/PushParsingTests.cs | Adds parser-level coverage for push challenge parsing helpers. |
| Tests/ParserTests/PasskeyParsingTests.cs | Adds parser-level coverage for passkey init/auth/registration parsing. |
| Tests/ParserTests/OtpValidationParsingTests.cs | Adds parser-level coverage for /validate/check ACCEPT/REJECT/CHALLENGE/error + preferred_client_mode translation. |
| Tests/ParserTests/MultichallengeEnrollParsingTests.cs | Adds parser-level coverage for enroll-via-multichallenge parsing (images/links/optional flag/cancel responses). |
| Tests/IntegrationTests/WebAuthnTests.cs | Adds WireMock integration tests for ValidateCheckWebAuthn (Origin header + parameter encoding rules + short-circuit). |
| Tests/IntegrationTests/RealmMappingTests.cs | Adds integration tests for realm mapping precedence and case sensitivity behavior. |
| Tests/IntegrationTests/PushPollTests.cs | Adds integration tests for /validate/polltransaction request shape + behavior. |
| Tests/IntegrationTests/PasskeyTests.cs | Adds integration tests for passkey initialize/check (Origin header + short-circuit). |
| Tests/IntegrationTests/OtpValidationTests.cs | Adds integration tests for ValidateCheck request body shape and URL encoding behavior. |
| Tests/IntegrationTests/JwtCachingTests.cs | Adds integration tests for JWT caching behavior based on exp/fallback windows. |
| Tests/IntegrationTests/CancelEnrollmentTests.cs | Adds integration tests for cancel_enrollment=True behavior and short-circuiting. |
| Tests/Fixtures/WebAuthnFixtures.cs | Adds JSON fixtures for WebAuthn challenge shapes. |
| Tests/Fixtures/PushFixtures.cs | Adds JSON fixtures for push challenge and poll transaction responses. |
| Tests/Fixtures/PasskeyFixtures.cs | Adds JSON fixtures for passkey init/auth/registration responses. |
| Tests/Fixtures/OtpFixtures.cs | Adds JSON fixtures for OTP ACCEPT/REJECT/CHALLENGE/error variants. |
| Tests/Fixtures/MultichallengeEnrollFixtures.cs | Adds JSON fixtures for enroll-via-multichallenge variants and cancel responses. |
| privacyIDEAADFSProvider/privacyIDEAADFSProvider.csproj | Includes new client source files in build. |
| privacyIDEAADFSProvider/PrivacyIDEA Client/RegistryReader.cs | Makes realm mapping dictionary case-insensitive and removes uppercasing. |
| privacyIDEAADFSProvider/PrivacyIDEA Client/PrivacyIDEA.cs | Major refactor: request sending, JWT caching, context plumbing, cancel enrollment, and parsing helpers refactor. |
| privacyIDEAADFSProvider/PrivacyIDEA Client/PIResponse.cs | Refactors transaction-id handling, adds EnrollmentOptional, improves parsing and sign-request merge logic. |
| privacyIDEAADFSProvider/PrivacyIDEA Client/PIRequestContext.cs | Introduces per-call context for domain/headers/custom parameters. |
| privacyIDEAADFSProvider/PrivacyIDEA Client/PIConstants.cs | Adds token type/client mode constants used by adapter/parser. |
| privacyIDEAADFSProvider/PrivacyIDEA Client/PIChallenge.cs | Simplifies challenge model (removes Attributes). |
| privacyIDEAADFSProvider/FormResult.cs | Adjusts submitted form model (enrollment cancellation flag). |
| privacyIDEAADFSProvider/Configuration.cs | Refactors registry config parsing and adds configurable log path. |
| privacyIDEAADFSProvider/AuthPage.html | Updates UI for enrollment modes, optional skip, and refactors JS for mode handling/encoding. |
| privacyIDEAADFSProvider/AdapterPresentationForm.cs | Refactors HTML templating to token map + regex substitution. |
| privacyIDEAADFSProvider/AdapterMetadata.cs | Refactors metadata to use static arrays/dictionaries and expands supported LCIDs. |
| privacyIDEAADFSProvider/Adapter.cs | Refactors request handling to use PIRequestContext, adds cancel enrollment flow, updates logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
Tests/Tests.csproj:20
- The test project targets net8.0 but has a ProjectReference to a .NET Framework 4.8 project (privacyIDEAADFSProvider.csproj). .NET SDK project references require compatible TFMs, so this configuration typically fails to build (net8.0 cannot reference net48). Consider targeting net48 for the test project (or multi-targeting with conditional references) so the tests can compile against the provider code.
<PropertyGroup>
<TargetFramework>net8.0</TargetFramework>
<IsPackable>false</IsPackable>
<LangVersion>latest</LangVersion>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.11.1" />
<PackageReference Include="MSTest.TestAdapter" Version="3.6.1" />
<PackageReference Include="MSTest.TestFramework" Version="3.6.1" />
<PackageReference Include="coverlet.collector" Version="6.0.2" />
<PackageReference Include="Newtonsoft.Json" Version="13.0.3" />
<PackageReference Include="WireMock.Net" Version="1.6.7" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\privacyIDEAADFSProvider\privacyIDEAADFSProvider.csproj" />
</ItemGroup>
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
enrollment offers can be declined.
Security: secret at rest
ciphertext.
first read, with a one-time event-log warning.
event if it finds an enc: value it can't decrypt; re-entering the password re-encrypts it locally.
not a boundary against a local admin, who on an ADFS server already controls the token-signing key.
GmbH\PrivacyIDEA-ADFS). After install only SYSTEM, Administrators, and the ADFS service account can read it. Re-run Install.ps1 if the
key is created after installation.
Installer & operational changes
(relevant on Server 2016/2019, which don't ship it).
FS/Admin). A source registered under a different log by an earlier version is moved to Application automatically; the source is removed
on uninstall (Uninstall.ps1).
Refactor / internals
RegistryReader.cs, AdapterMetadata.cs, AdapterPresentationForm.cs, Configuration.cs, AuthPage.html.
interactive, webauthn) as constants instead of scattered string literals.
Tests