feat(share): add access roles (view, comment, edit), link expiry, password protection, and share revocation (#1527) - #1534
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughShare links now support access roles, expiry options, optional passwords, active-link listing, copying, revocation, and password verification. The share dialog provides create/manage tabs, while URL helpers parse share roles and tests cover the new APIs. ChangesShare link access and management
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
Sequence Diagram(s)sequenceDiagram
participant User
participant ShareProjectDialog
participant share-geolibre
participant ShareAPI
User->>ShareProjectDialog: Configure role, expiry, and password
ShareProjectDialog->>share-geolibre: uploadProjectToShare(options)
share-geolibre->>ShareAPI: POST project share
ShareAPI-->>share-geolibre: Share result
share-geolibre-->>ShareProjectDialog: Upload result
ShareProjectDialog->>share-geolibre: fetchProjectShares(token)
share-geolibre->>ShareAPI: GET /api/shares
ShareAPI-->>share-geolibre: Active shares
share-geolibre-->>ShareProjectDialog: Active share list
User->>ShareProjectDialog: Revoke a share
ShareProjectDialog->>share-geolibre: revokeShare(shareId)
share-geolibre->>ShareAPI: DELETE /api/shares/{shareId}
ShareAPI-->>ShareProjectDialog: Revocation result
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
🔍 Cloudflare PR preview
|
🔍 GitHub Pages PR preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/geolibre-desktop/src/components/layout/ShareProjectDialog.tsx`:
- Around line 106-116: Update loadActiveShares to track fetch failures in a
dedicated error state, following the existing revokeError pattern, instead of
setting active shares to an empty array in the catch block. Surface that error
through the dialog’s existing UI while preserving setLoadingShares(false) in
finally and the successful shares flow.
- Around line 443-464: Wire the Manage tab’s existing i18n keys into
ShareProjectDialog: at
apps/geolibre-desktop/src/components/layout/ShareProjectDialog.tsx:443-464, use
t() for passwordProtected, expires, visibility, and role, and pass the active
language to toLocaleDateString(); at :477-489, add the revoke button aria-label
using revokingId, t("share.revoking"), and t("share.revoke").
In `@apps/geolibre-desktop/src/lib/share-geolibre.ts`:
- Around line 272-289: Update the role and visibility fallback values in the
share-item mapping to fail closed: change the unknown or missing role fallback
in the role normalization to "view" and the visibility fallback in visibility
normalization to "private". Preserve the existing accepted-value checks and all
other fields in the returned object.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b92d0e7b-c5ac-4bc4-803e-a8c47cf0b81f
📒 Files selected for processing (6)
apps/geolibre-desktop/src/components/layout/ShareProjectDialog.tsxapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/project-url.tsapps/geolibre-desktop/src/lib/share-geolibre.tstests/project-url.test.tstests/share-geolibre.test.ts
|
/claude-review |
|
/claude-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| import type { ShareRole } from "./share-geolibre"; | ||
|
|
||
| /** | ||
| * Parses a share role string ("view", "comment", "edit") into a valid ShareRole or null. | ||
| */ | ||
| export function parseShareRole(value: unknown): ShareRole | null { | ||
| if (value === "view" || value === "comment" || value === "edit") { | ||
| return value; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Reads a share access role from the current `window.location` query string if present (?role=view, ?role=comment, ?role=edit). | ||
| */ | ||
| export function shareRoleFromLocation(): ShareRole | null { | ||
| if (typeof window === "undefined") return null; | ||
| const params = new URLSearchParams(window.location.search); | ||
| return parseShareRole(params.get("role") || params.get("shareRole")); | ||
| } |
There was a problem hiding this comment.
Quality: the import for ShareRole is inserted mid-file (line 18), sandwiched between a stale duplicate JSDoc block (lines 9-17, which documents projectUrlFromLocation but now sits above parseShareRole/shareRoleFromLocation) and the real projectUrlFromLocation doc comment at line 39. This looks like a copy/paste artifact from adding the new exports. Move the import to the top of the file with the other imports and delete the duplicate comment block.
More significantly (functional, medium confidence): shareRoleFromLocation/parseShareRole are exported here but grepping the repo shows they're never called anywhere outside this file and its tests — not in useProjectUrlLoader.ts, useEmbedApi.ts, or any component that loads a shared project. So even though ShareProjectDialog lets a user pick view/comment/edit when creating a share, nothing in the client actually reads ?role= back out and restricts editing/commenting when a shared link is opened. As written, the "access role" feature only affects what's sent to the server on creation and is purely decorative in the desktop/web app itself.
| import type { ShareRole } from "./share-geolibre"; | |
| /** | |
| * Parses a share role string ("view", "comment", "edit") into a valid ShareRole or null. | |
| */ | |
| export function parseShareRole(value: unknown): ShareRole | null { | |
| if (value === "view" || value === "comment" || value === "edit") { | |
| return value; | |
| } | |
| return null; | |
| } | |
| /** | |
| * Reads a share access role from the current `window.location` query string if present (?role=view, ?role=comment, ?role=edit). | |
| */ | |
| export function shareRoleFromLocation(): ShareRole | null { | |
| if (typeof window === "undefined") return null; | |
| const params = new URLSearchParams(window.location.search); | |
| return parseShareRole(params.get("role") || params.get("shareRole")); | |
| } | |
| import type { ShareRole } from "./share-geolibre"; | |
| /** | |
| * Parses a share role string ("view", "comment", "edit") into a valid ShareRole or null. | |
| */ | |
| export function parseShareRole(value: unknown): ShareRole | null { | |
| if (value === "view" || value === "comment" || value === "edit") { | |
| return value; | |
| } | |
| return null; | |
| } | |
| /** | |
| * Reads a share access role from the current `window.location` query string if present (?role=view, ?role=comment, ?role=edit). | |
| */ | |
| export function shareRoleFromLocation(): ShareRole | null { | |
| if (typeof window === "undefined") return null; | |
| const params = new URLSearchParams(window.location.search); | |
| return parseShareRole(params.get("role") || params.get("shareRole")); | |
| } |
There was a problem hiding this comment.
Import/comment part done in 38aca65 — the ShareRole import moved to the top with the others and the stale duplicate JSDoc block is gone.
The second part is correct and I'm leaving this thread open for @giswqs rather than fixing it here: parseShareRole/shareRoleFromLocation are only referenced by their own tests. Enforcing view/comment on link open needs an app-wide read-only mode that doesn't exist yet — the only "view-only" concept in the repo is collaboration.mode, which is a live-session notion, not a share-link one. So this needs a design decision (new feature vs. dropping the helpers until the enforcement lands), not a review-comment patch.
| fetchImpl?: typeof fetch; | ||
| } | ||
|
|
||
| export async function verifySharePassword( |
There was a problem hiding this comment.
Bug/incomplete feature (medium-high confidence): verifySharePassword is exported and unit-tested but is never called from anywhere in the app (ShareProjectDialog.tsx, useProjectUrlLoader.ts, useEmbedApi.ts — none reference it). fetchProjectFromUrl/useProjectUrlLoader fetch the raw project JSON directly and have no branch for a 401/403 "password required" response, so there's currently no UI path where a recipient of a password-protected share link is ever prompted to enter the password — they'd just see the generic "could not fetch the project" error from fetchProjectFromUrl. Password protection appears to be wired up only on the creation side (sending password in uploadProjectToShare), not the consumption side.
There was a problem hiding this comment.
Confirmed, and left unfixed in 38aca65 on purpose — leaving this thread open for @giswqs.
verifySharePassword really is unreferenced outside its tests, and fetchProjectFromUrl has no 401/403 "password required" branch, so a recipient of a protected link gets the generic fetch error. Wiring the consumption side isn't a minimal fix: the POST <shareUrl>/access contract it assumes has to be confirmed against share.geolibre.app, and useProjectUrlLoader needs a new password-prompt state. Same call as the role helpers (#discussion_r3675757119) — groundwork for a follow-up, or drop it until the server side exists.
|
All inline comments posted. Here's the final summary. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
|
I did a PR explainer for this during Lenzon testing - I hope it's helpful in some way: https://www.lenzon.ai/viewer/cms6rx1w8000ak9ceg476dy1h?voice=google-chirp3 |
- ShareProjectDialog: surface a dedicated `sharesError` instead of swallowing a failed share fetch into the "no active shares" empty state, which hid an expired token or dropped connection from the user. - ShareProjectDialog: give `loadActiveShares` its own AbortController so a close/reopen or token edit cancels the in-flight request; a superseded response can no longer overwrite a newer list or write state after the dialog is gone. - ShareProjectDialog: wire the Manage tab through i18n — the previously unused `share.passwordProtected` / `share.expires` keys, new short visibility/role labels instead of rendering the raw enum with `capitalize`, and a locale-aware expiry date via `i18n.language`. - ShareProjectDialog: give the icon-only revoke button an accessible name (`share.revoke` / `share.revoking`) and a `window.confirm` step, since revoking is immediate and irreversible. - ShareProjectDialog: restore the explanatory comments dropped in the refactor (re-entry guard, current-controller check, username-required routing, clipboard-copy rationale) — the logic they document is unchanged. - share-geolibre: fail closed to the `view` role when the server sends an unknown or missing one, instead of defaulting to full `edit`. - share-geolibre: percent-encode the project URL in the fallback viewer link so a raw `&`/`#` cannot truncate it, and encode the slug path segment; replace `item: any` in the mapper with `unknown` narrowed to `Record<string, unknown>`. - share-geolibre: stop duplicating the share password into a custom `X-Share-Password` header; the JSON body alone is enough and headers are captured separately by proxy/logging layers. - Tests for the fail-closed role and the encoded fallback viewer URL. - project-url: hoist the `ShareRole` import to the top and delete the stale duplicate JSDoc block left above `parseShareRole`.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/geolibre-desktop/src/components/layout/ShareProjectDialog.tsx (1)
557-566: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPer-item Copy button in the Manage tab has no
aria-label, unlike its sibling revoke button.The revoke button right next to it gets both
aria-labelandtitle(Line 571-574), and the top-level share-result copy button also usesaria-label={t("share.copyLink")}(Line 314). This per-share Copy button only hastitle={t("share.copyLink")}, which is a weaker accessible-name signal for screen readers on an icon-only control (<Copy className="h-3.5 w-3.5" />).♿ Suggested fix
<Button type="button" variant="secondary" size="sm" + aria-label={t("share.copyLink")} title={t("share.copyLink")} onClick={() => handleCopy(s.projectUrl)} >🤖 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 `@apps/geolibre-desktop/src/components/layout/ShareProjectDialog.tsx` around lines 557 - 566, Add aria-label={t("share.copyLink")} to the per-item Copy Button in the Manage tab, matching the existing top-level share-result copy button and sibling revoke button while preserving the current title and click behavior.apps/geolibre-desktop/src/lib/share-geolibre.ts (2)
386-390: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
verifySharePasswordreturns the server'sroleunchecked, unlikefetchProjectShares's fail-closed normalization.
fetchProjectSharesexplicitly normalizes an unknown/missingroleto"view"const role: ShareRole = item.role === "view" || item.role === "comment" || item.role === "edit" ? item.role : "view";, butverifySharePasswordtype-asserts the response and returnsdata.roleverbatim without the same check:role: data.role. Theas { content?: string; role?: ShareRole }cast doesn't validate at runtime — a server (or a compromised/misbehaving proxy) sendingrole: "owner"would flow through untouched as aShareRole. This function is currently unwired to any consumer per the earlier open thread, but if/when itsroleis used to gate permissions on link-open, this fail-open path would undermine the exact protection just added on the listing side.Extract the normalization into a shared helper (e.g.
normalizeShareRole(value: unknown): ShareRole) and reuse it in bothfetchProjectSharesand here.🔒 Suggested fix
+function normalizeShareRole(value: unknown): ShareRole { + return value === "view" || value === "comment" || value === "edit" ? value : "view"; +} + export async function fetchProjectShares(options: FetchSharesOptions): Promise<ActiveShare[]> { ... - const role: ShareRole = - item.role === "view" || item.role === "comment" || item.role === "edit" - ? item.role - : "view"; + const role = normalizeShareRole(item.role); ... } export async function verifySharePassword(...) { ... const data = (await response.json()) as { content?: string; role?: unknown }; return { projectContent: typeof data.content === "string" ? data.content : JSON.stringify(data), - role: data.role, + role: data.role === undefined ? undefined : normalizeShareRole(data.role), }; }🤖 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 `@apps/geolibre-desktop/src/lib/share-geolibre.ts` around lines 386 - 390, Extract the existing fail-closed role validation into a shared normalizeShareRole helper that accepts unknown values and returns "view" unless the value is "view", "comment", or "edit". Update both fetchProjectShares and verifySharePassword to use this helper, replacing the unchecked data.role return while preserving their existing response behavior.
357-357: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
getShareFetch()for password verification unless caller providesfetchImpl.
UploadProjectToShare,fetchProjectShares, andrevokeSharefall back to the sharedgetShareFetch()transport, whose desktop build overrides fetch to avoid WebView CORS issues.verifySharePasswordskips that fallback and uses barefetch; useoptions.fetchImpl ?? getShareFetch()unless theshareUrlis intentionally meant for browser fetch only.🤖 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 `@apps/geolibre-desktop/src/lib/share-geolibre.ts` at line 357, Update verifySharePassword’s fetch implementation to use the caller-provided options.fetchImpl when present, otherwise fall back to getShareFetch() instead of bare fetch. Preserve the existing password-verification behavior and URL handling.
🤖 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.
Outside diff comments:
In `@apps/geolibre-desktop/src/components/layout/ShareProjectDialog.tsx`:
- Around line 557-566: Add aria-label={t("share.copyLink")} to the per-item Copy
Button in the Manage tab, matching the existing top-level share-result copy
button and sibling revoke button while preserving the current title and click
behavior.
In `@apps/geolibre-desktop/src/lib/share-geolibre.ts`:
- Around line 386-390: Extract the existing fail-closed role validation into a
shared normalizeShareRole helper that accepts unknown values and returns "view"
unless the value is "view", "comment", or "edit". Update both fetchProjectShares
and verifySharePassword to use this helper, replacing the unchecked data.role
return while preserving their existing response behavior.
- Line 357: Update verifySharePassword’s fetch implementation to use the
caller-provided options.fetchImpl when present, otherwise fall back to
getShareFetch() instead of bare fetch. Preserve the existing
password-verification behavior and URL handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bbd29507-da22-4dd5-b865-0d180e4e0c00
📒 Files selected for processing (5)
apps/geolibre-desktop/src/components/layout/ShareProjectDialog.tsxapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/project-url.tsapps/geolibre-desktop/src/lib/share-geolibre.tstests/share-geolibre.test.ts
The server side of this feature does not exist yetI pushed fixes for all the inline review comments in 38aca65, but while doing so I checked whether the endpoints this PR calls actually exist on share.geolibre.app. Most of them don't. Flagging it here because it changes how this PR should be sequenced, not because anything is wrong with the client code — the UI and the client helpers are reasonable. The share backend isn't in this repo ( # Control: a route that exists rejects a bad token with 401
curl -s -o /dev/null -w "%{http_code}\n" -X POST \
-H "Authorization: Bearer glb_bogus_probe" -H "Content-Type: application/json" \
-d '{"filename":"x.geolibre.json","content":"{}","visibility":"unlisted"}' \
https://share.geolibre.app/api/projects
# -> 401 {"error":"Unauthorized"}
curl -s -o /dev/null -w "%{http_code}\n" -H "Authorization: Bearer glb_bogus_probe" \
https://share.geolibre.app/api/shares # -> 404
curl -s -o /dev/null -w "%{http_code}\n" -X DELETE -H "Authorization: Bearer glb_bogus_probe" \
https://share.geolibre.app/api/shares/xyz # -> 404
curl -s -o /dev/null -w "%{http_code}\n" -X POST -H "Content-Type: application/json" \
-d '{"password":"x"}' https://share.geolibre.app/tlo/lidar/access # -> 404
The 401-vs-404 contrast is what makes this conclusive: the auth layer answers 401 for routes that exist, so a 404 means there is no route, not that auth ran first. What that means feature by feature1. Active Shares tab — errors in production. 2. Revoke — reports success while the link stays live. This is the one I'd treat as blocking: if (!response.ok && response.status !== 404) {
throw new Error(`Failed to revoke share (HTTP ${response.status}).`);
}With the route absent, the DELETE 404s, 3. Password protection — not wired at either end. 4. Create path — the open risk to functionality that works today. The PR adds
This is the blocking question for the PR. Someone with access to the share.geolibre.app source or a valid API token should confirm which it is before this merges, because one branch is a regression of a shipping feature. Suggested sequencing
To be clear about how this got missed: three automated reviewers (Copilot, CodeRabbit, and the Claude review bot) went several rounds on this PR and all of us reviewed the client code against itself. The two "exported but never called" findings were the visible edge of it; none of us checked the endpoints against the deployed API. The client-side work here is solid — it just needs the backend to exist. |
|
Thanks @giswqs for probing the backend endpoints and catching the 401 vs 404 contrast on I've pushed commit
I completely agree with your suggested sequencing — we can hold/coordinate merging this client PR alongside the server-side endpoint deployment on |
3 participants
Summary
This PR addresses issue #1527 by extending project sharing on
share.geolibre.appbeyond basic visibility (unlisted,public,private). It introduces explicit access roles (view,comment,edit), link hygiene options (link expiry and password protection), active share management, and share revocation directly fromShareProjectDialog.Key Changes
Access Roles (
ShareRole):view: Read-only access (open, pan/zoom, toggle layer visibility, feature identify popups, export image).comment: View capabilities plus adding & replying to comments.edit: Full application editing capabilities (default)."view" | "comment" | "edit").parseShareRoleandshareRoleFromLocationhelpers to parse role parameters from share URLs (e.g.?role=view).Link Hygiene (Expiry & Password Protection):
never,24h,7d,30d).verifySharePassword) and never stored or passed in the URL.fetchProjectShares: Retrieves active share links for the authenticated user.revokeShare: Deletes an existing active share link.UI Enhancements (
ShareProjectDialog.tsx):View,Comment,Edit) and Link Expiry (Never,24 hours,7 days,30 days).I18n & Test Coverage:
en.jsonundershare.*.tests/share-geolibre.test.tsto test uploading with roles/expiry/passwords, listing active shares, revoking shares, and password verification.tests/project-url.test.tsto test role parsing.Verification
node --import tsx --test tests/share-geolibre.test.ts(28/28 passed) andtests/project-url.test.ts(12/12 passed).npm run typecheck(tsc -b && vite build) with zero errors.Closes #1527
Summary by CodeRabbit
New Features
Bug Fixes