Android certificates resent_certificate#49171
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses #49007 by ensuring Fleet emits resent_certificate host activities when certificate templates are automatically resent due to SCIM-driven host variable changes (e.g., IdP attribute updates), aligning the auto-resend path with the manual resend behavior.
Changes:
- Updated SCIM datastore methods to return the set of impacted certificate-template resend activities when variable-triggered resends occur.
- Added logic in the SCIM user handlers to create
resent_certificateactivities (system actor) based on those returned details. - Adjusted call sites and tests to accommodate the updated datastore interface and validate returned activity details.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_mdm_profiles_test.go | Updated SCIM call signature usage in integration test. |
| server/service/integration_enterprise_test.go | Updated SCIM deletion call signature usage in integration test defer cleanup. |
| server/mock/datastore_mock.go | Updated mock datastore method signatures for SCIM user replace/delete. |
| server/fleet/datastore.go | Updated Datastore interface for SCIM user replace/delete to return resent-certificate activity details. |
| server/datastore/mysql/scim.go | Collects certificate-template resend activity details during variable-triggered resend operations and returns them to callers. |
| server/datastore/mysql/scim_test.go | Adds assertions that ReplaceScimUser returns the expected resent-certificate activity details. |
| server/datastore/mysql/hosts.go | Updated call site for triggerResendProfilesUsingVariables return signature. |
| server/cron/google_workspace_cron.go | Updated cron SCIM replace/delete calls for new return signature. |
| server/cron/google_workspace_cron_test.go | Updated cron test datastore mock functions for new return signature. |
| ee/server/scim/users.go | Emits resent_certificate activities in SCIM handlers based on datastore-returned details. |
| ee/server/scim/users_test.go | Updated SCIM handler tests’ mocks for new return signature. |
💡 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughSCIM datastore replacement and deletion now return certificates affected by automatic resend processing. SCIM user create reactivation, replace, delete, and patch handlers emit Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/cron/google_workspace_cron.go (1)
180-222: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEmit
resent_certificateactivities from Google Workspace sync.
ReplaceScimUserandDeleteScimUserreturn resent-certificate details, but this cron drops them (if _, err := ...). That means directory-driven SCIM updates/deletes won’t create theresent_certificateactivity trail thatee/server/scim/users.gorecords. Wire these results into the same activity-creation path here.🤖 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/cron/google_workspace_cron.go` around lines 180 - 222, The Google Workspace sync discards resent-certificate details from ReplaceScimUser and DeleteScimUser, so required activities are not recorded. Capture the returned resent-certificate data in the update and deletion branches of the sync function, then pass it through the same activity-creation path used by SCIM user operations, preserving existing error handling and counters.server/datastore/mysql/scim.go (1)
1223-1277: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPropagate resent-certificate activities from SCIM group resends.
triggerResendProfilesForIDPGroupChange*andtriggerResendProfilesForIDPUserAddedToHostdiscard the[]fleet.ActivityTypeResentCertificatereturned bytriggerResendProfilesUsingVariables, so these SCIM group/host-reassignment paths update profiles but never emitresent_certificateactivities. Thread the slice back up to the datastore callers.🤖 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/scim.go` around lines 1223 - 1277, Update triggerResendProfilesForIDPGroupChange, triggerResendProfilesForIDPGroupChangeByUsers, and triggerResendProfilesForIDPUserAddedToHost to return the []fleet.ActivityTypeResentCertificate from triggerResendProfilesUsingVariables alongside any error, preserving it through early returns. Then update every datastore caller to accept and propagate the returned activities so SCIM group and host-reassignment profile resends emit resent_certificate activities.
🧹 Nitpick comments (1)
ee/server/scim/users_test.go (1)
381-386: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new resend-activity emission path.
All updated mocks in this file return a
nilcerts slice, so no test verifies thatu.newActivityis actually invoked withfleet.ActivityTypeResentCertificatewhenReplaceScimUser/DeleteScimUserreturn non-empty results — the core behavior this PR introduces. Consider adding at least one case per handler (Create reactivation, Replace, Delete, Patch) where the mock returns a non-empty slice and the test assertsNewActivityFuncwas called with the expected certificate.Also applies to: 539-545, 700-703, 881-885, 1041-1048
🤖 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 `@ee/server/scim/users_test.go` around lines 381 - 386, Add test coverage for resend-activity emission in the Create reactivation, ReplaceScimUser, DeleteScimUser, and Patch handler tests. Configure each relevant mock to return a non-empty certificate slice, then assert NewActivityFunc is called with fleet.ActivityTypeResentCertificate and the expected certificate data.
🤖 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.
Outside diff comments:
In `@server/cron/google_workspace_cron.go`:
- Around line 180-222: The Google Workspace sync discards resent-certificate
details from ReplaceScimUser and DeleteScimUser, so required activities are not
recorded. Capture the returned resent-certificate data in the update and
deletion branches of the sync function, then pass it through the same
activity-creation path used by SCIM user operations, preserving existing error
handling and counters.
In `@server/datastore/mysql/scim.go`:
- Around line 1223-1277: Update triggerResendProfilesForIDPGroupChange,
triggerResendProfilesForIDPGroupChangeByUsers, and
triggerResendProfilesForIDPUserAddedToHost to return the
[]fleet.ActivityTypeResentCertificate from triggerResendProfilesUsingVariables
alongside any error, preserving it through early returns. Then update every
datastore caller to accept and propagate the returned activities so SCIM group
and host-reassignment profile resends emit resent_certificate activities.
---
Nitpick comments:
In `@ee/server/scim/users_test.go`:
- Around line 381-386: Add test coverage for resend-activity emission in the
Create reactivation, ReplaceScimUser, DeleteScimUser, and Patch handler tests.
Configure each relevant mock to return a non-empty certificate slice, then
assert NewActivityFunc is called with fleet.ActivityTypeResentCertificate and
the expected certificate data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52f10783-f6bd-4b14-ae6b-19aa6890b05d
📒 Files selected for processing (11)
ee/server/scim/users.goee/server/scim/users_test.goserver/cron/google_workspace_cron.goserver/cron/google_workspace_cron_test.goserver/datastore/mysql/hosts.goserver/datastore/mysql/scim.goserver/datastore/mysql/scim_test.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/service/integration_enterprise_test.goserver/service/integration_mdm_profiles_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #49171 +/- ##
==========================================
+ Coverage 67.95% 67.99% +0.04%
==========================================
Files 3766 3768 +2
Lines 238601 239175 +574
Branches 12579 12579
==========================================
+ Hits 162132 162633 +501
+ Misses 61817 61765 -52
- Partials 14652 14777 +125
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/datastore/mysql/scim.go (1)
265-277: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep collected certificates local to each transaction attempt.
Appending to an outer
resentCertsslice inside a retried transaction can retain certificates from rolled-back attempts, producing duplicate or stale audit activities. Build an attempt-local slice and replace the returned result only at the end of each successful callback attempt.Also applies to: 361-373, 410-444
🤖 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/scim.go` around lines 265 - 277, Update the retry callback in the user email update flow around withRetryTxx so resent certificates are accumulated in an attempt-local slice rather than the outer resentCerts variable. Assign the local slice to resentCerts only after the transaction callback completes successfully, and apply the same isolation to the corresponding retry blocks around the references at lines 361-373 and 410-444.
🤖 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 `@server/datastore/mysql/scim.go`:
- Around line 1252-1253: Update triggerResendProfilesUsingVariables and its
variable-triggered callers to return the affected certificate payloads instead
of discarding them. Apply this to all resend paths for
FleetVarHostEndUserIDPGroups and the other variables handled by the nearby
wrappers, then propagate those payloads so each automatic resend emits both the
resend activity and resent_certificate.
---
Outside diff comments:
In `@server/datastore/mysql/scim.go`:
- Around line 265-277: Update the retry callback in the user email update flow
around withRetryTxx so resent certificates are accumulated in an attempt-local
slice rather than the outer resentCerts variable. Assign the local slice to
resentCerts only after the transaction callback completes successfully, and
apply the same isolation to the corresponding retry blocks around the references
at lines 361-373 and 410-444.
🪄 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: ec13eac4-1ce8-4e93-92b5-b4a816b7b7c0
📒 Files selected for processing (1)
server/datastore/mysql/scim.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/datastore/mysql/scim.go (1)
265-277: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep collected certificates local to each transaction attempt.
Appending to an outer
resentCertsslice inside a retried transaction can retain certificates from rolled-back attempts, producing duplicate or stale audit activities. Build an attempt-local slice and replace the returned result only at the end of each successful callback attempt.Also applies to: 361-373, 410-444
🤖 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/scim.go` around lines 265 - 277, Update the retry callback in the user email update flow around withRetryTxx so resent certificates are accumulated in an attempt-local slice rather than the outer resentCerts variable. Assign the local slice to resentCerts only after the transaction callback completes successfully, and apply the same isolation to the corresponding retry blocks around the references at lines 361-373 and 410-444.
🤖 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 `@server/datastore/mysql/scim.go`:
- Around line 1252-1253: Update triggerResendProfilesUsingVariables and its
variable-triggered callers to return the affected certificate payloads instead
of discarding them. Apply this to all resend paths for
FleetVarHostEndUserIDPGroups and the other variables handled by the nearby
wrappers, then propagate those payloads so each automatic resend emits both the
resend activity and resent_certificate.
---
Outside diff comments:
In `@server/datastore/mysql/scim.go`:
- Around line 265-277: Update the retry callback in the user email update flow
around withRetryTxx so resent certificates are accumulated in an attempt-local
slice rather than the outer resentCerts variable. Assign the local slice to
resentCerts only after the transaction callback completes successfully, and
apply the same isolation to the corresponding retry blocks around the references
at lines 361-373 and 410-444.
🪄 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: ec13eac4-1ce8-4e93-92b5-b4a816b7b7c0
📒 Files selected for processing (1)
server/datastore/mysql/scim.go
🛑 Comments failed to post (1)
server/datastore/mysql/scim.go (1)
1252-1253: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Propagate resend activities from every variable-triggered certificate resend path.
Lines 1252, 1265, and 1280 can resend certificate templates but discard the affected certificates. Consequently, group changes and host-user association changes still cannot emit
resent_certificate, contrary to the requirement that automatic resends produce both activity types. Extend these wrappers and their callers to return the certificate payloads.Also applies to: 1265-1266, 1280-1287
🤖 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/scim.go` around lines 1252 - 1253, Update triggerResendProfilesUsingVariables and its variable-triggered callers to return the affected certificate payloads instead of discarding them. Apply this to all resend paths for FleetVarHostEndUserIDPGroups and the other variables handled by the nearby wrappers, then propagate those payloads so each automatic resend emits both the resend activity and resent_certificate.
|
LGTM - only thing that need addressing are those failing tests |
Related issue: Resolves #49007
Checklist for submitter
For unreleased bug fixes in a release candidate, one of:
Summary by CodeRabbit