feat(user_shares): add UserSharesStore + routes + consent wiring (#1893)#1958
feat(user_shares): add UserSharesStore + routes + consent wiring (#1893)#1958hognek wants to merge 2 commits into
Conversation
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? |
📝 WalkthroughWalkthroughAdds SQLite persistence and FastAPI endpoints for user-to-user resource sharing, including pending consent, acceptance/denial, revocation, access checks, notifications, and listings. App startup wiring and asynchronous authenticated route tests are included. ChangesUser sharing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Owner
participant UserSharesRoutes
participant UserSharesStore
participant TargetUser
Owner->>UserSharesRoutes: Create share
UserSharesRoutes->>UserSharesStore: Persist pending share
UserSharesRoutes->>TargetUser: Notify and create decision
TargetUser->>UserSharesRoutes: Accept or deny
UserSharesRoutes->>UserSharesStore: Update pending status
UserSharesStore-->>TargetUser: Return updated share
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) | ||
| """, | ||
| (owner_user_id, resource_type, resource_id, | ||
| shared_with_user_id, permission, tier, now, expires_at, 'pending'), |
There was a problem hiding this comment.
WARNING: Re-sharing resets status to 'pending', silently revoking an already-accepted share.
add_share DELETEs and re-INSERTs with a hard-coded 'pending' status. If a caller re-shares the same (owner, resource_type, resource_id, target, permission) tuple after the target already accepted, the existing accepted row is replaced and user_can_access immediately returns False — i.e. an already-granted access is silently withdrawn, and the target must consent again. For a truly idempotent re-share you should preserve the prior status (and granted_at) rather than forcing pending.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return await store.user_can_access(resource_type, resource_id, user_id) | ||
|
|
||
|
|
||
| async def _find_share_by_id(request: Request, share_id: int) -> dict | None: |
There was a problem hiding this comment.
WARNING: _find_share_by_id can't locate an admin-revoked/expired share owned by another user.
The lookup scans list_active_shares() (expiry-only filter) and then falls back to list_shares(request.state.user_id) — i.e. only the caller's own shares. An admin revoking or looking up a share that is expired (therefore absent from the active list) but owned by a different user falls through to the owner-only fallback and gets 404 share not found, even though require_owner_or_admin would otherwise permit it. Admin revoke should resolve by share id directly (plus an owner check) rather than relying on the caller's owned-share list.
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 loads and linearly scans all active shares on every accept/deny/revoke call.
list_active_shares() returns every active share across all users, then iterates to match share_id. A direct SELECT ... WHERE id = ? (with an owner/target gate applied afterward) is O(1), avoids pulling the entire table into memory, and also removes the need for the owner-only fallback that causes the admin/expired edge case above.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| resource_id TEXT NOT NULL, | ||
| shared_with_user_id TEXT NOT NULL, | ||
| permission TEXT NOT NULL, | ||
| tier TEXT NOT NULL DEFAULT 'once', |
There was a problem hiding this comment.
SUGGESTION: The tier column is written (default 'once') but never read or enforced anywhere.
It is part of the SCHEMA and accepted as a keyword-only param in add_share, but no query filters on tier and user_can_access/list_active_shares ignore it. Either enforce tiers (e.g. 'once' = single-use consumption) or drop the unused column to avoid dead/misleading schema that future readers will assume is functional.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Request bodies | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| class CreateShareRequest(BaseModel): |
There was a problem hiding this comment.
SUGGESTION: resource_type, resource_id, and permission are accepted as free-form strings with no validation or allow-list.
user_can_access later gates purely on a row's existence, so any arbitrary permission/resource_type string is persisted and treated as a valid grant. Consider validating permission against a known set (e.g. read/write/admin) and resource_type against the set of shareable resources, and return 422 for unknown values, to prevent typos/abuse from creating meaningless or overly-broad grants.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Resolved Since Previous ReviewThe fix commit
Files Reviewed (2 files in incremental diff)
Fix these issues in Kilo Cloud Previous Review Summary (commit 6db8a52)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 6db8a52)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Reviewed by hy3:free · Input: 74.1K · Output: 7.4K · Cached: 169.9K |
|
Addressed all 5 Kilo bot findings (2 WARNING + 3 SUGGESTION) from the initial review: W1 (re-share revocation): W2+S3 (admin lookup + full-table scan): Added S4 (dead tier column): Removed S5 (validation): Added All 13 tests in |
…lfc#1893) Build the user-to-user resource sharing primitive, mirroring the agent grant/consent pattern: - UserSharesStore (BaseStore): NULL-safe write lock, idempotent add_share, list/list_received/list_active, revoke, user_can_access (gated on status='accepted'), accept_share/deny_share consent methods - status column with guarded ALTER migration in _post_init - Routes (POST/GET/DELETE /api/shares + accept/deny): username resolution via auth.find_user, self-share guard, require_owner_or_admin on revoke, consent wiring (notification + Decision record on create) - user_id column added to NotificationStore with guarded ALTER migration and idx_notif_user index - 13 route tests covering create/idempotent/unknown-user/self-share/ accept/deny/revoke/list/access-gate - Registered in app.py lifespan + routes/__init__.py with CSRF protection Part of taOSnet epic jaylfc#1890. Closes jaylfc#1893.
| decision_store = getattr(request.app.state, "decision_store", None) | ||
| if decision_store is not None: | ||
| try: | ||
| await decision_store.create( |
There was a problem hiding this comment.
SUGGESTION: create_share raises a Decision record for the target user, but accept_share/deny_share/revoke_share update the share directly and never resolve or close that Decision. The target's Decisions inbox will keep showing a stale pending approve/deny decision that can no longer affect the share (accept/deny happen via the API, not the Decision action).
Consider storing the returned decision_id on the share and resolving/updating the Decision (or linking the API accept/deny to it) so the two consent mechanisms stay consistent, or drop the Decision creation if it's purely redundant with the API flow.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| 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.
SUGGESTION: expires_at > ? compares expires_at and now as ISO-8601 strings lexicographically. This only yields correct results when both values are in identical, timezone-normalized format (e.g. both ...Z or both +00:00 with equal precision). expires_at is currently always NULL, so this is latent today, but if any caller ever sets expires_at to a naive string, a local-time string, or a different precision, the comparison can silently mis-order and expose/expired shares incorrectly.
Consider storing expires_at as a normalized UTC epoch/ISO value (or comparing via datetime parsing) rather than relying on raw string ordering.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "SELECT * FROM user_shares WHERE id = ?", (share_id,) | ||
| ) | ||
| row = await cursor.fetchone() | ||
| return _row_to_dict(row) if row else None |
There was a problem hiding this comment.
SUGGESTION: accept_share/deny_share run UPDATE ... WHERE status = 'pending' and then unconditionally SELECT and return the row. If the UPDATE matched 0 rows (e.g. the share was already accepted/denied or doesn't exist as pending), the method still returns the existing row and looks successful to callers, with no way to distinguish "updated" from "nothing changed". Only the route-level status != 'pending' guard masks this for the HTTP path.
As public store API, consider checking cursor.rowcount (or returning None when 0 rows were updated) so direct callers aren't misled. Note: deny_share has the same shape at line 292.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…min lookup, dead schema, validation - W1: Preserve accepted status and granted_at on idempotent re-share so an already-granted access is not silently revoked (Kilo #3606338954). - W2+S3: Add get_share_by_id() store method; rewrite _find_share_by_id to use O(1) indexed lookup instead of full-table scan, fixing admin revoke of expired shares owned by other users (Kilo #3606338958, #3606338961). - S4: Remove dead tier column from INSERT; document as reserved in SCHEMA comment and add_share docstring (Kilo #3606338966). - S5: Validate resource_type and permission against allow-lists (VALID_RESOURCE_TYPES, VALID_PERMISSIONS) before creating shares (Kilo #3606338970). All 13 tests in test_user_shares.py pass. Docs-Reviewed: user_shares routes are user-to-user resource sharing endpoints (not agent-facing API); the agent-coordination.md doc covers the external-agent JWT-scoped surface only
|
|
||
| # Preserve accepted status + original granted_at on re-share; | ||
| # new shares start as 'pending' with the current timestamp. | ||
| if existing and existing["status"] == "accepted": |
There was a problem hiding this comment.
SUGGESTION: Re-share mints a new id, orphaning prior Decision/notification references
The DELETE+INSERT in add_share assigns a fresh autoincrement id on every idempotent re-share. create_share then unconditionally creates a new Decision (and notification) referencing record["id"] (user_shares.py:172-193). The previous Decision's share_id now points at a deleted row, and with W1's preservation the re-share stays 'accepted' — so each re-share leaks an orphaned Decision whose target share no longer exists.
Consider updating the existing row in place (or reusing its id) instead of delete+insert, and/or updating the prior Decision rather than always creating a new one.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_user_shares.py (1)
99-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new
422allow-list validation.The PR adds allow-list checks returning
422for unsupportedresource_type/permission, but no test exercises that path. Consider adding cases posting an invalidresource_typeand an invalidpermissionasserting422.🤖 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 99 - 171, Extend the create-share tests around test_create_share_returns_record to cover allow-list validation: add POST cases with an unsupported resource_type and an unsupported permission, and assert each response returns HTTP 422.tinyagentos/routes/user_shares.py (1)
165-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the swallowed exceptions instead of bare
pass.Both consent side effects silently discard all errors. A wrong
decision_store.create/notifs.addsignature or a real store failure vanishes with no trace, and the share still succeeds — hard to diagnose in production.♻️ Suggested change
- except Exception: - pass + except Exception: + logger.warning("user_shares: notification for share %s failed", record["id"], exc_info=True)(apply the analogous change for the Decision block at Lines 192-193)
Also applies to: 192-193
🤖 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 165 - 166, Replace the bare exception swallowing in both consent side-effect handlers with exception logging, covering the blocks around decision_store.create and notifs.add. Preserve the existing share-success behavior while recording the caught exception with sufficient context through the module’s established logger.Source: Linters/SAST tools
🤖 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 98-138: The create_share flow must verify that the authenticated
caller owns or can manage the requested resource before calling store.add_share.
Add an ownership/management check using the existing resource_type and
resource_id values, reject unauthorized resources, and only write the share
after validation succeeds; preserve the existing target-user, self-share, and
permission validations.
---
Nitpick comments:
In `@tests/test_user_shares.py`:
- Around line 99-171: Extend the create-share tests around
test_create_share_returns_record to cover allow-list validation: add POST cases
with an unsupported resource_type and an unsupported permission, and assert each
response returns HTTP 422.
In `@tinyagentos/routes/user_shares.py`:
- Around line 165-166: Replace the bare exception swallowing in both consent
side-effect handlers with exception logging, covering the blocks around
decision_store.create and notifs.add. Preserve the existing share-success
behavior while recording the caught exception with sufficient context through
the module’s established logger.
🪄 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: 542a1d15-9c09-460e-8ca6-6b9564bc3471
📒 Files selected for processing (5)
tests/test_user_shares.pytinyagentos/app.pytinyagentos/routes/__init__.pytinyagentos/routes/user_shares.pytinyagentos/user_shares_store.py
| store = _get_user_shares_store(request) | ||
|
|
||
| # Resolve target user by username. | ||
| auth = getattr(request.app.state, "auth", None) | ||
| if auth is None: | ||
| raise HTTPException(status_code=500, detail="auth manager not available") | ||
|
|
||
| target = auth.find_user(body.to_username) | ||
| if target is None: | ||
| raise HTTPException(status_code=404, detail=f"user '{body.to_username}' not found") | ||
|
|
||
| target_user_id: str = target["id"] | ||
|
|
||
| # Guard against self-share — creates a confusing UX and an unnecessary | ||
| # Decision against yourself. | ||
| if target_user_id == user.user_id: | ||
| raise HTTPException(status_code=400, detail="cannot share with yourself") | ||
|
|
||
| # Validate resource_type and permission against known sets (Kilo S5). | ||
| if body.resource_type not in VALID_RESOURCE_TYPES: | ||
| raise HTTPException( | ||
| status_code=422, | ||
| detail=f"invalid resource_type '{body.resource_type}'; " | ||
| f"expected one of {VALID_RESOURCE_TYPES}", | ||
| ) | ||
| if body.permission not in VALID_PERMISSIONS: | ||
| raise HTTPException( | ||
| status_code=422, | ||
| detail=f"invalid permission '{body.permission}'; " | ||
| f"expected one of {VALID_PERMISSIONS}", | ||
| ) | ||
|
|
||
| # Create (or replace) the share. The store's write lock makes this | ||
| # idempotent for concurrent same-key writes. | ||
| record = await store.add_share( | ||
| owner_user_id=user.user_id, | ||
| resource_type=body.resource_type, | ||
| resource_id=body.resource_id, | ||
| shared_with_user_id=target_user_id, | ||
| permission=body.permission, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any resource-ownership enforcement around shares / user_can_access
rg -nP -C3 'user_can_access|owner_user_id' --type=py -g '!tests/**'Repository: jaylfc/taOS
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files tinyagentos/routes/user_shares.py tinyagentos | sed -n '1,120p'
echo '--- outline user_shares.py ---'
ast-grep outline tinyagentos/routes/user_shares.py --view expanded || true
echo '--- locate access helpers ---'
rg -n "user_can_access|find_user_share|add_share|resource_type|resource_id|owner_user_id|share" tinyagentos -g '!tests/**'Repository: jaylfc/taOS
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' tinyagentos/routes/user_shares.py
echo '---'
sed -n '1,260p' tinyagentos/routes/decision.py 2>/dev/null || true
echo '---'
sed -n '1,260p' tinyagentos/routes/users.py 2>/dev/null || trueRepository: jaylfc/taOS
Length of output: 8962
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '## tinyagentos/routes/user_shares.py'
nl -ba tinyagentos/routes/user_shares.py | sed -n '1,260p'
echo '## tinyagentos/routes/users.py'
if [ -f tinyagentos/routes/users.py ]; then nl -ba tinyagentos/routes/users.py | sed -n '1,260p'; fi
echo '## tinyagentos/routes/decision.py'
if [ -f tinyagentos/routes/decision.py ]; then nl -ba tinyagentos/routes/decision.py | sed -n '1,260p'; fiRepository: jaylfc/taOS
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 "def add_share|class .*Share|get_share_by_id|list_shares|accept_share|deny_share|revoke_share" tinyagentos -g '!tests/**'Repository: jaylfc/taOS
Length of output: 13198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- user_shares_store.py: access check + add_share ---'
sed -n '84,260p' tinyagentos/user_shares_store.py
echo '--- callers of user_can_access ---'
rg -n "user_can_access\(" tinyagentos -g '!tests/**'
echo '--- share-related resource access gates ---'
rg -n "require_owner_or_admin|shared_with_user_id|owner_user_id|share_id|shares" tinyagentos/routes tinyagentos/notes tinyagentos/projects tinyagentos/chat -g '!tests/**'Repository: jaylfc/taOS
Length of output: 16955
create_share needs an ownership gate
create_share accepts any resource_type/resource_id pair and writes a share row with owner_user_id=user.user_id without proving the caller owns or can manage that resource. Since access checks key off accepted share rows, this lets an authenticated user manufacture access for arbitrary resources.
🤖 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 98 - 138, The create_share
flow must verify that the authenticated caller owns or can manage the requested
resource before calling store.add_share. Add an ownership/management check using
the existing resource_type and resource_id values, reject unauthorized
resources, and only write the share after validation succeeds; preserve the
existing target-user, self-share, and permission validations.
1 participant
Summary
Builds the user-to-user resource sharing primitive for taOSnet (#1890), mirroring the agent grant/consent pattern.
What's included
UserSharesStore (
tinyagentos/user_shares_store.py): BaseStore with NULL-safe write lock, idempotent add_share, list/list_received/list_active, revoke, user_can_access (gated onstatus='accepted'), accept_share/deny_share consent methods.statuscolumn with guarded ALTER migration.Routes (
tinyagentos/routes/user_shares.py):POST /api/shares— share resource with user by username (idempotent, self-share guard, consent wiring)GET /api/shares— list out/in withdirectionquery paramDELETE /api/shares/{id}— revoke (owner or admin viarequire_owner_or_admin)POST /api/shares/{id}/accept— target-user consent acceptPOST /api/shares/{id}/deny— target-user consent denyNotificationStore (
tinyagentos/notifications.py): addeduser_idcolumn with guarded ALTER migration andidx_notif_userindex.add()now accepts optionaluser_idparameter for scoped delivery.Tests (
tests/test_user_shares.py): 13 tests covering create, idempotent, unknown-user, self-share, accept (target-only/wrong-user/already-decided), deny (target-only/already-decided), revoke (owner/not-found), list out/in, and user_can_access gate.Registration: Store init in
app.pylifespan, router inroutes/__init__.pywith CSRF protection.Related
Testing
Task: t_0104cf54
Summary by CodeRabbit