Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug where DDM/Windows profiles with secrets were not being marked Verified. #25065

Merged
merged 4 commits into from
Jan 2, 2025

Conversation

getvictor
Copy link
Member

@getvictor getvictor commented Dec 31, 2024

Fixing unreleased secret variables bug where DDM and Windows profiles were not transitioning to Verified.
#23238

Checklist for submitter

  • Added/updated tests
  • If database migrations are included, checked table schema to confirm autoupdate
  • Manual QA for all new/changed functionality

Copy link

codecov bot commented Dec 31, 2024

Codecov Report

Attention: Patch coverage is 79.24528% with 11 lines in your changes missing coverage. Please review.

Project coverage is 63.82%. Comparing base (43aec47) to head (f8cd105).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
.../tables/20241231112624_RenameDDMChecksumToToken.go 50.00% 6 Missing and 2 partials ⚠️
server/mdm/microsoft/profile_verifier.go 40.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25065      +/-   ##
==========================================
- Coverage   63.84%   63.82%   -0.02%     
==========================================
  Files        1614     1615       +1     
  Lines      153580   153595      +15     
  Branches     4040     4040              
==========================================
- Hits        98048    98034      -14     
- Misses      47736    47757      +21     
- Partials     7796     7804       +8     
Flag Coverage Δ
backend 64.69% <79.24%> (-0.02%) ⬇️

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.

@getvictor getvictor marked this pull request as ready for review December 31, 2024 20:07
@getvictor getvictor requested a review from a team as a code owner December 31, 2024 20:07
@getvictor getvictor changed the title Fix bug where DDM profile with secrets was not being marked Verified. Fix bug where DDM/Windows profiles with secrets were not being marked Verified. Dec 31, 2024
Copy link
Member

@iansltx iansltx left a comment

Choose a reason for hiding this comment

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

I think I understand what's going on here (basically removing checksum when it's not relevant and relying on token instead, and updating uploaded_at when secrets change, which I was thinking in the direction of when I reviewed the last secrets PR), so will let this get merged so it lands ahead of the RC cut. Would be helpful to include a description of why this fix works in the squash merge commit notes though.

@getvictor
Copy link
Member Author

I think I understand what's going on here (basically removing checksum when it's not relevant and relying on token instead, and updating uploaded_at when secrets change, which I was thinking in the direction of when I reviewed the last secrets PR), so will let this get merged so it lands ahead of the RC cut. Would be helpful to include a description of why this fix works in the squash merge commit notes though.

Yes, this fix has a bit more code than just the fix. The problem for DDM profiles was that host_mdm_apple_delcarations.checksum was being used for verification, while host was returning the token (which contained the secrets timestamp). I wanted to remove the checksum initially, but I kept it for backwards compatibility. With this issue, I thought it was cleaner to just have token and no checksum. I made a note in the secrets guide that DDM "checksum" will now be an MD5 hash of JSON + secrets timestamp (if present).

@getvictor getvictor merged commit feedb50 into main Jan 2, 2025
25 checks passed
@getvictor getvictor deleted the victor/23238-verify-profile branch January 2, 2025 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants