Skip to content

Conversation

@iinuwa
Copy link
Contributor

@iinuwa iinuwa commented Sep 25, 2025

🎟️ Tracking

PM-26177
PM-27177
PM-27265
PM-27264

📔 Objective

As part of the initiative to have unphishable onboarding from mobile clients to the web vault or browser extension, we need to allow mobile clients to be able to create new passkeys.

This PR allows mobile clients to create and update passkeys. At this time, we are not expecting mobile clients to authenticate directly with passkeys, so the assertion endpoint was left restricted to the web vault.

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@iinuwa iinuwa changed the title auth/PM-26177/mobile-device-passkey [PM-26177] Allow mobile clients to create passkeys Sep 25, 2025
@sonarqubecloud
Copy link

@github-actions
Copy link
Contributor

github-actions bot commented Sep 25, 2025

Logo
Checkmarx One – Scan Summary & Details8ac313fb-4202-4c5c-b036-7153f8eaadd1

Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 208

@codecov
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.57%. Comparing base (b3437b3) to head (ddf4822).
⚠️ Report is 33 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6383   +/-   ##
=======================================
  Coverage   54.57%   54.57%           
=======================================
  Files        1921     1921           
  Lines       85405    85405           
  Branches     7635     7635           
=======================================
+ Hits        46607    46612    +5     
+ Misses      37027    37020    -7     
- Partials     1771     1773    +2     

☔ View full report in Codecov by Sentry.
📢 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.

@claude
Copy link
Contributor

claude bot commented Oct 10, 2025

Claude finished @iinuwa's task in 2m 42s —— View job


Code Review Complete

Overall Assessment: APPROVE

I've reviewed the authorization policy changes that enable mobile clients to create passkeys while maintaining web-only restrictions for assertion operations. The implementation correctly separates creation (now allowed for all authenticated clients via Policies.Application) from authentication operations (kept web-only via Policies.Web).

Key Findings

Finding 1 - Documentation typo in Policies.cs:65:
The XML example shows [Authorize(Policy = Policies.Licensing)] but this documentation is for the Organization constant. Should be Policies.Organization.

Finding 2 - Authorization policy consistency question:
The UpdateCredential endpoint allows Application (all clients) but Delete is Web-only. If mobile clients can create and update passkeys, should they also be able to delete them? Consider documenting the rationale if this is intentional.

Finding 3 - Authorization policy consistency question:
The Get() endpoint (list passkeys) is Web-only, but mobile clients can create passkeys. Should mobile clients be able to list their passkeys for UI purposes? Consider documenting the rationale if listing should remain web-only.

Security Analysis

✅ Zero-knowledge principles maintained - passkeys are authentication credentials, not vault data
✅ Same security validations apply to both Web and Application clients
✅ Policy restrictions correctly enforced via claims
✅ No new security vulnerabilities introduced

The change achieves its objective of enabling mobile clients to create passkeys for unphishable onboarding while keeping assertion operations web-only.

@iinuwa iinuwa force-pushed the auth/PM-26177/mobile-device-passkey branch from 1589f3e to 6da6998 Compare December 16, 2025 13:49
@iinuwa iinuwa marked this pull request as ready for review December 17, 2025 14:11
@iinuwa iinuwa requested a review from a team as a code owner December 17, 2025 14:11
@iinuwa iinuwa requested a review from rr-bw December 17, 2025 14:11
@claude
Copy link
Contributor

claude bot commented Dec 17, 2025

Overall Assessment: APPROVE

Reviewed authorization policy changes for mobile passkey creation support. The change correctly enables mobile clients to create passkeys while maintaining web-only restrictions for assertion operations, aligning with the unphishable onboarding objectives.

Copy link
Contributor

Choose a reason for hiding this comment

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

While you are in here and updating the webauthn access, could you update this comment in the src/Core/Auth/Identity/Policies.cs to more appropriately reflect how these policies are being used? Looks like we probably should have done that in a previous commit and didn't catch it, apologies for that.

/// <summary>
/// Policy for managing access to the Send feature.
/// </summary>

Otherwise, I think this all looks great!

@iinuwa iinuwa force-pushed the auth/PM-26177/mobile-device-passkey branch from 6da6998 to 31f9700 Compare December 17, 2025 17:30
@iinuwa
Copy link
Contributor Author

iinuwa commented Dec 17, 2025

Thanks for the review @Patrick-Pimentel-Bitwarden. Did you mean that I should add similar comments to the rest of the const strings, or that I should update the comment on the Send field?

I went ahead and made them all uniform (except for Licensing, which doesn't seem to be used), but I'm not sure if that's what you were looking for.

@iinuwa iinuwa force-pushed the auth/PM-26177/mobile-device-passkey branch from 31f9700 to ddf4822 Compare December 17, 2025 18:07
Copy link
Contributor

@Patrick-Pimentel-Bitwarden Patrick-Pimentel-Bitwarden left a comment

Choose a reason for hiding this comment

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

👍 Thank you @iinuwa! Yeah just more clarity on how the different policies were being applied / updating them to be correct.

@iinuwa iinuwa merged commit 9a340c0 into main Dec 30, 2025
44 of 45 checks passed
@iinuwa iinuwa deleted the auth/PM-26177/mobile-device-passkey branch December 30, 2025 13:31
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