Skip to content

feat: Enforce effective member access on shared drive moves - #4926

Open
taratatach wants to merge 4 commits into
masterfrom
feat/use-effective-access-in-move-operations
Open

feat: Enforce effective member access on shared drive moves#4926
taratatach wants to merge 4 commits into
masterfrom
feat/use-effective-access-in-move-operations

Conversation

@taratatach

@taratatach taratatach commented Sep 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #4844

Moves and copies on shared drives were authorized by a flat
membership plus ReadOnly check, wrong with nested shared folders,
and cross-stack moves bypassed member authorization entirely via the
share-interact token.

  • Route cross-stack moves through the drive routes: client.InDrive
    scopes a client to a shared drive, so the hosting stack authorizes
    each request via guardSharedDriveRouteForMember
  • Resolve effective access on source and destination in
    MoveHandler via AccessResolver.ResolveForMember — 404 out of
    scope, 403 on insufficient access
  • Support permanent_delete on drive PATCH, previously a silent
    no-op
  • Reserve drive root trash/destroy to the owner: members get a 403

@taratatach taratatach self-assigned this Sep 16, 2026
@taratatach
taratatach force-pushed the feat/use-effective-access-in-move-operations branch from 935cc3d to 41600e6 Compare September 16, 2026 10:42
@taratatach
feat: Support permanent_delete on drive PATCH
0df967b 
  The drive PATCH handler parsed but silently ignored
  `permanent_delete`: the `docPatch` of `web/sharings` had no
  `Delete` field, so a permanent delete requested through
  `/sharings/drives/:id/:file-id` was a no-op returning 200.

  Add the `Delete` field and handle it in `applyPatch` with
  `fs.DestroyDirAndContent(dir, fs.EnsureErased)` for directories
  and `fs.DestroyFile` for files, bringing the drive route to
  parity with the `/files` API in a single call. Authorization
  needs nothing new: `guardSharedDriveRouteForMember` already
  requires effective PATCH access on the target, so read-only
  members get a 403.
@taratatach
feat: Route shared drive moves via drive routes
fb5eb57 
  Cross-stack moves involving a shared drive were executed through
  the direct `/files` routes with the share-interact token. That
  token carries all verbs on the drive content, so the member
  authorization enforced by `guardSharedDriveRouteForMember` on the
  dedicated drive routes was bypassed: a read-only member could add
  files to a drive or permanently delete from it. Remote errors
  were also mishandled: logged and swallowed, or mapped to 500 by
  `files.WrapVfsError`.

  The `client` package gains `InDrive`, which scopes a client to a
  shared drive: `filesPath` swaps the `/files` prefix for
  `/sharings/drives/<id>` on every file operation, so the stack
  hosting the drive authorizes each request. The move flows scope
  their remote clients accordingly, and `wrapRemoteErr` surfaces
  the remote status faithfully instead of the previous
  log-and-continue, with `remoteStatusTextCodes` covering the
  common texts. The per-file deletions inside the copy loop of
  `moveDirBetweenSharedDrives` are dropped: the recursive root
  directory deletion after the loop already covers them, and a
  transient deletion error no longer aborts the remaining copies.
@taratatach
taratatach force-pushed the feat/use-effective-access-in-move-operations branch from 41600e6 to 7e2fdf9 Compare September 16, 2026 10:47
@taratatach
taratatach marked this pull request as ready for review September 16, 2026 10:48
@taratatach
taratatach requested a review from shepilov September 16, 2026 10:48
shepilov
shepilov reviewed Sep 16, 2026
View reviewed changes
Comment thread client/files.go
// routed through filesPath have a drive-route equivalent (reads, creation,
// metadata patch, upload, overwrite, trash, restore, destroy). It mutates
// and returns the receiver for chaining.
func (c *Client) InDrive(driveID string) *Client {

@shepilov shepilov Sep 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why new method instead of constructor? It's always called NewRemoteClient.InDrive

@taratatach taratatach Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

NewRemoteClient is part of the sharings package while the Client struct (which keeps hold of the drive ID) is part of the client package.

The new InDrive method allows other clients to set the driveID attribute.

@shepilov shepilov Sep 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add one more param to NewRemoteClient or override?

@taratatach taratatach Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Then only clients created via NewRemoteClient could set the driveID attribute while it's an attribute of Client.

@shepilov shepilov Sep 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it can be set also with Client struct init by any consumer, as in NewRemoteClient

@taratatach taratatach Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We'd need to export the attribute then. Which could be a good idea actually.

@shepilov shepilov mentioned this pull request Sep 16, 2026
Open
@taratatach
feat: Use effective access in drive move operations
7b335d0 
  Move and copy on shared drives were authorized by a flat check on
  the sharing: membership plus the member's `ReadOnly` flag. With
  nested shared folders this is wrong in both directions — it rejects
  a member read-only at the drive root but read-write on a nested
  folder, and it never accumulates scopes along the target's path.

  `MoveHandler` now resolves effective access on the source and the
  destination through `AccessResolver.ResolveForMember`: the resolver
  runs on the instance hosting the target while membership is matched
  for the calling instance. Copy requires effective read on the
  source; move requires effective write on the source plus effective
  write/create on the destination parent. Targets outside every scope
  return 404, insufficient access returns 403, and file-backed-drive
  validation keeps its 422 by running before the effective checks.
  The owner of a drive keeps full access on it without resolving.

  A remote target cannot be resolved locally, so the local stack
  decides nothing there: the remote stack is the only authority and
  applies its own access rules. `checkSharedDrivePermission` still
  serves membership loading and the other callers.
@taratatach
fix: Reserve drive root trash/destroy to the owner
504d117 
  A read-write member could trash or destroy the root of a shared drive
  via `DELETE /sharings/drives/:id/:file-id` or
  `DELETE /sharings/drives/:id/trash/:file-id`, taking the whole drive
  down, as removing the root revokes the sharing. These requests are now
  rejected with 403 for members; only the owner keeps them.
@taratatach
taratatach force-pushed the feat/use-effective-access-in-move-operations branch from 7e2fdf9 to 504d117 Compare September 16, 2026 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shepilov shepilov shepilov approved these changes

Assignees

@taratatach taratatach

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use Effective Access In Backend Move Operations

2 participants

@taratatach @shepilov