Skip to content

feat(sso): check assertion type (#8301) - #8324

Merged
enmande merged 1 commit into
rcfrom
cherry-pick-pm-42627
Sep 8, 2026
Merged

feat(sso): check assertion type (#8301)#8324
enmande merged 1 commit into
rcfrom
cherry-pick-pm-42627

Conversation

@enmande

@enmande enmande commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

(cherry picked from commit b89568d)

🎟️ Tracking

PM-42627

📔 Objective

PM-42627

* fix(saml2options): Do not throw for encryptedassertion at the boundary.

* feat(saml): Add inspection for assertion encryption method

* feat(saml): Add assertion type inspection and census

* docs(saml): Clean up assertion summaries

* refactor(saml) Rename algorithms and update tests

* refactor(saml): Encapsulate observability into assertion inspection static method

* fix(saml): Split wantassertionssigned to its own issue

* test(saml): Add saml acs integration tests

* fix(saml): Check all keys and update naming

* feat(saml): Rework algorithm inspection as Try* method

* fix(saml): Use an aggregate metric

* fix(saml): Update description and unit

* test(saml): remove unused fake

* test(saml): Dispose of disposable fixtures

(cherry picked from commit b89568d)
@enmande
enmande requested a review from a team as a code owner September 8, 2026 20:45
@enmande enmande added the t:feature Change Type - Feature Development label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This is a backport to rc of the already-merged SAML2 assertion-inspection change (#8301, commit b89568d), and the bitwarden_license/ contents on this branch are byte-identical to that commit — the cherry-pick introduced no divergence. Reviewed the new Saml2EncryptedAssertionInspector on the unauthenticated ACS path: it walks an already-parsed DOM, never decrypts or returns assertion content, swallows all exceptions so a malformed envelope cannot block SSO login, and ClassifyAlgorithm collapses unknown values to a fixed unrecognized/none set, which keeps metric tag cardinality bounded and keeps organization and user identifiers out of the measurement. Saml2AssertionMetrics is registered with TryAddSingleton per ADR 0026, the SamlEncryptionAlgorithmsSaml2KeyTransportEncryptionAlgorithms rename is confined to the Sso assembly with all call sites and tests updated, and the deliberate deferral of the WantAssertionsSigned throw is documented against PM-42982 and pinned by a test. Unit and integration coverage is thorough, including multi-key, multi-assertion, sibling-key, missing-algorithm, and metrics-resolution-failure cases; every symbol the new tests reference (SsoConfigurationData.BuildSaml2AcsUrl, SamlEnvironment, WebApplicationFactoryBase.ConfigureServices) exists on this branch.

Code Review Details

No findings.

Dependency Changes

Package Change Ecosystem
Microsoft.Extensions.Diagnostics.Testing New (10.8.0) NuGet

Not a net-new dependency for this repository — it is already a direct reference in seven other test projects (test/Core.Test, test/Api.IntegrationTest, test/Identity.Test, test/SharedWeb.Test, and others), so the AppSec dependency-approval process does not apply here. Both affected test projects (SSO.Test, Sso.IntegrationTest) had their packages.lock.json regenerated alongside the manifest change, and the additions are test-only.

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 64.03%. Comparing base (4c664a9) to head (3846842).

Files with missing lines Patch % Lines
.../Sso/Utilities/Saml2EncryptedAssertionInspector.cs 97.72% 0 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (4c664a9) and HEAD (3846842). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (4c664a9) HEAD (3846842)
3 1
Additional details and impacted files
@@            Coverage Diff             @@
##               rc    #8324      +/-   ##
==========================================
- Coverage   69.56%   64.03%   -5.53%     
==========================================
  Files        2471     2473       +2     
  Lines      105932   105990      +58     
  Branches     9601     9611      +10     
==========================================
- Hits        73689    67869    -5820     
- Misses      29779    35762    +5983     
+ Partials     2464     2359     -105     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@enmande
enmande merged commit e101144 into rc Sep 8, 2026
70 of 73 checks passed
@enmande
enmande deleted the cherry-pick-pm-42627 branch September 8, 2026 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants