fix(CPV): Retry CPV initial consent and check SAM manifest changes by hash, not mtime - #299
Merged
Merged
Conversation
Push-UpdatePermissionsQueue skipped Set-CIPPCPVConsent whenever a cpvtenants row named the current SAM app. Its finally block writes that row on every run including failures, so the first failed attempt left behind exactly the record that suppressed all later attempts. The tenant then failed indefinitely on the token call that only consent could have made possible, and Start-UpdatePermissionsOrchestrator re-queued it nightly to no effect. The gate now also re-consents when LastStatus is Failed, matching the status check the orchestrator already makes. Rows predating LastStatus are left alone so deploying this does not re-consent an entire estate. A plain re-consent cannot repair every case: Set-CIPPCPVConsent short-circuits on 'Permission entry already exists', which leaves both a recorded-but- ineffective entry and one whose scopes no longer cover what CIPP needs unfixable. Escalate to ResetSP on a recognised consent error, or after a re-consent has already been tried, tracked in ConsentAttempts. Resets are limited to one per week via LastResetUtc since they briefly remove access. Test-CIPPAccessPermissions judged CPV freshness from the row Timestamp alone, which the failed runs kept current - a permanently broken tenant never appeared in "Some tenants need a CPV refresh". It now also reports failures.
Get-CippSamPermissions reported the manifest's LastWriteTime as "when the required permission set last changed". Git does not store mtimes, so every checkout and every container build restamps SAMManifest.json with the build time. Docker COPY faithfully preserves that fresh timestamp, so the value was really "when was this image built". Consumers compare it against each tenant's cpvtenants row, so every release made it newer than every row: Start-UpdatePermissionsOrchestrator re-queued the entire estate through the permission and admin-role calls, and Test-CIPPAccessPermissions reported that tenants needed a CPV refresh. Only instances with saved extra permissions escaped, their AppPermissions row being newer than the image. Hash SAMManifest.json + AdditionalPermissions.json instead and keep the time the hash was first seen, in an AppPermissions/ManifestHash row. A rebuild with unchanged permissions no longer moves the timestamp; a genuine change still does. If the row cannot be written, fall back to the mtime rather than treating every call as first sight. Upgrading records the hash once, which costs one final estate-wide refresh.
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
2 participants
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.
No description provided.