chore: cover the upload coordinator with tests

[f-firas]

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.

[mklos-kw]

Two bugs found in coordinator.go / tus_adapter.go during review of #714. Confirmed locally against this test suite. Both fixes are small; proposing them inline below.

---

Bug 1 — SpaceOwner empty in events for new files on project spaces

File: pkg/upload/coordinator.go, touchNode (~line 308)

Root cause: When TouchFile returns nil SpaceOwner (normal for project spaces — their stored owner is a SPACE_OWNER service account), touchNode simply skips all three SpaceOwnerOrManager writes. describeExisting already handles this correctly by calling spaceOwnerOrManager which falls back to ListGrants. touchNode doesn't.

Fix:

// before
if result.SpaceOwner != nil {
    session.SetStorageValue("SpaceOwnerOrManager", result.SpaceOwner.GetOpaqueId())
    session.SetStorageValue("SpaceOwnerIdp", result.SpaceOwner.GetIdp())
    session.SetStorageValue("SpaceOwnerType", utils.UserTypeToString(result.SpaceOwner.GetType()))
}

// after
owner := c.spaceOwnerOrManager(ctx, result.SpaceOwner, result.SpaceID)
if owner != nil {
    session.SetStorageValue("SpaceOwnerOrManager", owner.GetOpaqueId())
    session.SetStorageValue("SpaceOwnerIdp", owner.GetIdp())
    session.SetStorageValue("SpaceOwnerType", utils.UserTypeToString(owner.GetType()))
}

Tests to add — fakeFS needs ListGrants first (it currently panics on any unimplemented method when called through the embedded storage.FS):

In fakefs_test.go, add to the struct and a method:

// in fakeFS struct:
grants    []*provider.Grant
grantsErr error

// new method:
func (f *fakeFS) ListGrants(_ context.Context, _ *provider.Reference) ([]*provider.Grant, error) {
    f.record("ListGrants")
    return f.grants, f.grantsErr
}

Then in coordinator_test.go, in the finishUpload / new-file context:

It("records the SpaceOwner from TouchFile directly when it is a real user", func() {
    session := newSession(false)
    fs.touchedOwner = &userpb.UserId{OpaqueId: "owner-1", Idp: "idp.example.com",
        Type: userpb.UserType_USER_TYPE_PRIMARY}

    _, err := c.finishUpload(ctx, session)

    Expect(err).ToNot(HaveOccurred())
    Expect(session.SpaceOwner().GetOpaqueId()).To(Equal("owner-1"))
})

It("falls back to ListGrants when TouchFile returns nil SpaceOwner", func() {
    session := newSession(false)
    fs.touchedOwner = nil
    fs.grants = []*provider.Grant{{
        Grantee: &provider.Grantee{
            Type: provider.GranteeType_GRANTEE_TYPE_USER,
            Id:   &provider.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: "manager-1", Idp: "idp.example.com"}},
        },
        Permissions: &provider.ResourcePermissions{
            Stat: true, ListContainer: true, InitiateFileDownload: true,
        },
    }}

    _, err := c.finishUpload(ctx, session)

    Expect(err).ToNot(HaveOccurred())
    Expect(session.SpaceOwner().GetOpaqueId()).To(Equal("manager-1"))
})

Note: existing tests in coordinator_test.go, put_test.go, and tus_adapter_test.go that use new-file sessions will need touchedOwner set to a non-nil, non-SPACE_OWNER value in their BeforeEach, otherwise the fix causes them to hit ListGrants and fail their call-order assertions.

---

Bug 2 — PermissionDenied falls through FinishUpload, tusd answers 500

File: pkg/upload/tus_adapter.go, FinishUpload (~line 58)

Root cause: The error-mapping switch has no case errtypes.IsPermissionDenied. A revoked share mid-upload hits the default branch; tusd returns a bare 500 with no machine-readable error code instead of 403.

Fix — add one case before default:

case errtypes.IsPermissionDenied:
    return tusd.NewError("ERR_PERMISSION_DENIED", err.Error(), http.StatusForbidden)

Test to add — in tus_adapter_test.go, add one Entry to the existing DescribeTable:

Entry("permission denied", errtypes.PermissionDenied("share was revoked"), "ERR_PERMISSION_DENIED", http.StatusForbidden),

[f-firas]

Two bugs found in coordinator.go / tus_adapter.go during review of #714. Confirmed locally against this test suite. Both fixes are small; proposing them inline below.

Bug 1 — SpaceOwner empty in events for new files on project spaces

File: pkg/upload/coordinator.go, touchNode (~line 308)

Root cause: When TouchFile returns nil SpaceOwner (normal for project spaces — their stored owner is a SPACE_OWNER service account), touchNode simply skips all three SpaceOwnerOrManager writes. describeExisting already handles this correctly by calling spaceOwnerOrManager which falls back to ListGrants. touchNode doesn't.

Fix:

// before
if result.SpaceOwner != nil {
    session.SetStorageValue("SpaceOwnerOrManager", result.SpaceOwner.GetOpaqueId())
    session.SetStorageValue("SpaceOwnerIdp", result.SpaceOwner.GetIdp())
    session.SetStorageValue("SpaceOwnerType", utils.UserTypeToString(result.SpaceOwner.GetType()))
}

// after
owner := c.spaceOwnerOrManager(ctx, result.SpaceOwner, result.SpaceID)
if owner != nil {
    session.SetStorageValue("SpaceOwnerOrManager", owner.GetOpaqueId())
    session.SetStorageValue("SpaceOwnerIdp", owner.GetIdp())
    session.SetStorageValue("SpaceOwnerType", utils.UserTypeToString(owner.GetType()))
}

Tests to add — fakeFS needs ListGrants first (it currently panics on any unimplemented method when called through the embedded storage.FS):

In fakefs_test.go, add to the struct and a method:

// in fakeFS struct:
grants    []*provider.Grant
grantsErr error

// new method:
func (f *fakeFS) ListGrants(_ context.Context, _ *provider.Reference) ([]*provider.Grant, error) {
    f.record("ListGrants")
    return f.grants, f.grantsErr
}

Then in coordinator_test.go, in the finishUpload / new-file context:

It("records the SpaceOwner from TouchFile directly when it is a real user", func() {
    session := newSession(false)
    fs.touchedOwner = &userpb.UserId{OpaqueId: "owner-1", Idp: "idp.example.com",
        Type: userpb.UserType_USER_TYPE_PRIMARY}

    _, err := c.finishUpload(ctx, session)

    Expect(err).ToNot(HaveOccurred())
    Expect(session.SpaceOwner().GetOpaqueId()).To(Equal("owner-1"))
})

It("falls back to ListGrants when TouchFile returns nil SpaceOwner", func() {
    session := newSession(false)
    fs.touchedOwner = nil
    fs.grants = []*provider.Grant{{
        Grantee: &provider.Grantee{
            Type: provider.GranteeType_GRANTEE_TYPE_USER,
            Id:   &provider.Grantee_UserId{UserId: &userpb.UserId{OpaqueId: "manager-1", Idp: "idp.example.com"}},
        },
        Permissions: &provider.ResourcePermissions{
            Stat: true, ListContainer: true, InitiateFileDownload: true,
        },
    }}

    _, err := c.finishUpload(ctx, session)

    Expect(err).ToNot(HaveOccurred())
    Expect(session.SpaceOwner().GetOpaqueId()).To(Equal("manager-1"))
})

Note: existing tests in coordinator_test.go, put_test.go, and tus_adapter_test.go that use new-file sessions will need touchedOwner set to a non-nil, non-SPACE_OWNER value in their BeforeEach, otherwise the fix causes them to hit ListGrants and fail their call-order assertions.

Bug 2 — PermissionDenied falls through FinishUpload, tusd answers 500

File: pkg/upload/tus_adapter.go, FinishUpload (~line 58)

Root cause: The error-mapping switch has no case errtypes.IsPermissionDenied. A revoked share mid-upload hits the default branch; tusd returns a bare 500 with no machine-readable error code instead of 403.

Fix — add one case before default:

case errtypes.IsPermissionDenied:
    return tusd.NewError("ERR_PERMISSION_DENIED", err.Error(), http.StatusForbidden)

Test to add — in tus_adapter_test.go, add one Entry to the existing DescribeTable:

Entry("permission denied", errtypes.PermissionDenied("share was revoked"), "ERR_PERMISSION_DENIED", http.StatusForbidden),

Thanks for both, the TUS one is a real bug, I've pushed your fix and the table entry.
Bug 1: I don't think holds. decomposedfs.TouchFile already resolves the owner before it returns:
spaceOwner := n.SpaceOwnerOrManager(ctx) at decomposedfs.go:863, which is the same grants fallback with the same permission triple as spaceOwnerOrManager in the coordinator. So on a project space touchNode receives a manager, not the placeholder, so nothing left to fall back to. describeExisting needs its fallback because GetMD returns the raw stored owner (node.go:701 → :487), which is where the placeholder does leak through. The asymmetry is the two driver calls behaving differently, not an oversight. The reason your test fails is fakeFS.TouchFile:
it returns f.touchedOwner verbatim (fakefs_test.go:107), so setting it to nil models a driver that skips the resolution decomposedfs does. I tried it with the fake resolving the way the real driver does, and the assertion passes without the fix. Also worth noting the fix is a no-op on a real value: a resolved manager isn't SPACE_OWNER, so the first guard returns it unchanged, which I think is what your note about the other specs needing touchedOwner set is telling us, since it's adding a ListGrants call that wasn't there before.