Resource Sharing v1 + v1.1 (backend): per-role access, grants, groups, requests, chart/KPI/metric sharing#1433
Open
siddhant3030 wants to merge 66 commits into
Open
Resource Sharing v1 + v1.1 (backend): per-role access, grants, groups, requests, chart/KPI/metric sharing#1433siddhant3030 wants to merge 66 commits into
siddhant3030 wants to merge 66 commits into
Conversation
…CADE->SET_NULL flip Lays the schema foundation for Resource Sharing Layer 1 (who can View/Edit dashboards, reports, metrics, KPIs, alerts): - New GeneralAudience/GeneralLevel TextChoices (ddpui/models/general_access.py). - general_audience/general_level/owner columns on Dashboard, ReportSnapshot, Metric, KPI, Alert; owner-only on Chart (charts ride along with their dashboard, not independently shareable). - Flip created_by from CASCADE to SET_NULL on Dashboard, Chart, Metric, KPI, Alert: deleting an OrgUser no longer silently deletes everything they created. ReportSnapshot was already SET_NULL. - Migrations 0168 (schema) + 0169 (data: owner = created_by backfill on all 6 models, guarded against clobbering an owner already set). - OrgPreferences: default_general_audience, default_general_level, allow_public_sharing. - can_delete_resource() is now owner-first: owner_id wins, falling back to created_by_id only when owner_id is null. Covers all 6 existing call-sites with one change. No API/serializer changes and no resolver/grants code in this task -- those are later tasks. Note for reviewers: since created_by can now be null, any future read path doing resource.created_by.user.* needs a null guard (the existing to_json() methods already guard it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…stry, access resolver Lays the correctness core of Resource Sharing Layer 2 (task 2): explicit per-principal grants on top of Task 1's general access columns. - ResourceShare model (ddpui/models/resource_share.py, migration 0170): soft (resource_type, resource_id) pointer rather than a FK -- a Layer 2/3 contract to support UUID pks and warehouse "schema.table" ids later. Open principal_type enum (user/group/audience); only user/group are matched in v1. - ddpui/core/sharing/shareable_types.py: RESOURCE_TYPES registry (data only) for dashboard/report/alert/metric/kpi with capability flags. `chart` is deliberately not registered. - ddpui/core/sharing/access_resolver.py: pure read-only decision ladder -- admin/super-admin override, ownership (owner_id falling back to created_by), role-gated general access, ResourceShare grants (group membership injected via a get_group_ids callable so Task 7 can plug in the real lookup without touching resolver logic), best-of with a Member cap at "view". ROLE_RANK derives from role slugs, not Role.level. All role reads are getattr-safe -- a null/unknown role denies rather than raises. accessible_filter() builds the same ladder as one ORM Q for list endpoints, verified at exactly one query via django_assert_num_queries. - ddpui/core/sharing/sharing_actions.py: stub (mutations land in Task 5). - 33 truth-table tests against real ORM fixtures (ddpui/tests/core/sharing/test_access_resolver.py), covering every cell in the brief plus admin-tier general-access rows, a cell pinning step 3 to the resource's actual general_level (not a hardcoded "view"), and a cell documenting that explicit grants apply independent of role rank. Full suite: 2131 passed, 2 skipped (baseline 2098 + 33 new, 0 regressions). `manage.py makemigrations --check` reports no drift. Note for Task 3: accessible_filter's owned-set is owner=viewer OR created_by=viewer (brief's literal formula) while effective_permission's ownership check is owner-first-with-created_by-fallback (matches can_delete_resource). A resource created by a viewer but later reassigned to a different owner can appear in that viewer's filtered list yet resolve to None on a per-resource effective_permission check -- this is each function's specified behavior, not a bug, but the two shouldn't be assumed interchangeable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… view-gate 5 detail GETs Part A — list scoping (service/queryset layer, where the org filter lives): - DashboardService.list_dashboards, ReportService.list_snapshots, AlertService.list_alerts, MetricService.list_metrics, KPIService.list_kpis now take the viewer (orguser) and AND accessible_filter(viewer, rtype) into the existing org-scoped Q — one composed query, no N+1. - Admin/super-admin skip the filter entirely (sees everything in-org), via new ownership.is_admin_or_super_admin(); can_delete_resource now reuses it. Charts list deliberately untouched (stays role-gated). Part B — detail view-gate (403 after org-scoped 404 check): - get_dashboard, get_snapshot_view, get_alert, get_metric, get_kpi deny with 403 when effective_permission(viewer, rtype, resource) is None. - Convention: 404 stays "doesn't exist / wrong org" (unchanged); 403 = "exists in your org but not shared with you" — matches the codebase's existing 403 usage (delete guards, sharing-settings guards) and gives the later request-access frontend flow a signal to key off. Tests: per-rtype admitted-set truth tables for member/analyst/admin with query-count assertions (ddpui/tests/core/sharing/test_list_scoping.py), detail-gate 403/200/404 cells (test_detail_view_gate.py); existing list-service tests updated for the new required orguser parameter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ic/kpi/alert/dashboard Task 3 gated the 5 single-resource detail GETs but left every by-id sub-endpoint on the same routers (chart/KPI data, previews, consumers, logs, notes, filters, PDF/email export, dashboard duplication) checking role only, so a Member could still reach a private resource's content directly by id. Closes that gap with a shared require_view_access() helper wrapping Task 3's effective_permission() gate, applied at 17 endpoints across report_api, metric_api, kpi_api, alert_api, and dashboard_native_api (including a few not in the original list, e.g. report share-via-email and dashboard duplication, documented in the task report). get_kpi_summary is flagged as an unresolved list-scoping gap for follow-up, out of scope for a by-id gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
get_kpi_summary returned computed values + RAG status for every KPI in
the org with no accessibility scoping, letting a Member see private
KPIs' values via the KPI page summary endpoint even though list_kpis
was already scoped in a prior commit.
Mirror the same admin-bypass + accessible_filter("kpi") pattern used by
list_kpis, at the service/queryset layer.
…text, Member standalone deny, run_chart_query seam Charts are not shareable: a chart is visible wherever its dashboards are visible. The two by-id chart GETs (detail + data) gain an optional dashboard_id access context: serve iff the chart is actually on that same-org dashboard (membership check — otherwise dashboard_id is an oracle to read arbitrary charts) and the resolver grants >= view on it. Standalone (no dashboard_id) keeps Analyst+ behavior, admits the chart's owner (owner_id, created_by fallback), and denies plain Members 403. All warehouse-bound execution on the gated data path now routes through run_chart_query in core/sharing/chart_access.py — a pass-through today, the single choke-point for Layer 2/3; ViewerContext already admits PublicLinkContext so public renders can be rewired later. dashboard_id (access context) is independent of dashboard_filters (filter payload). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s warn-and-offer, share-slug seed, update edit-guards
- sharing_actions.py: get_access_overview, upsert_grant (dedupe + re-share
cap), remove_grant, set_general_access with the narrowing warn-and-offer
protocol (first call returns requires_confirmation + persisting_grants,
re-send with remove_grant_ids commits)
- access_api.py mounted at /api/access: thin routes generic over {rtype};
GET gated by resolver view, mutations by the registry-driven share slug
(require_share_permission mirrors @has_permission) + resolver edit
- registry: share_permission_slug per rtype (data, not if/else)
- migration 0171 + seed JSON: can_share_reports/alerts/metrics/kpis for
super-admin/admin/analyst (member none); Redis key delete wrapped so
Redis-less CI passes; idempotent
- Part C: resolver-edit object check on the 5 rtypes' update endpoints
(dashboard/report/metric/kpi PUT, alert PUT + toggle PATCH)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ub-resource writes Task 5 added the resolver-edit object check to the 5 main resource UPDATE endpoints but missed sub-resource WRITE endpoints on the same routers, which were still gated only by role slug. A viewer with view-only access to a dashboard or KPI (via general access or an explicit view grant) could still create/update/delete its filters, notes, or acquire/refresh/release its edit lock, since the can_edit_* slug says what the role may do in general, not what the viewer may do to this object. Adds require_edit_access (the Task 5 gates.py helper) after the existing org-scoped fetch on: dashboard filter create/update/delete, dashboard lock acquire/refresh/release, and KPI note create/update/delete. Swept the remaining POST/PUT/DELETE endpoints across all 5 rtypes' routers; no other gaps found (creates, dry-runs, and already correctly-gated endpoints excluded; comments/public_api/api-access excluded per scope).
Adds UserGroup/UserGroupMember models and the /api/groups CRUD+membership
router (Task 7), and wires the access resolver's group-ids seam to a real
UserGroupMember-backed lookup as the default so every existing caller
(lists, detail gates, /api/access/*) picks up group membership with no
edits. Flips the pinned group-principal 400 on POST /api/access/{rtype}/
{id}/grants/ now that groups exist, and includes group name + member_count
in the access overview.
- New can_manage_user_groups / can_view_user_groups permission slugs
(Analyst+, mirrors can_share_dashboards' holder set); seeded via
migration 0173 following 0171's pattern.
- principal_match_q no longer force-materializes get_group_ids(viewer) with
set(...) — it passes the value straight through to principal_id__in= so
the real default (a lazy UserGroupMember queryset) embeds as a SQL
subquery instead of an extra round trip. Verified query counts stay flat
on the existing list-scoping tests plus a new one that seeds a real group
membership.
- Groups live in ddpui/models/user_group.py and ddpui/core/user_groups/,
not core/sharing — Layers 2-3 can import groups without the grant
machinery. Deleting a group also deletes its ResourceShare grant rows
(soft pointer, no DB cascade) so a dangling grant can't keep admitting
people.
47 new tests (2306 -> 2353 passed, 2 skipped baseline held).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… activation, invite-tier cap
Task 9 (Milestone 4 backend): Invitation.expires_at (30-day default,
backfilled; resend refreshes it; accept rejects expired invites cleanly),
a daily Celery cleanup task for stale invitations + their pending
ResourceShare/UserGroupMember rows, and the share-flow invite: POST
/api/access/{rtype}/{id}/grants/ now accepts an `email` alongside
principal_id for principal_type="user" — an existing OrgUser's email
grants instantly, an unknown email invites them as a Member (closing the
2>=2 loophole on this path only) via invite_user_v1's own machinery and
creates a pending grant. accept_invitation_v1 now activates the accepting
email's pending ResourceShare/UserGroupMember rows, matched by
invited_by__org + email (Invitation has no org FK) so cross-org invites
of the same email never cross-activate.
Migration 0174 adds Invitation.expires_at. sharing_actions.py calls into
orguserfunctions.py (one-directional) for the invite machinery; the
resolver (access_resolver.py) is untouched — its status="active" filter
already excludes pending grants.
…coping on invite accept Two follow-ups from review of the Task 9 invites commit: exercise the add_member/get_or_create collision branch in activate_pending_shares_and_memberships (an active (group, orguser) row already existing when a pending row would activate must drop the pending row, not violate the unique constraint), and assert accessible_filter (not just effective_permission) admits the resource for a newly-accepted user, matching the brief's "sees the resource in their list" wording.
…s, orguser_id, table/map chart-data gating Task 6b. Three backend gaps the ShareModal frontend task flagged: Part A — DashboardResponse (list + detail + create/update/duplicate) gains general_audience, general_level, is_owner, is_creator. Ownership semantics via new ddpui.core.ownership.is_owner (owner_id wins, created_by fallback; no admin override — mirrors the resolver's _is_owner). Zero extra queries per row (id-column comparisons only); the public dashboard response nulls the general-access fields for anonymous viewers. Part B — OrgUserResponse gains orguser_id (the OrgUser PK, what /api/access grants' principal_id and /api/groups members' orguser_id want; user_id is the Django User FK and stays untouched). Wired at all 3 construction sites (from_orguser, currentuserv2, organizations/users). Part C — POST /api/charts/chart-data-preview/, /chart-data-preview/ total-rows/ and /map-data-overlay/ gain optional chart_id + dashboard_id access context routed through Task 4's require_chart_view_access, with a schema/table match guard so a viewable chart isn't an oracle for arbitrary tables, and warehouse execution routed through the run_chart_query seam. Config-only requests (no chart_id — the builder's unsaved preview) are Analyst+ via new chart_access.require_analyst_plus: this tightens chart-data-preview/total-rows (previously open to Members through the has_schema_access stub) and preserves map-data-overlay's pre-task posture while its decorator moves can_view_warehouse_data → can_view_charts so Members can reach the gated dashboard-tile path. Tests: +33 (test_dashboard_dto_fields.py, test_chart_post_gate.py, orguser_id assertions in test_user_org_api.py). Full suite 2398 passed / 2 skipped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…OSTs
Task 6b admitted Members to chart-data-preview(/total-rows) and
map-data-overlay via chart_id+dashboard_id, but the body still carried the
chart CONFIG, so an admitted viewer could name OTHER columns of the
chart's table (or probe rows via novel filters) — a read primitive for
every column including PII.
require_payload_within_chart_config (chart_access.py) now pins, on the
dashboard-context path only: query columns (top-level + nested in
extra_config) to the saved config's column set incl. map layers/drilldown
hierarchy; metric column_expression (raw SQL) and saved_metric_id to
verbatim saved values; filter columns to saved-config columns plus the
framing dashboard's filter columns (values stay free); sort columns to
saved/allowed columns or guarded metric aliases; and the dashboard_filters
{filter_id: value} map to the framing dashboard's own filter ids (the
handlers' DashboardFilter lookup is unscoped). Violations 403 with the
same generic message as the schema/table guard — no column-name oracle.
Analyst+ standalone and config-only requests are untouched (pinned).
Legit tiles pinned green: saved config verbatim, dashboard filter added
both ways, table/map drill-down.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…aring fields, org-default general access Task 11 (backend): OrgPreferences.allow_public_sharing now gates all public sharing per org, read fresh each request (no row = default True): - Toggle endpoints (dashboard PUT /share/, ReportService.toggle_sharing) refuse to newly publish or re-enable with 403 while off; turning a link OFF always stays allowed. - Every token-gated public render in public_api.py treats existing public links as dead (404, matching the token-not-found convention) while off -- no data touched, so flipping the switch back on revives all links immediately. The X-Render-Secret PDF path stays ungated (internal, already access-checked at the authenticated endpoint). PUT /api/orgpreferences/sharing/ (admin/super-admin only, direct role check) updates allow_public_sharing / default_general_audience / default_general_level with enum validation (400 on bad values); GET / now returns all three via to_json(). New shared helper core/sharing/general_access_defaults.py seeds general_audience/general_level from the org's defaults (falling back to all_users/view) at all 6 creation paths: dashboard create + duplicate, report snapshot, alert, metric, KPI. A duplicated dashboard adopts org defaults, not the source's audience. 47 new tests incl. revival, unpublish-always-allowed, no-prefs-row, cross-org isolation, non-admin denial, invalid enum 400. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
POST /api/access/{rtype}/{resource_id}/owner/ transfers resource.owner to
another same-org, active OrgUser. Gate: rtype's share-permission slug plus
require_owner_access (new, stricter than require_edit_access -- only literal
ownership or admin/super-admin passes, mirroring ownership.can_delete_resource).
The old owner gets an explicit active Edit ResourceShare grant on transfer
(uniform rule, applied even for grants=False rtypes like metric/kpi by
writing the row directly rather than through upsert_grant, since that
capability flag gates the public grants endpoint, not this action). No
reclaim: the old owner simply stops passing the owner gate afterwards.
Self-transfer (new owner already owns the resource) is a 400; cross-org,
unknown, or inactive new-owner ids are a 404, consistent with upsert_grant's
existing PrincipalNotFoundError convention.
… (Task 13)
Recipients JSON gains a {"type": "group", "group_id": int} entry alongside
the existing orguser/external shapes, validated cross-org at create/update.
At fire time, delivery.deliver_all expands group entries to the group's
ACTIVE members' delivery targets, deduped against directly-listed
recipients (someone both direct and in a group gets one delivery); a
deleted/empty group is skipped gracefully rather than crashing the run;
pending (email-only) members are never delivery targets.
ResourceShare still governs alert config access only, not delivery -
recipients (direct or via group) grant no resource access, pinned with a
resolver test.
Every fired notification now appends a fixed footer with the alert id and
a deep link, on top of the trigger context (metric/kpi name, current/target
value) already carried via message_template tokens - closing the "alert
recipient without access lands on request-access" deliverable's backend
half.
…sk 14) Comment create relaxes from the can_edit_dashboards slug to can_view_dashboards + resolver-view on the report — a Member who can see a report can comment on it. Moderating OTHERS' comments (update/delete in CommentService) now requires resolver-edit on the report (owner, admin, or edit-grant holder); authors keep their exact self-edit/delete rights. Anonymous stays blocked (401 pinned via real HTTP; public router pinned to zero comment routes). Comment READ gates unchanged from Task 3b (view-gated GETs, 400-not-404 convention). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AccessRequest model (migration 0175) + POST /api/access/{rtype}/{id}/requests/
(create, any org member, no share slug), GET /api/access/requests/
(incoming = decidable pending requests, outgoing = caller's own, any
status), POST /api/access/requests/{id}/approve/ and /decline/. Approve
inserts a ResourceShare grant via the same internal-write pattern Task 12's
ownership transfer used, so it works for grants=False rtypes (metric/kpi)
too; an owner may only downgrade the requested permission, never escalate.
Duplicate pending requests (including expired-but-unswept ones) refresh in
place instead of stacking. Approve/decline gate on require_owner_access
only (owner-or-admin), deliberately without require_share_permission -- a
Member who owns a resource via ownership transfer holds no can_share_*
slug but must still be able to decide requests on their own resource.
New requests notify the resource owner (fallback: org admins); decisions
notify the requester -- reusing the existing Notification/NotificationRecipient
models, in-app only. Deep links mirror Task 13's per-rtype convention.
Logic lives in a new core/sharing/access_requests.py sibling module rather
than sharing_actions.py, per the plan's deferred module-layout decision for
this task. The daily cleanup task (Task 9) now also sweeps stale pending
AccessRequest rows to expired, on the same beat tick.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
POST /api/access/bulk applies one action (add_grant / set_general / toggle_public) across a mixed-rtype selection, apply-where-possible: per-item gates (registry, share slug, org-scoped fetch, resolver edit, capability flags) produce skipped rows with reason codes, never an all-or-nothing 4xx. Unknown-email grants invite exactly once across the selection; set_general aggregates the narrowing warn-and-offer prompt; toggle_public honors the org kill switch (enable blocked, disable always allowed). Selection capped at BULK_MAX_ITEMS=100; no selection-wide transaction by design. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…k paths (Task 11b) toggle_dashboard_sharing and toggle_report_sharing were still creator-only, while Task 17's bulk toggle_public gates on share slug + resolver edit (the plan's intended model) -- a non-creator editor could toggle via bulk but not via the single-item modal endpoints. Both legacy toggles now use require_edit_access (same as bulk) and route the actual flip through sharing_actions.set_public, so the kill-switch rule lives in exactly one place. Also fixed toggle_report_sharing's @has_permission decorator, which checked can_share_dashboards instead of the report rtype's registered can_share_reports slug (behavior-preserving: the same roles hold both slugs today). Deleted the now-dead ReportService.toggle_sharing (duplicated creator check + kill-switch + mutation logic). Response shapes are unchanged. Widens who may toggle from creator-only to any editor (grant or general access) with the share slug -- pinned with new tests; renamed one pre-existing creator-only pin whose assertion (403) still holds but for a new reason (view-only, not creator-only).
Task 11b widened the public-sharing TOGGLES to slug + resolver-edit, but the sharing-status GETs stayed creator-only, so a newly-empowered non-creator editor got a 403 opening the ShareModal and never saw Copy-Public-Link/access-count. - get_dashboard_sharing_status: replace the inline creator check with require_view_access(orguser, "dashboard", dashboard). - get_report_sharing_status / ReportService.get_sharing_status: apply the same layering refactor 11b used for the toggle -- fetch + require_view_access move up to the API layer, the service only shapes the response from an already-authorized snapshot (core can't raise HttpError). - The status GET only reveals whether an already-visible resource is public, so view access is correct; the toggle (write) stays edit-gated. Updates the pre-existing creator-only GET pins in test_report_api.py and test_report_permissions.py to the new model, and adds equivalent coverage for get_dashboard_sharing_status (previously untested). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ads (Task 15b) The request-access notification links emitted ?metricId=/?kpiId=, but the live pages read different params -- /metrics reads ?highlight= (row highlight in metrics-library) and /kpis reads ?open= (auto-opens the detail drawer). The old links silently no-oped. /alerts?alertId= was already correct (Task 13/16 convention). Pins the full per-rtype deep-link contract in TestNotificationDeepLinks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…alizers (I1) created_by was flipped from CASCADE to SET_NULL on Chart/Metric/KPI, so a deleted user's resources now surface with created_by=None. Ten serializer sites still dereferenced created_by.user.email unguarded and would 500 the list/detail endpoints org-wide the first time that happens. Applies the existing Dashboard/ReportSnapshot idiom (x.created_by.user.email if x.created_by else None) at all ten sites: charts_api.py (list/get/create/update), metric_api.py (list/create/get/ update), and kpi_service.py's shared kpi_to_response (KPI's own creator and its nested metric's creator). Response schema created_by fields (ChartResponse, MetricResponse, KPIResponse) are now Optional[str]. Swept charts/metric/kpi api+service+schema code for any other unguarded created_by dereference; found none beyond the ten (owner/last_modified_by are never serialized on these responses, so they aren't reachable via this bug class).
…iew on groups list (Phase A) - OrgUserResponse.invited_by: inviter's email from the most recent Invitation matching the user's email (iexact), None when the user joined without an invitation; built from one query, no N+1 (A1) - GroupOut.member_preview: up to 4 ACTIVE member emails for the Groups-table avatar stack, filled only in list_groups via a single prefetch (A2) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When a NEW direct user-principal grant is created for an existing ACTIVE
org user via upsert_grant, email the grantee ("{granter} shared a {type}
with you"). Fires only on grant CREATE — never on a permission update,
never for group grants, never on the invite/pending path (that path
already sends its own invitation email). Email failure is logged and
swallowed so it can never fail the share request.
- Extract the per-rtype deep-link map + URL/label builders out of
access_requests.py into a new deep_links.py so the write path
(sharing_actions) can reuse them without an import cycle;
access_requests re-imports them under their old names (tests unchanged).
- Add awsses.send_resource_shared_email (+ send_added_to_group_email for
D2) as plain-text senders, matching the invitation flow's transport.
Names/types/links only — never resource data.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When an existing ACTIVE org user is added to a group via add_member,
email them ("You have been added to the {group} group by {adder}").
Fires only on a genuinely NEW active membership — never on an idempotent
re-add. add_member only creates orguser-backed rows, so pending-email
rows never trigger it. Email failure is logged and swallowed so it can
never fail the add-member request.
The "Explore Workspace" CTA points at the webapp root; the frontend URL
is computed locally (settings fallback pattern) so user_groups keeps no
dependency on core/sharing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hase C: C3) The grants POST body gains an optional invite_role (member/analyst/admin slugs, default member), consulted ONLY on the unknown-email invite path. Non-member values are admin/super-admin callers only — enforced in _resolve_invite_role BEFORE any invitation email or pending grant row exists, raising the new SharingPermissionError which the API layer maps to 403 (create_grant and the bulk fan-out alike). This is deliberately stricter than invite_user_v1's inviter-tier level check, which still runs downstream as defense in depth. The invitation carries the chosen role, so the existing accept-activation path grants the invited user that role with no further changes. Existing callers are unchanged (param optional; Member default pinned by the untouched T9 tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… side effects The bulk add_grant invite path had no invite_role coverage. New test mirrors the single-grant pin (test_access_api.py, Phase C3): a non-admin requesting invite_role='analyst' on POST /api/access/bulk/ fails the whole request with 403, and no Invitation or pending ResourceShare row is written. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GroupMemberOut.members rows now carry role (the OrgUser's new_role.slug),
populated on the GET /api/groups/{id} detail path with select_related to
avoid N+1. Pending-email rows (no OrgUser yet) keep role None.
No behavior changes -- these tests document what _invite_email_once /
invite_user_v1 actually do today, adversarially verified against an
audit finding that the group-add-by-email consumer had no coverage of
the share flow's inherited quirks:
- Cross-org existing-user email (group consumer): an email matching
ANY existing platform User -- even one that only belongs to a
different org -- instantly provisions a brand-new OrgUser in THIS
org (no Invitation, no confirmation step) at whatever role
_resolve_invite_role resolves for the caller, not the role they hold
elsewhere. Fires both send_youve_been_added_email and the group's D2
send_added_to_group_email. Flagged inline as a product decision
worth revisiting ("instant cross-org provisioning — pinned pending
product review").
- Same-org re-invite dedupe (both share flow and group consumer): a
second invite to an email with an existing pending Invitation
refreshes the same row (invited_on/expires_at, resent email) instead
of creating a second one, and never changes invited_new_role even if
a later, more-privileged caller requests a higher one.
… layers, migrations 0177+0178
/v1/users/invitations/ previously returned only invitations the caller themselves sent (Invitation.objects.filter(invited_by=orguser)), so a second admin could not see invites another admin sent. The merged People table needs org-wide visibility with real "Created By" per row. - get_invitations_from_orguser_v1: scope by invited_by__org instead of invited_by (select_related to avoid N+1), and add invited_by (email string, matching how OrgUserResponse.invited_by is already shaped) to each row. - Permission gate is unchanged (can_view_invitations) -- its holders (Super User/Admin/Analyst) are already exactly the "administer users" population; Member holds can_view_orgusers but not can_view_invitations, so switching the gate to can_view_orgusers would let Members fetch org-wide invitation data via direct API call even though the frontend hides it. Flagging as a judgment call in the report in case the intent was to broaden visibility to Member. - Tests: org-wide visibility (admin B sees admin A's invite, correct inviter attributed to each row), inviter field populated, permission gate still rejects a role without can_view_invitations.
…rg-wide list Reviewer finding: the role FK lazy-loaded per row; org-wide scope multiplies it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Public dashboard chart endpoints (metadata, data, data-preview, map overlay, CSV download, total-rows) checked only that a requested chart_id belonged to the public dashboard's org -- not that it was actually placed as a tile on that dashboard. Any anonymous visitor with one public dashboard link could fetch every chart in the org (config + data) by guessing/iterating chart_id. Reuses the existing tabs->components->config.chartId walk (chart_access._dashboard_chart_ids) that the authenticated render gate already checks membership against, instead of adding a fourth copy of that walk. Legitimate tile requests are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ing, gates) Charts become a registered shareable resource while container-context inline rendering stays byte-identical (spec Sec 3 inheritance rule): - Chart gains analyst_level/member_level; migration 0179 backfills existing rows behavior-preserving (edit/none, decision #3) via the AddField defaults, and seeds can_share_charts for existing DBs (fixtures pk 92 + role grants cover fresh installs). - Registry: chart entry (general+grants+requests, no public link) with a new member_sharing flag (False for charts) encoding decision #2 — Member chart sharing deferred — as registry data, not rtype branches. - Resolver + accessible_filter: Member viewers get no general/grant contribution on member_sharing=False rtypes (group grants allowed; their Member members resolve to nothing). Ownership untouched. - Grant rules: user-principal grants Analyst/Admin only (Member -> 400); email invites only at an Analyst/Admin invite role (checked before any invitation is sent, single + bulk); member_level pinned to none in set_general_access and model clean(). - Request-access: Member requesters on charts get a clear 400 pointing at the dashboard request path. - list_charts scoped through accessible_filter (admins unrestricted); new charts seed org defaults with the member level clamped to none. - Standalone gates: require_chart_view_access's standalone branch now asks the resolver for view on the chart (dashboard-context branch unchanged); chart update requires resolver Edit; /dashboards/ sub-endpoint requires resolver View. Deep-link map + gate nouns know charts. Regression suites (render gate, column guard, post gate, list scoping) green unmodified; 36 new tests in test_chart_sharing_v11.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…block) Flips metric/kpi to grants=True in the sharing registry so the existing generic /api/access/* grant endpoints, share modal, and bulk actions work for them -- previously only general access + request-approve worked. Adds the v1.1 consistency rule (mirrors the plan's chart rule): a Member principal can't be granted metric/kpi directly, and an unknown-email invite that would resolve to Member is blocked too -- both 400 with a clear message. Group grants are untouched (a Member reached via a group grant still resolves capped at "view", the existing resolver rule). The request-approve path is untouched -- it still writes Member grants for approved requests, since the deferral only targets new proactive shares. Kept shareable_types.py to the grants=True flip only; the Member-grants policy lives in sharing_actions.py as a plain rtype set so a concurrent branch adding a chart registry entry stays a trivial merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…, member_sharing exclusion, scoping, gates)
…olution per spec §6b) Conflicts in sharing_actions.py resolved by running BOTH mechanisms: charts' registry member_sharing flag AND metric/kpi's deferred-grants set. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e gates
Coverage (exposure honesty, spec §3):
- core/sharing/coverage.py: per-chart verdicts vs a dashboard's audience —
role gaps (analyst extendable / member informational), principal gaps
(users+groups, Member principals flagged skipped), public exposure
(never extendable). Batched queries, resolver-parity incl. the new
principal's own group memberships.
- GET /api/dashboards/{id}/chart-coverage/?chart_id= — embed pre-flight
(single) + all under-covering tiles (bulk). Gate: dashboard Edit.
Broadening warnings (requires_confirmation, mirroring the narrowing
contract; re-send with extend_chart_ids and/or proceed):
- set_general_access raising a dashboard role level (single + bulk;
combined narrow+widen returns both prompts in one response)
- grant-add on a dashboard (single POST grants — response is now
GrantCreateResponse{requires_confirmation, under_covering_charts,
grant} — and bulk add_grant with the aggregated prompt; unknown-email
invites coverage-checked BEFORE any Invitation is sent)
- public-link enable (PUT /share/ + bulk toggle_public; proceed-only)
- extend = raise chart analyst_level none->view + copy the dashboard's
Analyst/Admin/group principals onto the chart at View (Member
principals skipped, existing Edit grants never downgraded,
member_level untouched); requires actor Edit on each chart.
update_dashboard tile validation (blind-JSON overwrite closed):
- newly-added chart ids must be org-owned (400) + caller resolver-View
(403); under-covering embeds 409 with verdicts unless confirmed
(extend_chart_ids/proceed on DashboardUpdate); confirmed embeds are
never blocked.
Warehouse gates (promoted M1-review Important):
- /chart-data/, /map-data/, /download-csv/ now carry the raw-payload
gate ladder (_gate_raw_chart_payload); the three preview siblings
migrated onto the same helper — one ladder definition across all six.
chart_id context: org fetch + schema/table match + resolver view +
column pinning under a dashboard context; no chart_id: Analyst+ only.
Tile-walk consolidation (M0 follow-up):
- chart_access.chart_ids_in_tabs/dashboard_chart_ids — the ONE walk,
fail-closed on malformed tabs; all duplicate sites migrated
(public_api, chart_service, dashboard_service, render gate).
M1 minors: _insert_grant docstring cross-refs _member_excluded;
Chart.clean() comment softened (best-effort backstop).
Tests: +79 (coverage/malformed-tabs 25, broadening 28, update_dashboard
tiles 12, warehouse gates 14); full backend suite 2779 green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…coverage/resolver parity guard - duplicate_dashboard no longer seeds the copy at the org's default General access (a silent broadening path around M2's warnings when the source was narrowed): the copy INHERITS the source's analyst_level/member_level -- same-or-narrower audience by construction, so no coverage warning is needed. Grants and public-link state are deliberately NOT copied (both documented in the docstring + inline comment). Unused get_org_role_level_defaults import dropped. - Parity guard (pulled forward from the review's deferred list): TestCoverageResolverParity asserts coverage.chart_covers_orguser agrees with effective_permission(...,'chart',...) across principal classes (admin/owner/analyst-with-level/analyst-without/direct-grant/group-member/ member-with-grant), plus one assertNumQueries case pinning coverage_for_charts' fixed-query design (5 queries for this fixture shape). - Doc closes: _insert_grant docstring now states approve deliberately bypasses the M2 broadening warning (owner-decided); cross-reference comments added between sharing_actions._validate_extend_subset and the inline copy in dashboard_native_api._validate_new_tile_charts, spelling out the silently-ignore-when-clean asymmetry between the two call sites. Tests: duplicate suite rewritten for the inherit behavior (narrowed source keeps levels; org-default source unchanged; grants/public not copied). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 119 files, which is 19 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (119)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
siddhant3030
changed the title
Comments/docstrings only — no code, error-message, or test-logic changes
(every touched file AST-verified identical to HEAD modulo docstrings).
- Shorten verbose module/function docstrings to plain 1-4 line summaries
- Drop planning-process references (Task/Milestone/Phase/spec-section)
- Keep load-bearing warnings compressed: the two deliberately-separate
member-block mechanisms ("do not unify without a product decision"),
the extend-subset validation asymmetry, security no-oracle notes, and
ordering constraints around invites/saves/migrations
- Trim test section headers and module-docstring prefixes; test logic untouched
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 134 files, which is 34 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (134)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
The create-request path rejected any requester with existing access, but the embed-time 'request Edit access' flow fires precisely for users who hold View and need Edit — every such request 400'd. Reject only when current access already covers the ask. Found in the live Playwright pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tches on it Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- resource_share gains a Permission FK (column permission_id, PROTECT), backfilled from the rtype+level map; varchar level kept and synced on save until every rtype reads the FK - new decorators: @extract_resource (org-scoped fetch, 404 wall) and @has_resource_permission (pool = grants + floors + owner/admin, closed under edit-implies-view); @has_permission untouched - dashboard instance routes move to the three-decorator stack; edit routes carry the view slug in layer 2 and the action slug in the resource gate, so a Member with an edit grant can edit that dashboard (plan v1.2 §5); delete stays owner-gated - member_edit_grants registry flag: resolver's Member edit-grant cap now skips dashboards, holds for all other rtypes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
user_permission (view|edit) is read off request.resource_permissions that the resource gate already attached — no extra query. The webapp's edit affordances key on it so per-resource grants show up in the UI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Prod creates resource_share empty at this same deploy, and every ORM write stamps the FK; the pool builder reads the varchar for any older null-FK row. Owner call: keep the migration additive-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
extract_resource + has_resource_permission join has_permission so every gate lives in one file (sharing imports deferred to decoration time — the sharing package imports role constants from auth). The pool builder moves next to its sibling effective_permission in access_resolver as get_resource_permissions; decorators.py is gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed models The origin/main merge (f19630d) resolved ownership.py to main's version, dropping is_admin_or_super_admin/is_owner that four modules import. The cleanup commit also removed the imports that transitively registered Notification, OrgPlans, OrgTnC, PrefectFlowRun, UserPreferences and CanvasLock — makemigrations would have generated deletions for all six. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces the deleted 0168-0180 chain: creates resource_share, orguser_group and orguser_group_member, adds invitation.expires_at, notification.metadata and the org-preferences sharing defaults. Also drops ReportSnapshot.owner - ownership is created_by everywhere now; reports no longer carry a separate transferable owner. Note: unlike the old chain, this migration does not seed the can_share_*/can_*_user_groups permission rows - fresh installs get them from 002_permissions.json; existing deployments need a seed step. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Last of the four emails still building the full HTML shell inline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This reverts commit 9543753.
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.
What this is
The complete Resource Sharing backend: per-resource access control for dashboards, reports, alerts — and (v1.1) charts, KPIs, and metrics.
Core model
analyst_level/member_level∈ none·view·edit (replaces audience×level; migration 0177 maps legacy values, 0178 sets view/view org defaults)shareable_types.py), no per-rtype branchingv1.1 additions
analyst=edit/member=none— day-one behavior preserved); Member sharing deferred via a registry flag (resolver-enforced, adversarially reviewed)requires_confirmationnaming the charts; extend requires Edit on each chart;update_dashboardvalidates new tiles (no blind layout writes); duplicate-dashboard inherits its source's audienceThis backend must ship together with the paired webapp PR (DalgoT4D/webapp_v2#347). Three deliberate contract changes (grant-create response shape,
requires_confirmationon public toggle, dashboard tile validation) break older webapp builds; the webapp PR (including its M3b warning-modal adaptations) is complete and ready.Migrations
0176–0179 (notification metadata, per-role levels + backfill, view/view defaults, chart levels). Note for the shared dev DB: it carries hybrid lineage from earlier attempt branches —
dashboard.owner→owner_idcolumn rename needed there (staging/prod migrate clean).Testing
Full suite green at tip: 2789 passed / 31 skipped (warehouse-credential integration files excluded as usual). Every milestone went through an independent adversarial review (spec + quality verdicts); security fixes verified by reproduction. Comments/docstrings swept to repo convention (plain 1–2 line constraint notes; load-bearing warnings kept). See
dalgo-core/features/access-control/resourcesharing/for specs, plans, per-milestone reports, and the DALGO-1532 test-plan audit (0 failures).🤖 Generated with Claude Code