feat(user_shares): add share routes + consent wiring (POST/GET/DELETE /api/shares)#1908
feat(user_shares): add share routes + consent wiring (POST/GET/DELETE /api/shares)#1908hognek wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughAdds a SQLite-backed user resource-sharing store, ChangesUser Resource Sharing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant user_shares_router
participant UserSharesStore
participant notifications
Client->>user_shares_router: POST /api/shares
user_shares_router->>UserSharesStore: add_share
UserSharesStore-->>user_shares_router: share record
user_shares_router->>notifications: create target notification
user_shares_router-->>Client: share record
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| now = datetime.now(timezone.utc).isoformat() | ||
| cursor = await self._db.execute( | ||
| "SELECT * FROM user_shares " | ||
| "WHERE expires_at IS NULL OR expires_at > ? " |
There was a problem hiding this comment.
WARNING: Expiry check relies on lexical string comparison of ISO timestamps, which is not reliable.
expires_at is stored as a caller-supplied Optional[str] in add_share with no normalization, while now here is datetime.now(timezone.utc).isoformat() (+00:00). Lexical expires_at > ? only yields correct ordering when both strings share an identical, comparable format. If a caller passes a timestamp with a different timezone offset (e.g. +02:00), a trailing Z, or different sub-second precision, the string comparison can classify an expired share as active (or vice versa), silently leaking access or hiding valid shares. Validate/normalize expires_at to UTC on write, or compare parsed datetime values instead of raw strings.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "SELECT 1 FROM user_shares " | ||
| "WHERE resource_type = ? AND resource_id = ? " | ||
| "AND shared_with_user_id = ? " | ||
| "AND (expires_at IS NULL OR expires_at > ?) " |
There was a problem hiding this comment.
WARNING: Same lexical-string expiry comparison risk as list_active_shares.
This is the access-gate query (user_can_access) — the correctness-sensitive path. Comparing expires_at > ? as raw ISO strings can misjudge expiry when expires_at was stored in a different format/offset than the +00:00 now produced here, potentially granting access to an expired share. Normalize expires_at to a canonical UTC form on write or parse both sides to datetime before comparing.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| """ | ||
| store = _get_user_shares_store(request) | ||
| # Active shares first (covers the common case for both owner and admin). | ||
| for s in await store.list_active_shares(): |
There was a problem hiding this comment.
SUGGESTION: _find_share_by_id scans every active share in the system on each DELETE.
store.list_active_shares() returns all active shares across all users, and this loops over them in Python to locate a single id. On an instance with many shares this is an unnecessary O(N) full scan per revoke request, and it materializes shares the caller does not own into memory before the require_owner_or_admin gate runs. Consider adding a store method to fetch a single share by id (e.g. get_share(share_id)) and gate on its owner_user_id directly.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file changed since last review)
Prior findings re-verified and resolved in this increment
Previous Review Summaries (7 snapshots, latest commit 0b22651)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 0b22651)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files, incremental since a2787cc)
Prior findings re-verified and resolved in this increment
Previous review (commit a2787cc)Status: No Issues Found | Recommendation: Merge Files Reviewed (incremental — 2 files changed since last review)
Prior findings — re-verified and resolved
Previous review (commit 4ea8d80)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Previously reported findings — resolved in this runThe following prior findings were re-verified against HEAD
Files Reviewed (5 files changed in this PR)
Fix these issues in Kilo Cloud Previous review (commit ad6673d)Status: No Issues Found | Recommendation: Merge Incremental Review (since 4e52ffe)The only incremental change is in Files Reviewed (1 file changed since last review)
Previous review (commit 4e52ffe)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files changed since last review)
Fix these issues in Kilo Cloud Previous review (commit b5d8ebc)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous review (commit 0f45ec6)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Notes / Assumptions
Reviewed by hy3:free · Input: 42.6K · Output: 1.9K · Cached: 125.6K |
|
Store/wiring is sound and imports cleanly, but there's a hard security blocker plus a spec gap. Also: rebase onto current BLOCKER 1 — CSRF gap. BLOCKER 2 — notification is a broadcast. Spec gap — no accept-gate. Tests missing. Zero route tests; #1893 acceptance requires create/accept/revoke/idempotent/unknown-user. Nits: |
BLOCKER 1: Add dependencies=_csrf to user_shares_router in routes/__init__.py BLOCKER 2: Pass user_id=target_user_id to notifs.add() BLOCKER 3: Add status column + accept_share()/deny_share() + accept/deny routes Spec gap: user_can_access() now gates on status='accepted' Added 17 tests (create, idempotent, unknown-user, self-share, accept, deny, revoke, list, access-gate). Tests use conftest client fixture + CSRF setup. Rebased on origin/dev (post-jaylfc#1897 merge). Closes jaylfc review on jaylfc#1908
| # Create the per-user index if it doesn't exist yet. Cannot live in | ||
| # the SCHEMA because the column is added via guarded ALTER after init. | ||
| await self._db.execute( | ||
| "CREATE INDEX IF NOT EXISTS idx_notif_user ON notifications(user_id)" |
There was a problem hiding this comment.
WARNING: Blocking CREATE INDEX runs at startup on a potentially populated table.
This index is built synchronously inside _post_init, which runs on app startup. On an instance with a large notifications table, CREATE INDEX takes a write lock on the table for the duration of the build and blocks all reads/writes to that table (SQLite has no CREATE INDEX CONCURRENTLY). The first boot after deploying this change can stall notification writes (and any query touching notifications) for the whole build window. Consider building the index lazily/asynchronously after startup, or documenting the one-time startup cost and bounding it (e.g. only create when the table is known-small, or build it outside the request-serving window).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| try: | ||
| dt = datetime.fromisoformat(expires_at) | ||
| except ValueError: | ||
| return expires_at |
There was a problem hiding this comment.
SUGGESTION: Fail-safe path keeps a malformed expires_at un-normalized
If datetime.fromisoformat raises ValueError, the original string is stored unchanged. Any string that fromisoformat rejects but that is still a plausible date (e.g. a non-ISO format, or a value with separator) will later be compared lexicographically against datetime.now(UTC).isoformat() in list_active_shares/user_can_access, where the mismatch in field width/format can misjudge active vs expired. Consider instead storing an empty/None value (treating unparseable as "never expires" or rejecting the input at the API boundary) so the expires_at IS NULL branch handles it consistently.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
BLOCKER 1: Add dependencies=_csrf to user_shares_router in routes/__init__.py BLOCKER 2: Pass user_id=target_user_id to notifs.add() BLOCKER 3: Add status column + accept_share()/deny_share() + accept/deny routes Spec gap: user_can_access() now gates on status='accepted' Added 17 tests (create, idempotent, unknown-user, self-share, accept, deny, revoke, list, access-gate). Tests use conftest client fixture + CSRF setup. Rebased on origin/dev (post-jaylfc#1897 merge). Closes jaylfc review on jaylfc#1908
9a0d415 to
4ea8d80
Compare
|
Rebased onto origin/dev (e895f71). All review points addressed: BLOCKER 1 (CSRF): BLOCKER 2 (notification broadcast): Spec gap (accept-gate):
Nits: Tests: 13/13 pass (create, idempotent, unknown-user, self-share, accept, deny, already-decided, revoke, not-found, list-out, list-in, access-gate). Rebase: Clean rebase onto current origin/dev — the duplicate UserSharesStore commit from the PR's old base (already in dev via #1897) was auto-dropped. Local merge test shows clean fast-forward. Ready for re-review. |
| """, | ||
| (owner_user_id, resource_type, resource_id, | ||
| shared_with_user_id, permission, tier, now, expires_at), | ||
| shared_with_user_id, permission, tier, now, expires_at, 'pending'), |
There was a problem hiding this comment.
SUGGESTION: Idempotent re-share silently resets status to 'pending'
The INSERT hardcodes status = 'pending' (line 147), and add_share is documented as idempotent (the delete+insert runs under the write lock). If the owner re-shares a resource the target has already accepted, the existing accepted share is deleted and a fresh pending one is inserted — silently revoking access until the target re-accepts, with no notification that consent was reset. This is surprising for a caller that invokes add_share merely to "ensure it exists". Consider preserving the prior status on conflict (e.g. INSERT ... ON CONFLICT ... DO NOTHING, or read the existing row's status before the delete) so an accepted share is not downgraded to pending.
|
@kilocode-bot This fixes the Kilo suggestion about idempotent re-share silently downgrading accepted→pending: Fix: now reads the prior row's status before the DELETE+INSERT (under the write lock), and preserves it in the INSERT. A new share defaults to ; re-sharing an already-accepted share keeps . Changed files:
Tests: All user_shares tests pass including the new test. |
|
To use Kilo from GitHub you first need to link your GitHub account to Kilo. Link your Kilo account to continue. After linking, mention me again in this issue or pull request. |
… /api/shares)
- New tinyagentos/routes/user_shares.py with three authenticated routes:
POST /api/shares — share a resource with another user by username
GET /api/shares — list shares (direction=out|in)
DELETE /api/shares/{id} — revoke a share (owner or admin)
- Consent wiring: on share-create, raises a notification and a Decision
record for the target user (mirrors agent_auth_requests.py pattern)
- Module-level user_can_access() helper for route consumers
- Registered UserSharesStore in app.py lifespan and router in
routes/__init__.py
Depends on PR jaylfc#1897 (UserSharesStore — feat/user-shares-store branch)
BLOCKER 1: Add dependencies=_csrf to user_shares_router in routes/__init__.py BLOCKER 2: Pass user_id=target_user_id to notifs.add() BLOCKER 3: Add status column + accept_share()/deny_share() + accept/deny routes Spec gap: user_can_access() now gates on status='accepted' Added 17 tests (create, idempotent, unknown-user, self-share, accept, deny, revoke, list, access-gate). Tests use conftest client fixture + CSRF setup. Rebased on origin/dev (post-jaylfc#1897 merge). Closes jaylfc review on jaylfc#1908
…(1) share lookup, index cost doc - _normalise_expiry: parse expires_at to UTC-aware datetime on write so lexical ISO comparisons in list_active_shares/user_can_access are safe against timezone offsets, Z suffix, naive timestamps, and sub-second precision (Kilo WARNING ×2 on lines 175 & 208) - Add get_share(share_id) method to UserSharesStore (indexed PK lookup) and refactor _find_share_by_id to use it instead of O(N) list scan (Kilo SUGGESTION on line 72) - Document one-time CREATE INDEX cost in NotificationStore._post_init noting IF NOT EXISTS makes it no-op on subsequent starts (Kilo WARNING on line 134)
…aw string Kilo SUGGESTION: if _normalise_expiry cannot parse expires_at, store None rather than the raw string to avoid miscomparison in lexical ISO checks. Unparseable expiry now means 'never expires' — safer than silently passing a non-ISO string through to list_active_shares/user_can_access.
add_share() now reads the prior row's status before the DELETE+INSERT so an already-accepted share is not silently downgraded to 'pending' when the owner re-shares merely to 'ensure the share exists'. - Fetch prior status before DELETE under the write lock - Use prior_status in the INSERT instead of hardcoded 'pending' - New test: test_re_share_preserves_accepted_status
a2787cc to
0b22651
Compare
Doc-gate rule "routes" requires updating docs/agent-coordination.md when a new route module is added or removed. PR jaylfc#1908 added tinyagentos/routes/user_shares.py without the corresponding doc update — gate was failing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tinyagentos/routes/user_shares.py (1)
64-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFallback branch in
_find_share_by_idlooks unreachable, and is semantically wrong for accept/deny if it ever fires.
store.get_share(share_id)is an unconditional, live query against the same tablelist_sharesqueries — there's no caching layer inUserSharesStorefor the docstring's "the caller still sees it in their list through caching" scenario to apply, so ifget_sharereturnsNonethe fallback loop will not find the row either. Even setting that aside, the fallback searcheslist_shares(user_id)— shares owned by the caller — which is only relevant forrevoke_share; foraccept_share/deny_sharethe relevant party is the target (shared_with_user_id), so if this branch were ever exercised there it would search the wrong list.Consider dropping the fallback and relying solely on
get_share, simplifying this helper.🤖 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 `@tinyagentos/routes/user_shares.py` around lines 64 - 84, Remove the unreachable fallback loop from `_find_share_by_id`, including the `user_id` lookup and `list_shares` call. Keep the helper limited to the direct `store.get_share(share_id)` lookup, returning the share when found and `None` otherwise.tinyagentos/user_shares_store.py (1)
245-263: 🚀 Performance & Scalability | 🔵 Trivial
user_can_access(andlist_shares_received) can't use the existing unique index.The composite
UNIQUE(owner_user_id, resource_type, resource_id, shared_with_user_id, permission)index only helps queries that filter on a leftmost prefix starting withowner_user_id.user_can_accessfilters onresource_type,resource_id,shared_with_user_id, andstatus— none of which are a usable prefix — so every access check does a full table scan. This is the hot-path query gating every shared-resource access; consider addingCREATE INDEX idx_user_shares_target ON user_shares(shared_with_user_id, resource_type, resource_id)(guarded in_post_init, same pattern already used for thestatuscolumn) to cover both this andlist_shares_received.🤖 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 `@tinyagentos/user_shares_store.py` around lines 245 - 263, Add the proposed target index during _post_init, following the existing guarded status-index creation pattern: create idx_user_shares_target on user_shares(shared_with_user_id, resource_type, resource_id). Ensure initialization creates it for existing databases so user_can_access and list_shares_received can use the indexed lookup.tinyagentos/app.py (1)
508-510: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
app.state.user_sharesis only bound insidelifespan(), unlike every sibling store.Every other store added around here (e.g.
agent_grants,agent_registry) gets both a lifespan-time assignment and an eagerapp.state.<name> = <store>binding further down increate_app()(~line 1666), so the attribute always exists even for harnesses that construct the app without running lifespan.user_sharesonly gets the lifespan-time binding here. In production this is harmless (the startup guard blocks requests until lifespan finishes), buttests/conftest.py'sappfixture builds the app without running lifespan, androutes/user_shares.py._get_user_shares_storeraises a bareRuntimeError(→ unhandled 500) if the attribute is missing — this PR's own tests work around it by manually settingapp.state.user_sharesin a bespoke fixture, but any other harness hitting/api/shareswithout that workaround will 500 instead of getting a clean error.Add the matching eager binding for consistency with the rest of
create_app().🤖 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 `@tinyagentos/app.py` around lines 508 - 510, Add an eager app.state.user_shares = user_shares_store binding in create_app(), alongside the existing eager bindings for sibling stores, while retaining the lifespan assignment near user_shares_store.init(). Ensure _get_user_shares_store can find the attribute when lifespan has not run.tests/test_user_shares.py (1)
333-360: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo test for
DELETE /api/shares/{id}rejecting a non-owner, non-admin caller.Coverage exists for the owner-happy-path and not-found, but not for the
require_owner_or_admingate itself — e.g.shares_client_target(a plain, non-admin target user) attempting to delete a share it doesn't own should get 403. Worth adding given the fixtures for this already exist.✅ Suggested test
async def test_revoke_share_unauthorized(self, shares_client, shares_client_target): """Non-owner, non-admin caller cannot revoke another user's share.""" r = await shares_client.post( "/api/shares", json={ "resource_type": "project", "resource_id": "proj-revoke-unauth", "to_username": "target", "permission": "read", }, ) assert r.status_code == 200 share_id = r.json()["id"] resp = await shares_client_target.delete(f"/api/shares/{share_id}") assert resp.status_code == 403🤖 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 `@tests/test_user_shares.py` around lines 333 - 360, Add a test alongside test_revoke_share_owner and test_revoke_share_not_found that creates a share as the owner, then uses shares_client_target to delete it and asserts a 403 response, covering rejection by the require_owner_or_admin authorization gate.
🤖 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 `@tinyagentos/routes/user_shares.py`:
- Around line 139-160: Update the exception handlers in both the notification
block and the Decision-creation block of the user-share consent wiring to log
failures at warning level instead of silently passing. Preserve the best-effort
behavior so share creation still succeeds, and include the caught exception and
relevant operation context in each warning.
In `@tinyagentos/user_shares_store.py`:
- Around line 106-177: Update add_share to preserve an existing share’s id
during re-share: retain the initial lookup, capture the existing row’s id, and
use UPDATE ... WHERE id = ? to replace tier, timestamps, and expiry while
leaving status unchanged. Only INSERT a new row when no matching share exists,
and preserve the current commit and returned-row behavior.
---
Nitpick comments:
In `@tests/test_user_shares.py`:
- Around line 333-360: Add a test alongside test_revoke_share_owner and
test_revoke_share_not_found that creates a share as the owner, then uses
shares_client_target to delete it and asserts a 403 response, covering rejection
by the require_owner_or_admin authorization gate.
In `@tinyagentos/app.py`:
- Around line 508-510: Add an eager app.state.user_shares = user_shares_store
binding in create_app(), alongside the existing eager bindings for sibling
stores, while retaining the lifespan assignment near user_shares_store.init().
Ensure _get_user_shares_store can find the attribute when lifespan has not run.
In `@tinyagentos/routes/user_shares.py`:
- Around line 64-84: Remove the unreachable fallback loop from
`_find_share_by_id`, including the `user_id` lookup and `list_shares` call. Keep
the helper limited to the direct `store.get_share(share_id)` lookup, returning
the share when found and `None` otherwise.
In `@tinyagentos/user_shares_store.py`:
- Around line 245-263: Add the proposed target index during _post_init,
following the existing guarded status-index creation pattern: create
idx_user_shares_target on user_shares(shared_with_user_id, resource_type,
resource_id). Ensure initialization creates it for existing databases so
user_can_access and list_shares_received can use the indexed lookup.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b07ec44-a5b8-4ed8-85e3-73f8fa65816d
📒 Files selected for processing (7)
docs/agent-coordination.mdtests/test_user_shares.pytinyagentos/app.pytinyagentos/notifications.pytinyagentos/routes/__init__.pytinyagentos/routes/user_shares.pytinyagentos/user_shares_store.py
| notifs = getattr(request.app.state, "notifications", None) | ||
| if notifs is not None: | ||
| try: | ||
| await notifs.add( | ||
| title="Resource shared with you", | ||
| message=( | ||
| f"{user.user_id} shared {body.resource_type}/{body.resource_id} " | ||
| f"with you (permission: {body.permission})" | ||
| ), | ||
| level="info", | ||
| source="user_shares", | ||
| user_id=target_user_id, | ||
| data={ | ||
| "share_id": record["id"], | ||
| "owner_user_id": user.user_id, | ||
| "resource_type": body.resource_type, | ||
| "resource_id": body.resource_id, | ||
| "permission": body.permission, | ||
| }, | ||
| ) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Silently swallowed exceptions in the consent-wiring paths.
Both the notification and Decision-creation blocks catch with a bare except Exception: pass, flagged by static analysis (S110/BLE001). This is intentionally best-effort (should never fail share creation), but silently dropping the exception with no log line makes a broken notification/Decision pipeline invisible. Log at warning level (as NotificationStore._dispatch_push does) instead of a bare pass.
Also applies to: 166-187
🧰 Tools
🪛 Ruff (0.15.21)
[error] 159-160: try-except-pass detected, consider logging the exception
(S110)
[warning] 159-159: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@tinyagentos/routes/user_shares.py` around lines 139 - 160, Update the
exception handlers in both the notification block and the Decision-creation
block of the user-share consent wiring to log failures at warning level instead
of silently passing. Preserve the best-effort behavior so share creation still
succeeds, and include the caught exception and relevant operation context in
each warning.
Source: Linters/SAST tools
| async def add_share( | ||
| self, | ||
| owner_user_id: str, | ||
| resource_type: str, | ||
| resource_id: str, | ||
| shared_with_user_id: str, | ||
| permission: str, | ||
| *, | ||
| tier: str = "once", | ||
| expires_at: Optional[str] = None, | ||
| ) -> dict: | ||
| """Insert or replace a share for the exact 5-column key. | ||
|
|
||
| Idempotent re-share: calling add_share again with the same | ||
| (owner_user_id, resource_type, resource_id, shared_with_user_id, | ||
| permission) tuple replaces the existing share rather than creating | ||
| a duplicate. The prior row's ``status`` is preserved (e.g. an | ||
| already-accepted share stays accepted), so re-sharing merely to | ||
| "ensure the share exists" does not silently revoke consent. | ||
| The delete+insert+select runs under the write lock for atomicity. | ||
| """ | ||
| if self._db is None: | ||
| raise RuntimeError("UserSharesStore not initialised — call init() first") | ||
|
|
||
| expires_at = self._normalise_expiry(expires_at) | ||
| now = datetime.now(timezone.utc).isoformat() | ||
| async with self._write_lock: | ||
| # Fetch the prior status before deleting, so an already-accepted | ||
| # share is not silently downgraded to 'pending' when the owner | ||
| # calls add_share again to "ensure the share exists". | ||
| prior = await ( | ||
| await self._db.execute( | ||
| "SELECT status FROM user_shares " | ||
| "WHERE owner_user_id = ? AND resource_type = ? " | ||
| "AND resource_id = ? AND shared_with_user_id = ? " | ||
| "AND permission = ?", | ||
| (owner_user_id, resource_type, resource_id, | ||
| shared_with_user_id, permission), | ||
| ) | ||
| ).fetchone() | ||
| prior_status = prior["status"] if prior else "pending" | ||
|
|
||
| # Remove any existing row for the exact key first. | ||
| await self._db.execute( | ||
| "DELETE FROM user_shares " | ||
| "WHERE owner_user_id = ? AND resource_type = ? AND resource_id = ? " | ||
| "AND shared_with_user_id = ? AND permission = ?", | ||
| (owner_user_id, resource_type, resource_id, | ||
| shared_with_user_id, permission), | ||
| ) | ||
| await self._db.execute( | ||
| """ | ||
| INSERT INTO user_shares | ||
| (owner_user_id, resource_type, resource_id, | ||
| shared_with_user_id, permission, tier, granted_at, expires_at, status) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) | ||
| """, | ||
| (owner_user_id, resource_type, resource_id, | ||
| shared_with_user_id, permission, tier, now, expires_at, prior_status), | ||
| ) | ||
| await self._db.commit() | ||
| row = await ( | ||
| await self._db.execute( | ||
| "SELECT * FROM user_shares " | ||
| "WHERE owner_user_id = ? AND resource_type = ? " | ||
| "AND resource_id = ? AND shared_with_user_id = ? " | ||
| "AND permission = ?", | ||
| (owner_user_id, resource_type, resource_id, | ||
| shared_with_user_id, permission), | ||
| ) | ||
| ).fetchone() | ||
| return _row_to_dict(row) # type: ignore[return-value] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Re-share still mutates the share's id, even though status is now preserved.
add_share fixed the "resets to pending" bug by carrying over prior_status, but the underlying DELETE+INSERT still allocates a fresh AUTOINCREMENT id on every re-share (SQLite never reuses an AUTOINCREMENT id, even the one just deleted). This silently breaks any reference to the original id: the notification (data.share_id) and Decision (metadata.share_id) sent on first share, and any /api/shares/{id}/accept|deny link the target already has, all 404 after the owner "ensures the share exists" again. test_re_share_preserves_accepted_status (tests/test_user_shares.py:145-182) checks status is preserved but never asserts id is preserved, so this regression isn't caught.
Fix: look up the existing row's id first, then UPDATE ... WHERE id = ? in place (naturally preserving both id and status) instead of DELETE+INSERT; fall back to INSERT only when no row exists.
🛠️ Proposed fix — UPSERT by id instead of delete+insert
async with self._write_lock:
- # Fetch the prior status before deleting, so an already-accepted
- # share is not silently downgraded to 'pending' when the owner
- # calls add_share again to "ensure the share exists".
- prior = await (
- await self._db.execute(
- "SELECT status FROM user_shares "
- "WHERE owner_user_id = ? AND resource_type = ? "
- "AND resource_id = ? AND shared_with_user_id = ? "
- "AND permission = ?",
- (owner_user_id, resource_type, resource_id,
- shared_with_user_id, permission),
- )
- ).fetchone()
- prior_status = prior["status"] if prior else "pending"
-
- # Remove any existing row for the exact key first.
- await self._db.execute(
- "DELETE FROM user_shares "
- "WHERE owner_user_id = ? AND resource_type = ? AND resource_id = ? "
- "AND shared_with_user_id = ? AND permission = ?",
- (owner_user_id, resource_type, resource_id,
- shared_with_user_id, permission),
- )
- await self._db.execute(
- """
- INSERT INTO user_shares
- (owner_user_id, resource_type, resource_id,
- shared_with_user_id, permission, tier, granted_at, expires_at, status)
- VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)
- """,
- (owner_user_id, resource_type, resource_id,
- shared_with_user_id, permission, tier, now, expires_at, prior_status),
- )
+ existing = await (
+ await self._db.execute(
+ "SELECT id FROM user_shares "
+ "WHERE owner_user_id = ? AND resource_type = ? "
+ "AND resource_id = ? AND shared_with_user_id = ? "
+ "AND permission = ?",
+ (owner_user_id, resource_type, resource_id,
+ shared_with_user_id, permission),
+ )
+ ).fetchone()
+ if existing is not None:
+ # Preserves both id and status — a re-share never invalidates
+ # links already handed out for the original share.
+ await self._db.execute(
+ "UPDATE user_shares SET tier = ?, granted_at = ?, expires_at = ? "
+ "WHERE id = ?",
+ (tier, now, expires_at, existing["id"]),
+ )
+ share_id = existing["id"]
+ else:
+ cursor = await self._db.execute(
+ """
+ INSERT INTO user_shares
+ (owner_user_id, resource_type, resource_id,
+ shared_with_user_id, permission, tier, granted_at, expires_at, status)
+ VALUES (?, ?, ?, ?, ?, ?, ?, ?, 'pending')
+ """,
+ (owner_user_id, resource_type, resource_id,
+ shared_with_user_id, permission, tier, now, expires_at),
+ )
+ share_id = cursor.lastrowid
await self._db.commit()
row = await (
await self._db.execute(
- "SELECT * FROM user_shares "
- "WHERE owner_user_id = ? AND resource_type = ? "
- "AND resource_id = ? AND shared_with_user_id = ? "
- "AND permission = ?",
- (owner_user_id, resource_type, resource_id,
- shared_with_user_id, permission),
+ "SELECT * FROM user_shares WHERE id = ?", (share_id,)
)
).fetchone()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def add_share( | |
| self, | |
| owner_user_id: str, | |
| resource_type: str, | |
| resource_id: str, | |
| shared_with_user_id: str, | |
| permission: str, | |
| *, | |
| tier: str = "once", | |
| expires_at: Optional[str] = None, | |
| ) -> dict: | |
| """Insert or replace a share for the exact 5-column key. | |
| Idempotent re-share: calling add_share again with the same | |
| (owner_user_id, resource_type, resource_id, shared_with_user_id, | |
| permission) tuple replaces the existing share rather than creating | |
| a duplicate. The prior row's ``status`` is preserved (e.g. an | |
| already-accepted share stays accepted), so re-sharing merely to | |
| "ensure the share exists" does not silently revoke consent. | |
| The delete+insert+select runs under the write lock for atomicity. | |
| """ | |
| if self._db is None: | |
| raise RuntimeError("UserSharesStore not initialised — call init() first") | |
| expires_at = self._normalise_expiry(expires_at) | |
| now = datetime.now(timezone.utc).isoformat() | |
| async with self._write_lock: | |
| # Fetch the prior status before deleting, so an already-accepted | |
| # share is not silently downgraded to 'pending' when the owner | |
| # calls add_share again to "ensure the share exists". | |
| prior = await ( | |
| await self._db.execute( | |
| "SELECT status FROM user_shares " | |
| "WHERE owner_user_id = ? AND resource_type = ? " | |
| "AND resource_id = ? AND shared_with_user_id = ? " | |
| "AND permission = ?", | |
| (owner_user_id, resource_type, resource_id, | |
| shared_with_user_id, permission), | |
| ) | |
| ).fetchone() | |
| prior_status = prior["status"] if prior else "pending" | |
| # Remove any existing row for the exact key first. | |
| await self._db.execute( | |
| "DELETE FROM user_shares " | |
| "WHERE owner_user_id = ? AND resource_type = ? AND resource_id = ? " | |
| "AND shared_with_user_id = ? AND permission = ?", | |
| (owner_user_id, resource_type, resource_id, | |
| shared_with_user_id, permission), | |
| ) | |
| await self._db.execute( | |
| """ | |
| INSERT INTO user_shares | |
| (owner_user_id, resource_type, resource_id, | |
| shared_with_user_id, permission, tier, granted_at, expires_at, status) | |
| VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) | |
| """, | |
| (owner_user_id, resource_type, resource_id, | |
| shared_with_user_id, permission, tier, now, expires_at, prior_status), | |
| ) | |
| await self._db.commit() | |
| row = await ( | |
| await self._db.execute( | |
| "SELECT * FROM user_shares " | |
| "WHERE owner_user_id = ? AND resource_type = ? " | |
| "AND resource_id = ? AND shared_with_user_id = ? " | |
| "AND permission = ?", | |
| (owner_user_id, resource_type, resource_id, | |
| shared_with_user_id, permission), | |
| ) | |
| ).fetchone() | |
| return _row_to_dict(row) # type: ignore[return-value] | |
| async def add_share( | |
| self, | |
| owner_user_id: str, | |
| resource_type: str, | |
| resource_id: str, | |
| shared_with_user_id: str, | |
| permission: str, | |
| *, | |
| tier: str = "once", | |
| expires_at: Optional[str] = None, | |
| ) -> dict: | |
| """Insert or replace a share for the exact 5-column key. | |
| Idempotent re-share: calling add_share again with the same | |
| (owner_user_id, resource_type, resource_id, shared_with_user_id, | |
| permission) tuple replaces the existing share rather than creating | |
| a duplicate. The prior row's ``status`` is preserved (e.g. an | |
| already-accepted share stays accepted), so re-sharing merely to | |
| "ensure the share exists" does not silently revoke consent. | |
| The delete+insert+select runs under the write lock for atomicity. | |
| """ | |
| if self._db is None: | |
| raise RuntimeError("UserSharesStore not initialised — call init() first") | |
| expires_at = self._normalise_expiry(expires_at) | |
| now = datetime.now(timezone.utc).isoformat() | |
| async with self._write_lock: | |
| existing = await ( | |
| await self._db.execute( | |
| "SELECT id FROM user_shares " | |
| "WHERE owner_user_id = ? AND resource_type = ? " | |
| "AND resource_id = ? AND shared_with_user_id = ? " | |
| "AND permission = ?", | |
| (owner_user_id, resource_type, resource_id, | |
| shared_with_user_id, permission), | |
| ) | |
| ).fetchone() | |
| if existing is not None: | |
| # Preserves both id and status — a re-share never invalidates | |
| # links already handed out for the original share. | |
| await self._db.execute( | |
| "UPDATE user_shares SET tier = ?, granted_at = ?, expires_at = ? " | |
| "WHERE id = ?", | |
| (tier, now, expires_at, existing["id"]), | |
| ) | |
| share_id = existing["id"] | |
| else: | |
| cursor = await self._db.execute( | |
| """ | |
| INSERT INTO user_shares | |
| (owner_user_id, resource_type, resource_id, | |
| shared_with_user_id, permission, tier, granted_at, expires_at, status) | |
| VALUES (?, ?, ?, ?, ?, ?, ?, ?, 'pending') | |
| """, | |
| (owner_user_id, resource_type, resource_id, | |
| shared_with_user_id, permission, tier, now, expires_at), | |
| ) | |
| share_id = cursor.lastrowid | |
| await self._db.commit() | |
| row = await ( | |
| await self._db.execute( | |
| "SELECT * FROM user_shares WHERE id = ?", (share_id,) | |
| ) | |
| ).fetchone() | |
| return _row_to_dict(row) # type: ignore[return-value] |
🤖 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 `@tinyagentos/user_shares_store.py` around lines 106 - 177, Update add_share to
preserve an existing share’s id during re-share: retain the initial lookup,
capture the existing row’s id, and use UPDATE ... WHERE id = ? to replace tier,
timestamps, and expiry while leaving status unchanged. Only INSERT a new row
when no matching share exists, and preserve the current commit and returned-row
behavior.
2 participants
Summary
Implement
tinyagentos/routes/user_shares.pywith three authenticated routes for user-to-user resource sharing, plus consent wiring (notifications + Decision records).Routes:
POST /api/shares {resource_type, resource_id, to_username, permission}— share a resource with another user by username. Resolves via AuthManager.find_user(), creates share in UserSharesStore, raises notification + Decision record for consent.GET /api/shares?direction=out|in— list shares (out = what you have shared, in = what is shared with you). Defaults toout.DELETE /api/shares/{id}— revoke a share. Owner or admin only (uses require_owner_or_admin against the share owner_user_id).Consent wiring (mirrors agent_auth_requests.py lines 204-221):
Also:
user_can_access()helper for route consumersDepends on: PR #1897 (UserSharesStore)
Tests: 19/19 pass (test_register_all_routers, test_app_startup_guard, test_main_entry, test_version_endpoint)
Kilo bot fixes (commit 4e52ffe):
_normalise_expiryparses caller-suppliedexpires_atto UTC-aware datetime on write, so lexical ISO comparisons inlist_active_shares/user_can_accessare safe against timezone offsets, Z suffix, naive timestamps, and sub-second precision.get_share(share_id)to UserSharesStore (indexed PK lookup); refactored_find_share_by_idto use it instead of O(N) full scan.CREATE INDEXcost in NotificationStore._post_init (IF NOT EXISTS makes it no-op on subsequent starts).Summary by CodeRabbit
New Features
Documentation
Tests