Skip to content

Conversation

@JaredSnider-Bitwarden
Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden commented Nov 3, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-23835

📔 Objective

To stub out the auth client > identity client

⏰ 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

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2025

Logo
Checkmarx One – Scan Summary & Details0e3aeb27-294d-46c0-8690-5a83d18b8500

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Nov 4, 2025

Codecov Report

❌ Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.95%. Comparing base (95e329a) to head (96c0c12).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/bitwarden-auth/src/auth_client.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #546      +/-   ##
==========================================
- Coverage   78.95%   78.95%   -0.01%     
==========================================
  Files         296      297       +1     
  Lines       30904    30918      +14     
==========================================
+ Hits        24400    24411      +11     
- Misses       6504     6507       +3     

☔ 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.

@JaredSnider-Bitwarden JaredSnider-Bitwarden marked this pull request as ready for review November 4, 2025 18:38
@JaredSnider-Bitwarden JaredSnider-Bitwarden requested review from a team as code owners November 4, 2025 18:38
@claude
Copy link

claude bot commented Nov 4, 2025

Claude finished @JaredSnider-Bitwarden's task —— View job


PR Review: Auth/PM-23835 - Stub out auth identity client


Summary of Changes

This PR introduces a new IdentityClient stub within the bitwarden-auth crate. The changes include:

  • New identity module with IdentityClient struct (38 lines)
  • Integration with AuthClient via new identity() method
  • Basic test for client creation
  • Proper module exports in lib.rs

The implementation follows the existing pattern established by SendAccessClient and serves as a foundation for future identity/access token functionality.


Critical Issues

1. ⚠️ Missing Coverage for Public API Surface

Location: crates/bitwarden-auth/src/auth_client.rs:27-29

The new identity() method is missing test coverage (as indicated by codecov showing 3 lines missing). While the IdentityClient::new() has a test, the public accessor method doesn't.

pub fn identity(&self) -> IdentityClient {
    IdentityClient::new(self.client.clone())
}

Recommendation: Add a test that exercises this method through the AuthClient:

Suggested test
#[test]
fn test_auth_client_identity_accessor() {
    let client = Client::new(None);
    let auth_client = AuthClient::new(client);
    let identity_client = auth_client.identity();
    // Verify the identity client was created successfully
    let _ = identity_client.client;
}

Severity: Medium - Affects codecov metrics and leaves public API untested.


Suggested Improvements

2. 🎨 Consider Consistent Constructor Visibility

Location: crates/bitwarden-auth/src/identity/client.rs:15-17

The IdentityClient::new() is pub(crate) while SendAccessClient::new() is also pub(crate). This is correct, but worth noting that these constructors are only accessible within the crate, forcing users to go through AuthClient.

Observation: This enforces proper client hierarchy and prevents direct instantiation. Good pattern consistency with SendAccessClient.

3. 📝 Test Could Be More Descriptive

Location: crates/bitwarden-auth/src/identity/client.rs:30-37

The test accesses identity_client.client just to verify it exists, but the comment says "verify the identity client was created successfully." Consider whether the test name and implementation fully express intent.

Suggestion: The test is adequate for a stub, but when methods are added, ensure tests verify functional behavior rather than just field access.


Good Practices Observed

  • ✅ Follows established client pattern from SendAccessClient
  • ✅ Proper module structure with separation of concerns
  • ✅ Appropriate use of #[cfg(feature = "wasm")] for wasm_bindgen attributes
  • #[allow(dead_code)] with TODO comment for unused client field
  • ✅ Clear documentation comments describing purpose
  • ✅ Module-level documentation in mod.rs
  • ✅ Public module export in lib.rs (vs send_access pattern)

Additional Observations

Architecture Consistency

The implementation mirrors SendAccessClient closely:

  • Same WASM feature gating
  • Same Clone derivation
  • Same constructor pattern (pub(crate) fn new)
  • Same client field access pattern

This consistency is excellent for maintainability.

Client Cloning Pattern

Both identity() and send_access() clone the underlying Client:

IdentityClient::new(self.client.clone())

This is consistent with the existing pattern, though the TODO comment in auth_client.rs:11-13 suggests this architecture may be refactored in the future. The current approach is reasonable for now.


Action Items for Author

  1. Required: Add test coverage for AuthClient::identity() method to address the 3 missing lines in codecov
  2. Optional: Consider if the test in identity/client.rs could be enhanced to be more meaningful (though it's adequate for a stub)

Conclusion

This is a clean, well-structured stub that follows established patterns in the codebase. The only actionable issue is the missing test coverage for the public accessor method. Once that's addressed, this PR is ready to merge.

The implementation properly sets up the foundation for future identity token functionality while maintaining consistency with existing client architecture.

Copy link

@ike-kottlowski ike-kottlowski left a comment

Choose a reason for hiding this comment

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

Looks like a stub to me.

@JaredSnider-Bitwarden JaredSnider-Bitwarden merged commit a36a8ef into main Nov 5, 2025
55 checks passed
@JaredSnider-Bitwarden JaredSnider-Bitwarden deleted the auth/pm-23835/create-auth-identity-client branch November 5, 2025 15:52
bw-ghapp bot pushed a commit to bitwarden/sdk-swift that referenced this pull request Nov 5, 2025
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