UN-3494 [GATED-FEAT] Email users and groups on access grant and revoke - #2224
UN-3494 [GATED-FEAT] Email users and groups on access grant and revoke#2224kirtimanmishrazipstack wants to merge 11 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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe change adds asynchronous group resource-sharing and membership notifications through feature-gated dispatch, worker tasks, internal APIs, and notification services. Legacy partial-update notification paths are removed. Frontend co-owner management now stages and applies combined additions and removals. ChangesGroup notification pipeline
Staged co-owner management
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ShareChange
participant share_notifications
participant NotificationWorker
participant InternalNotificationAPI
participant group_notification_service
participant NotificationPlugin
ShareChange->>share_notifications: Dispatch share or membership event
share_notifications->>NotificationWorker: Enqueue organization-scoped task
NotificationWorker->>InternalNotificationAPI: POST notification payload
InternalNotificationAPI->>group_notification_service: Validate and process payload
group_notification_service->>NotificationPlugin: Send filtered notification
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
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
…tton Adding a co-owner was staged until Apply, but revoking one fired the DELETE straight from the Popconfirm — so Cancel could not undo it, Apply stayed disabled for a removal-only edit, and the revoke email went out on click. Stage the roster the way SharePermission does: one selected-owners list seeded from the server, edited locally by both add and revoke, committed only by Apply. Collapse the hook's two mutation callbacks into one onApplyCoOwners that runs adds before removes (so a one-shot owner swap clears the backend's last-owner guard), refreshes once, and emits one summary alert. Apply now closes on a clean run and stays open on failure, matching useShareModal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kirtimanmishrazipstack
left a comment
kirtimanmishrazipstack
left a comment
There was a problem hiding this comment.
Self-review — UN-3494 (OSS)
Ran a multi-pass review over this branch (correctness, error handling, type design, comments) and verified each finding against the code rather than taking the analysis at face value. Six candidate findings turned out to be false and are not listed. Low-severity items are withheld; below is everything blocking / high / medium.
Cloud half: Zipstack/unstract-cloud#1698 — findings that span both repos are stated there from the cloud side.
Blocking
B1 — Every send result is discarded, so a failed send reports success and the queue acks it.
backend/tenant_account_v2/group_notification_service.py:102 and :153 call service.send_group_resource_shared_notification(...) / send_group_membership_notification(...) as bare statements. Both return bool. backend/tenant_account_v2/internal_views.py:83 and :95 then return 200 {"status": "success"} unconditionally, _post_group_notification sees 200 and returns, and the PG consumer deletes the message.
Failure path: SendGrid 429/503 → the plugin returns False → email silently lost, message acked, nothing above DEBUG anywhere.
This inverts the contract this file states about itself at internal_views.py:8-10 ("any unhandled problem must surface as non-2xx so the queue redelivers"), and it makes the whole retry apparatus in workers/notification/tasks.py — the 3-attempt loop, the httpx transport retries, the 120s VT, and the "deliberately raises on failure … a swallowed error would be a silently unsent email" docstring — guard a path that can no longer fail.
The plugin-missing case has the same shape: _service() returns None behind a logger.debug (:162-166). A backend built without sendgrid emails nobody while every layer reports green — and the recipient_count= INFO line never executes, so the one metric you would grep for is absent rather than zero, which is indistinguishable from "nobody was shared with".
Fix: collect the booleans in send_resource_shared / send_membership_changed and return non-2xx when any send failed retryably; log non-retryable causes (template unset, notifications disabled) at WARNING with a distinct {"status": "skipped", "reason": ...} so misconfiguration is separable from delivery.
B2 — Group-revoke emails ignore remaining access.
group_notification_service.py:89-111 mails every current member of each revoked group with no effective-access check. The direct-user path deliberately does the opposite — _users_left_without_access in backend/permissions/resource_share_views.py:81-93, with the comment "telling them their access was removed would be wrong."
Failure path: workflow W is shared with Group A and Group B; Alice is in both; an owner removes Group B. Alice is told her access was removed, and because share_action == revoked the cloud side rewrites both the CTA and resource URL to the dashboard — so the email walks her away from a resource she still fully reaches via Group A. Same for a shared_to_org=True resource, where nobody lost anything, and for members who also hold a direct VIEWER row.
The revoke recipient list should go through the same compute_effective_members filter the direct path uses.
High
H1 — The notification path in share() can 500 a share that already committed.
permissions/resource_share_views.py:175-195: only _send_share_notification and _send_revoke_notification are wrapped. _notification_context (:188, which invokes the host viewset's get_notification_resource_type override) and _users_left_without_access (:193, a DB query) run bare — after ShareAuthorizationService.authorize_and_commit has already committed at :155-161. A DB hiccup or a raising seam returns 500 for a share that succeeded, and the client retries.
The group path escapes this only by luck: _organization_slug and kind_for_instance are pure getattr/_meta reads and _feature_enabled is wrapped. Wrap the whole _notify_shared_users body plus the notify_resource_group_share_changed call at the share() call site.
H2 — A synchronous SendGrid HTTPS call is now on the live POST /share/ path.
The group path was deliberately made async (worker + internal API); the direct-user path in _notify_shared_users calls the plugin inline. This is newly-introduced request latency, not pre-existing — the previous home (partial_update) was dead code, so these emails were not firing at all before this branch.
H3 — Unbounded org-member scan pulled into that same request.
_users_left_without_access → compute_effective_members → _add_org_members (backend/tenant_account_v2/sharing_helpers.py:316-340) runs OrganizationMember.objects.filter(organization=...) with no pagination and iterates the whole result in Python, whenever shared_to_org is true. Un-sharing one user on an org-shared workflow in a 5,000-member org hydrates 5,000 OrganizationMember + User rows to answer "does this one user still have access?" Against ARCHITECTURE_PRINCIPLES §6 on unbounded querysets and heavy work in the request cycle.
Cheap and correct: guard-clause if getattr(instance, "shared_to_org", False): return [] — if the resource is org-shared, nobody who lost a direct row actually lost access.
H4 — The new direct-share revoke email ships with no feature flag.
_notify_shared_users has no Flipt check at all; only the group path is gated by GROUP_NOTIFICATION_FLAG_KEY. So the new revoke email goes live for every org the moment this deploys, with no kill switch. The module docstring at backend/tenant_account_v2/share_notifications.py:13-15 claims "The whole feature sits behind its own Flipt flag and fails closed everywhere", and the PR description repeats it — neither is true for this path. Either gate it or correct both statements. (Template-reuse half of this is on the cloud PR.)
H5 — Lookups are group-shareable but get no group email, and nothing logs it.
LookupDefinition is absent from SHAREABLE_RESOURCES (backend/tenant_account_v2/shareable_resources.py:28-52), so kind_for_instance returns None and share_notifications.py:100 returns with no log line at all — kind is None is collapsed into the same silent early-return as feature-flag-off. Direct-user lookup emails do fire (the cloud PR wires get_notification_resource_type for exactly that), so the result reads as a flaky feature rather than a gap.
Split that guard: flag-off is expected silence, but an unregistered kind and a resource with no organization are both bugs and should log at WARNING. Then either register LookupDefinition or reject shared_groups for hosts absent from the registry.
H6 — _get_user is the one org-unscoped query on a tenant-scoped path.
group_notification_service.py:170-171 resolves the actor with User.objects.filter(pk=user_id).first(). Every sibling lookup on this path re-validates against the org (_groups_in_org, _live_member_users, and _load_resource, which filters organization= explicitly and explains why). The resolved user's name and email render into the outgoing mail. Not exploitable today since the payload is worker-generated, but it is an unscoped query on a multi-tenant path. One filter through OrganizationMember fixes it.
H7 — Rolling deploy: new backend to an old worker drops the message.
notify_resource_shared_with_group (workers/notification/tasks.py:528-535) has a closed signature. A message carrying share_action delivered to a pod on the previous build raises TypeError — terminal on Celery, burns the attempt cap on PG. The producer has already returned 200 to the user via _dispatch_quietly, so nothing surfaces. **_: Any on both new task signatures closes it.
Related: the "defaulted so messages enqueued before this field existed still validate" comments (internal_views.py:40, tasks.py:538) describe a state that never existed — both the task and share_action were added on this branch, so there are no in-flight messages. The defaults are fine to keep; the stated rationale is not, and it obscures the fact that the real hazard runs the other way.
H8 — Rollout ordering: PG transport with no consumer deployed.
_dispatch routes to the PG queue whenever resolve_transport says so, and the notification consumer is off by default. Any org with pg_queue_enabled ramped but the consumer not running gets messages durably stored and never executed — logged as "group-notification: %s enqueued on PG queue %r (msg_id=%s)" at INFO, which reads as delivery. Needs to be an explicit ordering constraint in Env Config, not an inference. (Chart side on the cloud PR.)
Medium
- Dead exception handler. The
except Exceptionin_feature_enabled(share_notifications.py:164-170) can never fire — bothcheck_feature_flag_statusandFliptClient.evaluate_booleancatch and returnFalsefirst. Remove it or stop relying on it. - The default Flipt path logs nothing.
FLIPT_SERVICE_AVAILABLE != "true"at:154returnsFalsewith zero logging, and that is the default. Combined with H5's collapsed guard, "I ramped the flag and no email arrived" has no log line distinguishing which of four causes applied. recipient_countis post-filter only.group_notification_service.py:93-99and:144-150log the surviving count; the requested count is never logged, and_groups_in_org/_live_member_usersboth drop silently. "Half my team didn't get it" is unfalsifiable from logs. Logrequested / resolved / dropped.- 2N+1 on the group fan-out.
:89-92runs onevalues_listplus oneOrganizationMemberquery per group. Collapse to a singleGroupMembership.objects.filter(group__in=…).select_related("user", "group")grouped in Python. - Over the 30-line ceiling (CLAUDE.md):
send_resource_shared40,_post_group_notification35,send_membership_changed31. _notification_contextis duplicated. The module-level function atresource_share_views.py:62is a line-for-line copy ofOwnerManagementMixin._notification_context(permissions/membership_views.py:88). Two copies that will drift, and their docstrings already contradict each other on whether hosts overrideget_notification_resource_type— all seven do.- Docstrings that misstate contracts:
internal_views.py:8-10— "non-2xx so the queue redelivers" holds only on the PG transport; on Celery a raise is terminal, astasks.py:471-472itself says.tasks.py:481-487— self-contradictory: "a 4xx is not retried" versus "the raise leaves the message on the queue for redelivery". Thebreakat:513still falls through to theraiseat:524. Also the guard is< 500, not 4xx.share_notifications.py:9-10— transport is resolved per resource, not per org:_dispatchpasses the resource/group pk asexecution_id, which is whatresolve_transportbuckets the rollout on.resource_share_views.py:3-6— the mixin is no longer axis-agnostic (theshare_axesClassVar is gone and_read_axishardcodes both names), and it does not read_SUPPORTED_SHARE_AXES— only_extract_desired_share_statedoes.
- Frontend, partial-failure UX contradicts itself.
CoOwnerManagement.jsxkeeps the modal open on partial failure "so the user can see what was rejected and retry", butonApplyCoOwnersalways callsrefreshCoOwnerDatafirst, and theuseEffectre-seedsselectedOwnersfrom the refreshed roster — so the staged edits are already wiped and there is nothing to retry from.
Verified clean
The dead-code removal holds up: the shared_users M2Ms were dropped by the UN-2202 migrations and every serializer now exposes shared_users as a read-only SerializerMethodField, so those partial_update hooks could never have fired. No references to share_axes, AxisDiff, snapshot_share_axes, or diff_share_axes remain in either repo.
Auth on the new internal endpoints is genuinely enforced — InternalAPIAuthMiddleware gates every /internal/ path before DRF runs, and the route is reachable in all three deployments. Org scoping on _load_resource / _groups_in_org / _live_member_users is correct. Every OSS→cloud ResourceType mapping checks out. The onApplyCoOwners rename is fully propagated across all 11 consumers in both repos.
_users_left_without_access is safe despite compute_effective_members excluding owners: ResourceMembership has UniqueConstraint(user, content_type, object_id), so a user is OWNER or VIEWER and never both. Reading the diffs after _commit is also safe — ATOMIC_REQUESTS defaults to False and is pinned False in the chart.
|
| Filename | Overview |
|---|---|
| backend/tenant_account_v2/group_notification_service.py | Resolves resources and recipients at delivery time, filters stale grants and retained access, and applies the revoke membership cutoff correctly. |
| backend/tenant_account_v2/share_notifications.py | Feature-gates and dispatches group notification events while capturing revoke timestamps before external flag evaluation. |
| backend/permissions/resource_share_views.py | Centralizes post-commit direct grant and revoke notifications while suppressing revokes for users retaining effective access. |
| backend/tenant_account_v2/internal_views.py | Validates worker payloads, resolves organization context, and preserves retry behavior for transient failures. |
| workers/notification/tasks.py | Adds thin Django-free worker callbacks for the new internal group-notification endpoints. |
| frontend/src/components/widgets/co-owner-management/CoOwnerManagement.jsx | Stages co-owner changes locally so removals occur only when the user applies the modal. |
| frontend/src/hooks/useCoOwnerManagement.jsx | Consolidates co-owner additions and removals into a single apply callback. |
Sequence Diagram
sequenceDiagram
participant User
participant API as Django share/group API
participant Queue as Notification queue
participant Worker as Notification worker
participant Internal as Internal notification API
participant Email as Email service
User->>API: Grant or revoke access
API->>API: Commit sharing change
API->>Queue: Enqueue notification event
Queue->>Worker: Deliver task
Worker->>Internal: POST event with organization context
Internal->>Internal: Revalidate resource, group, and recipients
Internal->>Email: Send notification to eligible recipients
Internal-->>Worker: Success
Reviews (8): Last reviewed commit: "UN-3494 [TEST] Cover share/revoke notifi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
frontend/src/components/widgets/co-owner-management/CoOwnerManagement.jsx (2)
140-142: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSet an explicit
rowKeyon the List.
Listfalls back to the array index whenrowKeyis absent.selectedOwnersnow changes by insertion and removal, so index keys make React reuse a row component for a different user. Thekeyon the innerPopconfirmdoes not controlList.Itemreconciliation, so an open confirm popup can attach to the wrong row after a staged removal.♻️ Proposed change
<List dataSource={selectedOwners} + rowKey={(item) => item?.id} renderItem={(item) => (🤖 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 `@frontend/src/components/widgets/co-owner-management/CoOwnerManagement.jsx` around lines 140 - 142, Update the List rendering in CoOwnerManagement to provide an explicit rowKey based on each selected owner’s stable unique identifier, rather than allowing index-based keys. Keep the existing renderItem and Popconfirm behavior unchanged.
114-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClose and Cancel stay active during Apply.
confirmLoadingdisables the OK button only. The close icon and the Cancel button remain clickable whileapplyingis true. The user can dismiss the modal while requests are in flight. The requests still complete and the alert still appears, so the outcome is not lost, but the state is confusing.Disable both controls while
applyingis true.♻️ Proposed change
confirmLoading={applying} okButtonProps={{ disabled: !hasChanges }} + cancelButtonProps={{ disabled: applying }} maskClosable={false} centered - closable={true} + closable={!applying}🤖 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 `@frontend/src/components/widgets/co-owner-management/CoOwnerManagement.jsx` around lines 114 - 119, Update the CoOwnerManagement modal configuration so both the close control and Cancel action are disabled while applying is true, while preserving the existing confirmLoading behavior. Use the existing applying state in the modal’s closable and cancel-button properties.frontend/src/hooks/useCoOwnerManagement.jsx (1)
19-22: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard the zero-change call in the hook.
If
addUsersandremoveUsersare both empty,totalis 0 andfailed.length === totalis true.buildApplyAlertthen callshandleException(null, "Unable to update co-owners")and shows an error alert for a no-op.CoOwnerManagement.handleApplycurrently blocks this case, but the hook is a shared export and should not depend on that caller guard.♻️ Proposed guard
const total = addUsers.length + removeUsers.length; - if (failed.length === total) { + if (total === 0) { + return null; + } + if (failed.length === total) { return handleException(lastError, "Unable to update co-owners"); }
setAlertDetailswould then need to skip anullalert inonApplyCoOwners.🤖 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 `@frontend/src/hooks/useCoOwnerManagement.jsx` around lines 19 - 22, Guard the zero-change case in the hook’s apply-result handling before comparing failed.length with total: when both addUsers and removeUsers are empty, skip error handling and avoid calling handleException with null. Update the related onApplyCoOwners alert flow as needed so setAlertDetails does not process a null alert, while preserving failure handling for actual changes.
🤖 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 `@backend/tenant_account_v2/share_notifications.py`:
- Around line 150-170: Update the internal sender flow after organization
resolution to call _feature_enabled again before delivering the notification.
When the flag is disabled or Flipt is unavailable, skip delivery and return the
existing successful skipped response, preserving normal sending when the flag
remains enabled.
In `@docker/docker-compose.yaml`:
- Around line 854-857: Update the notification worker visibility-timeout
configuration around WORKER_PG_QUEUE_CONSUMER_VT_SECONDS to account for up to
three 30-second POST attempts with HTTPTransport retries=2, ensuring the
configured timeout exceeds the worst-case transport retry duration; keep
WORKER_PG_QUEUE_CONSUMER_HEALTH_STALE_SECONDS above the resulting visibility
timeout.
In `@frontend/src/hooks/useCoOwnerManagement.jsx`:
- Around line 144-155: Update refreshCoOwnerData and its caller in the apply
flow so it returns whether the resource-not-found (404) branch was reached;
after awaiting refreshCoOwnerData, only call setAlertDetails with
buildApplyAlert when that result indicates no 404 occurred, preserving the
existing resource-gone alert and modal/list behavior.
In `@workers/notification/tasks.py`:
- Around line 503-524: Add an immutable job ID to each notification task and
propagate it through the internal notification API and payload. In the backend
handler, deduplicate requests using that job ID before invoking the notification
plugin, recording successful delivery so retries and PG queue redelivery do not
send the same notification again. Update the retry flow around client.post and
the corresponding task/API symbols while preserving existing retry behavior for
failed deliveries.
---
Nitpick comments:
In `@frontend/src/components/widgets/co-owner-management/CoOwnerManagement.jsx`:
- Around line 140-142: Update the List rendering in CoOwnerManagement to provide
an explicit rowKey based on each selected owner’s stable unique identifier,
rather than allowing index-based keys. Keep the existing renderItem and
Popconfirm behavior unchanged.
- Around line 114-119: Update the CoOwnerManagement modal configuration so both
the close control and Cancel action are disabled while applying is true, while
preserving the existing confirmLoading behavior. Use the existing applying state
in the modal’s closable and cancel-button properties.
In `@frontend/src/hooks/useCoOwnerManagement.jsx`:
- Around line 19-22: Guard the zero-change case in the hook’s apply-result
handling before comparing failed.length with total: when both addUsers and
removeUsers are empty, skip error handling and avoid calling handleException
with null. Update the related onApplyCoOwners alert flow as needed so
setAlertDetails does not process a null alert, while preserving failure handling
for actual changes.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b71cb647-64c1-4252-a93d-5996186b95b2
📒 Files selected for processing (22)
backend/adapter_processor_v2/views.pybackend/api_v2/api_deployment_views.pybackend/backend/internal_base_urls.pybackend/connector_v2/views.pybackend/permissions/resource_share_views.pybackend/pipeline_v2/views.pybackend/prompt_studio/prompt_studio_core_v2/views.pybackend/tenant_account_v2/group_notification_service.pybackend/tenant_account_v2/group_views.pybackend/tenant_account_v2/internal_urls.pybackend/tenant_account_v2/internal_views.pybackend/tenant_account_v2/share_notifications.pybackend/tenant_account_v2/shareable_resources.pybackend/workflow_manager/workflow_v2/views.pydocker/docker-compose.yamlfrontend/src/components/deployments/api-deployment/ApiDeployment.jsxfrontend/src/components/pipelines-or-deployments/pipelines/Pipelines.jsxfrontend/src/components/widgets/co-owner-management/CoOwnerManagement.cssfrontend/src/components/widgets/co-owner-management/CoOwnerManagement.jsxfrontend/src/components/widgets/co-owner-management/CoOwnerModal.jsxfrontend/src/hooks/useCoOwnerManagement.jsxworkers/notification/tasks.py
💤 Files with no reviewable changes (7)
- frontend/src/components/widgets/co-owner-management/CoOwnerManagement.css
- backend/prompt_studio/prompt_studio_core_v2/views.py
- backend/pipeline_v2/views.py
- backend/api_v2/api_deployment_views.py
- backend/workflow_manager/workflow_v2/views.py
- backend/adapter_processor_v2/views.py
- backend/connector_v2/views.py
Group revoke no longer mails members who kept access another way. The revoke recipient list now runs through the same effective-access filter the direct path uses, with owners folded in — compute_effective_members excludes them by design, and the sharer is usually a member of the group they shared with, so revoking told the owner their own access was removed and pointed them at the dashboard. - _get_user is org-scoped through OrganizationMember, the one unscoped query left on this tenant path. Service accounts are kept so a platform-account share still notifies. - _notify_shared_users is wrapped: the share has already committed by the time it runs, so a raising seam or a DB hiccup must not 500 a share that worked. - _users_left_without_access short-circuits on shared_to_org — nobody lost access, and answering it otherwise hydrates every member of the org. - _notification_context loses its duplicate copy and uses the OwnerManagementMixin definition every host already inherits. - Logs the Flipt decision, and how many recipients were dropped versus requested, so a missing email is diagnosable. - Docstrings corrected: the mixin is not axis-agnostic, transport resolves per resource id not per org, the flag is evaluated once at enqueue, and delivery is at-least-once. - Sonar S7632: the noqa directive carried trailing prose. - Co-owner apply no longer overwrites the resource-gone alert with its summary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…vice The PG-queue notification consumer is local dev config and does not belong in the PR. The k8s chart already carries workerPgNotification from UN-3445 (#1688), which is the real deployment surface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Self-review dispositions (OSS)Re-verified every finding from my review above against the code, with an independent adversarial pass on each verdict. Five of my own findings did not survive and are withdrawn below rather than quietly dropped. Code changes in c1d3095; the compose service removal in 9e9f57c. Fixed
Withdrawn — my findings, wrong
Not changing
|
… revoked A revoke resolves recipients from the group's live membership at delivery time, so anyone who joined between the click and the send was told their access was removed for a group through which they never held it. Normally a few seconds; on the PG transport with no consumer deployed the backlog can sit far longer. The revoke now carries the timestamp of the change and delivery drops memberships created after it. One string on the payload rather than the frozen member list, which would grow with the group. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ready gone A grant enqueued before a revoke could still be delivered after it, mailing the resource name and id to members who can no longer reach the resource. Delivery now revalidates the live ResourceGroupShare on the grant direction and drops groups that no longer hold it. The revoke direction needs no equivalent check — its share row is gone by delivery, and _retained_user_ids already covers members who kept access another way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@greptileai review |
revoked_at was captured after _feature_enabled(), so the window between the share-removal commit and the timestamp spanned a Flipt network call. A user joining the group inside it passed the cutoff and was mailed a revocation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
965f4d2 to
70b42c8
Compare
|
@greptileai please review |
Enqueue side (unit tier, no DB): payload shape, the revoked_at stamp landing before the Flipt round-trip, and the skip/swallow paths. Delivery side (integration tier): recipient selection - the live re-read on a grant, the revoked_at cutoff, org scoping and retained access - plus the direct-user share/revoke wiring on the share endpoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
Unstract test resultsPer-group results
Critical paths
|
1 participant
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.shared_to_org, or ownership. Both the direct and the group path run that filter.CoOwnerManagementstages one owner list and diffs it at Apply likeSharePermission; the hook's two callbacks collapse intoonApplyCoOwners, adds before removes.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)
group_sharing_notifications_enabledgates the group paths (share/revoke to a group, group membership change). The direct-user share and revoke mail is not on that flag — like the co-owner mail it reuses, it is bounded only by the cloudENABLE_EMAIL_NOTIFICATIONSsetting, and it goes live for every org on merge. That path was the intended behaviour before UN-2977 broke it, so it is a restore rather than a new dark feature.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. Co-owner removal is intentionally no longer instant; the widget prop becameonApplyCoOwnersand all consumers are updated.Database Migrations
Env Config
group_sharing_notifications_enabled— new, gates the group notification paths. Off or unreachable means no group emails.pg_queue_enabled— existing, reused to pick PG queue vs Celery transport.workerPgNotificationmust be enabled in an environment beforepg_queue_enabledis ramped there, or notifications land durably in the PGnotificationsqueue with nothing consuming them.Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.