Adding my device link on host details page with backend logic to generate it if needed#45659
Open
georgekarrv wants to merge 2 commits into
Open
Adding my device link on host details page with backend logic to generate it if needed#45659georgekarrv wants to merge 2 commits into
georgekarrv wants to merge 2 commits into
Conversation
…rate it if needed
![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.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a global-admin-only way to open a host’s My device page from host details, with backend support to return or generate a device auth URL when needed. It also adjusts self-service activity text to avoid attributing those actions to “End user” when an admin may have opened the device page.
Changes:
- Added
/hosts/{id}/device_urlservice, route, datastore helpers, mocks, and integration coverage. - Added frontend host API wiring and a My device button on the User card for global admins.
- Updated My device page header/title and self-service activity rendering to passive voice.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
server/service/integration_core_test.go |
Adds integration tests for host device URL generation and authorization. |
server/service/hosts.go |
Implements backend HostDeviceURL logic. |
server/service/handler.go |
Registers the new host device URL route. |
server/mock/service/service_mock.go |
Adds service mock support for HostDeviceURL. |
server/mock/datastore_mock.go |
Adds datastore mock support for fresh/rotated device tokens. |
server/fleet/service.go |
Extends the service interface. |
server/fleet/datastore.go |
Extends the datastore interface. |
server/datastore/mysql/hosts.go |
Adds MySQL helpers for fresh token lookup and token rotation. |
frontend/utilities/endpoints.ts |
Adds the host device URL endpoint. |
frontend/services/entities/hosts.ts |
Adds frontend API method and response type. |
frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx |
Wires My device button behavior into host details. |
frontend/pages/hosts/details/DeviceUserPage/DeviceUserPage.tsx |
Updates device page title/header based on end user name. |
frontend/pages/hosts/details/cards/User/User.tsx |
Adds My device button rendering. |
frontend/pages/hosts/details/cards/User/User.tests.tsx |
Adds User card button visibility tests. |
frontend/pages/hosts/details/cards/User/_styles.scss |
Styles User card header actions. |
frontend/pages/hosts/details/cards/HostHeader/HostHeader.tsx |
Allows device-user header override. |
frontend/pages/hosts/details/cards/Activity/ActivityItems/InstalledSoftwareActivityItem/InstalledSoftwareActivityItem.tsx |
Renders self-service host activity in passive voice. |
frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsx |
Renders self-service global activity in passive voice. |
frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tests.tsx |
Updates self-service activity tests. |
frontend/interfaces/software.ts |
Adds passive install/uninstall status predicates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Confirm the host exists; 404 cleanly if not. | ||
| host, err := svc.ds.HostLite(ctx, hostID) |
Comment on lines
+3048
to
+3062
| const stmt = ` | ||
| INSERT INTO | ||
| host_device_auth ( host_id, token ) | ||
| VALUES | ||
| (?, ?) | ||
| ON DUPLICATE KEY UPDATE | ||
| token = VALUES(token), | ||
| previous_token = NULL | ||
| ` | ||
| if _, err := ds.writer(ctx).ExecContext(ctx, stmt, hostID, newToken); err != nil { | ||
| if IsDuplicate(err) { | ||
| return fleet.ConflictError{Message: "auth token conflicts with another host"} | ||
| } | ||
| return ctxerr.Wrap(ctx, err, "rotate host's device auth token") | ||
| } |
| // HostDeviceURL returns the full "My device" end-user URL for the | ||
| // specified host, embedding its device auth token. Global admin only — | ||
| // the URL is effectively a credential to that host's device-user page. | ||
| // Returns a NotFoundError if the host has no device auth token yet. |
| ue.GET("/api/_version_/fleet/hosts/{id:[0-9]+}/certificates", listHostCertificatesEndpoint, listHostCertificatesRequest{}) | ||
| ue.POST("/api/_version_/fleet/hosts/{id:[0-9]+}/certificates/{template_id:[0-9]+}/resend", resendHostCertificateTemplateEndpoint, resendHostCertificateTemplateRequest{}) | ||
| ue.GET("/api/_version_/fleet/hosts/{id:[0-9]+}/recovery_lock_password", getHostRecoveryLockPasswordEndpoint, getHostRecoveryLockPasswordRequest{}) | ||
| ue.GET("/api/_version_/fleet/hosts/{id:[0-9]+}/device_url", getHostDeviceURLEndpoint, getHostDeviceURLRequest{}) |
Contributor
WalkthroughAdds a "My device" feature and passive-voice self-service activity rendering. Frontend: new passive predicate helper, updated activity components/tests to render self-service events without actor context, HostDetailsPage handler to fetch/open a device URL, a guarded "My device" button on the user card, client API for device URLs, and device-user page header customization. Backend: datastore methods for TTL-bounded token lookup and token rotation, a Service.HostDeviceURL implementation and HTTP endpoint that reuses or rotates tokens, and integration tests for token reuse/rotation and access control. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
frontend/pages/hosts/details/cards/User/User.tests.tsx (1)
56-66: ⚡ Quick winAdd a click-behavior assertion for the new button callback.
At Line 56, this verifies rendering only; it should also assert
onClickMyDevicefires on click so the new prop contract is covered.Suggested test addition
import React from "react"; import { screen, render } from "`@testing-library/react`"; +import userEvent from "`@testing-library/user-event`"; import { noop } from "lodash"; @@ describe("My device button", () => { @@ it("renders the 'My device' button when canViewMyDeviceLink is true", () => { @@ expect(screen.getByText("My device")).toBeInTheDocument(); }); + + it("calls onClickMyDevice when clicked", async () => { + const onClickMyDevice = jest.fn(); + const user = userEvent.setup(); + render( + <User + endUsers={[]} + canViewMyDeviceLink + onClickMyDevice={onClickMyDevice} + onClickUpdateUser={noop} + /> + ); + + await user.click(screen.getByRole("button", { name: /my device/i })); + expect(onClickMyDevice).toHaveBeenCalledTimes(1); + }); });🤖 Prompt for 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. In `@frontend/pages/hosts/details/cards/User/User.tests.tsx` around lines 56 - 66, The test currently only checks rendering; update the "renders the 'My device' button when canViewMyDeviceLink is true" spec to also assert the callback fires by replacing the noop with a mock (e.g., jest.fn()) passed to the User component's onClickMyDevice prop, simulate a click on the "My device" button using fireEvent or userEvent, and expect the mock to have been called; locate the test for User and the onClickMyDevice prop to make this change.
🤖 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 `@frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx`:
- Around line 1073-1084: The onClickMyDevice handler uses window.open(...) which
can return null when the browser blocks popups; update onClickMyDevice to
capture the return value from window.open (after awaiting
hostAPI.getDeviceURL(host.id)) and explicitly handle a null result by calling
renderFlash("error", "Couldn't open My device page. The host may not have
checked in yet or your browser blocked popups.") (or a similar message) so users
get feedback when popups are blocked; reference the onClickMyDevice function,
hostAPI.getDeviceURL, window.open return value, and renderFlash when making this
change.
In `@server/datastore/mysql/hosts.go`:
- Line 3033: The token freshness check currently reads from a replica via
sqlx.GetContext(ctx, ds.reader(ctx), &token, stmt, hostID, tokenTTL.Seconds());
which can return stale data; change the DB handle to the primary by using
ds.writer(ctx) instead of ds.reader(ctx) in that sqlx.GetContext call (keeping
the same parameters: &token, stmt, hostID, tokenTTL.Seconds()) so token
lifecycle checks use the writer for read-after-write consistency.
In `@server/fleet/service.go`:
- Around line 518-522: The comment for HostDeviceURL is inaccurate about error
behavior: update the public contract comment for HostDeviceURL(ctx
context.Context, hostID uint) to state that it will create or rotate the host's
device auth token on demand (so it does not return a NotFoundError when no token
exists), keep that it is global-admin only and that the returned URL is a
credential to the host's device page, and instead note the actual error cases it
may return (e.g., permission or internal errors) so callers and mocks are not
misled.
In `@server/service/hosts.go`:
- Around line 2512-2564: HostDeviceURL returns a credential-like URL but doesn't
record who retrieved it; after you successfully obtain or generate the token
(i.e. after the block that sets token and before returning the formatted URL in
HostDeviceURL), emit a durable admin activity record including the acting user
(from viewer.FromContext's vc.User), the target host ID (host.ID) and an action
like "host_device_url_retrieved" (do not log the raw token). Use the service's
existing activity/audit function (call the project’s activity logging method
used elsewhere — e.g. svc.<activityLogMethod>(ctx, actorID, host.ID,
"host_device_url_retrieved", additional metadata)) and handle/log any error from
that call without preventing the URL from being returned.
- Around line 2536-2552: The current read-then-rotate flow using
svc.ds.GetDeviceAuthTokenIfFresh + server.GenerateRandomURLSafeText +
svc.ds.RotateDeviceAuthToken is racy: two concurrent callers can both see
NotFound and generate different tokens so one rotation immediately invalidates
the other; fix by moving the freshness-check-and-rotate logic into a single
atomic datastore operation (or DB transaction/row lock) so that a single helper
on svc.ds will return either the existing fresh token or perform exactly one
rotation and return the new token; implement e.g. a new method like
RotateOrGetDeviceAuthToken(ctx, hostID, ttl) on svc.ds that performs the
SELECT/UPDATE in one transaction and replace the GetDeviceAuthTokenIfFresh +
RotateDeviceAuthToken sequence with a single call to that method.
In `@server/service/integration_core_test.go`:
- Around line 17221-17227: The cleanup currently swallows errors from
s.ds.AppConfig and s.ds.SaveAppConfig; update the t.Cleanup closure to check
both errors and fail the test if either operation returns an error (e.g., call
t.Fatalf or t.Error with the error), so that failures restoring
ac.ServerSettings.ServerURL (and any SaveAppConfig failure) are surfaced instead
of ignored; specifically modify the t.Cleanup block that calls
s.ds.AppConfig(ctx) and s.ds.SaveAppConfig(ctx, ac) and validate/handle errors
for both while restoring ac.ServerSettings.ServerURL = origServerURL.
---
Nitpick comments:
In `@frontend/pages/hosts/details/cards/User/User.tests.tsx`:
- Around line 56-66: The test currently only checks rendering; update the
"renders the 'My device' button when canViewMyDeviceLink is true" spec to also
assert the callback fires by replacing the noop with a mock (e.g., jest.fn())
passed to the User component's onClickMyDevice prop, simulate a click on the "My
device" button using fireEvent or userEvent, and expect the mock to have been
called; locate the test for User and the onClickMyDevice prop to make this
change.
🪄 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: 6b996f2f-f80c-4a10-b4c5-1ff44ec2d243
📒 Files selected for processing (20)
frontend/interfaces/software.tsfrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tests.tsxfrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsxfrontend/pages/hosts/details/DeviceUserPage/DeviceUserPage.tsxfrontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsxfrontend/pages/hosts/details/cards/Activity/ActivityItems/InstalledSoftwareActivityItem/InstalledSoftwareActivityItem.tsxfrontend/pages/hosts/details/cards/HostHeader/HostHeader.tsxfrontend/pages/hosts/details/cards/User/User.tests.tsxfrontend/pages/hosts/details/cards/User/User.tsxfrontend/pages/hosts/details/cards/User/_styles.scssfrontend/services/entities/hosts.tsfrontend/utilities/endpoints.tsserver/datastore/mysql/hosts.goserver/fleet/datastore.goserver/fleet/service.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/handler.goserver/service/hosts.goserver/service/integration_core_test.go
Comment on lines
+1073
to
+1084
| const onClickMyDevice = async () => { | ||
| if (!host) return; | ||
| try { | ||
| const { device_url } = await hostAPI.getDeviceURL(host.id); | ||
| window.open(device_url, "_blank", "noopener,noreferrer"); | ||
| } catch (e) { | ||
| renderFlash( | ||
| "error", | ||
| "Couldn't open My device page. The host may not have checked in yet." | ||
| ); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Handle popup-blocked opens explicitly.
At Line 1077, window.open(...) can return null without throwing, so users may get no feedback when popups are blocked.
Suggested fix
try {
const { device_url } = await hostAPI.getDeviceURL(host.id);
- window.open(device_url, "_blank", "noopener,noreferrer");
+ const opened = window.open(device_url, "_blank", "noopener,noreferrer");
+ if (!opened) {
+ renderFlash(
+ "error",
+ "Couldn't open My device page. Please allow pop-ups and try again."
+ );
+ return;
+ }
} catch (e) {
renderFlash(
"error",
"Couldn't open My device page. The host may not have checked in yet."
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const onClickMyDevice = async () => { | |
| if (!host) return; | |
| try { | |
| const { device_url } = await hostAPI.getDeviceURL(host.id); | |
| window.open(device_url, "_blank", "noopener,noreferrer"); | |
| } catch (e) { | |
| renderFlash( | |
| "error", | |
| "Couldn't open My device page. The host may not have checked in yet." | |
| ); | |
| } | |
| }; | |
| const onClickMyDevice = async () => { | |
| if (!host) return; | |
| try { | |
| const { device_url } = await hostAPI.getDeviceURL(host.id); | |
| const opened = window.open(device_url, "_blank", "noopener,noreferrer"); | |
| if (!opened) { | |
| renderFlash( | |
| "error", | |
| "Couldn't open My device page. Please allow pop-ups and try again." | |
| ); | |
| return; | |
| } | |
| } catch (e) { | |
| renderFlash( | |
| "error", | |
| "Couldn't open My device page. The host may not have checked in yet." | |
| ); | |
| } | |
| }; |
🤖 Prompt for 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.
In `@frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx` around
lines 1073 - 1084, The onClickMyDevice handler uses window.open(...) which can
return null when the browser blocks popups; update onClickMyDevice to capture
the return value from window.open (after awaiting hostAPI.getDeviceURL(host.id))
and explicitly handle a null result by calling renderFlash("error", "Couldn't
open My device page. The host may not have checked in yet or your browser
blocked popups.") (or a similar message) so users get feedback when popups are
blocked; reference the onClickMyDevice function, hostAPI.getDeviceURL,
window.open return value, and renderFlash when making this change.
| const stmt = `SELECT token FROM host_device_auth WHERE host_id = ? AND updated_at >= DATE_SUB(NOW(), INTERVAL ? SECOND)` //nolint:gosec // G101 false positive, this is a SQL query | ||
|
|
||
| var token string | ||
| switch err := sqlx.GetContext(ctx, ds.reader(ctx), &token, stmt, hostID, tokenTTL.Seconds()); { |
Contributor
There was a problem hiding this comment.
Use primary DB for token freshness checks in auth flow.
Line 3033 reads from ds.reader(ctx). For device-auth token lifecycle, replica lag can return stale/missing data and cause unnecessary rotations or inconsistent link behavior. Use ds.writer(ctx) for read-after-write consistency.
Suggested patch
- switch err := sqlx.GetContext(ctx, ds.reader(ctx), &token, stmt, hostID, tokenTTL.Seconds()); {
+ switch err := sqlx.GetContext(ctx, ds.writer(ctx), &token, stmt, hostID, tokenTTL.Seconds()); {🤖 Prompt for 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.
In `@server/datastore/mysql/hosts.go` at line 3033, The token freshness check
currently reads from a replica via sqlx.GetContext(ctx, ds.reader(ctx), &token,
stmt, hostID, tokenTTL.Seconds()); which can return stale data; change the DB
handle to the primary by using ds.writer(ctx) instead of ds.reader(ctx) in that
sqlx.GetContext call (keeping the same parameters: &token, stmt, hostID,
tokenTTL.Seconds()) so token lifecycle checks use the writer for
read-after-write consistency.
Comment on lines
+518
to
+522
| // HostDeviceURL returns the full "My device" end-user URL for the | ||
| // specified host, embedding its device auth token. Global admin only — | ||
| // the URL is effectively a credential to that host's device-user page. | ||
| // Returns a NotFoundError if the host has no device auth token yet. | ||
| HostDeviceURL(ctx context.Context, hostID uint) (string, error) |
Contributor
There was a problem hiding this comment.
Fix the public contract comment.
HostDeviceURL no longer returns NotFoundError when a host lacks a token; the implementation now creates or rotates one on demand. Leaving this comment as-is will mislead callers and mock implementations.
📝 Suggested doc fix
- // Returns a NotFoundError if the host has no device auth token yet.
+ // Generates a device auth token if needed and returns the resulting URL.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // HostDeviceURL returns the full "My device" end-user URL for the | |
| // specified host, embedding its device auth token. Global admin only — | |
| // the URL is effectively a credential to that host's device-user page. | |
| // Returns a NotFoundError if the host has no device auth token yet. | |
| HostDeviceURL(ctx context.Context, hostID uint) (string, error) | |
| // HostDeviceURL returns the full "My device" end-user URL for the | |
| // specified host, embedding its device auth token. Global admin only — | |
| // the URL is effectively a credential to that host's device-user page. | |
| // Generates a device auth token if needed and returns the resulting URL. | |
| HostDeviceURL(ctx context.Context, hostID uint) (string, error) |
🤖 Prompt for 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.
In `@server/fleet/service.go` around lines 518 - 522, The comment for
HostDeviceURL is inaccurate about error behavior: update the public contract
comment for HostDeviceURL(ctx context.Context, hostID uint) to state that it
will create or rotate the host's device auth token on demand (so it does not
return a NotFoundError when no token exists), keep that it is global-admin only
and that the returned URL is a credential to the host's device page, and instead
note the actual error cases it may return (e.g., permission or internal errors)
so callers and mocks are not misled.
Comment on lines
+2512
to
+2564
| // The URL is effectively a credential to act as the device's end user, so | ||
| // access is restricted to global admins regardless of team-scoped | ||
| // permissions. | ||
| func (svc *Service) HostDeviceURL(ctx context.Context, hostID uint) (string, error) { | ||
| // First-pass authz so the middleware is satisfied; we apply a stricter | ||
| // global-admin check below. | ||
| if err := svc.authz.Authorize(ctx, &fleet.Host{}, fleet.ActionList); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| vc, ok := viewer.FromContext(ctx) | ||
| if !ok { | ||
| return "", fleet.ErrNoContext | ||
| } | ||
| if vc.User == nil || vc.User.GlobalRole == nil || *vc.User.GlobalRole != fleet.RoleAdmin { | ||
| return "", fleet.NewPermissionError("only global admins can retrieve a host's device URL") | ||
| } | ||
|
|
||
| // Confirm the host exists; 404 cleanly if not. | ||
| host, err := svc.ds.HostLite(ctx, hostID) | ||
| if err != nil { | ||
| return "", ctxerr.Wrap(ctx, err, "get host for device url") | ||
| } | ||
|
|
||
| // Reuse the existing token if it's still within the TTL — saves us from | ||
| // invalidating a link a user may already be holding. | ||
| token, err := svc.ds.GetDeviceAuthTokenIfFresh(ctx, host.ID, hostDeviceAuthTokenTTL) | ||
| switch { | ||
| case err == nil: | ||
| // fresh token in hand | ||
| case fleet.IsNotFound(err): | ||
| // Either no token row exists yet, or the existing one is expired. | ||
| // Generate a fresh token and upsert it, clearing previous_token. | ||
| newToken, genErr := server.GenerateRandomURLSafeText(24) | ||
| if genErr != nil { | ||
| return "", ctxerr.Wrap(ctx, genErr, "generate new device auth token") | ||
| } | ||
| if rotErr := svc.ds.RotateDeviceAuthToken(ctx, host.ID, newToken); rotErr != nil { | ||
| return "", ctxerr.Wrap(ctx, rotErr, "rotate device auth token") | ||
| } | ||
| token = newToken | ||
| default: | ||
| return "", ctxerr.Wrap(ctx, err, "check fresh device auth token") | ||
| } | ||
|
|
||
| ac, err := svc.ds.AppConfig(ctx) | ||
| if err != nil { | ||
| return "", ctxerr.Wrap(ctx, err, "get app config for server url") | ||
| } | ||
|
|
||
| base := strings.TrimRight(ac.ServerSettings.ServerURL, "/") | ||
| return fmt.Sprintf("%s/device/%s", base, token), nil | ||
| } |
Contributor
There was a problem hiding this comment.
Audit device-URL issuance.
This method returns a credential-equivalent URL but never records who retrieved it. Unlike the other secret-retrieval paths in this file, that leaves no durable activity trail if a link is shared or misused. Please emit an admin activity whenever a device URL is returned.
🤖 Prompt for 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.
In `@server/service/hosts.go` around lines 2512 - 2564, HostDeviceURL returns a
credential-like URL but doesn't record who retrieved it; after you successfully
obtain or generate the token (i.e. after the block that sets token and before
returning the formatted URL in HostDeviceURL), emit a durable admin activity
record including the acting user (from viewer.FromContext's vc.User), the target
host ID (host.ID) and an action like "host_device_url_retrieved" (do not log the
raw token). Use the service's existing activity/audit function (call the
project’s activity logging method used elsewhere — e.g.
svc.<activityLogMethod>(ctx, actorID, host.ID, "host_device_url_retrieved",
additional metadata)) and handle/log any error from that call without preventing
the URL from being returned.
Comment on lines
+2536
to
+2552
| // Reuse the existing token if it's still within the TTL — saves us from | ||
| // invalidating a link a user may already be holding. | ||
| token, err := svc.ds.GetDeviceAuthTokenIfFresh(ctx, host.ID, hostDeviceAuthTokenTTL) | ||
| switch { | ||
| case err == nil: | ||
| // fresh token in hand | ||
| case fleet.IsNotFound(err): | ||
| // Either no token row exists yet, or the existing one is expired. | ||
| // Generate a fresh token and upsert it, clearing previous_token. | ||
| newToken, genErr := server.GenerateRandomURLSafeText(24) | ||
| if genErr != nil { | ||
| return "", ctxerr.Wrap(ctx, genErr, "generate new device auth token") | ||
| } | ||
| if rotErr := svc.ds.RotateDeviceAuthToken(ctx, host.ID, newToken); rotErr != nil { | ||
| return "", ctxerr.Wrap(ctx, rotErr, "rotate device auth token") | ||
| } | ||
| token = newToken |
Contributor
There was a problem hiding this comment.
Make token reuse/rotation atomic.
This read-then-rotate sequence is racy. If two admins request a URL for the same host while the token is missing or expired, both calls can observe NotFound, both generate a new token, and the second RotateDeviceAuthToken invalidates the first URL immediately. This needs to be a single datastore operation or transaction that reuses the fresh token or rotates exactly once.
🔒 Suggested direction
- token, err := svc.ds.GetDeviceAuthTokenIfFresh(ctx, host.ID, hostDeviceAuthTokenTTL)
- switch {
- case err == nil:
- case fleet.IsNotFound(err):
- newToken, genErr := server.GenerateRandomURLSafeText(24)
- if genErr != nil {
- return "", ctxerr.Wrap(ctx, genErr, "generate new device auth token")
- }
- if rotErr := svc.ds.RotateDeviceAuthToken(ctx, host.ID, newToken); rotErr != nil {
- return "", ctxerr.Wrap(ctx, rotErr, "rotate device auth token")
- }
- token = newToken
- default:
- return "", ctxerr.Wrap(ctx, err, "check fresh device auth token")
- }
+ token, err := svc.ds.GetOrRotateDeviceAuthToken(ctx, host.ID, hostDeviceAuthTokenTTL)
+ if err != nil {
+ return "", ctxerr.Wrap(ctx, err, "get or rotate device auth token")
+ }That helper should perform the freshness check and rotation under one transaction or row lock.
🤖 Prompt for 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.
In `@server/service/hosts.go` around lines 2536 - 2552, The current
read-then-rotate flow using svc.ds.GetDeviceAuthTokenIfFresh +
server.GenerateRandomURLSafeText + svc.ds.RotateDeviceAuthToken is racy: two
concurrent callers can both see NotFound and generate different tokens so one
rotation immediately invalidates the other; fix by moving the
freshness-check-and-rotate logic into a single atomic datastore operation (or DB
transaction/row lock) so that a single helper on svc.ds will return either the
existing fresh token or perform exactly one rotation and return the new token;
implement e.g. a new method like RotateOrGetDeviceAuthToken(ctx, hostID, ttl) on
svc.ds that performs the SELECT/UPDATE in one transaction and replace the
GetDeviceAuthTokenIfFresh + RotateDeviceAuthToken sequence with a single call to
that method.
Comment on lines
+17221
to
+17227
| t.Cleanup(func() { | ||
| ac, err := s.ds.AppConfig(ctx) | ||
| if err != nil { | ||
| return | ||
| } | ||
| ac.ServerSettings.ServerURL = origServerURL | ||
| _ = s.ds.SaveAppConfig(ctx, ac) |
Contributor
There was a problem hiding this comment.
Don’t swallow app-config restore failures in cleanup.
At Line 17223 and Line 17227, cleanup errors are ignored, which can leak test state into later tests and hide the root cause of flakiness. Fail explicitly if restore cannot complete.
Proposed fix
t.Cleanup(func() {
ac, err := s.ds.AppConfig(ctx)
- if err != nil {
- return
- }
+ require.NoError(t, err)
ac.ServerSettings.ServerURL = origServerURL
- _ = s.ds.SaveAppConfig(ctx, ac)
+ require.NoError(t, s.ds.SaveAppConfig(ctx, ac))
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| t.Cleanup(func() { | |
| ac, err := s.ds.AppConfig(ctx) | |
| if err != nil { | |
| return | |
| } | |
| ac.ServerSettings.ServerURL = origServerURL | |
| _ = s.ds.SaveAppConfig(ctx, ac) | |
| t.Cleanup(func() { | |
| ac, err := s.ds.AppConfig(ctx) | |
| require.NoError(t, err) | |
| ac.ServerSettings.ServerURL = origServerURL | |
| require.NoError(t, s.ds.SaveAppConfig(ctx, ac)) | |
| }) |
🤖 Prompt for 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.
In `@server/service/integration_core_test.go` around lines 17221 - 17227, The
cleanup currently swallows errors from s.ds.AppConfig and s.ds.SaveAppConfig;
update the t.Cleanup closure to check both errors and fail the test if either
operation returns an error (e.g., call t.Fatalf or t.Error with the error), so
that failures restoring ac.ServerSettings.ServerURL (and any SaveAppConfig
failure) are surfaced instead of ignored; specifically modify the t.Cleanup
block that calls s.ds.AppConfig(ctx) and s.ds.SaveAppConfig(ctx, ac) and
validate/handle errors for both while restoring ac.ServerSettings.ServerURL =
origServerURL.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #45659 +/- ##
==========================================
- Coverage 66.74% 66.74% -0.01%
==========================================
Files 2744 2744
Lines 219316 219421 +105
Branches 10831 10975 +144
==========================================
+ Hits 146393 146448 +55
- Misses 59697 59738 +41
- Partials 13226 13235 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #43895
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
For unreleased bug fixes in a release candidate, one of:
Database migrations
COLLATE utf8mb4_unicode_ci).New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsfleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit
New Features
Tests