Added endpoint to fleetdm.com proxy - #46107
Conversation
|
@coderabbitai full review |
|
/agentic_review |
✅ Actions performedFull review triggered. |
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Pull request overview
Adds a new fleetdm.com (website) Android proxy endpoint that allows Fleet to issue Android Management API (AMAPI) commands (e.g. LOCK / RESET_PASSWORD / WIPE) against an enterprise-managed device.
Changes:
- Added a new
POST /api/android/v1/enterprises/:androidEnterpriseId/devices/...route for issuing device commands. - Implemented a new Sails controller that authenticates via Fleet server secret and forwards an AMAPI
issueCommandrequest to Google.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| website/config/routes.js | Adds the new Android proxy route for device command issuance. |
| website/api/controllers/android-proxy/issue-command-on-android-device.js | Implements the authenticated proxying logic to Google’s Android Management API enterprises.devices.issueCommand. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new POST endpoint and Cloud SDK method to issue Android Management API commands to a device. The controller extracts and validates a bearer token and enterprise secret, verifies the enterprise is managed by Fleet, builds an explicit AMAPI commandBody (preserving intentionally empty values), calls enterprises.devices.issueCommand via sails.helpers.flow.build with service-account credentials, maps rate-limit and "device no longer managed" errors to structured exits, and returns the operation response. The route is mounted at /api/android/v1/enterprises/:androidEnterpriseId/devices/:deviceId::issueCommand with CSRF disabled. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@website/api/controllers/android-proxy/issue-command-on-android-device.js`:
- Around line 179-181: Instead of returning an object from the intercept block,
throw the custom exit signal so the action triggers the deviceNoLongerManaged
exit; locate the intercept where it currently does "return
{'deviceNoLongerManaged': 'The device is no longer managed by the Android
enterprise.'};" and replace that return with a throw of the exit object (throw {
deviceNoLongerManaged: 'The device is no longer managed by the Android
enterprise.' }) so the action pipeline invokes the deviceNoLongerManaged exit
properly.
- Around line 173-183: The intercept handlers in
issue-command-on-android-device.js currently interpolate raw error objects (err)
into new Error strings in the 429 handler and the general intercept, which can
leak sensitive upstream details; update both intercept callbacks to build a
sanitized error summary (e.g., extract only err.status/err.code and err.message
or String(err)) and include that sanitized summary in the returned Error or in
the object returned for deviceNoLongerManaged, rather than interpolating the
full err object; reference the existing intercepts (the 429 intercept that logs
via sails.log.warn and returns new Error(...), and the subsequent .intercept
that checks deviceNoLongerManaged) and replace err interpolation with a small
sanitizedError variable composed of permitted fields before including it in the
returned Error message.
- Around line 90-92: The current authHeader parsing (checking
authHeader.startsWith('Bearer') and then replace) is too permissive and
case-sensitive; change the logic around authHeader to strictly match the pattern
"Bearer <token>" using a case-insensitive regex (e.g., /^Bearer\s+(.+)$/i) to
capture the token into fleetServerSecret and otherwise treat the header as
invalid (fall through to the error/rejection branch). Update the block that
references authHeader and fleetServerSecret so only a successful regex match
sets fleetServerSecret; do not accept headers missing the space or with
different casing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4bc9f332-aa4d-4dd0-a89e-d01e5f990a24
📒 Files selected for processing (2)
website/api/controllers/android-proxy/issue-command-on-android-device.jswebsite/config/routes.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
@eashaw are you the right person to review this change? I manually QA'd it with the corresponding Fleet server changes. |
|
@eashaw Are you able to review this one today? All the server changes are already on main for this. |
|
@getvictor Yes! Sorry for the delay. |
| let issueCommandResponse = await sails.helpers.flow.build(async () => { | ||
| let { google } = require('googleapis'); | ||
| let androidmanagement = google.androidmanagement('v1'); | ||
| let googleAuth = new google.auth.GoogleAuth({ |
There was a problem hiding this comment.
Does this mean that the Google service account auth client is re-instantiated on every request?
There was a problem hiding this comment.
Does this mean that the Google service account auth client is re-instantiated on every request?
Yes, that is our existing pattern. I'm not sure how much overhead this is. Maybe we can speed things up with a shared client.
There was a problem hiding this comment.
https://docs.cloud.google.com/apis/docs/client-libraries-best-practices
When making requests with the same library, you should reuse the same client object for many requests when possible, instead of creating a new one for every request. Requests from the same instance will share authentication credential instances.
"can take multiple seconds"
lol. well the good news is that we aren't introducing a new pattern here. But seems like a good thing to address in the near future.
There was a problem hiding this comment.
@ksykulev Yes, seems like a good reliability issue. Can you file one? Also we should audit how we handle the client on the server.
ksykulev
left a comment
ksykulev
left a comment
There was a problem hiding this comment.
From what I see this is following the same patterns as we have in other places. Looks like it handles errors properly and proxies correctly. So i'm good with this.
Related issue: Resolves #41683
fleetdm.com for Android commands:
https://developers.google.com/android/management/reference/rest/v1/enterprises.devices/issueCommand
Checklist for submitter
Testing
Summary by CodeRabbit