Restrict guest visibility to accessible spaces - #65
Open
ArtyomSavchenko wants to merge 31 commits into
Open
Conversation
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
marked this pull request as ready for review
August 1, 2026 03:43
ArtyomSavchenko
marked this pull request as draft
August 13, 2026 05:24
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>
aonnikov
reviewed
Aug 23, 2026
aonnikov
reviewed
Aug 23, 2026
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>
ArtyomSavchenko
marked this pull request as ready for review
September 2, 2026 08:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces a consistent security layer for restricted workspace roles:
ReadOnlyGuestDocGuestGuestAccountRole.UserIt 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/Employeerecords live in the sharedcontact.space.Contactssystem space.core.space.Workspacewere not protected by ordinary space membership:Collaboratorlove.class.MeetingMinuteslove.class.RoomInfohr.class.Requestnotification.class.PushSubscriptionguest.class.PublicLinkfindAllor full-text search.Security model
Restricted-role threshold
roleOrderis now the single source of truth for identifying a restricted account: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
TxAccessLevelmetadata:ModulePermissionGroup/ClassPermissionconfiguration is cached;TxApplyIf;TxMixinis checked as an update operation instead of being unconditionally allowed;createdByvalue no longer bypasses class-level access checks.Layer 2 — row visibility
core.mixin.RowVisibilitydeclares how a restricted role may access records in shared or system spaces:ownerFieldlinkedViaRecordspaceMemberdenyAllpublicReadablePolicies are registered with the affected class model and inherited through the hierarchy.
allowKnownIdBypassremains 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
findAllRowVisibilitypolicy are narrowed to permitted records.searchFulltextRowVisibilitywithout a known-id bypass.Declared policies
core.class.Collaboratorcollaboratoraccountlove.class.MeetingMinuteslove.class.RoomInfohr.class.RequestattachedTopersonnotification.class.PushSubscriptionuseraccountguest.class.PublicLinklinkId; no known-id bypassGuest 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: