feat(server-api): add /api/shares with link role, expiry, and password - #2807
Conversation
opengeos#2803) The Share dialog's Active Shares tab calls GET/DELETE /api/shares, which the projects server never implemented, so it failed with HTTP 404. - GET /api/shares lists the caller's managed public/unlisted projects. - DELETE /api/shares/{id} makes the project private and resets link settings, keeping the project, versions, and group shares. - POST /api/projects accepts role, expiresIn, password; project JSON returns role, expiresAt, hasPassword. - Expiry (410) and password (401) are enforced on every non-manager read; POST /{username}/{slug}/access and /org/{org}/{slug}/access unlock a link, throttled to 10 wrong guesses per 5 minutes.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughProject shares now support roles, expiry, and password protection. The API adds endpoints to list and revoke manageable shares, and routes to unlock password-protected links. Database upgrades preserve the new share settings. ChangesShare links
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Reader
participant PasswordAccessRoute
participant SharedAccessHandler
participant AttemptTracker
participant ProjectRecord
Reader->>PasswordAccessRoute: Submit project and password
PasswordAccessRoute->>SharedAccessHandler: Request share access
SharedAccessHandler->>AttemptTracker: Reserve attempt for project and client IP
AttemptTracker-->>SharedAccessHandler: Allow attempt or reject with 429
SharedAccessHandler->>ProjectRecord: Check share settings and retrieve latest content
ProjectRecord-->>SharedAccessHandler: Return share settings and content
SharedAccessHandler-->>Reader: Return content and role, or access error
Merge Risk: ⚪ Minimal · up to The reviewed share-management and access paths have no identified issue that needs to be fixed before merge. The documented per-process password throttle remains a deployment limitation. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Share protection is centralized and the inspected content routes enforce it before returning data. However, ordinary visibility changes can leave old link restrictions attached to projects, blocking authorized readers outside the link-sharing lifecycle. Password-guessing protection also depends on process-local state and correct proxy configuration. These warrant a design-level review, although no content-access bypass was established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue Full details: Docstring CoverageExplanation Docstring coverage is 53.85% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 39 functions across 3 files. (1 skipped: 1 unsupported.)
✨ 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. A rabbit checks a link at dawn Comment |
🔍 Cloudflare PR preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
- Gate the version list with the share link's expiry/password. - Key the access throttle on the trusted-proxy-aware client address. - List shares for organization administrators who did not create the project. - Correct the docs (public/unlisted only, revoke 403/404, 429 throttle). - Add tests for the version gate, administrator listing, and input validation.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
Concurrent wrong passwords could all pass the limit check before any failure was recorded. Reserve the attempt under a lock before checking the password, release it unless the password was wrong, and prune stale keys.
| Managers always read their own projects. Everyone else gets 410 once the | ||
| link has expired, and 401 until the project's password is supplied. | ||
| """ | ||
| if project.share_expires_at is None and project.share_password_hash is None: |
There was a problem hiding this comment.
Bug (medium confidence): link_gate enforces expiry/password against every non-manager reader, but role/expiresIn/password can be set on a project of any visibility — not just public/unlisted link shares. ProjectCreate (and create_project) accept these fields regardless of visibility, and validate_access_targets allows visibility: "organization" together with a password.
Concretely: if a project is created with visibility: "organization" and a password, a legitimate organization member who reaches it through the visible() "organization" branch will still be blocked here with 401/410, even though they have real access rights and have no way to know a password that was only ever meant to gate the anonymous public link. The same applies to group-shared private projects.
Consider scoping this gate (and/or the acceptance of role/expiresIn/password at creation) to visibility in {"public", "unlisted"}, since GET/DELETE /api/shares already treat only those visibilities as "link shares".
|
|
||
|
|
||
| class ShareAccessRequest(BaseModel): | ||
| password: str = Field(max_length=200) |
There was a problem hiding this comment.
Minor nit: ShareAccessRequest.password has max_length=200 but no min_length, unlike ProjectCreate.password (min_length=1, max_length=200) a few lines up. Not exploitable (an empty share password can never actually be set, so "" will just never match), but worth aligning for consistency.
| password: str = Field(max_length=200) | |
| password: str = Field(min_length=1, max_length=200) |
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Expiry/password gate every non-manager reader, so on an organization or private project they would lock out members who cannot know the password. Return 422 instead, and require a non-empty password on /access.
|
Addressed the latest review in 6704a84.
|
| account = principal.account | ||
| candidates = session.scalars( | ||
| select(Project) | ||
| .options(*LISTING_EAGER_LOADS) |
There was a problem hiding this comment.
Performance (low confidence, minor): list_shares filters can_manage_project(...) in Python after loading every public/unlisted project the account owns, created, or administers. can_manage_project calls organization_role(...) (a DB query) per project with an organization_id, so this is an N+1 for accounts/orgs with many shares. Also unlike list_projects, there's no limit/offset here, so a prolific org could return an unbounded list. Likely fine at current scale, but worth keeping in mind if this list grows.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Closes #2803
Problem
Project > Share > Active Shares fails with
Failed to fetch shares (HTTP 404). The client (added in #1534) callsGET /api/sharesandDELETE /api/shares/{id}, butgeolibre_server_apinever implemented them, nor the role, expiry and password fields the client sends.Changes
GET /api/shares: the caller's managed public and unlisted projects. The share id is the project id.DELETE /api/shares/{id}: makes the project private and resets role, expiry and password. The project, its versions and group shares are kept. Organization-visible projects are not link shares and return 404.POST /api/projectsacceptsrole,expiresInandpassword. Project JSON returnsrole,expiresAtandhasPassword.410) and password (401) are enforced on every non-manager read.POST /{username}/{slug}/accessandPOST /org/{org}/{slug}/accessunlock a link. Wrong guesses are throttled to 10 per 5 minutes per project and client address (in memory, per process).roleis stored and returned only.share_role,share_expires_at,share_password_hashare added to the Postgres and SQLite upgrade paths, including the SQLite table rebuild.docs/server-api.mddocuments the routes and fields.Testing
pytest backend/geolibre_server_api/tests: 220 passed (10 new intest_shares.py).ruff checkandruff formatare clean.Deployment
The hosted share.geolibre.app returns 404 until this backend is deployed. The client needs no change.
Summary by CodeRabbit