UN-3494 [GATED-FEAT] Email users and groups on access grant and revoke - #2224
Draft
kirtimanmishrazipstack wants to merge 3 commits into
Draft
UN-3494 [GATED-FEAT] Email users and groups on access grant and revoke#2224kirtimanmishrazipstack wants to merge 3 commits into
kirtimanmishrazipstack wants to merge 3 commits into
Conversation
…ship changes Sharing a resource with a group gave its members access silently, and adding or removing someone from a group told nobody. Both now send email. - share_notifications.py holds the feature flag, the two task names and the two enqueue hooks. Dispatch uses the same resolve_transport branch the execution path uses: the PG queue where pg_queue_enabled is on for the org, Celery otherwise. - One hook in ResourceShareManagementMixin.share covers all 7 resource types plus cloud agentic, including service-account shares — every group share funnels through it and shared_groups has no PATCH path. No on_commit needed: _commit's transaction has closed by the time the view resumes, so the diff reads committed state. - Group membership hooks on the add and remove actions. The add serializer already subtracts existing members, so nobody is mailed twice. - Internal endpoints under /internal/v1/group-notification/ do the work the worker cannot: group expansion, OrganizationMember re-validation (this is where the offboarding race closes), resource lookup via ShareableResource, and the kind -> ResourceType mapping, which is not 1:1 — pipelines split on pipeline_type and adapters four ways on adapter_type. - Two worker tasks that only POST to that endpoint, since workers/ has no Django. They raise on failure, unlike _mark_buffer_outcome which has a reaper behind it, and retry transient 5xx in-task because a raise is terminal on the Celery transport. - The whole feature is gated on Flipt group_sharing_notifications_enabled and fails closed: a blind Flipt, a missing org, or any dispatch error means no notification, never a broken share. - worker-pg-notification compose service so the PG arm is not a black hole. Membership changes with no actor (the org-removal cascade, Django admin, group deletion) do not notify — SharingNotificationService requires an actor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
UN-2977 moved sharing from PATCH to POST /{id}/share/, but the mixin's
share action only diffed the groups axis. The per-viewset
_notify_shared_users hooks stayed on partial_update, which nothing calls
anymore, so sharing a resource with a user sent no email.
Snapshot every declared axis and invoke the hook after the commit; declare
it on the mixin as a no-op for hosts without a direct-share email.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kirtimanmishrazipstack
changed the title
…revoked Sharing already emailed on grant; revoking told nobody. Both axes now notify, and the seven duplicated copies of the user hook collapse into the share mixin. - ResourceShareManagementMixin gains a concrete _notify_shared_users covering grant and revoke, driven by the OwnerManagementMixin seam every host already declares. The seven per-viewset overrides and their dead partial_update wrappers go with it — a host override would otherwise shadow the mixin and silently swallow the revoke mail. - share() diffs both axes through _read_axis directly; AxisDiff, snapshot_share_axes, diff_share_axes and the share_axes ClassVar had no callers left. - Group revoke rides the existing resource-shared route with a share_action discriminator, mirroring membership-changed — no new endpoint or worker task. Defaulted at every hop so in-flight messages still run. - Suppressed when the user still reaches the resource via a group or shared_to_org: losing one axis is not losing access. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
kirtimanmishrazipstack
changed the title
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
1 participant
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.
What
/internal/v1/group-notification/and twoworkers/notificationtasks that call them.ResourceShareManagementMixinand drops the dead PATCH path.Why
_notify_shared_userswas still wired topartial_update, which no client has called since UN-2977, so even the direct-user share email had quietly stopped.How
ResourceShareManagementMixin.sharecovers all 7 shareable resources plus cloud agentic; dispatch reusesresolve_transport(PG queue wherepg_queue_enabledis on for the org, Celery otherwise).resource-sharedroute with ashare_actiondiscriminator, mirroringmembership-changed, defaulted at every hop so in-flight messages still run.group_sharing_notifications_enabled, and a revoke is suppressed when the user still reaches the resource via a group orshared_to_org.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
partial_updatehooks cannot fire sinceshared_usersbecameResourceMembershiprows in UN-2202 Phase 2. Membership changes with no actor (org-removal cascade, Django admin, group deletion) deliberately do not notify.Database Migrations
Env Config
group_sharing_notifications_enabled— new, gates the whole feature. Off or unreachable means no emails.pg_queue_enabled— existing, reused to pick PG queue vs Celery transport.worker-pg-notificationcompose service (pg-queueprofile), with optional overridesPG_NOTIFICATION_CONCURRENCY,PG_NOTIFICATION_VT_SECONDS,PG_NOTIFICATION_HEALTH_STALE_SECONDS.Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.