Skip to content

Recovery password: GET API - #41436

Merged
mostlikelee merged 95 commits into
mainfrom
40658-get-password
Mar 16, 2026
Merged

Recovery password: GET API#41436
mostlikelee merged 95 commits into
mainfrom
40658-get-password

Conversation

@mostlikelee

@mostlikelee mostlikelee commented Mar 11, 2026

Copy link
Copy Markdown
Contributor

Related issue: Resolves #40658

Summary by CodeRabbit

  • New Features

    • Added a new API endpoint to retrieve host recovery lock passwords with proper authorization checks and MDM validation requirements.
  • Chores

    • Added activity tracking for recovery lock password access events.

@codecov

codecov Bot commented Mar 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 35.71429% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.39%. Comparing base (8b43190) to head (f7741c2).
⚠️ Report is 137 commits behind head on main.

Files with missing lines Patch % Lines
server/service/hosts.go 34.14% 20 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #41436      +/-   ##
==========================================
+ Coverage   66.36%   66.39%   +0.03%     
==========================================
  Files        2491     2480      -11     
  Lines      199288   199003     -285     
  Branches     8942     8611     -331     
==========================================
- Hits       132256   132135     -121     
+ Misses      55076    54911     -165     
- Partials    11956    11957       +1     
Flag Coverage Δ
backend 68.15% <35.71%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@mostlikelee mostlikelee linked an issue Mar 12, 2026 that may be closed by this pull request
Base automatically changed from 40656-mdm-refactor to main March 12, 2026 12:06
@mostlikelee

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Mar 12, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@coderabbitai

coderabbitai Bot commented Mar 12, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

This pull request adds a new API endpoint to retrieve the recovery lock password for a host. The changes include introducing a new activity type for tracking password access, registering a new service method in the Service interface, implementing the endpoint with authorization validation and MDM configuration checks, and updating mock implementations. The implementation logs an activity entry when the password is accessed and includes proper error handling.

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description only includes the related issue reference but lacks substantive details about the changes, testing, and completion of the template checklist. Fill out the PR description template with implementation details, testing confirmation, and completion of all relevant checklist items to demonstrate the work is ready for review.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Recovery password: GET API' clearly and concisely describes the main change - adding a GET API endpoint for recovery passwords.
Linked Issues check ✅ Passed The changes implement the core objectives from issue #40658: a GET API endpoint is added via GetHostRecoveryLockPassword method and handler, activity logging is implemented via ActivityTypeViewedHostRecoveryLockPassword, and mock/service structures support the feature.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing the GET API for recovery lock passwords as specified in issue #40658; no extraneous modifications detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 40658-get-password
📝 Coding Plan for PR comments
  • Generate coding plan

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
server/service/hosts.go (1)

3725-3737: Consider adding platform validation.

Recovery lock password is an Apple-specific feature (macOS Apple Silicon). While the database query will return "not found" for non-macOS hosts, adding a platform check would provide a clearer error message and avoid unnecessary database calls.

💡 Optional: Add platform check
 	// Check that MDM is enabled
 	appConfig, err := svc.ds.AppConfig(ctx)
 	if err != nil {
 		return nil, ctxerr.Wrap(ctx, err, "get app config")
 	}
 	if !appConfig.MDM.EnabledAndConfigured {
 		return nil, fleet.ErrMDMNotConfigured
 	}

+	// Recovery lock password is only available for macOS hosts
+	if host.Platform != "darwin" {
+		return nil, ctxerr.Wrap(ctx, newNotFoundError(), "recovery lock password is only available for macOS hosts")
+	}
+
 	password, err := svc.ds.GetHostRecoveryLockPassword(ctx, host.UUID)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/service/hosts.go` around lines 3725 - 3737, Before calling
svc.ds.GetHostRecoveryLockPassword, check the host platform (e.g., host.Platform
or host.OS) and only proceed for macOS/Apple Silicon hosts (e.g.,
"darwin"/"macos"); if the host is not macOS, return a clear platform-specific
error (e.g., a wrapped error like fmt.Errorf("recovery lock not supported on
platform %s", host.Platform)) to avoid the unnecessary DB query and provide a
clearer message—make this change around the existing
appConfig.MDM.EnabledAndConfigured check and the
svc.ds.GetHostRecoveryLockPassword call, referencing host.UUID and
svc.ds.GetHostRecoveryLockPassword.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@server/service/hosts.go`:
- Around line 3725-3737: Before calling svc.ds.GetHostRecoveryLockPassword,
check the host platform (e.g., host.Platform or host.OS) and only proceed for
macOS/Apple Silicon hosts (e.g., "darwin"/"macos"); if the host is not macOS,
return a clear platform-specific error (e.g., a wrapped error like
fmt.Errorf("recovery lock not supported on platform %s", host.Platform)) to
avoid the unnecessary DB query and provide a clearer message—make this change
around the existing appConfig.MDM.EnabledAndConfigured check and the
svc.ds.GetHostRecoveryLockPassword call, referencing host.UUID and
svc.ds.GetHostRecoveryLockPassword.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c78051e8-aa9e-44c5-b4b9-2e649340fce1

📥 Commits

Reviewing files that changed from the base of the PR and between 8b43190 and 7620800.

📒 Files selected for processing (5)
  • server/fleet/activities.go
  • server/fleet/service.go
  • server/mock/service/service_mock.go
  • server/service/handler.go
  • server/service/hosts.go

@mostlikelee
mostlikelee marked this pull request as ready for review March 12, 2026 15:51
@mostlikelee
mostlikelee requested a review from a team as a code owner March 12, 2026 15:51
@mostlikelee
mostlikelee merged commit 6268ebf into main Mar 16, 2026
51 checks passed
@mostlikelee
mostlikelee deleted the 40658-get-password branch March 16, 2026 19:48
mostlikelee pushed a commit that referenced this pull request Mar 18, 2026
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.

Set password: GET password for host API

3 participants