Skip to content

feat: limit ui access to external users - #693

Open
Saelmala wants to merge 2 commits into
mainfrom
feat/limit-access-external-users
Open

Saelmala wants to merge 2 commits into
mainfrom
feat/limit-access-external-users

Conversation

@Saelmala

@Saelmala Saelmala commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Users who are invited to call(s) can only access that/those call(s).
Previously, external users invited via the token link could access all productions and create/manage/delete productions.

The person who shares the link can select whether the access of the invited guest should be limited or not (by default this is checked to true)

@Saelmala Saelmala self-assigned this Sep 17, 2026

@birme birme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — Needs Changes

A cleanly-implemented, well-tested UI feature, but mislabeled as access control. "Guest" status is derived entirely from a client-controlled URL param (?guest=1) persisted in sessionStorage; every request still carries the same full-privilege shared Bearer ${API_KEY} (src/api/api.ts). The gating is purely cosmetic — which the in-code Note honestly admits — yet the PR title ("limit ui access to external users") and body ("can only access that/those call(s)") claim enforcement that does not exist. No existing check is weakened and no secret is introduced, so this is not a backdoor, but the framing plus a cluster of correctness warnings warrants changes.

Blocking: None (no weakened auth, no secret, REST via API, reducer not abused).

Warnings:

  • Client-side-only gating presented as an access boundary — a guest bypasses every restriction by editing the URL, clearing sessionStorage, or calling the REST API directly with the shared key. Either retitle/reword to "view-tailoring only", or add real backend enforcement (scoped guest tokens). Escalating this framing question to a human owner rather than merging on green CI.
  • src/utils/guest-session.ts:30-32 — no clear()/exit path; a user is stuck in guest mode for the whole tab session with no escalation.
  • src/hooks/use-is-guest.ts:5-6 — snapshots isGuest() once in a useState initializer; will show stale gating if state ever changes mid-session.
  • src/components/share-url-modal/share-url-modal.tsx:100-186 — a substantial rewrite of the companion-URL fetch/caching state machine (drops the in-flight flicker-avoidance) is bundled into an access-control PR — scope creep + an uncalled-out behavioral regression, untested.
  • Guest gating (!isGuest) is duplicated inline across many components rather than centralized — a single missed spot silently leaks an action to guests.

Suggestions: derive route prefixes in guest-session.ts:47 from shared route constants; add tests asserting guest-hidden adornments.

@birme birme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review (daily-backlog-pr Phase 3.5, orphan PR triage).

Verdict: LGTM

  • Client-side view tailoring for the guest UI restriction; honestly non-security (no server-side authorization is claimed or bypassed here).
  • No backdoors or suspicious exfiltration paths in the diff.
  • All 79 frontend tests pass; CI green (builds, e2e x2, lint, prettier, tests).

Leaving approved for a maintainer to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants