feat: add shareWithUser endpoint #1246
Conversation
WalkthroughAdds ledger-sharing-by-email: new request/response message types and a type guard; FP API method to share a ledger with a user (token verification, auth checks, DB updates); request handler routing for the new message; minor import reorganizations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Handler as Request Handler
participant API as FPApiSQL
participant DB as Database
participant JWT as Token Verifier
Client->>Handler: POST ReqShareWithUser
activate Handler
Handler->>Handler: isReqShareWithUser(jso)
Handler->>API: shareWithUser(req)
deactivate Handler
activate API
API->>JWT: verifyFPToken(auth token)
alt token invalid
JWT-->>API: Error
API-->>Client: Error (Invalid token)
else token valid
API->>DB: Lookup ledger & verify admin
alt ledger not found
API-->>Client: Error (Not found)
else not admin
API-->>Client: Error (Not authorized)
else admin
API->>DB: Lookup user by email
API->>DB: Add user to ledger (role/right)
API-->>Client: ResShareWithUser (success)
end
end
deactivate API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-09T19:56:31.545ZApplied to files:
🧬 Code graph analysis (2)dashboard/backend/api.ts (5)
dashboard/backend/create-handler.ts (1)
🔇 Additional comments (3)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![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 (1)
dashboard/backend/api.ts (1)
2008-2027: Manual expiry check is redundant.The manual expiry check at line 2019 is redundant because
jwtVerifyalready validates token expiry by default. While this doesn't cause issues, removing it would simplify the code.Consider applying this diff to remove the redundant check:
const payload = verifyResult.payload as FPCloudClaim; - // Check if token is expired - const now = Date.now(); - if (!payload.exp || payload.exp * 1000 <= now) { - return Result.Err("Token is expired"); - } - return Result.Ok(payload);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/protocols/dashboard/msg-is.ts(3 hunks)core/protocols/dashboard/msg-types.ts(1 hunks)dashboard/backend/api.ts(8 hunks)dashboard/backend/create-handler.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
core/protocols/dashboard/msg-types.ts (1)
core/types/protocols/cloud/msg-types.zod.ts (2)
Role(38-38)ReadWrite(39-39)
core/protocols/dashboard/msg-is.ts (1)
core/protocols/dashboard/msg-types.ts (1)
ReqShareWithUser(408-414)
dashboard/backend/api.ts (5)
core/protocols/dashboard/msg-types.ts (4)
ReqShareWithUser(408-414)ResShareWithUser(416-425)ReqExtendToken(519-522)ResExtendToken(524-527)dashboard/backend/create-fp-token.ts (3)
FPTokenContext(7-14)getFPTokenContext(31-53)createFPToken(16-29)dashboard/backend/ledgers.ts (1)
sqlLedgers(7-25)dashboard/backend/users.ts (1)
queryUser(56-62)core/types/protocols/cloud/msg-types.zod.ts (1)
FPCloudClaim(43-43)
🔇 Additional comments (6)
dashboard/backend/create-handler.ts (1)
275-277: LGTM!The new route handler for
shareWithUserfollows the established pattern and correctly routes the request tofpApi.shareWithUser(jso).core/protocols/dashboard/msg-types.ts (1)
408-425: LGTM!The new request and response interfaces for sharing ledgers with users are well-structured and follow the established patterns in this file.
core/protocols/dashboard/msg-is.ts (1)
108-110: LGTM!The type guard implementation for
ReqShareWithUserfollows the established pattern and correctly useshasTypeto check for the "reqShareWithUser" type string.dashboard/backend/api.ts (3)
1716-1746: LGTM!The token verification and authorization flow is well-implemented:
- Properly retrieves and validates the JWT context
- Extracts user and ledger IDs from the verified token payload
- Enforces admin-level access before allowing ledger sharing
1748-1778: LGTM!The user lookup and validation logic is robust:
- Properly validates ledger existence and active status
- Handles edge cases: user not found, multiple users, self-sharing
- Provides clear error messages guiding users to sign up first
2032-2057: LGTM!The token extension implementation is well-structured:
- Properly verifies the existing token before extending
- Creates a new token with the extended validity period from configuration
- Maintains the original payload claims
necrodome
had a problem deploying
to
production
GitHub Actions
Failure
|
|
||
| // 6. Add user to tenant first | ||
| const rAddUserToTenant = await this.addUserToTenant(this.db, { | ||
| userName: req.email, |
There was a problem hiding this comment.
you want only share a ledger not a hole tenant --- that basically shares all ledger of the user to the other.
mabels
left a comment
mabels
left a comment
There was a problem hiding this comment.
Where you do an invite? --- it does not help
necrodome
force-pushed
the
selem/sharing
branch
from
803b88b to
36d7bda
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
dashboard/backend/api.ts (1)
1814-1814: Fix inconsistent default forrightfield.The response returns
right: req.right || "write"but line 1799 adds the user to the ledger withright: req.right || "read". This mismatch means the response claims "write" access when "read" access was actually granted.Apply this diff:
role: req.role || "member", - right: req.right || "write", + right: req.right || "read", });
🧹 Nitpick comments (1)
dashboard/backend/api.ts (1)
2017-2021: Redundant expiration check.The
jwtVerifyfunction from the jose library automatically validates token expiration. The manual check at lines 2019-2021 is redundant and can be removed.Apply this diff to remove the redundant check:
const payload = verifyResult.payload as FPCloudClaim; - // Check if token is expired - const now = Date.now(); - if (!payload.exp || payload.exp * 1000 <= now) { - return Result.Err("Token is expired"); - } - return Result.Ok(payload);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/protocols/dashboard/msg-is.ts(3 hunks)core/protocols/dashboard/msg-types.ts(1 hunks)dashboard/backend/api.ts(8 hunks)dashboard/backend/create-handler.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/protocols/dashboard/msg-is.ts
- dashboard/backend/create-handler.ts
🧰 Additional context used
🧬 Code graph analysis (2)
core/protocols/dashboard/msg-types.ts (1)
core/types/protocols/cloud/msg-types.zod.ts (2)
Role(38-38)ReadWrite(39-39)
dashboard/backend/api.ts (5)
core/protocols/dashboard/msg-types.ts (4)
ReqShareWithUser(408-414)ResShareWithUser(416-425)ReqExtendToken(519-522)ResExtendToken(524-527)dashboard/backend/create-fp-token.ts (3)
FPTokenContext(7-14)getFPTokenContext(31-53)createFPToken(16-29)dashboard/backend/ledgers.ts (1)
sqlLedgers(7-25)dashboard/backend/users.ts (1)
queryUser(56-62)core/types/protocols/cloud/msg-types.zod.ts (1)
FPCloudClaim(43-43)
🔇 Additional comments (3)
core/protocols/dashboard/msg-types.ts (1)
408-425: LGTM! Well-structured interfaces for the share-with-user feature.The request interface appropriately uses optional
roleandrightfields, while the response ensures these fields are always populated with the actual values used. The structure aligns well with other request/response pairs in this file.dashboard/backend/api.ts (2)
2032-2057: LGTM! Clean refactor of token extension logic.The refactored
extendTokenmethod correctly leverages the newverifyFPTokenhelper andcreateFPTokenutility. The flow is clear: verify the existing token, then generate a new one with extended validity usingctx.extendValidFor.
1780-1790: Tenant membership alone does not grant ledger access
isAdminOfLedger only checks sqlLedgerUsers entries (and falls back to tenant-admin for existing ledgerUsers members); tenant “member” roles have no ledger permissions.
mabels
left a comment
mabels
left a comment
There was a problem hiding this comment.
From where the double CORS is coming from?
--- this is not correct, this will break the local version.
fireproof/dashboard/backend/cf-serve.ts Line 38 in b957cc6 |
|
that had been there for ever, the question where is the second coming from |
|
pls use u github handle in the branch name @necrodome |
|
move the CORS fix in mabels/fix-double-cors or #1257 |
necrodome
force-pushed
the
selem/sharing
branch
from
a7a362d to
1a58862
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
dashboard/src/pages/cloud/api/token-auto.tsx (1)
54-59: Good addition for shared ledger support.The new
getListLedgersByUser()query correctly retrieves all ledgers including shared ones. The dual-source approach (tenantsData + allUserLedgers) ensures shared ledgers are accessible even when not in the user's tenant list.Minor: The comment at line 54 suggests tenantsData includes shared ledgers, but the need for a separate query implies it may not—consider clarifying that comment.
dashboard/backend/api.ts (1)
2007-2011: Remove redundant expiration check.The manual expiration check on lines 2007-2011 is unnecessary because
jwtVerifyalready validates theexpclaim as part of the standard JWT verification process. This check will never be reached for an expired token sincejwtVerifywould throw first.Apply this diff to remove the redundant check:
const payload = verifyResult.payload as FPCloudClaim; - - // Check if token is expired - const now = Date.now(); - if (!payload.exp || payload.exp * 1000 <= now) { - return Result.Err("Token is expired"); - } return Result.Ok(payload);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/protocols/dashboard/msg-is.ts(3 hunks)core/protocols/dashboard/msg-types.ts(1 hunks)dashboard/backend/api.ts(8 hunks)dashboard/backend/create-handler.ts(2 hunks)dashboard/src/pages/cloud/api/token-auto.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/protocols/dashboard/msg-types.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-09T19:56:31.545Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-protocol.ts:47-60
Timestamp: 2025-09-09T19:56:31.545Z
Learning: In the Result type from adviser/cement, Result.Err can accept Result objects directly without needing to unwrap them with .Err(). The codebase consistently uses Result.Err(resultObject) pattern, as seen in core/keybag/key-bag.ts lines 94 and 102.
Applied to files:
dashboard/backend/api.ts
🧬 Code graph analysis (4)
dashboard/backend/create-handler.ts (1)
dashboard/backend/api.ts (1)
FPAPIMsg(125-125)
dashboard/src/pages/cloud/api/token-auto.tsx (1)
dashboard/src/cloud-context.ts (1)
createLedgerMutation(290-312)
dashboard/backend/api.ts (5)
core/protocols/dashboard/msg-types.ts (2)
ReqShareWithUser(408-414)ResShareWithUser(416-425)dashboard/backend/create-fp-token.ts (3)
FPTokenContext(7-14)getFPTokenContext(31-53)createFPToken(16-29)dashboard/backend/ledgers.ts (1)
sqlLedgers(7-25)dashboard/backend/users.ts (1)
queryUser(56-62)core/types/protocols/cloud/msg-types.zod.ts (1)
FPCloudClaim(43-43)
core/protocols/dashboard/msg-is.ts (1)
core/protocols/dashboard/msg-types.ts (1)
ReqShareWithUser(408-414)
🪛 Biome (2.1.2)
dashboard/src/pages/cloud/api/token-auto.tsx
[error] 77-77: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (15)
dashboard/src/pages/cloud/api/token-auto.tsx (7)
2-2: LGTM!The added imports support the new shared ledger functionality.
Also applies to: 5-5
77-80: Correct guard refinement for dual data source.The guard change from checking both
tenantsData && ledgerInfoto justledgerInfois correct—it allows the effect to process when eithertenantsDataorallUserLedgersloads, while preventing reprocessing onceledgerInfois set.
92-125: Well-structured two-stage ledger resolution.The new lookup strategy correctly prioritizes
allUserLedgers(which includes shared ledgers) before falling back totenantsData. This ensures shared ledgers are discovered even when not in the user's own tenants.
129-141: Excellent safeguard for shared-only users.The added condition prevents ledger creation attempts for users who only have access to shared ledgers but don't own any tenants. This is a thoughtful addition that prevents unnecessary API calls and potential errors.
142-142: Correct dependency array.The addition of
allUserLedgersto the effect dependencies is necessary and properly maintains effect correctness.
206-206: Error handling properly extended.The inclusion of
allUserLedgersErrorensures all data-fetching errors are captured and displayed to the user.
77-77: Static analysis false positive: hooks are correctly positioned.The Biome error claiming conditional hook usage is incorrect. All hooks (including this
useEffect) are called at the component's top level in a consistent order. The early return at line 33 is a conditional render, occurring after all hooks—this is a standard and correct React pattern.core/protocols/dashboard/msg-is.ts (2)
1-22: LGTM! Import organization is clean.The addition of
ReqShareWithUserand the reordering of imports follows a consistent alphabetical pattern, making the import block more maintainable.
44-44: LGTM! Type guard implementation follows established patterns.The new type guard for
ReqShareWithUsercorrectly uses thehasTypehelper and matches the pattern of all other type guards in this file.Also applies to: 108-110
dashboard/backend/create-handler.ts (2)
7-7: LGTM! Import improvements enhance code organization.The type-only import for
Envon line 11 is a good TypeScript practice that can help with tree-shaking and makes the intent clearer.Also applies to: 9-11
280-282: LGTM! Handler integration follows established patterns.The new case for
shareWithUseris correctly integrated into the request handler switch statement and follows the exact pattern of all other request handlers.dashboard/backend/api.ts (4)
3-3: LGTM! Import additions support the new sharing functionality.All new imports are necessary for the shareWithUser feature and token verification logic. The organization is clean and logical.
Also applies to: 9-9, 29-29, 48-48, 58-62
115-115: LGTM! Interface method declaration is properly typed.The
shareWithUsermethod signature follows the established pattern and uses appropriate Result-wrapped types.
1781-1792: Graceful handling of already-shared ledgers.The
addUserToLedgerhelper (lines 456-541) returns early if the user is already attached to the ledger (line 470-497), which meansshareWithUserwill succeed idempotently when called multiple times for the same user. This is good behavior, though the success message "Successfully shared ledger" might be slightly misleading on repeated calls. Consider whether the response should indicate if this was a new share or an existing one.
2022-2047: LGTM! Clean refactoring using the new verifyFPToken helper.The refactored
extendTokenmethod is more maintainable by leveraging the sharedverifyFPTokenhelper. The logic correctly extends the token expiry usingctx.extendValidForand follows the established Result pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dashboard/backend/cf-serve.ts (1)
1-7: Remove duplicate imports.Lines 1-3 duplicate lines 5-7, causing redeclaration errors that will prevent the code from compiling or running. Additionally,
CORSis imported but never used, andDefaultHttpHeadersis missing from the new imports but is used on line 39.Apply this diff to fix the duplicate imports:
-import { drizzle } from "drizzle-orm/d1"; -import { D1Database, Fetcher, Request as CFRequest, Response as CFResponse } from "@cloudflare/workers-types"; -import { DefaultHttpHeaders, createHandler } from "./create-handler.js"; -import { URI } from "@adviser/cement"; import { Request as CFRequest, Response as CFResponse, D1Database, Fetcher } from "@cloudflare/workers-types"; import { drizzle } from "drizzle-orm/d1"; -import { CORS, createHandler } from "./create-handler.js"; +import { DefaultHttpHeaders, createHandler } from "./create-handler.js"; +import { URI } from "@adviser/cement"; import { resWellKnownJwks } from "./well-known-jwks.js";
♻️ Duplicate comments (1)
dashboard/backend/api.ts (1)
1814-1814: Fix inconsistent default forrightfield (regression).The default value for the
rightfield in the response (line 1814) is still inconsistent with the database operation (line 1799). This issue was previously marked as addressed, but the fix appears to have been reverted or not applied:
- Line 1799: adds user with
right: req.right || "read"- Line 1814: returns
right: req.right || "write"This means the response claims "write" access when only "read" access was actually granted.
Apply this diff to fix the inconsistency:
return Result.Ok({ type: "resShareWithUser", success: true, message: `Successfully shared ledger with ${req.email}`, ledgerId: ledgerId, userId: targetUser.userId, email: req.email, role: req.role || "member", - right: req.right || "write", + right: req.right || "read", });
🧹 Nitpick comments (1)
dashboard/backend/api.ts (1)
1780-1786: Consider aligning tenant role with ledger role.The user is always added to the tenant with role "member" (line 1785), regardless of the ledger role being assigned. This could create a permissions mismatch where a user is a ledger admin but only a tenant member.
Consider whether the tenant role should match or be influenced by the ledger role, especially when
req.roleis "admin".Based on learnings
Apply this diff if tenant role should match ledger role:
// 6. Add user to tenant first const rAddUserToTenant = await this.addUserToTenant(this.db, { userName: req.email, tenantId: ledger.tenantId, userId: targetUser.userId, - role: "member", + role: req.role || "member", });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dashboard/backend/api.ts(8 hunks)dashboard/backend/cf-serve.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-09T19:56:31.545Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-protocol.ts:47-60
Timestamp: 2025-09-09T19:56:31.545Z
Learning: In the Result type from adviser/cement, Result.Err can accept Result objects directly without needing to unwrap them with .Err(). The codebase consistently uses Result.Err(resultObject) pattern, as seen in core/keybag/key-bag.ts lines 94 and 102.
Applied to files:
dashboard/backend/api.ts
🧬 Code graph analysis (1)
dashboard/backend/api.ts (5)
core/protocols/dashboard/msg-types.ts (4)
ReqShareWithUser(408-414)ResShareWithUser(416-425)ReqExtendToken(519-522)ResExtendToken(524-527)dashboard/backend/create-fp-token.ts (3)
FPTokenContext(7-14)getFPTokenContext(31-53)createFPToken(16-29)dashboard/backend/ledgers.ts (1)
sqlLedgers(7-25)dashboard/backend/users.ts (1)
queryUser(56-62)core/types/protocols/cloud/msg-types.zod.ts (1)
FPCloudClaim(43-43)
🪛 Biome (2.1.2)
dashboard/backend/cf-serve.ts
[error] 5-5: Shouldn't redeclare 'CFRequest'. Consider to delete it or rename it.
'CFRequest' is defined here:
(lint/suspicious/noRedeclare)
[error] 5-5: Shouldn't redeclare 'CFResponse'. Consider to delete it or rename it.
'CFResponse' is defined here:
(lint/suspicious/noRedeclare)
[error] 5-5: Shouldn't redeclare 'D1Database'. Consider to delete it or rename it.
'D1Database' is defined here:
(lint/suspicious/noRedeclare)
[error] 5-5: Shouldn't redeclare 'Fetcher'. Consider to delete it or rename it.
'Fetcher' is defined here:
(lint/suspicious/noRedeclare)
[error] 6-6: Shouldn't redeclare 'drizzle'. Consider to delete it or rename it.
'drizzle' is defined here:
(lint/suspicious/noRedeclare)
[error] 7-7: Shouldn't redeclare 'createHandler'. Consider to delete it or rename it.
'createHandler' is defined here:
(lint/suspicious/noRedeclare)
🪛 ESLint
dashboard/backend/cf-serve.ts
[error] 5-5: '@cloudflare/workers-types' imported multiple times.
(import/no-duplicates)
[error] 6-6: 'drizzle-orm/d1' imported multiple times.
(import/no-duplicates)
[error] 7-7: 'CORS' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 7-7: './create-handler.js' imported multiple times.
(import/no-duplicates)
🔇 Additional comments (2)
dashboard/backend/api.ts (2)
2008-2027: LGTM!The
verifyFPTokenhelper correctly verifies JWT tokens with proper signature validation, issuer/audience checks, and expiration handling. The explicit expiration check (lines 2018-2021) provides defense-in-depth even thoughjwtVerifyperforms similar validation.
2029-2057: LGTM!The refactored
extendTokenmethod properly leverages the newverifyFPTokenhelper and correctly extends token validity usingctx.extendValidFor. The implementation is clean and maintains proper separation of concerns.
| async shareWithUser(req: ReqShareWithUser, ictx: Partial<FPTokenContext> = {}): Promise<Result<ResShareWithUser>> { | ||
| // 1. Get JWT context and verify token | ||
| const rCtx = await getFPTokenContext(this.sthis, ictx); | ||
| if (rCtx.isErr()) { | ||
| return Result.Err(rCtx.Err()); | ||
| } | ||
| const ctx = rCtx.Ok(); | ||
|
|
||
| const rPayload = await this.verifyFPToken(req.auth.token, ctx); | ||
| if (rPayload.isErr()) { | ||
| return Result.Err(rPayload.Err()); | ||
| } | ||
| const payload = rPayload.Ok(); | ||
|
|
||
| // 2. Extract user ID and ledger ID from token | ||
| if (!payload.userId) { | ||
| return Result.Err("No user ID in token"); | ||
| } | ||
|
|
||
| if (!payload.selected?.ledger) { | ||
| return Result.Err("No ledger selected in token"); | ||
| } | ||
|
|
||
| const userId = payload.userId; | ||
| const ledgerId = payload.selected.ledger; | ||
|
|
||
| // 3. Check if user is admin of the ledger | ||
| if (!(await this.isAdminOfLedger(userId, ledgerId))) { | ||
| return Result.Err("Not authorized to share this ledger. Admin access required."); | ||
| } | ||
|
|
||
| // 4. Get ledger details | ||
| const ledger = await this.db | ||
| .select() | ||
| .from(sqlLedgers) | ||
| .where(and(eq(sqlLedgers.ledgerId, ledgerId), eq(sqlLedgers.status, "active"))) | ||
| .get(); | ||
|
|
||
| if (!ledger) { | ||
| return Result.Err("Ledger not found or inactive"); | ||
| } | ||
|
|
||
| // 5. Find user by email | ||
| const rUser = await queryUser(this.db, { byString: req.email }); | ||
| if (rUser.isErr()) { | ||
| return Result.Err(rUser.Err()); | ||
| } | ||
|
|
||
| const users = rUser.Ok(); | ||
| if (users.length === 0) { | ||
| return Result.Err(`User with email ${req.email} not found. User must sign up first.`); | ||
| } | ||
|
|
||
| if (users.length > 1) { | ||
| return Result.Err(`Multiple users found for email ${req.email}`); | ||
| } | ||
|
|
||
| const targetUser = users[0]; | ||
|
|
||
| // Prevent self-sharing | ||
| if (targetUser.userId === userId) { | ||
| return Result.Err("Cannot share ledger with yourself"); | ||
| } | ||
|
|
||
| // 6. Add user to tenant first | ||
| const rAddUserToTenant = await this.addUserToTenant(this.db, { | ||
| userName: req.email, | ||
| tenantId: ledger.tenantId, | ||
| userId: targetUser.userId, | ||
| role: "member", | ||
| }); | ||
|
|
||
| if (rAddUserToTenant.isErr()) { | ||
| return Result.Err(rAddUserToTenant.Err()); | ||
| } | ||
|
|
||
| // 7. Add user to ledger | ||
| const rAddUser = await this.addUserToLedger(this.db, { | ||
| userName: req.email, | ||
| ledgerId: ledgerId, | ||
| tenantId: ledger.tenantId, | ||
| userId: targetUser.userId, | ||
| role: req.role || "member", | ||
| right: req.right || "read", | ||
| }); | ||
|
|
||
| if (rAddUser.isErr()) { | ||
| return Result.Err(rAddUser.Err()); | ||
| } | ||
|
|
||
| return Result.Ok({ | ||
| type: "resShareWithUser", | ||
| success: true, | ||
| message: `Successfully shared ledger with ${req.email}`, | ||
| ledgerId: ledgerId, | ||
| userId: targetUser.userId, | ||
| email: req.email, | ||
| role: req.role || "member", | ||
| right: req.right || "write", | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add maxShares validation before sharing.
The shareWithUser method doesn't check the ledger's maxShares limit (defined in the sqlLedgers schema) before adding a new user. While addUserToLedger checks for duplicate users (lines 470-493), it doesn't validate against maxShares. The checkMaxRoles call validates tenant-level role limits, not ledger-level share limits.
Add a check after line 1756 to count existing active users on the ledger and reject if it would exceed maxShares:
// Check maxShares limit
const activeUsers = await this.db
.select()
.from(sqlLedgerUsers)
.where(
and(
eq(sqlLedgerUsers.ledgerId, ledgerId),
eq(sqlLedgerUsers.status, "active")
)
)
.all();
if (ledger.maxShares && activeUsers.length >= ledger.maxShares) {
return Result.Err("Ledger share limit reached. Maximum shares exceeded.");
}🤖 Prompt for AI Agents
In dashboard/backend/api.ts around lines 1716 to 1816 (add the new check after
line 1756), add a pre-share validation that queries sqlLedgerUsers for active
users on the ledger and compares the count against ledger.maxShares; if
ledger.maxShares is set and the active count is already >= maxShares, return a
Result.Err indicating the ledger share limit has been reached. Ensure the DB
query filters by ledgerId and status="active" and use the existing Result.Err
format/message consistent with other errors.
# Conflicts: # dashboard/backend/cf-serve.ts # dashboard/backend/create-handler.ts
This reverts commit 1a58862.
necrodome
force-pushed
the
selem/sharing
branch
from
269c3a3 to
7916aaa
Compare
Summary by CodeRabbit
New Features
Security
✏️ Tip: You can customize this high-level summary in your review settings.