Skip to content

Restrict guest visibility to accessible spaces - #65

Open
ArtyomSavchenko wants to merge 31 commits into
developfrom
person-visibility
Open

Restrict guest visibility to accessible spaces#65
ArtyomSavchenko wants to merge 31 commits into
developfrom
person-visibility

Conversation

@ArtyomSavchenko

@ArtyomSavchenko ArtyomSavchenko commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR introduces a consistent security layer for restricted workspace roles:

  • ReadOnlyGuest
  • DocGuest
  • Guest
  • future roles below AccountRole.User

It closes data-discovery gaps in shared/system spaces and aligns read, full-text search, and write handling around declarative row-visibility policies.

Problem

Restricted accounts could access or discover records outside their intended scope:

  • Person / Employee records live in the shared contact.space.Contacts system space.
  • Sensitive records in core.space.Workspace were not protected by ordinary space membership:
    • Collaborator
    • love.class.MeetingMinutes
    • love.class.RoomInfo
    • hr.class.Request
    • notification.class.PushSubscription
    • guest.class.PublicLink
  • Disabling an application in Guest permissions hid navigation but could still leave documents reachable through findAll or full-text search.
  • Known identifiers could bypass ownership checks in contexts where an identifier is not a sufficient authorization proof.
  • Guest-created documents could bypass the class-level write gate.

Security model

Restricted-role threshold

roleOrder is now the single source of truth for identifying a restricted account:

isRowLevelRestricted(role)

This avoids duplicating explicit lists of guest roles across authorization code.

Layer 1 — class/action access

Write operations use the existing permission-group configuration and TxAccessLevel metadata:

  • role-specific ModulePermissionGroup / ClassPermission configuration is cached;
  • the cache is invalidated when permission documents change, including nested TxApplyIf;
  • TxMixin is checked as an update operation instead of being unconditionally allowed;
  • a document’s createdBy value no longer bypasses class-level access checks.

Layer 2 — row visibility

core.mixin.RowVisibility declares how a restricted role may access records in shared or system spaces:

  • ownerField
  • linkedViaRecord
  • spaceMember
  • denyAll
  • publicReadable

Policies are registered with the affected class model and inherited through the hierarchy.

allowKnownIdBypass remains explicit per class. It is disabled for full-text search and mutation checks, where a caller-supplied identifier must not be treated as proof of authorization.

Read protection

findAll

  • Person and Employee discovery is restricted to people sharing a real space with the caller.
  • Classes with a declared RowVisibility policy are narrowed to permitted records.
  • Classes without a policy are denied in main/system spaces, while normal real-space membership continues to work.
  • Disabled application spaces are excluded regardless of a direct query.

searchFulltext

  • Applies the same person visibility rules.
  • Rechecks each result against RowVisibility without a known-id bypass.
  • Filters objects from applications disabled for the caller’s role.

Declared policies

Class Policy
core.class.Collaborator Own collaborator account
love.class.MeetingMinutes Linked collaborator record
love.class.RoomInfo Deny open browsing
hr.class.Request Own attachedTo person
notification.class.PushSubscription Own user account
guest.class.PublicLink Session linkId; no known-id bypass

Guest membership behavior

New named guests no longer inherit the anonymous read-only account’s space membership. Access is granted only through explicit invitation or autoJoinForRoles.

Tests

Added and updated regression coverage for:

  • row-level policies on sensitive shared-space classes;
  • denial of undeclared shared-space classes;
  • full-text results not acting as a known-id bypass;
  • public-link isolation;
  • class-level write access not being bypassed by document ownership;
  • permission-cache invalidation.

Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
@ArtyomSavchenko
ArtyomSavchenko marked this pull request as ready for review August 1, 2026 03:43
@ArtyomSavchenko
ArtyomSavchenko marked this pull request as draft August 13, 2026 05:24
ArtyomSavchenko and others added 22 commits August 13, 2026 21:33
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
- hr.class.Request and love.class.MeetingMinutes listed their own
  ownerField/linkTargetField (attachedTo) as a knownIdBypassFields
  entry, letting a caller skip the ownership check entirely by
  querying on that field directly (e.g. { attachedTo: coworkerId }).
  Drop attachedTo from both bypass lists.
- RowVisibilityResolver.canUpdate (the anti-ownership-transfer guard)
  only ran for TxUpdateDoc, even though Layer 1 access checks
  (hasClassAccessLevel) already treat TxMixin as an equivalent
  mutation. Route TxMixin through the same ownership check, applying
  TxProcessor.updateMixin4Doc instead of updateDoc2Doc.
- excludeSpacesFromQuery dereferenced current.$in without the
  null-guard its siblings (mergeEquals/mergeIn) have, throwing on a
  query like { space: null } for restricted-role reads.

Adds regression tests for each fix, including two new focused test
files (rowVisibilityCanUpdate.test.ts, guestVisibility.test.ts).

Signed-off-by: Artem Savchenko <armisav@gmail.com>
Product decision: guests should see all office rooms so the office
layout renders correctly, while the actual meeting content stays
protected. Room's RowVisibility policy moves from linkedViaRecord
(collaborator-only, via MeetingMinutes) to publicReadable, matching
the pattern already used for love.class.Floor. RoomInfo/ParticipantInfo
activity and the MeetingMinutes documents themselves are untouched and
stay collaborator-restricted.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Investigating the reported bug (Guest can't @-mention a co-member of
a shared chunter.class.Channel): this test runs the real
SpaceSecurityMiddleware.searchFulltext end-to-end against a mock that
mirrors real backend behavior (fulltext adapter honoring query.spaces,
Mixin _id lookups resolving to the base doc). The baseline scenario
passes - getGuestVisibleAccounts -> getGuestVisiblePersonIds ->
filterSearchResultsByRowVisibility correctly surfaces a shared-channel
member with default settings, so this mechanism is not by itself the
cause. Remaining suspects (workspace-specific Guest-permission module
config, the real fulltext index, or client-side rendering) need a live
repro to pin down further.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
FilePlaceholder.svelte's file-upload flow calls client.update(doc,
{ blobs }) directly on card.class.Card - a TxUpdateDoc - but Card had
no core.mixin.TxAccessLevel at all, so Layer 1 (hasClassAccessLevel)
denied every update from a Guest regardless of ownership. Ordinary
attachment uploads worked because they create a separate
attachment.class.Attachment doc instead, which already has its own
TxAccessLevel.

Grants Guest updateAccessLevel on Card (Layer 1) and adds a writePolicy
(ownerField: createdBy/socialId) via core.mixin.RowVisibility (Layer 2)
so the access is scoped to cards the guest created, reusing the
existing ownership-transfer guard. Read visibility is untouched
(policy: publicReadable is a no-op, still governed by ordinary space
membership).

This covers the "own cards only" case from the option the user picked.
A configurable own/collaborator/any scope (shared with the activity-
visibility permission) is a separate, larger follow-up.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
ClassAccessResolver's cache ignored Permission.txClass entirely,
treating every ModulePermissionGroup-covered ClassPermission as a
"may create this class" grant - isClassAccessAllowed only ever
consulted it for TxCreateDoc, falling back to the static
TxAccessLevel mixin for every other tx kind. That meant an
admin-configurable allow-list could never grant a restricted role
update/remove/mixin access to a class, only the code-declared minimum
role could.

Key the cache by (role, txClass), defaulting an unset txClass to
TxCreateDoc so every existing untyped ClassPermission keeps meaning
exactly what it did before. isClassAccessAllowed now looks up the
list for whatever tx._class actually is, so a ClassPermission with an
explicit txClass finally does what its own field name promises.
Needed as the foundation for admin-configurable guest permissions on
non-create actions (collaborator edits, process actions).

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
New per-role settings doc for restricted-role capabilities that don't
fit ModulePermissionGroup (whole-module toggle) or TxAccessLevel
(static per-class minimum role): each one reaches into data the
account's own row-visibility policy wouldn't otherwise let it touch,
so it needs its own explicit admin opt-in.

- editOwnDocCollaborators: create/remove core.class.Collaborator
  records on a document the account created.
- activityScope (own/collaborator/any): activity/message visibility
  on ownerField-write-policy documents (e.g. card.class.Card).
- runProcessActions: trigger workspace process actions.

Seeded once for AccountRole.Guest, off/most-restrictive by default -
no dedicated Settings UI yet, matching every other guest-permission
doc in this codebase; an admin opts a role in by editing the seeded
doc directly. Enforcement lands in a follow-up commit per capability.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
core.class.Collaborator's own RowVisibility policy (ownerField:
collaborator) requires the collaborator being named to be the caller's
own account - correct for self-service opt-in, but the wrong shape for
a card owner naming *someone else* a collaborator, which is what the
existing CollaboratorEditor UI actually does (client.addCollection /
client.remove on core.class.Collaborator).

Opens Layer 1 (TxAccessLevel) for any restricted role, and adds a
bespoke Layer 2 check in GuestPermissionsMiddleware that bypasses the
generic ownerField policy entirely for this class: gated on the new
GuestExtraPermissions.editOwnDocCollaborators admin opt-in (off by
default) plus the caller having created the document the collaborator
record attaches to (read generically off the tx's own attachedTo/
attachedToClass, and the parent's createdBy - no new dependency on the
card plugin needed).

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Adds core.mixin.RowVisibility.scopeActivityToOwner: a class (e.g.
card.class.Card, set here) opts in to having GuestExtraPermissions.
activityScope narrow a restricted role's view of AttachedDoc results
(chat/activity messages) attached to it - own authored activity only,
activity on documents where the caller is a listed core.class.
Collaborator, or (default, today's behavior) any it can already read.
Activity on a class that hasn't opted in - a chunter channel, say - is
untouched regardless of the setting, since every member should keep
seeing all of it.

Implemented as a post-fetch filter in SpaceSecurityMiddleware.findAll,
alongside the file's existing Person-visibility and disabled-module
special cases, rather than folding it into RowVisibilityPolicy itself:
that type is explicitly "decided once by the plugin author, never
admin-configurable", and this setting is exactly the opposite - a
runtime, per-role admin choice.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
process.class.ApproveRequest had no core.mixin.TxAccessLevel at all,
so Layer 1 denied the approve/reject buttons' TxUpdateDoc
(client.update(todo, { approved, ... })) for every restricted role
regardless of assignment.

Opens Layer 1 to any restricted role and adds a Layer 2 RowVisibility
ownerField policy (field: user, identity: personId) scoping it to the
assigned approver - reusing the existing generic TxUpdateDoc ownership
path unchanged, no bespoke code needed for that part. On top,
GuestPermissionsMiddleware gates the tx on the new
GuestExtraPermissions.runProcessActions admin opt-in (off by default),
referencing process.class.ApproveRequest by id rather than importing
@hcengineering/process, which would pull client-only packages
(@hcengineering/ui) into this server package.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
core.class.GuestExtraPermissions (added earlier this session) was an
unnecessary new mechanism: this codebase already has a working,
generic Settings -> Guest permissions panel (GuestPermissionsSettings.
svelte) that renders one toggle per core.class.Permission/ClassPermission
listed in a ModulePermissionGroup, with zero per-module UI code -
exactly what "Allow creating cards" already is for card.class.Card.

Item 4 (edit collaborators) and item 6 (approve/reject process
actions) are both plain booleans, so they belong in that same system
instead of a bespoke, UI-less settings doc:
- card.ids.GuestCollaboratorClassPermission, added to the existing
  Cards Guest ModulePermissionGroup (off by default) - shows up as
  "Allow editing collaborators on own cards" right next to "Allow
  creating cards".
- process.ids.GuestApproveRequestClassPermission, in a new Process
  Guest ModulePermissionGroup (off by default) - "Allow approve/reject
  actions".

GuestPermissionsMiddleware now reads both through the existing
ClassAccessResolver cache (the same one Layer 1 already uses) instead
of a bespoke resolveGuestExtraPermissions() lookup.

Renamed the now activityScope-only leftover to
core.class.GuestActivitySettings: a three-way choice genuinely doesn't
fit an on/off toggle, so it keeps its own (UI-less, for now) settings
doc - documented as such rather than left unexplained.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Cut multi-line comments down to what isn't already obvious from the
code or the type's own doc comment, removed a stale reference to a
field renamed away in the previous commit, and dropped "item N"
task-tracking labels that only made sense inside this session's
conversation.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
hierarchy.classHierarchyMixin<M extends D>'s generic unifies M with
whatever concrete type the _class ref carries when it's not already
Doc-typed - passing core.class.Collaborator (Ref<Class<Collaborator>>)
directly made TS infer the mixin lookup's return type as
Mixin<Collaborator> instead of TxAccessLevel/RowVisibility, so
.createAccessLevel/.removeAccessLevel/.policy don't exist on it.
ts-jest's transpile-only mode didn't catch this locally; the CI tsc
build did. Every other class ref in this file already goes through a
`... as Ref<Class<Doc>>` constant for the same reason - this one
missed it.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Comment thread foundations/core/packages/core/src/classes.ts Outdated
Comment thread models/core/src/security.ts Outdated
ArtyomSavchenko and others added 4 commits August 23, 2026 11:19
1. AccountIdentityResolver.resolve('socialId') only checked
   account.primarySocialId, dropping the account.socialIds fallback -
   a guest whose content was authored under a non-primary linked
   social id lost read/edit/delete access to their own chat messages,
   attachments, cards, and saved messages. resolve() now returns every
   linked social id (collapsed to a scalar in the common single-id
   case, so query shapes are unchanged unless an account actually has
   more than one), and every consumer (canCreate, canUpdate,
   applyPolicy's query narrowing) matches against all of them via a
   shared identityMatches() helper.

2. guestPermissions.ts's canEditDocCollaborator had the same
   primarySocialId-only narrowing, breaking its own "edit collaborators
   on own card" feature for guests with a non-primary createdBy.

3. spaceSecurity.ts's GuestActivityScope.Own filtering had the same
   bug, dropping a guest's own activity history.

4. canUpdate's ownership-transfer guard only handled ownerField write
   policies; a class combining write access with a linkedViaRecord
   policy (targetField reassignment) had no equivalent guard. Extracted
   the "resolve a linkedViaRecord policy's allowed target set" logic
   into resolveLinkedTargets(), shared between applyPolicy's read-side
   narrowing and canUpdate's new linkedViaRecord branch. canUpdate now
   takes ctx to run this lookup.

5. The Person/Employee search restriction only ran when query.classes
   was explicitly set, and getAllAllowedSpaces's search-exclusion list
   didn't include DocGuest - an unscoped full-text search from a
   DocGuest could surface Person/Employee docs from spaces it doesn't
   share. Added DocGuest to the exclusion list and made an unscoped
   search always run the result-level Person filter for restricted
   roles, rather than depending on space-exclusion already covering it.

Each fix has a regression test verified to fail on the prior code and
pass with the fix (checked by hand-reverting each change independently).

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Pulls the RowVisibility/TxAccessLevel declarative types out of the general
classes.ts/utils.ts grab-bag files into a dedicated
foundations/core/packages/core/src/security.ts, re-exported unchanged from
the package root. Adds docs/security-model.md, the design doc that
rowVisibilityInvariant.test.ts already referenced but that didn't exist,
covering the two-layer model, roleOrder threshold, policy kinds, and a
generated table of every class's declared RowVisibility policy sourced
from the actual model registrations. Cross-links it from accessGate.ts,
rowVisibility.ts, the invariant test, and ARCHITECTURE_OVERVIEW.md.

Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
Signed-off-by: Artem Savchenko <armisav@gmail.com>
@ArtyomSavchenko
ArtyomSavchenko marked this pull request as ready for review September 2, 2026 08:57
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.

3 participants