Configure an OIDC provider as test-only via RequireSyntheticPid (#1409)#1984
Merged
Conversation
Implements the authoritative synthetic-only gate from #1409 (referenced by #1983). A provider can be configured so that an ordinary national identity number can never be authenticated through it - the gate lives in the authentication component, not in the upstream IdP. - OidcProvider.RequireSyntheticPid (default false). When true, the provider may only authenticate synthetic (Tenor) test persons. - SyntheticPersonIdentifier.IsSyntheticTenor: positive, fail-closed check (11 digits, month 81-92, D-number day-40 normalization, mod11). Any ordinary fnr / real D-number fails unconditionally. - AuthenticationHelper.GetUserFromToken: when the selected provider has RequireSyntheticPid=true and the pid claim is not a well-formed synthetic fnr, the result is marked not authenticated (fail-closed), regardless of what the upstream IdP asserts. The gate applies only to a pid claim; other identity flows are unaffected. Set RequireSyntheticPid=true on test-only providers (e.g. mockporten) in OidcProviders config. Default behaviour is unchanged. Tests: SyntheticPersonIdentifier (ordinary fnr, real D-number, H-number, broken mod11, malformed input rejected; valid synthetic accepted) and GetUserFromToken gate behaviour. 17 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ChangesSynthetic PID enforcement in Altinn Authentication
Mockporten sample OIDC test IdP
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/Altinn.Platform.Authentication.Tests/SyntheticPersonIdentifierTests.cs (1)
15-43: ⚡ Quick winAdd a positive synthetic D-number test case.
The implementation has an explicit synthetic D-number normalization path, but this suite only proves that a real D-number is rejected. One valid synthetic D-number fixture would lock in that documented branch and make regressions there much easier to catch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/Altinn.Platform.Authentication.Tests/SyntheticPersonIdentifierTests.cs` around lines 15 - 43, Add a positive test case that asserts SyntheticPersonIdentifier.IsSyntheticTenor returns true for a valid synthetic D-number (a D-number where month is increased by 40, e.g. month 41-52) to cover the synthetic D-number normalization branch; update the test class SyntheticPersonIdentifierTests by adding a [Theory]/[InlineData] or [Fact] with a known-valid synthetic D-number fixture (ensure it passes mod11 and has 11 digits) so the explicit synthetic D-number path in SyntheticPersonIdentifier.IsSyntheticTenor is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Authentication/Helpers/SyntheticPersonIdentifier.cs`:
- Around line 49-57: The code currently only checks numeric ranges for
normalizedDay and normalizedMonth in SyntheticPersonIdentifier (variables
normalizedDay/normalizedMonth) then returns HasValidControlDigits(pid), which
allows impossible calendar dates (e.g., 31 Feb) to pass; update the validation
to construct the normalized birth date (using the computed normalizedDay,
normalizedMonth and the extracted year from the PID) and verify it represents a
real calendar date (accounting for leap years) before calling
HasValidControlDigits(pid) — if the date is invalid return false. Ensure you
reference/modify the same method where normalizedDay/normalizedMonth are
computed so the date check runs prior to returning true.
---
Nitpick comments:
In `@test/Altinn.Platform.Authentication.Tests/SyntheticPersonIdentifierTests.cs`:
- Around line 15-43: Add a positive test case that asserts
SyntheticPersonIdentifier.IsSyntheticTenor returns true for a valid synthetic
D-number (a D-number where month is increased by 40, e.g. month 41-52) to cover
the synthetic D-number normalization branch; update the test class
SyntheticPersonIdentifierTests by adding a [Theory]/[InlineData] or [Fact] with
a known-valid synthetic D-number fixture (ensure it passes mod11 and has 11
digits) so the explicit synthetic D-number path in
SyntheticPersonIdentifier.IsSyntheticTenor is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 539a6484-ba5e-45e0-b5e0-e953624eb6b8
📒 Files selected for processing (4)
src/Authentication/Helpers/AuthenticationHelper.cssrc/Authentication/Helpers/SyntheticPersonIdentifier.cssrc/Authentication/Model/OidcProvider.cstest/Altinn.Platform.Authentication.Tests/SyntheticPersonIdentifierTests.cs
Replace the hand-rolled mod11 / control-digit / D-number code with PersonIdentifier.TryParse from Altinn.Register.Contracts (already referenced) for fnr validity, and keep only the simple month-in-81-92 synthetic-marker check on top. Behaviour unchanged; 17 tests still green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The only new-code SonarCloud finding on this PR. Remaining reported issues/hotspot are pre-existing in AuthenticationHelper.cs (unrelated to this change) and the quality gate passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A RequireSyntheticPid provider now throws AuthenticationException when the pid is not a synthetic (Tenor) identifier, rather than returning a not-authenticated model. This guarantees the request aborts and cannot be mishandled by a caller that forgets to check IsAuthenticated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy the synthetic-only OIDC test provider (formerly the standalone IdProvider repo) into samples/mockporten, renamed to the Mockporten solution/namespace/assembly. Add SECURITY.md documenting why enabling it in production is safe: disjoint synthetic/real fodselsnummer ranges, the authoritative RequireSyntheticPid gate in Altinn Authentication, and the config-pinned OIDC trust (well-known endpoint + issuer). Scrub the App Insights connection string from appsettings.json and exclude samples/ from the Docker build context. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Implements a per-OIDC-provider “synthetic-only” authentication gate in Altinn Authentication, intended to make selected test IdPs (e.g. mockporten) only usable with Tenor synthetic PIDs even if the upstream IdP is compromised/misbehaving.
Changes:
- Added
OidcProvider.RequireSyntheticPidconfiguration flag and enforced it inAuthenticationHelper.GetUserFromToken. - Introduced
SyntheticPersonIdentifier.IsSyntheticTenorto validate Tenor synthetic PID format (viaPersonIdentifier.TryParse+ month 81–92). - Added unit tests for the synthetic PID validator and the provider gate; added a
samples/mockportenTest-IDP sample project and documentation.
Reviewed changes
Copilot reviewed 61 out of 63 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Altinn.Platform.Authentication.Tests/SyntheticPersonIdentifierTests.cs | Unit tests for Tenor PID detection and provider gating behavior. |
| src/Authentication/Model/OidcProvider.cs | Adds RequireSyntheticPid provider flag. |
| src/Authentication/Helpers/SyntheticPersonIdentifier.cs | Implements Tenor synthetic PID detection helper. |
| src/Authentication/Helpers/AuthenticationHelper.cs | Enforces synthetic-only PID gate when configured. |
| samples/mockporten/SECURITY.md | Documents safety rationale for enabling mockporten with downstream gating. |
| samples/mockporten/README.md | Usage + security model documentation for the sample Test-IDP. |
| samples/mockporten/Mockporten/wwwroot/lib/jquery/LICENSE.txt | Third-party license file (jQuery). |
| samples/mockporten/Mockporten/wwwroot/lib/jquery-validation/LICENSE.md | Third-party license file (jquery-validation). |
| samples/mockporten/Mockporten/wwwroot/lib/jquery-validation-unobtrusive/LICENSE.txt | Third-party license file (jquery-validation-unobtrusive). |
| samples/mockporten/Mockporten/wwwroot/lib/jquery-validation-unobtrusive/jquery.validate.unobtrusive.min.js | Third-party JS asset (minified unobtrusive validation). |
| samples/mockporten/Mockporten/wwwroot/lib/jquery-validation-unobtrusive/jquery.validate.unobtrusive.js | Third-party JS asset (unobtrusive validation). |
| samples/mockporten/Mockporten/wwwroot/lib/bootstrap/LICENSE | Third-party license file (Bootstrap). |
| samples/mockporten/Mockporten/wwwroot/js/site.js | Sample site JS scaffold. |
| samples/mockporten/Mockporten/wwwroot/css/site.css | Sample site styling incl. “test only” banner styles. |
| samples/mockporten/Mockporten/Views/Shared/Error.cshtml | Sample error view. |
| samples/mockporten/Mockporten/Views/Shared/_ValidationScriptsPartial.cshtml | Validation script includes for the sample UI. |
| samples/mockporten/Mockporten/Views/Shared/_Layout.cshtml | Sample layout + banners + script includes. |
| samples/mockporten/Mockporten/Views/Home/Privacy.cshtml | Sample privacy view. |
| samples/mockporten/Mockporten/Views/Home/Index.cshtml | Sample home page explaining test-only intent. |
| samples/mockporten/Mockporten/Views/Authorize/Index.cshtml | Test login form (shared password + synthetic PID). |
| samples/mockporten/Mockporten/Views/_ViewStart.cshtml | MVC layout selection. |
| samples/mockporten/Mockporten/Views/_ViewImports.cshtml | MVC view imports. |
| samples/mockporten/Mockporten/Services/OidcRequestException.cs | Exception type carrying OAuth error codes. |
| samples/mockporten/Mockporten/Services/Interfaces/IToken.cs | Token service interface (code/token/PAR). |
| samples/mockporten/Mockporten/Services/Interfaces/ISharedAccessPasswordValidator.cs | Shared-password validation contract + result enum. |
| samples/mockporten/Mockporten/Services/Interfaces/IJwtSigningCertificateProvider.cs | Cert provider interface for JWT signing. |
| samples/mockporten/Mockporten/Services/Implementation/TokenService.cs | Implements auth code issuance, PKCE binding, PAR JWT, token issuance. |
| samples/mockporten/Mockporten/Services/Implementation/SharedAccessPasswordValidator.cs | Constant-time shared password compare + global lockout. |
| samples/mockporten/Mockporten/Services/Implementation/JwtSigningCertificateProvider.cs | Loads signing cert versions from Key Vault. |
| samples/mockporten/Mockporten/Properties/ServiceDependencies/mockporten - Zip Deploy/profile.arm.json | Sample ARM deployment profile artifact. |
| samples/mockporten/Mockporten/Properties/serviceDependencies.json | Sample dependency mapping. |
| samples/mockporten/Mockporten/Properties/serviceDependencies.idprovider - Web Deploy.json | Sample dependency mapping with resource IDs. |
| samples/mockporten/Mockporten/Properties/PublishProfiles/mockporten - Zip Deploy1.pubxml | Sample publish profile artifact. |
| samples/mockporten/Mockporten/Properties/PublishProfiles/mockporten - Zip Deploy.pubxml | Sample publish profile artifact. |
| samples/mockporten/Mockporten/Properties/launchSettings.json | Local launch settings for the sample. |
| samples/mockporten/Mockporten/Program.cs | Sample app composition + Key Vault config source + telemetry. |
| samples/mockporten/Mockporten/Models/OidcAuthorizationModel.cs | Model used for query/form round-tripping of OIDC parameters. |
| samples/mockporten/Mockporten/Models/JwksDocument.cs | JWKS model. |
| samples/mockporten/Mockporten/Models/JwkDocument.cs | JWK model. |
| samples/mockporten/Mockporten/Models/GrantResponse.cs | Token endpoint response model. |
| samples/mockporten/Mockporten/Models/ErrorViewModel.cs | MVC error view model. |
| samples/mockporten/Mockporten/Models/DiscoveryDocument.cs | Discovery doc model (unused vs OpenIdConnectConfiguration). |
| samples/mockporten/Mockporten/Mockporten.csproj | Sample project file + package references. |
| samples/mockporten/Mockporten/Helpers/Pkce.cs | PKCE S256 compute/verify helper. |
| samples/mockporten/Mockporten/Helpers/NorwegianIdentityNumber.cs | Tenor synthetic PID validator (mod11 + month 81–92). |
| samples/mockporten/Mockporten/Controllers/TokenController.cs | Token endpoint controller. |
| samples/mockporten/Mockporten/Controllers/ParController.cs | PAR endpoint controller. |
| samples/mockporten/Mockporten/Controllers/OpenIdController.cs | OIDC discovery + JWKS controller. |
| samples/mockporten/Mockporten/Controllers/HomeController.cs | Sample home/privacy/error controller. |
| samples/mockporten/Mockporten/Controllers/AuthorizeController.cs | Login form + shared password + Tenor gate + code redirect. |
| samples/mockporten/Mockporten/Configuration/KeyVaultSettings.cs | Key Vault settings model. |
| samples/mockporten/Mockporten/Configuration/GeneralSettings.cs | Sample general settings (kill switch, shared password, PKCE). |
| samples/mockporten/Mockporten/Configuration/CertificateSettings.cs | Certificate settings model (currently unused). |
| samples/mockporten/Mockporten/appsettings.json | Sample base configuration. |
| samples/mockporten/Mockporten/appsettings.Development.json | Sample development configuration (enables Test-IDP + dev password). |
| samples/mockporten/Mockporten/.config/dotnet-tools.json | Local tool manifest. |
| samples/mockporten/Mockporten.Tests/SharedAccessPasswordValidatorTests.cs | Unit tests for shared-password validator and lockout. |
| samples/mockporten/Mockporten.Tests/PkceTests.cs | Unit tests for PKCE helper (RFC 7636 vector). |
| samples/mockporten/Mockporten.Tests/NorwegianIdentityNumberTests.cs | Unit tests for Tenor PID validator. |
| samples/mockporten/Mockporten.Tests/Mockporten.Tests.csproj | Sample test project file. |
| samples/mockporten/Mockporten.sln | Sample solution file. |
| .gitignore | Ignores *.lscache. |
| .dockerignore | Excludes samples from Docker build context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (2)
samples/mockporten/Mockporten/Services/Implementation/JwtSigningCertificateProvider.cs (1)
97-109: ⚡ Quick winMove
SecretClientinstantiation outside the loop.Creating a new
SecretClienton each iteration is unnecessary and wasteful. The client is stateless and can be reused for all secret fetches.♻️ Proposed fix
+ SecretClient secretClient = new SecretClient(new Uri(keyVaultUrl), new DefaultAzureCredential()); + await foreach (CertificateProperties certificateProperties in certificatePropertiesPage) { _logger.LogInformation("In loop"); if (certificateProperties.Enabled == true && (certificateProperties.ExpiresOn == null || certificateProperties.ExpiresOn >= DateTime.UtcNow)) { - SecretClient secretClient = new SecretClient(new Uri(keyVaultUrl), new DefaultAzureCredential()); - KeyVaultSecret secret = await secretClient.GetSecretAsync(certificateProperties.Name, certificateProperties.Version);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@samples/mockporten/Mockporten/Services/Implementation/JwtSigningCertificateProvider.cs` around lines 97 - 109, The SecretClient is being instantiated inside the await foreach loop that iterates over certificatePropertiesPage, causing a new client to be created on each iteration unnecessarily. Move the SecretClient instantiation outside the loop, before the await foreach statement, since the client is stateless and can be safely reused across all iterations. This will reduce resource allocation and improve performance without changing the functionality.samples/mockporten/Mockporten/Services/Implementation/TokenService.cs (1)
70-70: 💤 Low valueInconsistent use of
DateTime.NowvsDateTime.UtcNowfor token expiry.Token expiry is set using local time (
DateTime.Now) here and in other places (lines 159, 197, 279, 286), while certificate expiry checks useDateTime.UtcNow. For JWTexpclaims, UTC is the standard. WhileSecurityTokenDescriptormay handle conversion, usingDateTime.UtcNowthroughout avoids timezone ambiguity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@samples/mockporten/Mockporten/Services/Implementation/TokenService.cs` at line 70, The TokenService uses DateTime.Now for token expiry settings while JWT standards and certificate expiry checks use DateTime.UtcNow, causing timezone inconsistency. Replace all instances of DateTime.Now with DateTime.UtcNow in the GenerateAccessToken method and all other token expiry calculations throughout the file to maintain consistency and follow JWT conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@samples/mockporten/Mockporten/appsettings.json`:
- Around line 13-15: The TestIdpEnabled setting in the base appsettings.json
file is currently set to true, which enables the Test-IDP globally by default.
This should be disabled in the base configuration. Change the TestIdpEnabled
value from true to false in the GeneralSettings section of appsettings.json, and
then enable it selectively only in environment-specific configuration overrides
(such as appsettings.Development.json) where needed.
In `@samples/mockporten/Mockporten/Configuration/GeneralSettings.cs`:
- Line 62: The IdProviderEndpoint property in GeneralSettings.cs has an internal
set accessor which prevents the configuration options binder from binding
configuration values to it, causing environment and config file overrides to be
silently ignored despite the property being documented as configurable. Change
the accessor on the IdProviderEndpoint property from internal set to public set
to match the pattern used by the sibling properties IssCode and IssToken on
lines 64–66, which will allow the options binder to properly bind configuration
values to this property.
In `@samples/mockporten/Mockporten/Controllers/AuthorizeController.cs`:
- Around line 145-157: The AuthorizeController contains two locations where
OAuth redirect parameters are appended to query strings without URL encoding,
which can cause URL corruption or enable parameter injection if special
characters are present. In
samples/mockporten/Mockporten/Controllers/AuthorizeController.cs lines 145-157,
URL-encode both the code and state parameters using
System.Net.WebUtility.UrlEncode before appending them to the baseUri.Query
string. In samples/mockporten/Mockporten/Controllers/AuthorizeController.cs
lines 166-186 in the AccessDenied method, similarly URL-encode the state
parameter before appending it to the redirect URL's query string. Use
System.Net.WebUtility.UrlEncode to properly escape any special characters in
these parameter values.
In `@samples/mockporten/Mockporten/Controllers/OpenIdController.cs`:
- Around line 101-103: The code in the OpenIdController.cs file calls
GetRSAPublicKey() on the certificate but does not check if the result is null
before accessing it. If a non-RSA certificate is configured, GetRSAPublicKey()
will return null, causing a NullReferenceException when ExportParameters() is
called on the next line. Add a null check immediately after the
GetRSAPublicKey() call to verify that rsaPublicKey is not null, and throw a
clear, descriptive exception if it is null (indicating that the configured
certificate is not an RSA certificate).
- Around line 101-105: The OpenIdController.cs file has three issues in the RSA
public key extraction and JWK encoding around lines 101-105 and line 110. First,
the call to GetRSAPublicKey() returns a nullable RSA type, so add a null-check
before dereferencing rsaPublicKey to prevent NullReferenceException if a non-RSA
certificate is provided. Second, replace both instances of
Convert.ToBase64String with base64url encoding (which removes padding characters
and replaces + with - and / with _) to comply with RFC 7518 JWK specification
requirements. Third, ensure the KeyType is set to the literal string "RSA" as
required by RFC 7517, rather than using the oidFriendlyName value.
In `@samples/mockporten/Mockporten/Controllers/TokenController.cs`:
- Around line 26-46: The Index method in TokenController accepts a grant_type
parameter but never validates its value before redeeming the authorization code
via _tokenService.GetTokenFromCode. Add validation logic to check that
grant_type equals "authorization_code" before attempting to redeem the code,
returning an appropriate error response (such as BadRequest) if the grant_type
does not match the expected value.
- Around line 52-59: In the TokenController's GrantResponse object creation,
replace the hardcoded refresh_token string "ADFSFDSFSDFDSFDSF" with a
dynamically generated refresh token (such as a unique token generated per
request), and replace the hardcoded expires_in value of 3600 with the actual JWT
token lifetime or expiry value that corresponds to the token being returned.
This ensures security by avoiding shared static credentials and consistency
between the returned expiry claim and the actual token lifetime.
In `@samples/mockporten/Mockporten/Mockporten.csproj`:
- Around line 14-16: The Mockporten.csproj file contains references to
deprecated Azure SDK Track 1 packages: Microsoft.Azure.KeyVault (v3.0.5) and
Microsoft.IdentityModel.Clients.ActiveDirectory (v5.3.0). Replace these legacy
package references with their Track 2 equivalents:
Azure.Security.KeyVault.Secrets and Azure.Identity. After updating the project
file references, update all usages of these libraries throughout the sample code
and the authentication service in src/Authentication/ to use the new Track 2
APIs, ensuring the authentication flow and key vault access patterns are adapted
to the new library interfaces.
In `@samples/mockporten/Mockporten/Program.cs`:
- Around line 31-37: The KeyVaultSettings options registration is missing
ValidateDataAnnotations() which is required to enforce data annotations like the
[Required] attribute on KeyVaultURI. Add ValidateDataAnnotations() between
BindConfiguration() and ValidateOnStart() for both the GeneralSettings and
KeyVaultSettings service registrations to ensure all data-annotation constraints
are validated at startup.
In `@samples/mockporten/Mockporten/Properties/ServiceDependencies/mockporten` -
Zip Deploy/profile.arm.json:
- Around line 99-135: The ARM template is incorrectly configured as an Azure
Function App when it should be an ASP.NET Core web app targeting net9.0. Change
the "kind" property from "functionapp" to "webapp" to indicate this is a web
application, update the "linuxFxVersion" from "dotnet|3.1" to "dotnet|9.0" to
match the project's target framework, and remove or replace the
Functions-specific application settings (FUNCTIONS_EXTENSION_VERSION and
FUNCTIONS_WORKER_RUNTIME) in the appsettings configuration block since those are
only applicable to Function Apps, not web apps.
In
`@samples/mockporten/Mockporten/Services/Implementation/JwtSigningCertificateProvider.cs`:
- Line 106: The X509Certificate2 constructor in JwtSigningCertificateProvider is
using X509KeyStorageFlags.PersistKeySet which attempts to write to machine key
storage and fails in containerized or Linux environments. Remove the
PersistKeySet flag from the X509KeyStorageFlags parameter passed to the
X509Certificate2 constructor, keeping only MachineKeySet and Exportable, or
implement platform-conditional logic to use appropriate flags based on the
runtime environment. This ensures the certificate loading works reliably across
different deployment contexts while maintaining the caching and reusability of
the certificate.
In `@samples/mockporten/Mockporten/Services/Implementation/TokenService.cs`:
- Around line 295-318: The ValidateAuthorizationCode method validates the JWT
against only the single latest certificate obtained from
GetLatestCertificateWithRolloverDelay, which causes valid authorization codes
signed with a previous certificate during rotation to fail validation. Instead
of assigning a single IssuerSigningKey in the TokenValidationParameters, use the
plural IssuerSigningKeys property and populate it with SecurityKey objects
created from all available certificates in the certificates list returned by the
certificate provider. This allows the validator to accept tokens signed with any
valid certificate from the set, supporting proper certificate rotation without
invalidating previously-issued codes.
In `@samples/mockporten/Mockporten/Views/Shared/_Layout.cshtml`:
- Around line 23-24: The navbar toggler button uses Bootstrap 4 syntax instead
of Bootstrap 5. In the button element with class navbar-toggler on line 23,
update the attribute data-toggle to data-bs-toggle and update the attribute
data-target to data-bs-target. This will make the mobile menu toggle button
functional with Bootstrap 5.3.3.
---
Nitpick comments:
In
`@samples/mockporten/Mockporten/Services/Implementation/JwtSigningCertificateProvider.cs`:
- Around line 97-109: The SecretClient is being instantiated inside the await
foreach loop that iterates over certificatePropertiesPage, causing a new client
to be created on each iteration unnecessarily. Move the SecretClient
instantiation outside the loop, before the await foreach statement, since the
client is stateless and can be safely reused across all iterations. This will
reduce resource allocation and improve performance without changing the
functionality.
In `@samples/mockporten/Mockporten/Services/Implementation/TokenService.cs`:
- Line 70: The TokenService uses DateTime.Now for token expiry settings while
JWT standards and certificate expiry checks use DateTime.UtcNow, causing
timezone inconsistency. Replace all instances of DateTime.Now with
DateTime.UtcNow in the GenerateAccessToken method and all other token expiry
calculations throughout the file to maintain consistency and follow JWT
conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d57deae-4b72-485b-8180-a887c1f42ad4
⛔ Files ignored due to path filters (2)
samples/mockporten/Mockporten/wwwroot/favicon.icois excluded by!**/*.icosamples/mockporten/Mockporten/wwwroot/lib/jquery-validation-unobtrusive/jquery.validate.unobtrusive.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (58)
.dockerignoresamples/mockporten/Mockporten.Tests/Mockporten.Tests.csprojsamples/mockporten/Mockporten.Tests/NorwegianIdentityNumberTests.cssamples/mockporten/Mockporten.Tests/PkceTests.cssamples/mockporten/Mockporten.Tests/SharedAccessPasswordValidatorTests.cssamples/mockporten/Mockporten.slnsamples/mockporten/Mockporten/.config/dotnet-tools.jsonsamples/mockporten/Mockporten/Configuration/CertificateSettings.cssamples/mockporten/Mockporten/Configuration/GeneralSettings.cssamples/mockporten/Mockporten/Configuration/KeyVaultSettings.cssamples/mockporten/Mockporten/Controllers/AuthorizeController.cssamples/mockporten/Mockporten/Controllers/HomeController.cssamples/mockporten/Mockporten/Controllers/OpenIdController.cssamples/mockporten/Mockporten/Controllers/ParController.cssamples/mockporten/Mockporten/Controllers/TokenController.cssamples/mockporten/Mockporten/Helpers/NorwegianIdentityNumber.cssamples/mockporten/Mockporten/Helpers/Pkce.cssamples/mockporten/Mockporten/Mockporten.csprojsamples/mockporten/Mockporten/Models/DiscoveryDocument.cssamples/mockporten/Mockporten/Models/ErrorViewModel.cssamples/mockporten/Mockporten/Models/GrantResponse.cssamples/mockporten/Mockporten/Models/JwkDocument.cssamples/mockporten/Mockporten/Models/JwksDocument.cssamples/mockporten/Mockporten/Models/OidcAuthorizationModel.cssamples/mockporten/Mockporten/Program.cssamples/mockporten/Mockporten/Properties/PublishProfiles/mockporten - Zip Deploy.pubxmlsamples/mockporten/Mockporten/Properties/PublishProfiles/mockporten - Zip Deploy1.pubxmlsamples/mockporten/Mockporten/Properties/ServiceDependencies/mockporten - Zip Deploy/profile.arm.jsonsamples/mockporten/Mockporten/Properties/launchSettings.jsonsamples/mockporten/Mockporten/Properties/serviceDependencies.idprovider - Web Deploy.jsonsamples/mockporten/Mockporten/Properties/serviceDependencies.jsonsamples/mockporten/Mockporten/Services/Implementation/JwtSigningCertificateProvider.cssamples/mockporten/Mockporten/Services/Implementation/SharedAccessPasswordValidator.cssamples/mockporten/Mockporten/Services/Implementation/TokenService.cssamples/mockporten/Mockporten/Services/Interfaces/IJwtSigningCertificateProvider.cssamples/mockporten/Mockporten/Services/Interfaces/ISharedAccessPasswordValidator.cssamples/mockporten/Mockporten/Services/Interfaces/IToken.cssamples/mockporten/Mockporten/Services/OidcRequestException.cssamples/mockporten/Mockporten/Views/Authorize/Index.cshtmlsamples/mockporten/Mockporten/Views/Home/Index.cshtmlsamples/mockporten/Mockporten/Views/Home/Privacy.cshtmlsamples/mockporten/Mockporten/Views/Shared/Error.cshtmlsamples/mockporten/Mockporten/Views/Shared/_Layout.cshtmlsamples/mockporten/Mockporten/Views/Shared/_ValidationScriptsPartial.cshtmlsamples/mockporten/Mockporten/Views/_ViewImports.cshtmlsamples/mockporten/Mockporten/Views/_ViewStart.cshtmlsamples/mockporten/Mockporten/appsettings.Development.jsonsamples/mockporten/Mockporten/appsettings.jsonsamples/mockporten/Mockporten/wwwroot/css/site.csssamples/mockporten/Mockporten/wwwroot/js/site.jssamples/mockporten/Mockporten/wwwroot/lib/bootstrap/LICENSEsamples/mockporten/Mockporten/wwwroot/lib/jquery-validation-unobtrusive/LICENSE.txtsamples/mockporten/Mockporten/wwwroot/lib/jquery-validation-unobtrusive/jquery.validate.unobtrusive.jssamples/mockporten/Mockporten/wwwroot/lib/jquery-validation/LICENSE.mdsamples/mockporten/Mockporten/wwwroot/lib/jquery/LICENSE.txtsamples/mockporten/README.mdsamples/mockporten/SECURITY.mdsrc/Authentication/Helpers/AuthenticationHelper.cs
✅ Files skipped from review due to trivial changes (20)
- samples/mockporten/Mockporten/Views/_ViewImports.cshtml
- samples/mockporten/Mockporten/wwwroot/js/site.js
- samples/mockporten/Mockporten.Tests/Mockporten.Tests.csproj
- samples/mockporten/Mockporten/Services/OidcRequestException.cs
- samples/mockporten/Mockporten/Views/Shared/_ValidationScriptsPartial.cshtml
- samples/mockporten/Mockporten/Views/Home/Privacy.cshtml
- samples/mockporten/Mockporten/Properties/PublishProfiles/mockporten - Zip Deploy1.pubxml
- samples/mockporten/Mockporten/wwwroot/lib/jquery-validation-unobtrusive/LICENSE.txt
- samples/mockporten/Mockporten/wwwroot/lib/jquery-validation/LICENSE.md
- samples/mockporten/Mockporten/Services/Interfaces/ISharedAccessPasswordValidator.cs
- samples/mockporten/Mockporten/wwwroot/lib/bootstrap/LICENSE
- samples/mockporten/Mockporten/appsettings.Development.json
- .dockerignore
- samples/mockporten/Mockporten/Models/JwksDocument.cs
- samples/mockporten/Mockporten/wwwroot/lib/jquery/LICENSE.txt
- samples/mockporten/Mockporten/Views/_ViewStart.cshtml
- samples/mockporten/Mockporten/Properties/PublishProfiles/mockporten - Zip Deploy.pubxml
- samples/mockporten/Mockporten/.config/dotnet-tools.json
- samples/mockporten/Mockporten/Models/ErrorViewModel.cs
- samples/mockporten/Mockporten/Configuration/CertificateSettings.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Authentication/Helpers/AuthenticationHelper.cs
CodeRabbit/Copilot review of #1984: mockporten sample: - appsettings.json: TestIdpEnabled defaults to false (enable per-environment) - GeneralSettings.IdProviderEndpoint + OidcAuthorizationModel: internal->public setters so config/PKCE form fields actually bind - AuthorizeController: URL-encode code/state on success and error redirects - TokenController: validate grant_type=authorization_code; replace the static refresh_token and hardcoded expires_in with generated/derived values - OpenIdController: advertise real authorization/token/jwks endpoints and the code response type; base64url-encode JWK n/e, set kty=RSA, null-check RSA key - TokenService: validate auth codes against all current certs (rollover-safe); use UtcNow for token expiry - JwtSigningCertificateProvider: reuse one credential/SecretClient, drop PersistKeySet (fails on Linux) via X509CertificateLoader, remove debug logs - _Layout/_ValidationScriptsPartial: Bootstrap 5 toggler attrs; load jQuery/ Bootstrap/validation from CDN (local assets were missing -> 404) - csproj: drop deprecated Azure Track-1 packages (Microsoft.Azure.KeyVault, ADAL); add Azure.Security.KeyVault.Secrets explicitly - Program.cs: ValidateDataAnnotations() so [Required] KeyVaultURI is enforced - delete committed Azure deploy artifacts (PublishProfiles, ServiceDependencies) product gate: - add positive synthetic D-number test case (41818520024) to lock in the D-number normalization branch Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…idation Per review: avoid loading scripts from external sources. The client-side jquery/jquery-validation/bootstrap scripts are not required - the login form is validated server-side (shared password, synthetic pid, ModelState) and the asp-validation tag helpers render messages without JS. - _Layout: drop jQuery + Bootstrap JS <script> tags and the now-inert navbar toggler; keep local site.js - Authorize/Index: remove the @section Scripts block - delete _ValidationScriptsPartial.cshtml and the unused wwwroot/lib/ assets Only the Bootstrap CSS link remains external (stylesheet, no code execution). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… CDN Removes the last external front-end dependency. Bootstrap 5.3.3 CSS is now served from wwwroot/lib/bootstrap/css (source-map reference stripped; MIT LICENSE included). No view references an external source anymore. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The acr claim must be a single fulfilled LoA. altinn-authentication maps it via GetAuthenticationLevelForIdPorten, which matches a single token. When the user is anonymous the product forwards a multi-valued acr_values (e.g. "selfregistered-email idporten-loa-high"); echoing that list verbatim produced a multi-token acr that fell through to the lowest level (SelfIdentified). Previously the value was dropped silently (Acr_values had an internal setter, so model binding never populated it), which made the token default to a single high-LoA acr. Making the setter public (review feedback) re-enabled the echo and broke the level. Stop deriving acr from the requested acr_values; test users are asserted at the default high level. Also ignore empty/whitespace acr entries. Adds TokenServiceAcrTests as a regression guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The IDE regenerated mockporten - Zip Deploy.pubxml from the leftover .pubxml.user, and it slipped back in via git add -A. Untrack it and add .gitignore rules (*.pubxml, Properties/ServiceDependencies, serviceDependencies*.json) so these deploy artifacts cannot be committed again. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
Concept and the product gate are sound, from my perspective there is only two areas i would like to highlight: 1: The if (provider.RequireSyntheticPid
&& !string.IsNullOrEmpty(userAuthenticationModel.SSN)
&& !SyntheticPersonIdentifier.IsSyntheticTenor(userAuthenticationModel.SSN))From my understading, the guarantee this provides is "no real pid can authenticate through this provider," not "only synthetic identities can." A token that reaches a RequireSyntheticPid provider with no pid claim (or that carries identity via some other claim) passes through ungated. For mockporten that's fine today since it currently only mints person/pid tokens. 2: I would suggest leaving this empty: |
…iew) Addresses urb4n3 review on #1984: 1. RequireSyntheticPid now requires a *valid synthetic pid* to be present, rather than only rejecting non-synthetic pids. Dropped the !IsNullOrEmpty(SSN) condition so a token with no pid (or identity via another claim) is rejected fail-closed (IsSyntheticTenor(null) is false). The guarantee is now "only synthetic identities can authenticate", not merely "no real pid can". Updated the NoPid test to expect rejection, plus OidcProvider/method docs and SECURITY.md. 2. appsettings.Development.json no longer ships a placeholder shared password; TestIdpSharedPassword is empty (fail-closed) with a note to set it via user-secrets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
@- |
|
allinox
approved these changes
Jun 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Implements the authoritative synthetic-only gate from #1409 (referenced by #1983): an OIDC provider can be configured so that only synthetic (Tenor) test persons can be authenticated through it — and crucially, this gate lives in the authentication component, not in the upstream IdP, so a compromised/Misbehaving test IdP that asserts a real
pidis still rejected here.OidcProvider.RequireSyntheticPid(defaultfalse). Settrueon test-only providers (e.g.mockporten) in theOidcProvidersconfig.SyntheticPersonIdentifier.IsSyntheticTenor— positive, fail-closed check: 11 digits, month ∈ 81–92 (the synthetic marker = real month + 80), D-numberday−40normalization, mod11 on both control digits. Any ordinary fnr or real D-number fails theMM ∈ 81–92test unconditionally.AuthenticationHelper.GetUserFromToken— when the selected provider hasRequireSyntheticPid=trueand thepidclaim is present but not a well-formed synthetic fnr, the user is marked not authenticated (fail-closed). Covers both the legacyauthentication?goto=flow and the OIDC authorization-server flow, since both extract identity through this method. The gate applies only to apidclaim; absence ofpidand other identity flows are unaffected.Behaviour / compatibility
RequireSyntheticPid=false→ no behaviour change for existing providers (ID-porten, Feide, etc.).RequireSyntheticPid: trueon the test provider entry inOidcProvidersconfiguration.Not in this PR
The optional inverse rule from #1409 (real providers also rejecting synthetic pids, for a fully disjoint partition) is intentionally not included — it changes behaviour for production providers and warrants its own change. This PR is the requested per-provider opt-in only.
Tests
SyntheticPersonIdentifierTests— the validator (ordinary fnr, real D-number, H-number, broken mod11, wrong length, non-digit, null, out-of-range months all rejected; valid synthetic incl. constructed mod11-valid number accepted) and theGetUserFromTokengate (synthetic → authenticated; ordinary → fail-closed; flag off → unchanged; no pid → unaffected). 17/17 green.Relates to #1409, #1983.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation