Skip to content

Manually update & delete host IdP mappings - #35325

Merged
jacobshandling merged 31 commits into
mainfrom
34222-IdP-UI-DELETE-activities
Nov 13, 2025
Merged

Manually update & delete host IdP mappings#35325
jacobshandling merged 31 commits into
mainfrom
34222-IdP-UI-DELETE-activities

Conversation

@jacobshandling

@jacobshandling jacobshandling commented Nov 7, 2025

Copy link
Copy Markdown
Contributor

Related issue: Resolves #34222

Demo

Screenshot 2025-11-10 at 4 45 48 PM

Checklist for submitter

If some of the following don't apply, delete the relevant line.

  • Changes file added for user-visible changes in changes/,

Testing

  • Added/updated automated tests
  • QA'd all new/changed functionality manually

@jacobshandling jacobshandling changed the title 34222 id p UI delete activities Update & Delete Host IdP Mappings Nov 7, 2025
@codecov

codecov Bot commented Nov 7, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 39.86014% with 86 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.19%. Comparing base (31f533d) to head (4f78e72).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
.../hosts/details/HostDetailsPage/HostDetailsPage.tsx 0.00% 28 Missing ⚠️
...mponents/UpdateEndUserModal/UpdateEndUserModal.tsx 12.50% 14 Missing ⚠️
server/datastore/mysql/hosts.go 45.45% 6 Missing and 6 partials ⚠️
server/service/hosts.go 73.91% 6 Missing and 6 partials ⚠️
...vityFeed/GlobalActivityItem/GlobalActivityItem.tsx 0.00% 8 Missing ⚠️
frontend/services/entities/hosts.ts 0.00% 6 Missing ⚠️
frontend/pages/hosts/details/cards/User/User.tsx 69.23% 3 Missing and 1 partial ⚠️
frontend/utilities/endpoints.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #35325      +/-   ##
==========================================
- Coverage   66.24%   66.19%   -0.05%     
==========================================
  Files        2104     2089      -15     
  Lines      178307   177484     -823     
  Branches     7424     7234     -190     
==========================================
- Hits       118122   117493     -629     
+ Misses      49363    49158     -205     
- Partials    10822    10833      +11     
Flag Coverage Δ
backend 67.90% <65.21%> (+<0.01%) ⬆️
fleetd-chrome ?
frontend 53.64% <16.21%> (-0.09%) ⬇️

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.

Comment thread server/service/handler.go Outdated
@@ -437,6 +437,7 @@ func attachFleetAPIRoutes(r *mux.Router, svc fleet.Service, config config.FleetC
// Deprecated: Device mappings are included in the host details endpoint: /api/_version_/fleet/hosts/{id}
ue.GET("/api/_version_/fleet/hosts/{id:[0-9]+}/device_mapping", listHostDeviceMappingEndpoint, listHostDeviceMappingRequest{})
ue.PUT("/api/_version_/fleet/hosts/{id:[0-9]+}/device_mapping", putHostDeviceMappingEndpoint, putHostDeviceMappingRequest{})
ue.DELETE("/api/_version_/fleet/hosts/{id:[0-9]+}/device_mapping", deleteHostIDPEndpoint, deleteHostIDPRequest{})

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.

I would make this change to make it clearer to customers which kind of device mapping this endpoint is deleting

Suggested change
ue.DELETE("/api/_version_/fleet/hosts/{id:[0-9]+}/device_mapping", deleteHostIDPEndpoint, deleteHostIDPRequest{})
ue.DELETE("/api/_version_/fleet/hosts/{id:[0-9]+}/device_mapping/idp", deleteHostIDPEndpoint, deleteHostIDPRequest{})

@jacobshandling

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Nov 11, 2025

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@jacobshandling jacobshandling changed the title Update & Delete Host IdP Mappings Manually update & delete host IdP mappings Nov 11, 2025
@coderabbitai

coderabbitai Bot commented Nov 11, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Implement manual IdP (Identity Provider) host mapping management by replacing the Add End User modal with an Update/Delete-capable modal, adding a DELETE API endpoint, and integrating activity logging across the frontend UI and backend service layers.

Changes

Cohort / File(s) Summary
Frontend Activity Types & Interfaces
frontend/interfaces/activity.ts, frontend/interfaces/host.ts
Added ActivityType.EditedHostIdpData enum value and optional host_idp_username field to IActivityDetails. Added documentation comment to IHost.end_users property.
Frontend Activity Display
frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsx
Added editedHostIdpData template function to render activity text indicating when host end-user data was set or removed; wired into getDetail switch for new activity type.
Frontend User Card & Modal Components
frontend/pages/hosts/details/cards/User/User.tsx, frontend/pages/hosts/details/cards/User/User.tests.tsx, frontend/pages/hosts/details/cards/User/components/AddEndUserModal/*, frontend/pages/hosts/details/cards/User/components/UpdateEndUserModal/*
Removed AddEndUserModal component and styling. Added new UpdateEndUserModal component. Refactored User card props: removed platform and enableAddEndUser; added canWriteEndUser and onClickUpdateUser. Updated button logic to show "Add user" or "Edit user" based on IdP username presence. Updated tests to verify permission-based button visibility.
Frontend Host Details Pages
frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx, frontend/pages/hosts/details/DeviceUserPage/DeviceUserPage.tsx
Replaced AddEndUserModal with UpdateEndUserModal in HostDetailsPage; added isUpdating state coordination. Simplified DeviceUserPage to always render AboutCard with default width and UserCard. Integrated canWriteEndUser permission check using AppContext (isTeamMaintainerOrTeamAdmin, isGlobalAdmin, isGlobalMaintainer). Updated modal and card prop signatures accordingly.
Frontend API Client & Endpoints
frontend/services/entities/hosts.ts, frontend/utilities/endpoints.ts
Added updateHostIdp(hostId, idpUsername) and deleteHostIdp(hostId) service methods. Added HOST_DEVICE_MAPPING and HOST_DEVICE_MAPPING_IDP endpoint constants.
Backend Activity Types
server/fleet/activities.go
Added ActivityTypeEditedHostIdpData struct with host_id, host_display_name, and host_idp_username fields; implemented ActivityName() and Documentation() methods; registered in ActivityDetailsList.
Backend Datastore & Service Interfaces
server/fleet/datastore.go, server/fleet/service.go
Added DeleteHostIDP(ctx context.Context, id uint) error method to both Datastore and Service interfaces.
Backend Mock Implementations
server/mock/datastore_mock.go, server/mock/service/service_mock.go
Added DeleteHostIDPFunc function type, DeleteHostIDPFunc field, and DeleteHostIDPFuncInvoked tracking flag to both DataStore and Service mock structs; implemented corresponding DeleteHostIDP methods.
Backend Service Handlers & Implementation
server/service/handler.go, server/service/hosts.go
Added DELETE endpoint at /api/_version_/fleet/hosts/{id}/device_mapping/idp. Implemented deleteHostIDPRequest and deleteHostIDPResponse types; deleteHostIDPEndpoint handler. Implemented Service.DeleteHostIDP method calling datastore; updated SetHostDeviceMapping to create EditedHostIdpData activity logs and retrieve host details.
Backend Service Tests
server/service/hosts_test.go
Added NewActivityFunc and AppConfigFunc mocks to TestSetHostDeviceMapping. Added TestDeleteHostDeviceIDPMapping with subtests for premium tier success and free tier license failure.

Sequence Diagram

sequenceDiagram
    participant UI as User Interface
    participant Modal as UpdateEndUserModal
    participant Service as Frontend Service
    participant Backend as Backend API
    participant Store as Datastore
    participant Log as Activity Log

    UI->>Modal: Open modal (edit/add IdP username)
    Modal->>Modal: Render form with existing or empty username
    
    UI->>Modal: Submit username
    Modal->>Service: updateHostIdp(hostId, idpUsername)
    Service->>Backend: PUT /fleet/hosts/{id}/device_mapping
    Backend->>Store: SetHostDeviceMapping(...)
    Store->>Log: Create EditedHostIdpData activity
    Store-->>Backend: Success
    Backend-->>Service: 200 OK
    Service-->>Modal: Success
    Modal->>UI: Show toast, close modal
    
    UI->>Modal: Open delete flow
    Modal->>Service: deleteHostIdp(hostId)
    Service->>Backend: DELETE /fleet/hosts/{id}/device_mapping/idp
    Backend->>Store: DeleteHostIDP(...)
    Store->>Log: Create EditedHostIdpData activity (empty username)
    Store-->>Backend: Success
    Backend-->>Service: 200 OK
    Service-->>Modal: Success
    Modal->>UI: Show toast, close modal
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Areas requiring attention during review:

  • Permission logic in HostDetailsPage: Verify canWriteEndUser calculation correctly combines isTeamMaintainerOrTeamAdmin, isGlobalAdmin, and isGlobalMaintainer from AppContext
  • DeleteHostIDP datastore implementation: Confirm deletion of both DeviceMappingIDP and DeviceMappingMDMIdpAccounts, SCIM user mapping cleanup, and transactional consistency
  • Activity logging consistency: Ensure EditedHostIdpData activities are created for both PUT (update) and DELETE flows with correct host_idp_username values
  • User component prop migration: Validate that all callers of User card now provide canWriteEndUser and onClickUpdateUser instead of removed props
  • Modal integration: Confirm UpdateEndUserModal properly handles premium tier gating and both add/edit/delete flows via onUpdate handler
  • Test coverage: Review new DeleteHostIDP endpoint tests cover both premium and free tier license scenarios

Possibly related PRs

Suggested reviewers

  • mostlikelee
  • getvictor

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ⚠️ Warning The PR description is largely incomplete. While it includes the issue reference and demo link, it lacks required sections such as database migration checks, input validation verification, SQL injection prevention confirmation, and testing details beyond the template checkboxes. Complete the PR description by filling in all applicable checklist items with explanations, including database migration considerations, validation details, testing methodology, and any other relevant sections from the template.
Out of Scope Changes check ❓ Inconclusive Changes are focused on IdP mapping management with some necessary refactoring to the User component and related modals. The UserCard refactoring (simplified prop set, always-visible rendering) appears to be an incidental simplification rather than a primary objective. Clarify whether the UserCard/DeviceUserPage refactoring (always-visible rendering, simplified props) was intentional scope creep or a necessary prerequisite for the IdP feature implementation.
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR successfully implements all three main objectives from issue #34222: adds/updates modal UI for IdP username management, implements a DELETE endpoint for IdP mappings, and adds activity tracking with proper UI display.
Title check ✅ Passed The title 'Manually update & delete host IdP mappings' clearly and concisely summarizes the main changes in the pull request, which involve UI updates for modifying and removing IdP mappings and new DELETE endpoint functionality.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 34222-IdP-UI-DELETE-activities

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.

Actionable comments posted: 2

🧹 Nitpick comments (13)
frontend/interfaces/host.ts (1)

311-313: Encode the “at most 1 end user” invariant in the type (or clarify).

If there is truly max 1 item, consider:

  • end_user?: IHostEndUser (singular), or
  • end_users?: [IHostEndUser] to enforce a 0–1 tuple.

If changing the shape is risky now, add TSDoc clarifying consumers should only read index 0. Your call.

changes/34222-manual-IdP-update-ui-delete-activities (1)

1-3: Include concrete API and activity identifiers.

Add explicit path and identifiers to aid release notes and QA:

  • DELETE /api/v1/fleet/hosts/{id}/device_mapping/idp
  • ActivityType: "edited_host_idp_data"
  • Details fields: host_id, host_display_name, host_idp_username
frontend/interfaces/activity.ts (1)

262-263: Document semantics: empty vs undefined host_idp_username.

If deletion is represented by empty string or by omitting the field, document it here to keep frontend logic consistent across consumers.

frontend/services/entities/hosts.ts (1)

677-684: Implement input validation and clarify API field semantics.

Endpoints are correctly defined and both methods are actively used in HostDetailsPage.tsx (lines 900, 903). However, the suggested improvements should still be applied:

  • Add clarifying comment about the "email" field carrying IdP username
  • Move trim logic to service layer for robustness; while the UI checks for empty string (line 899 in HostDetailsPage.tsx), the service should be defensive and trim input directly

Proposed implementation at frontend/services/entities/hosts.ts lines 677-680:

  updateHostIdp(hostId: number, idpUsername: string) {
    const path = endpoints.HOST_DEVICE_MAPPING(hostId);
+   const username = idpUsername?.trim();
+   // Note: API uses "email" field to carry IdP username for source "idp".
+   if (!username) {
+     return this.deleteHostIdp(hostId);
+   }
-   return sendRequest("PUT", path, { source: "idp", email: idpUsername });
+   return sendRequest("PUT", path, { source: "idp", email: username });
  },
server/mock/datastore_mock.go (1)

264-265: New mock hook for DeleteHostIDP added — consistent with pattern.

Signature and placement align with existing host mapping funcs. No changes requested. Optional: if feasible at interface level, prefer naming the param hostID for clarity (this file is autogenerated, so not here).

server/service/hosts_test.go (1)

3314-3363: Consider expanding test coverage for DeleteHostIDP.

The test covers basic license validation, which is good. However, compared to similar tests in this file (e.g., TestSetHostDeviceMapping, TestLockUnlockWipeHostAuth), the coverage is limited. Consider adding:

  1. Authorization test cases for different user roles:

    • Global/team observer (should fail)
    • Global/team maintainer (should succeed on premium)
    • Team admin for wrong team (should fail)
  2. Error handling scenarios:

    • Host not found (HostLiteFunc returns error)
    • DeleteHostIDPFunc returns an error
    • NewActivityFunc returns an error
  3. Parameter validation in mocks to verify correct values are passed:

 ds.DeleteHostIDPFunc = func(ctx context.Context, id uint) error {
+    require.Equal(t, uint(1), id)
     return nil
 }
 ds.NewActivityFunc = func(ctx context.Context, user *fleet.User, activity fleet.ActivityDetails, details []byte, createdAt time.Time) error {
+    act, ok := activity.(fleet.ActivityTypeEditedHostIdpData)
+    require.True(t, ok)
+    require.Equal(t, uint(1), act.HostID)
     return nil
 }

These additions would bring the test coverage in line with similar functionality in this file.

server/datastore/mysql/hosts.go (1)

3874-3874: Consider renaming parameter for consistency.

The parameter id represents a host ID. For consistency with other functions in this file (e.g., deleteHostSCIMUserMapping at line 4428, associateHostWithScimUser at line 4411), consider renaming it to hostID to improve code clarity.

-func (ds *Datastore) DeleteHostIDP(ctx context.Context, id uint) error {
+func (ds *Datastore) DeleteHostIDP(ctx context.Context, hostID uint) error {
 	delStmt := `DELETE FROM host_emails WHERE host_id = ? AND source = ?`
 
 	err := ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error {
 		var idpDelRes, mdmIdpDelRes sql.Result
-		idpDelRes, err := tx.ExecContext(ctx, delStmt, id, fleet.DeviceMappingIDP)
+		idpDelRes, err := tx.ExecContext(ctx, delStmt, hostID, fleet.DeviceMappingIDP)
 		if err != nil {
 			return ctxerr.Wrap(ctx, err, "delete existing IdP device mappings")
 		}
 		idpRowsAffected, err := idpDelRes.RowsAffected()
 		if err != nil {
 			return ctxerr.Wrap(ctx, err, "delete existing IdP device mappings - get IdP rows affected")
 		}
 
-		mdmIdpDelRes, err = tx.ExecContext(ctx, delStmt, id, fleet.DeviceMappingMDMIdpAccounts)
+		mdmIdpDelRes, err = tx.ExecContext(ctx, delStmt, hostID, fleet.DeviceMappingMDMIdpAccounts)
 		if err != nil {
 			return ctxerr.Wrap(ctx, err, "delete existing MDM IdP device mappings")
 		}
 		mdmIdpRowsAffected, err := mdmIdpDelRes.RowsAffected()
 		if err != nil {
 			return ctxerr.Wrap(ctx, err, "delete existing IdP device mappings - get mdm IdP rows affected")
 		}
 
 		if idpRowsAffected+mdmIdpRowsAffected == 0 {
 			return fleet.NewInvalidArgumentError("delete host IdP mapping", "no existing IdP mappings for this host")
 		}
 
-		if err := deleteHostSCIMUserMapping(ctx, tx, id); err != nil {
+		if err := deleteHostSCIMUserMapping(ctx, tx, hostID); err != nil {
 			return ctxerr.Wrap(ctx, err, "delete existing host SCIM user mapping")
 		}
 
 		return nil
 	})
 	return err
 }
frontend/pages/hosts/details/cards/User/components/UpdateEndUserModal/UpdateEndUserModal.tsx (5)

5-7: Avoid ts-ignore on InputField import.

Silencing types hides real issues. Import correctly or add a local module declaration.

Example:

-// @ts-ignore
-import InputField from "components/forms/fields/InputField";
+import InputField from "components/forms/fields/InputField";

If types are missing, add a minimal ambient declaration:

// types/global.d.ts
declare module "components/forms/fields/InputField" {
  import * as React from "react";
  export interface InputFieldProps {
    label: string; name: string; value: string;
    onChange: (val: string) => void;
    helpText?: string; autoFocus?: boolean;
  }
  const InputField: React.FC<InputFieldProps>;
  export default InputField;
}

31-35: Trim input and avoid submitting unchanged values.

  • Trim whitespace before update.
  • Disable Save when unchanged to cut no‑op API calls.
- const [idpUsername, setIdpUsername] = useState(userNameDisplayValue || "");
+ const [idpUsername, setIdpUsername] = useState(userNameDisplayValue || "");
+ const trimmed = idpUsername.trim();
+ const isUnchanged = trimmed === (userNameDisplayValue || "");
 
- const onSave = () => {
-   onUpdate(idpUsername);
- };
+ const onSave = () => {
+   onUpdate(trimmed);
+ };

And update the disabled prop (see next comment).

Also applies to: 36-38


46-65: Handle Enter key and button semantics; prevent default form submit.

  • Add onSubmit to the form and set Button type="submit" so Enter saves.
  • Prevent default submit to avoid navigation.
  • Disable Save while updating, when adding with empty value, and when unchanged.
-      <>
-        <form>
+      <>
+        <form
+          onSubmit={(e) => {
+            e.preventDefault();
+            onSave();
+          }}
+        >
           <InputField
             label="Username (IdP)"
             name="username_idp"
             value={idpUsername}
             onChange={(val: string) => setIdpUsername(val)}
             helpText="This will be used to populate additional user data, e.g. full name and department."
+            autoFocus
           />
 
           <div className="modal-cta-wrap">
             <Button
+              type="submit"
               isLoading={isUpdating}
-              disabled={isUpdating || (!isEditing && idpUsername === "")}
-              onClick={onSave}
+              disabled={isUpdating || (!isEditing && trimmed === "") || isUnchanged}
             >
               Save
             </Button>
           </div>
         </form>
       </>

Also applies to: 55-63, 69-77


24-30: Consider explicit delete UX to use the new DELETE endpoint.

Relying on “clear username + Save” couples UI to backend heuristics. Expose an optional onDelete prop and show a Delete action when editing.

 interface IUpdateEndUserModalProps {
   isPremiumTier: boolean;
   /** There will be at most 1 end user */
   endUsers: IHostEndUser[];
   onUpdate: (username: string) => void;
   isUpdating?: boolean;
   onExit: () => void;
+  onDelete?: () => void;
 }
 ...
     <Modal
       title={isEditing ? "Edit user" : "Add user"}
       onExit={onExit}
       className={baseClass}
     >
       {renderContent()}
     </Modal>

And render a secondary “Delete” button when isEditing && onDelete to call the DELETE path. Please confirm desired UX.

Also applies to: 69-77


31-35: Sync local state when endUsers prop changes.

If the modal remains open across updates, keep the field in sync.

+ React.useEffect(() => {
+   setIdpUsername(generateUsernameValues(endUsers)[0] || "");
+ }, [endUsers]);
server/service/hosts.go (1)

1699-1759: Consider refactoring to eliminate redundant host fetch.

The host is fetched at line 1710 for activity context, but it was already fetched during authorization at line 1675. This redundancy occurs because the host variable from the authorization block is not in scope here.

Consider refactoring to declare and fetch the host at the function level (after authorization checks) so it can be reused throughout the function, avoiding the duplicate database call.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d9726ba and 5485de6.

📒 Files selected for processing (24)
  • changes/34222-manual-IdP-update-ui-delete-activities (1 hunks)
  • frontend/interfaces/activity.ts (2 hunks)
  • frontend/interfaces/host.ts (1 hunks)
  • frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsx (2 hunks)
  • frontend/pages/hosts/details/DeviceUserPage/DeviceUserPage.tsx (1 hunks)
  • frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx (13 hunks)
  • frontend/pages/hosts/details/cards/User/User.tests.tsx (3 hunks)
  • frontend/pages/hosts/details/cards/User/User.tsx (2 hunks)
  • frontend/pages/hosts/details/cards/User/components/AddEndUserModal/AddEndUserModal.tsx (0 hunks)
  • frontend/pages/hosts/details/cards/User/components/AddEndUserModal/_styles.scss (0 hunks)
  • frontend/pages/hosts/details/cards/User/components/AddEndUserModal/index.ts (0 hunks)
  • frontend/pages/hosts/details/cards/User/components/UpdateEndUserModal/UpdateEndUserModal.tsx (1 hunks)
  • frontend/pages/hosts/details/cards/User/components/UpdateEndUserModal/index.ts (1 hunks)
  • frontend/services/entities/hosts.ts (1 hunks)
  • frontend/utilities/endpoints.ts (1 hunks)
  • server/datastore/mysql/hosts.go (1 hunks)
  • server/fleet/activities.go (2 hunks)
  • server/fleet/datastore.go (1 hunks)
  • server/fleet/service.go (1 hunks)
  • server/mock/datastore_mock.go (3 hunks)
  • server/mock/service/service_mock.go (3 hunks)
  • server/service/handler.go (1 hunks)
  • server/service/hosts.go (3 hunks)
  • server/service/hosts_test.go (3 hunks)
💤 Files with no reviewable changes (3)
  • frontend/pages/hosts/details/cards/User/components/AddEndUserModal/_styles.scss
  • frontend/pages/hosts/details/cards/User/components/AddEndUserModal/AddEndUserModal.tsx
  • frontend/pages/hosts/details/cards/User/components/AddEndUserModal/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go

⚙️ CodeRabbit configuration file

When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.

Files:

  • server/fleet/service.go
  • server/datastore/mysql/hosts.go
  • server/fleet/activities.go
  • server/mock/service/service_mock.go
  • server/service/hosts.go
  • server/fleet/datastore.go
  • server/service/hosts_test.go
  • server/mock/datastore_mock.go
  • server/service/handler.go
🧠 Learnings (1)
📚 Learning: 2025-10-03T18:16:11.482Z
Learnt from: MagnusHJensen
Repo: fleetdm/fleet PR: 33805
File: server/service/integration_mdm_test.go:1248-1251
Timestamp: 2025-10-03T18:16:11.482Z
Learning: In server/service/integration_mdm_test.go, the helper createAppleMobileHostThenEnrollMDM(platform string) is exclusively for iOS/iPadOS hosts (mobile). Do not flag macOS model/behavior issues based on changes within this helper; macOS provisioning uses different helpers such as createHostThenEnrollMDM.

Applied to files:

  • server/service/hosts.go
  • server/service/hosts_test.go
🔇 Additional comments (21)
frontend/interfaces/activity.ts (1)

146-147: Enum addition looks good; ensure UI wires the new type.

Confirm GlobalActivityItem getDetail handles EditedHostIdpData and that tests cover both update (non-empty username) and delete (empty/undefined).

frontend/pages/hosts/details/cards/User/components/UpdateEndUserModal/index.ts (1)

1-1: LGTM.

Barrel re-export is correct and simplifies imports.

frontend/pages/hosts/details/DeviceUserPage/DeviceUserPage.tsx (1)

698-701: Empty-state UX handling is already in place.

The UserCard (User component) gracefully handles endUsers=[]: all value generators return empty arrays when empty, optional fields hide conditionally (chrome profiles, other emails only render when data exists), and the write button only appears when canWriteEndUser is true. Tests confirm this scenario works as intended. No action needed.

server/fleet/service.go (1)

403-404: Add documentation to DataStore interface DeleteHostIDP and clarify implementation scope.

From code inspection:

  • DataStore interface (line 403) lacks a doc comment; Service interface (line 351) has one: "deletes an existing host IDP device mapping"
  • License gating is present in tests (returns fleet.ErrMissingLicense when unlicensed) but not documented in code
  • Service implementation comment (line 1808) claims "delete host IdP and SCIM mappings," but the datastore implementation only executes: DELETE FROM host_emails WHERE host_id = ? AND source = ?. No explicit SCIM mapping deletion visible in that method
  • Idempotency semantics remain unclear (e.g., behavior when the mapping is already absent)

Add doc comments to the DataStore interface method and reconcile the implementation comment with actual behavior (particularly regarding SCIM mappings).

server/mock/datastore_mock.go (2)

1964-1966: DataStore fields wired for DeleteHostIDP.

Fields and Invoked flag follow established conventions. Ensure tests set DeleteHostIDPFunc to avoid nil panics in mocks.


4813-4819: All verification checks passed; no issues found.

Interface, service method, HTTP route, and unit tests are all in place:

  • Datastore interface: server/fleet/datastore.go:351
  • Service method: server/service/hosts.go:1783
  • HTTP DELETE route: server/service/handler.go:440
  • Unit test: server/service/hosts_test.go:3314 (TestDeleteHostDeviceIDPMapping)

Mock implementation correctly follows the established pattern with proper locking and delegation.

server/fleet/datastore.go (1)

350-351: LGTM!

The new DeleteHostIDP method follows the established naming conventions and patterns in the interface. The method signature is appropriate for a deletion operation, accepting a context and ID parameter.

frontend/utilities/endpoints.ts (1)

93-96: LGTM!

The new endpoint constants are well-defined and follow the established patterns in this file. The endpoint paths are RESTful and the naming convention is consistent with other host-related endpoints.

frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsx (2)

1610-1620: LGTM!

The editedHostIdpData template function correctly handles both setting and removing end-user IdP data. The logic to differentiate between the two cases (checking for empty string) is appropriate, and the message formatting is consistent with other activity templates in this file.


1975-1977: LGTM!

The switch case for ActivityType.EditedHostIdpData is properly wired to the corresponding template function, following the established pattern used throughout the getDetail function.

server/service/hosts_test.go (2)

3167-3172: LGTM! Necessary mocks for activity logging.

These mock additions support the new activity logging functionality in the IDP source success test case.


3205-3210: LGTM! Consistent mock additions.

Same necessary mocks added for activity logging in the "IDP source success with any username when SCIM user not found" test case.

server/datastore/mysql/hosts.go (2)

3874-3908: Verify error type appropriateness for "no existing mappings" case.

When no IDP mappings exist to delete, the function returns fleet.NewInvalidArgumentError. Consider whether a NotFoundError would be more semantically appropriate, since the issue is that no mapping was found rather than the host ID argument being invalid. The choice depends on the intended API semantics:

  • If this operation should be idempotent (safe to call even when nothing exists), consider making it a no-op that succeeds
  • If the caller is expected to know mappings exist before deleting, NotFoundError might better signal the condition
  • If the current behavior is intentional, it's worth documenting why InvalidArgumentError was chosen

3874-3908: SQL queries are properly scoped and secure.

The DELETE queries correctly filter by host_id and use parameterized statements to prevent SQL injection. The function also properly executes within a transaction to ensure atomicity of the multi-table delete operation.

server/fleet/activities.go (1)

242-244: Activity registered in ActivityDetailsList — LGTM.

The new activity type is included in ActivityDetailsList so it will be documented and exposed. Good.

server/mock/service/service_mock.go (1)

243-244: Verification complete—all concerns addressed.

The mock wiring is correct:

  • Interface declares DeleteHostIDP(ctx context.Context, id uint) error (server/fleet/service.go:403) ✓
  • Func type definition present (lines 243-244) ✓
  • Struct fields DeleteHostIDPFunc and DeleteHostIDPFuncInvoked defined (lines 1185-1186) ✓
  • Method implementation follows pattern with lock and Invoked flag (lines 2877-2883) ✓
  • Tests properly initialize DeleteHostIDPFunc (hosts_test.go lines 3322, 3345) to prevent nil panics ✓

Acronym casing inconsistency (IDP vs IdP vs idp) exists across the codebase but remains optional to address.

server/service/hosts.go (1)

1761-1824: LGTM! DeleteHostIDP implementation follows established patterns.

The implementation correctly:

  • Follows the authorization pattern (list → fetch host → write with team)
  • Checks for premium license before allowing the operation
  • Deletes the IdP mapping and cleans up SCIM mappings
  • Logs activity with empty HostIdPUsername to indicate removal
  • Handles errors appropriately
frontend/pages/hosts/details/cards/User/User.tests.tsx (1)

10-47: LGTM! Tests properly cover the updated User component API.

The consolidated IdP test suite correctly verifies:

  • All IdP data fields render properly (username, full name, groups, department)
  • Write permission controls button visibility
  • "Add user" button appears when no IdP username exists (with write permission)
  • "Edit user" button appears when IdP username exists (with write permission)
  • No buttons appear without write permission
frontend/pages/hosts/details/cards/User/User.tsx (1)

27-144: LGTM! User component API successfully simplified.

The refactored component correctly:

  • Simplifies props by removing platform-specific logic
  • Provides clear write permission controls via canWriteEndUser
  • Dynamically shows "Add user" vs "Edit user" based on endUsers presence
  • Consistently renders all IdP fields with appropriate tooltips
  • Maintains backward compatibility through default prop values

The new API is more intuitive and easier to use.

frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx (2)

896-913: LGTM! UpdateEndUser handler correctly implements delete and update flows.

The implementation properly:

  • Sets loading state before the operation
  • Distinguishes between delete (empty username) and update cases
  • Calls the appropriate API methods (deleteHostIdp or updateHostIdp)
  • Refetches host details after success to update the UI
  • Shows appropriate success/error messages
  • Ensures loading state is cleared in the finally block

1218-1227: LGTM! UserCard integration correctly configured.

The UserCard correctly receives:

  • endUsers from host data
  • canWriteEndUser computed from appropriate permissions (team maintainer/admin or global maintainer/admin)
  • onClickUpdateUser callback to show the modal

This properly enforces write permissions for IdP username management.

Comment thread server/fleet/activities.go
Comment thread server/service/handler.go
ghernandez345
ghernandez345 previously approved these changes Nov 11, 2025

@ghernandez345 ghernandez345 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.

Nice PR. easy to follow these changes 👍🏽

const otherEmailsDisplayValues = generateOtherEmailsValues(endUsers);

const [writeButtonText, writeButtonIcon] = userNameDisplayValues.length
? ["Edit user", "pencil" as const]

@ghernandez345 ghernandez345 Nov 11, 2025

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.

just curious, why the as const for the second string values?

@jacobshandling jacobshandling Nov 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

To narrow the type - string is not a valid type for Icon's name prop, but both "pencil" and "plus" are

noahtalerman
noahtalerman previously approved these changes Nov 11, 2025

@noahtalerman noahtalerman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Docs (audit-logs.md) look good to me!

@jacobshandling
jacobshandling merged commit 926cdc6 into main Nov 13, 2025
46 checks passed
@jacobshandling
jacobshandling deleted the 34222-IdP-UI-DELETE-activities branch November 13, 2025 17:05
jacobshandling added a commit that referenced this pull request Nov 13, 2025
**Related issue:** Resolves #34222 


[Demo](https://drive.google.com/file/d/1MyLlyUW8Qoad_3_FLwiMhMBbb8wJNwGk/view?usp=drive_link)

<img width="1504" height="986" alt="Screenshot 2025-11-10 at 4 45 48 PM"
src="https://github.com/user-attachments/assets/9ee80fd3-c9e7-4712-b150-11ac08c70db6"
/>

# Checklist for submitter
If some of the following don't apply, delete the relevant line.

- [x] Changes file added for user-visible changes in `changes/`, 
## Testing
- [x] Added/updated automated tests
- [x] QA'd all new/changed functionality manually
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.

IdP Groups Tooltip doesn't show on hover Update IdP username: user modal and DELETE endpoint and activities

4 participants