Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[PM-25652] Add endpoint to fetch key connector confirmation details#6635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
Thomas-Avery wants to merge4 commits intomain
base:main
Choose a base branch
Loading
fromkm/pm-25652

Conversation

@Thomas-Avery
Copy link
Contributor

@Thomas-AveryThomas-Avery commentedNov 24, 2025•
edited
Loading

🎟️ Tracking

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

📔 Objective

The objective of this PR is to provide a new endpoint to fetch information needed when confirming/migrating a new key connector user.

See the associated client PRbitwarden/clients#17642

⏰ 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

@Thomas-AveryThomas-Avery self-assigned thisNov 24, 2025
@github-actions
Copy link
Contributor

github-actionsbot commentedNov 24, 2025•
edited
Loading

Logo
Checkmarx One – Scan Summary & Details –1e6d369f-21a8-4496-9ee5-733aebb71b8e

Fixed Issues (1)

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

SeverityIssueSource File / Package
MEDIUMCSRF/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs:97

@codecov
Copy link

codecovbot commentedNov 24, 2025•
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.12%. Comparing base (9573cab) to head (3ee3974).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@##             main    #6635       +/-   ##===========================================+ Coverage   13.38%   53.12%   +39.73%===========================================  Files        1125     1906      +781       Lines       49593    84915    +35322       Branches     3914     7636     +3722     ===========================================+ Hits         6640    45112    +38472+ Misses      42831    38053     -4778- Partials      122     1750     +1628

☔ 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

claudebot commentedNov 24, 2025•
edited
Loading

Claude finished@Thomas-Avery's task ——View job

Code Review - Changes Since Last Review

  • Read project documentation and guidelines
  • Review changes since last Claude review
  • Analyze all modified files
  • Address open questions from human reviewers
  • Provide comprehensive code review feedback

Summary

The most recent commit (3ee3974) addresses the previous finding about null user handling by changingNotFoundException toUnauthorizedAccessException inAccountsKeyManagementController.cs:192, which correctly aligns with the pattern used throughout the controller. The corresponding unit test was also updated.

However, oneopen question from@quexten remains unresolved regarding exception handling for the organization user check inKeyConnectorConfirmationDetailsQuery.cs:30.

Findings

Finding 1: 💭Exception type for null organization user needs clarification

InKeyConnectorConfirmationDetailsQuery.cs:27-30, whenGetByOrganizationAsync returns null, the code throwsNotFoundException.@quexten raised a valid question about whether this should beUnauthorizedAccessException instead, since the user is not part of the organization.

Analysis of exception patterns in codebase

I examined other usages ofGetByOrganizationAsync in the codebase:

  1. OrganizationsController.cs:172-176 - ThrowsNotFoundException when orgUser is null
  2. OrganizationUsersController.cs:479-480 - Assumes non-null and accesses.Status (would throw NullReferenceException if null)
  3. UserDecryptionOptionsBuilder.cs:147 - Assumes non-null in subsequent logic

The pattern isinconsistent across the codebase, which makes this a judgment call. However, semantically:

  • NotFoundException suggests "the resource you're looking for doesn't exist"
  • UnauthorizedAccessException suggests "you don't have permission to access this"

For a user attempting to access an organization they're not a member of,UnauthorizedAccessException seems more semantically correct as it's an authorization issue, not a missing resource issue.

Recommendation: Consider changing line 30 tothrow new UnauthorizedAccessException(); for semantic clarity. If keepingNotFoundException, please document the reasoning.

Good Practices Observed

  • ✅ Comprehensive unit test coverage for the new endpoint and query
  • ✅ Integration tests updated to cover the new functionality
  • ✅ Proper dependency injection using TryAdd pattern
  • ✅ Consistent with existing controller patterns
  • ✅ Test addressed for user null check

Action Items

  1. @Thomas-Avery: Address@quexten's question about the orgUser null check exception type inKeyConnectorConfirmationDetailsQuery.cs:30 - clarify whetherNotFoundException orUnauthorizedAccessException is the intended behavior and update code/tests if needed

Copy link
Contributor

@quextenquexten left a comment

Choose a reason for hiding this comment

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

Nice, mostly looks good. My only concern is with the returned exception types, which seem a little bit unintutive as a consumer. If you feel that these are the right types to use, please at least provide an explanation why we are choosing not found over unauthorized, especially for user.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@claudeclaude[bot]claude[bot] left review comments

@quextenquextenquexten approved these changes

Assignees

@Thomas-AveryThomas-Avery

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Thomas-Avery@quexten
[8]ページ先頭
©2009-2025 Movatter.jp