Ocisdev 900

[LarsJurgensen]

No description provided.

[kw-security]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[f-firas]

You are not handling the case when qErr is not null here: If there is an error, I think we should stop and throw that error

[LarsJurgensen]

There is one conceptual issue here. There are expected cases where GetQuota can fail here. It's possible that someone is allowed to upload a document to a folder, but does not have access to the space as a whole. In that case GetQuota fails. The user is still allowed to upload, so we cannot fail. This is more of a best effort try. There is also one acceptance test failing in ocis because of this, see this error on the ocis branch:

# BROKEN with coordinator - returns 201 instead of 507
  # brian uploads into alice space via share. coordinator calls GetQuota with brian context.
  # brian has no GetQuota perm on alice space -> PermissionDenied -> quota check silently skipped -> upload succeeds.
  # old decomposedfs called node.CheckQuota directly (reads xattr, no perm check) so it worked.
  # fix needs driver-independent quota enforcement hook in storage.FS interface.

I'll bring this topic into the discussion tomorrow

[f-firas]

For a new file, info.Storage["NodeId"] is still "" here, because the refactor moved NodeId assignment for new files to TouchFile at the end of the upload, but setHeaders runs before PatchFile, so the node hasn't been created yet at this point. FormatResourceID then drops the empty OpaqueId and the OC-FileId header comes out as $, which is missing its node segment.

[f-firas]

We can't fix it here since the id genuinely doesn't exist yet. Better to fix it downstream in ocdav (internal/http/services/owncloud/ocdav/tus.go, ~line 322), where we blindly overwrite the path-based sReq.Ref with this node header. I propose we only trust the header when it actually carries a node, and otherwise keep the path-based ref (like the /public branch right above):

  if resid, err := storagespace.ParseID(httpRes.Header.Get(net.HeaderOCFileID)); err == nil &&
      resid.GetOpaqueId() != "" && resid.GetOpaqueId() != resid.GetSpaceId() {
      sReq.Ref = &provider.Reference{ResourceId: &resid}
  }