From 32f98c1e71eaa5cb87a79a7b961bc795ae370778 Mon Sep 17 00:00:00 2001 From: Kilian Boute Date: Tue, 25 Aug 2026 07:10:06 +0000 Subject: [PATCH 1/2] feat(tenancy): resolve devices within trusted scope Add _id-, project-, organisation-, and legacy-owner-scoped device selection. Permit the same non-secret key outside the trusted scope while rejecting ambiguity within it, and deprecate key-only lookup for new ingress paths. --- pkg/tenancy/device.go | 116 ++++++++++++++++++++++++++++++ pkg/tenancy/device_test.go | 144 +++++++++++++++++++++++++++++++++++++ 2 files changed, 260 insertions(+) diff --git a/pkg/tenancy/device.go b/pkg/tenancy/device.go index f801a4a..030632c 100644 --- a/pkg/tenancy/device.go +++ b/pkg/tenancy/device.go @@ -29,6 +29,7 @@ import ( "github.com/uug-ai/database/pkg/database" "github.com/uug-ai/models/pkg/models" "github.com/uug-ai/models/pkg/properties" + "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/bson/primitive" "go.mongodb.org/mongo-driver/mongo" "go.mongodb.org/mongo-driver/mongo/options" @@ -81,6 +82,20 @@ type Ownership struct { ProjectId primitive.ObjectID } +// DeviceLookup is trusted server-side context for selecting one persisted +// device. DeviceId is authoritative when present. Otherwise a project scope +// requires its parent organisation, and organisation scope retains the legacy +// owner fallback for unstamped devices. A key alone is never sufficient. +type DeviceLookup struct { + DeviceId primitive.ObjectID + DeviceKey string + OrganisationId primitive.ObjectID + ProjectId primitive.ObjectID + LegacyOwnerId primitive.ObjectID +} + +var ErrDeviceScopeRequired = errors.New("trusted device scope is required") + // DeviceResolver resolves device ownership against one database. type DeviceResolver struct { db *database.Database @@ -122,6 +137,9 @@ func DeviceProjection() *database.Projection { } // LoadDevice reads the ownership fields of exactly one source device by its key. +// +// Deprecated: key-only lookup cannot distinguish the same non-secret key in +// different projects. New ingress paths must use LoadScopedDevice. func (r *DeviceResolver) LoadDevice(ctx context.Context, deviceKey string) (models.Device, error) { if r == nil || r.db == nil || r.db.Client == nil { return models.Device{}, fmt.Errorf("source device %q lookup requires a database", deviceKey) @@ -155,6 +173,104 @@ func (r *DeviceResolver) LoadDevice(ctx context.Context, deviceKey string) (mode return device, nil } +// LoadScopedDevice selects exactly one device within trusted server-side +// identity. It permits the same non-secret device key in other projects while +// still rejecting duplicates inside the selected scope. +func (r *DeviceResolver) LoadScopedDevice(ctx context.Context, lookup DeviceLookup) (models.Device, error) { + if r == nil || r.db == nil || r.db.Client == nil { + return models.Device{}, fmt.Errorf("source device %q lookup requires a database", lookup.DeviceKey) + } + if lookup.DeviceKey == "" { + return models.Device{}, fmt.Errorf("%w: device key is empty", ErrDeviceScopeRequired) + } + + filter, err := scopedDeviceFilter(lookup) + if err != nil { + return models.Device{}, err + } + + databaseCtx, cancel := context.WithTimeout(ctx, r.db.Client.GetTimeout()) + defer cancel() + + var devices []models.Device + err = r.db.Client.Find( + databaseCtx, + r.collections.Database, + r.collections.Devices, + filter, + DeviceProjection(), + options.Find().SetLimit(2), + ).All(&devices) + if err != nil { + return models.Device{}, fmt.Errorf("find scoped source device %q: %w", lookup.DeviceKey, err) + } + if len(devices) == 0 { + return models.Device{}, fmt.Errorf("find scoped source device %q: %w", lookup.DeviceKey, mongo.ErrNoDocuments) + } + if len(devices) > 1 { + return models.Device{}, fmt.Errorf("source device key %q resolves to multiple persisted devices inside its trusted scope", lookup.DeviceKey) + } + if devices[0].Id.IsZero() { + return models.Device{}, fmt.Errorf("source device %q has no persisted identity", lookup.DeviceKey) + } + return devices[0], nil +} + +func scopedDeviceFilter(lookup DeviceLookup) (bson.M, error) { + if !lookup.DeviceId.IsZero() { + return bson.M{ + properties.DeviceId: lookup.DeviceId, + properties.DeviceKey: lookup.DeviceKey, + }, nil + } + + legacyOwnerId := lookup.LegacyOwnerId + if legacyOwnerId.IsZero() { + legacyOwnerId = lookup.OrganisationId + } + + if !lookup.ProjectId.IsZero() { + if lookup.OrganisationId.IsZero() { + return nil, fmt.Errorf("%w: project scope has no parent organisation", ErrDeviceScopeRequired) + } + filter := database.ScopeWithProject( + lookup.OrganisationId.Hex(), + properties.DeviceUserId, + legacyOwnerId.Hex(), + lookup.ProjectId, + ) + for _, arm := range filter["$or"].([]bson.M) { + arm[properties.DeviceKey] = lookup.DeviceKey + } + return filter, nil + } + + if !lookup.OrganisationId.IsZero() { + filter := database.CanonicalFirstOwnership( + lookup.OrganisationId.Hex(), + properties.DeviceUserId, + legacyOwnerId.Hex(), + ) + for _, arm := range filter["$or"].([]bson.M) { + arm[properties.DeviceKey] = lookup.DeviceKey + } + return filter, nil + } + + if !lookup.LegacyOwnerId.IsZero() { + filter := bson.M{ + properties.DeviceKey: lookup.DeviceKey, + properties.DeviceUserId: lookup.LegacyOwnerId.Hex(), + } + for field, test := range database.MissingCanonicalOrganisation() { + filter[field] = test + } + return filter, nil + } + + return nil, fmt.Errorf("%w for key %q", ErrDeviceScopeRequired, lookup.DeviceKey) +} + // ResolveDeviceByKey loads a device and resolves its ownership in one step. func (r *DeviceResolver) ResolveDeviceByKey(ctx context.Context, deviceKey string) (models.Device, Ownership, error) { device, err := r.LoadDevice(ctx, deviceKey) diff --git a/pkg/tenancy/device_test.go b/pkg/tenancy/device_test.go index 1c7a2ee..bc605bf 100644 --- a/pkg/tenancy/device_test.go +++ b/pkg/tenancy/device_test.go @@ -9,6 +9,7 @@ import ( "github.com/uug-ai/database/pkg/database" "github.com/uug-ai/models/pkg/models" "github.com/uug-ai/models/pkg/properties" + "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/bson/primitive" "go.mongodb.org/mongo-driver/mongo" "go.mongodb.org/mongo-driver/mongo/options" @@ -363,6 +364,149 @@ func TestLoadDeviceRejectsAmbiguousKey(t *testing.T) { } } +func TestLoadScopedDeviceAllowsSameKeyInDifferentProjects(t *testing.T) { + resolver, mock := testResolver(Collections{}) + organisationId := primitive.NewObjectID() + projectId := primitive.NewObjectID() + deviceId := primitive.NewObjectID() + mock.QueueFind([]models.Device{{ + Id: deviceId, + Key: "device-1", + OrganisationId: organisationId.Hex(), + ProjectId: &projectId, + }}, nil) + + device, err := resolver.LoadScopedDevice(context.Background(), DeviceLookup{ + DeviceKey: "device-1", + OrganisationId: organisationId, + ProjectId: projectId, + }) + if err != nil { + t.Fatalf("LoadScopedDevice() error = %v", err) + } + if device.Id != deviceId { + t.Fatalf("device id = %s, want %s", device.Id.Hex(), deviceId.Hex()) + } + + filter := mock.FindCalls[0].Filter.(bson.M) + for _, arm := range filter["$or"].([]bson.M) { + if arm[properties.DeviceKey] != "device-1" || arm[properties.DeviceProjectId] != projectId { + t.Fatalf("project-scoped arm = %#v", arm) + } + } +} + +func TestLoadScopedDeviceRequiresTrustedScope(t *testing.T) { + resolver, _ := testResolver(Collections{}) + + _, err := resolver.LoadScopedDevice(context.Background(), DeviceLookup{DeviceKey: "device-1"}) + if !errors.Is(err, ErrDeviceScopeRequired) { + t.Fatalf("LoadScopedDevice() error = %v, want ErrDeviceScopeRequired", err) + } +} + +func TestLoadScopedDeviceScopesCanonicalAndLegacyOrganisationOwnership(t *testing.T) { + resolver, mock := testResolver(Collections{}) + organisationId := primitive.NewObjectID() + legacyOwnerId := primitive.NewObjectID() + deviceId := primitive.NewObjectID() + mock.QueueFind([]models.Device{{Id: deviceId, Key: "device-1"}}, nil) + + _, err := resolver.LoadScopedDevice(context.Background(), DeviceLookup{ + DeviceKey: "device-1", + OrganisationId: organisationId, + LegacyOwnerId: legacyOwnerId, + }) + if err != nil { + t.Fatalf("LoadScopedDevice() error = %v", err) + } + + want := bson.M{"$or": []bson.M{ + {properties.DeviceOrganisationId: organisationId.Hex(), properties.DeviceKey: "device-1"}, + { + properties.DeviceOrganisationId: bson.M{"$in": bson.A{nil, ""}}, + properties.DeviceUserId: legacyOwnerId.Hex(), + properties.DeviceKey: "device-1", + }, + }} + if !reflect.DeepEqual(mock.FindCalls[0].Filter, want) { + t.Fatalf("filter = %#v, want %#v", mock.FindCalls[0].Filter, want) + } +} + +func TestLoadScopedDeviceRejectsDuplicateInsideTrustedProject(t *testing.T) { + resolver, mock := testResolver(Collections{}) + organisationId := primitive.NewObjectID() + projectId := primitive.NewObjectID() + mock.QueueFind([]models.Device{ + {Id: primitive.NewObjectID(), Key: "device-1"}, + {Id: primitive.NewObjectID(), Key: "device-1"}, + }, nil) + + _, err := resolver.LoadScopedDevice(context.Background(), DeviceLookup{ + DeviceKey: "device-1", + OrganisationId: organisationId, + ProjectId: projectId, + }) + if err == nil { + t.Fatal("duplicate devices inside one trusted project must fail closed") + } +} + +func TestLoadScopedDeviceRejectsProjectWithoutOrganisation(t *testing.T) { + resolver, _ := testResolver(Collections{}) + + _, err := resolver.LoadScopedDevice(context.Background(), DeviceLookup{ + DeviceKey: "device-1", + ProjectId: primitive.NewObjectID(), + }) + if !errors.Is(err, ErrDeviceScopeRequired) { + t.Fatalf("LoadScopedDevice() error = %v, want ErrDeviceScopeRequired", err) + } +} + +func TestLoadScopedDeviceUsesOwnerOnlyForCanonicalMissingDevices(t *testing.T) { + resolver, mock := testResolver(Collections{}) + ownerId := primitive.NewObjectID() + mock.QueueFind([]models.Device{{Id: primitive.NewObjectID(), Key: "device-1"}}, nil) + + _, err := resolver.LoadScopedDevice(context.Background(), DeviceLookup{ + DeviceKey: "device-1", + LegacyOwnerId: ownerId, + }) + if err != nil { + t.Fatalf("LoadScopedDevice() error = %v", err) + } + want := bson.M{ + properties.DeviceKey: "device-1", + properties.DeviceUserId: ownerId.Hex(), + properties.DeviceOrganisationId: bson.M{"$in": bson.A{nil, ""}}, + } + if !reflect.DeepEqual(mock.FindCalls[0].Filter, want) { + t.Fatalf("filter = %#v, want %#v", mock.FindCalls[0].Filter, want) + } +} + +func TestLoadScopedDeviceUsesIdBeforeOwnershipScope(t *testing.T) { + resolver, mock := testResolver(Collections{}) + deviceId := primitive.NewObjectID() + mock.QueueFind([]models.Device{{Id: deviceId, Key: "device-1"}}, nil) + + _, err := resolver.LoadScopedDevice(context.Background(), DeviceLookup{ + DeviceId: deviceId, + DeviceKey: "device-1", + OrganisationId: primitive.NewObjectID(), + ProjectId: primitive.NewObjectID(), + }) + if err != nil { + t.Fatalf("LoadScopedDevice() error = %v", err) + } + want := bson.M{properties.DeviceId: deviceId, properties.DeviceKey: "device-1"} + if !reflect.DeepEqual(mock.FindCalls[0].Filter, want) { + t.Fatalf("filter = %#v, want %#v", mock.FindCalls[0].Filter, want) + } +} + func TestResolveDeviceByKeyReturnsDeviceAndOwnership(t *testing.T) { resolver, mock := testResolver(Collections{}) deviceId := primitive.NewObjectID() From 5f2475a202fa448b7d2d7be386342b260e3d4024 Mon Sep 17 00:00:00 2001 From: Kilian Boute Date: Tue, 25 Aug 2026 08:53:19 +0000 Subject: [PATCH 2/2] fix(tenancy): keep legacy devices in owner default Resolve owner-only device user_id deterministically as organisationId = projectId = ownerUserId. Preserve stable sub-user-to-master resolution and never infer a secondary organisation from ownership. --- pkg/tenancy/device.go | 48 +++--------------------------- pkg/tenancy/device_test.go | 60 +++++--------------------------------- 2 files changed, 11 insertions(+), 97 deletions(-) diff --git a/pkg/tenancy/device.go b/pkg/tenancy/device.go index 030632c..d25c950 100644 --- a/pkg/tenancy/device.go +++ b/pkg/tenancy/device.go @@ -108,13 +108,8 @@ func NewDeviceResolver(db *database.Database, collections Collections) *DeviceRe return &DeviceResolver{db: db, collections: collections.withDefaults()} } -// persistedOrganisationOwnership reads only the canonical owner. Unlike the -// resource collections, the organisation document never had a snake_case owner -// field: models.Organisation has always declared bson:"ownerId", and it is the -// only spelling the ownerId_1 index covers. type persistedOrganisationOwnership struct { - Id primitive.ObjectID `bson:"_id"` - OwnerId primitive.ObjectID `bson:"ownerId"` + Id primitive.ObjectID `bson:"_id"` } type persistedUserOwnership struct { @@ -343,22 +338,9 @@ func (r *DeviceResolver) resolveOrganisation(ctx context.Context, device models. } } - organisations, err := r.findOrganisationsByOwner(ctx, ownerId) - if err != nil { - return primitive.NilObjectID, err - } - if len(organisations) == 1 { - return organisations[0].Id, nil - } - if len(organisations) > 1 { - return primitive.NilObjectID, fmt.Errorf("source device %q legacy owner resolves to %d organisations", device.Key, len(organisations)) - } - - // No organisation record exists at all, so organisations-bootstrap has not - // run on this instance. A primary organisation deterministically reuses the - // master user's id, so deriving it here produces exactly the value the - // migration will later persist. Failing instead would drop every message on - // an un-migrated deployment. + // A legacy owner id deterministically denotes that owner's default + // organisation; it never selects one of the owner's secondary organisations. + // This produces exactly the identity organisations-bootstrap materialises. // // The derivation is only meaningful if the owner has a user record, because // bootstrap mints one organisation per master user. An owner with no user @@ -416,25 +398,3 @@ func (r *DeviceResolver) findUserOwnership(ctx context.Context, id primitive.Obj } return user, true, nil } - -func (r *DeviceResolver) findOrganisationsByOwner(ctx context.Context, ownerId primitive.ObjectID) ([]persistedOrganisationOwnership, error) { - databaseCtx, cancel := context.WithTimeout(ctx, r.db.Client.GetTimeout()) - defer cancel() - - // A plain equality rather than an ownership $or: organisations only ever - // stored the canonical owner, so a second arm would add no reachable - // document while costing the ownerId_1 index. An $or is only index-served - // when every arm is index-bounded, and no owner_id index exists to bound - // one — the whole lookup would degrade to a collection scan. - filter := map[string]any{properties.OrganisationOwnerId: ownerId} - var organisations []persistedOrganisationOwnership - if err := r.db.Client.Find(databaseCtx, r.collections.Database, r.collections.Organisations, filter, options.Find().SetLimit(2)).All(&organisations); err != nil { - return nil, fmt.Errorf("find organisations owned by %s: %w", ownerId.Hex(), err) - } - for _, organisation := range organisations { - if organisation.Id.IsZero() { - return nil, fmt.Errorf("organisation owned by %s has no persisted identity", ownerId.Hex()) - } - } - return organisations, nil -} diff --git a/pkg/tenancy/device_test.go b/pkg/tenancy/device_test.go index bc605bf..ba91584 100644 --- a/pkg/tenancy/device_test.go +++ b/pkg/tenancy/device_test.go @@ -128,20 +128,21 @@ func TestResolveDeviceResolvesLegacySubUserThroughMaster(t *testing.T) { }, masterId, masterId) } -func TestResolveDeviceResolvesLegacyOwnerToSingleSecondaryOrganisation(t *testing.T) { +func TestResolveDeviceKeepsLegacyOwnerInDefaultOrganisation(t *testing.T) { resolver, mock := testResolver(Collections{}) ownerId := primitive.NewObjectID() - organisationId := primitive.NewObjectID() mock.QueueFindOne(nil, mongo.ErrNoDocuments) mock.QueueFindOne(map[string]any{"_id": ownerId}, nil) - mock.QueueFind([]persistedOrganisationOwnership{{Id: organisationId, OwnerId: ownerId}}, nil) - // A secondary organisation has an independent id, so the project default - // follows the organisation rather than the owning user. + // An owner-only legacy device belongs to the deterministic default. It must + // never be inferred into one of the owner's secondary organisations. assertOwnership(t, resolver, models.Device{ Key: "device-1", UserId: ownerId.Hex(), - }, organisationId, organisationId) + }, ownerId, ownerId) + if len(mock.FindCalls) != 0 { + t.Fatalf("owner compatibility queried secondary organisations: %#v", mock.FindCalls) + } } func TestResolveDeviceDerivesPrimaryOrganisationBeforeBootstrap(t *testing.T) { @@ -153,7 +154,6 @@ func TestResolveDeviceDerivesPrimaryOrganisationBeforeBootstrap(t *testing.T) { masterId := primitive.NewObjectID() mock.QueueFindOne(nil, mongo.ErrNoDocuments) mock.QueueFindOne(map[string]any{"_id": masterId}, nil) - mock.QueueFind([]persistedOrganisationOwnership{}, nil) assertOwnership(t, resolver, models.Device{ Key: "device-1", @@ -169,7 +169,6 @@ func TestResolveDeviceDerivesSubUserMasterOrganisationBeforeBootstrap(t *testing mock.QueueFindOne(map[string]any{"_id": userId, "user_id": masterId.Hex()}, nil) mock.QueueFindOne(nil, mongo.ErrNoDocuments) mock.QueueFindOne(map[string]any{"_id": masterId}, nil) - mock.QueueFind([]persistedOrganisationOwnership{}, nil) assertOwnership(t, resolver, models.Device{ Key: "device-1", @@ -189,7 +188,6 @@ func TestResolveDeviceRejectsDanglingMasterBeforeBootstrap(t *testing.T) { mock.QueueFindOne(map[string]any{"_id": userId, "user_id": masterId.Hex()}, nil) mock.QueueFindOne(nil, mongo.ErrNoDocuments) mock.QueueFindOne(nil, mongo.ErrNoDocuments) - mock.QueueFind([]persistedOrganisationOwnership{}, nil) assertFailsClosed(t, resolver, models.Device{ Key: "device-1", @@ -204,7 +202,6 @@ func TestResolveDeviceDoesNotRecheckDirectOwnerBeforeBootstrap(t *testing.T) { masterId := primitive.NewObjectID() mock.QueueFindOne(nil, mongo.ErrNoDocuments) mock.QueueFindOne(map[string]any{"_id": masterId}, nil) - mock.QueueFind([]persistedOrganisationOwnership{}, nil) assertOwnership(t, resolver, models.Device{ Key: "device-1", @@ -216,24 +213,10 @@ func TestResolveDeviceDoesNotRecheckDirectOwnerBeforeBootstrap(t *testing.T) { } } -func TestResolveDeviceFailsOnAmbiguousLegacyOwner(t *testing.T) { - resolver, mock := testResolver(Collections{}) - ownerId := primitive.NewObjectID() - mock.QueueFindOne(nil, mongo.ErrNoDocuments) - mock.QueueFindOne(map[string]any{"_id": ownerId}, nil) - mock.QueueFind([]persistedOrganisationOwnership{ - {Id: primitive.NewObjectID(), OwnerId: ownerId}, - {Id: primitive.NewObjectID(), OwnerId: ownerId}, - }, nil) - - assertFailsClosed(t, resolver, models.Device{Key: "device-1", UserId: ownerId.Hex()}, "ambiguous legacy ownership") -} - func TestResolveDeviceRejectsOrphanedLegacyOwner(t *testing.T) { resolver, mock := testResolver(Collections{}) mock.QueueFindOne(nil, mongo.ErrNoDocuments) mock.QueueFindOne(nil, mongo.ErrNoDocuments) - mock.QueueFind([]persistedOrganisationOwnership{}, nil) assertFailsClosed(t, resolver, models.Device{ Key: "device-1", @@ -250,35 +233,6 @@ func TestResolveDeviceRejectsInvalidMasterRelationship(t *testing.T) { assertFailsClosed(t, resolver, models.Device{Key: "device-1", UserId: userId.Hex()}, "invalid master ownership") } -// The organisation lookup must stay a plain canonical equality. An ownership -// $or here would be unreachable — organisations never stored a snake_case owner -// — and would cost the ownerId_1 index, because an $or is only index-served -// when every arm is index-bounded and no owner_id index exists. -func TestResolveDeviceLooksUpOrganisationOwnerByCanonicalFieldOnly(t *testing.T) { - resolver, mock := testResolver(Collections{}) - ownerId := primitive.NewObjectID() - organisationId := primitive.NewObjectID() - mock.QueueFindOne(nil, mongo.ErrNoDocuments) - mock.QueueFindOne(map[string]any{"_id": ownerId}, nil) - mock.QueueFind([]persistedOrganisationOwnership{{Id: organisationId, OwnerId: ownerId}}, nil) - - ownership, err := resolver.ResolveDevice(context.Background(), models.Device{Key: "device-1", UserId: ownerId.Hex()}) - if err != nil { - t.Fatalf("resolve device: %v", err) - } - if ownership.OrganisationId != organisationId { - t.Fatalf("organisation = %s, want %s", ownership.OrganisationId.Hex(), organisationId.Hex()) - } - - if len(mock.FindCalls) != 1 { - t.Fatalf("Find calls = %d, want 1", len(mock.FindCalls)) - } - want := map[string]any{properties.OrganisationOwnerId: ownerId} - if !reflect.DeepEqual(mock.FindCalls[0].Filter, want) { - t.Fatalf("owner filter = %#v, want %#v", mock.FindCalls[0].Filter, want) - } -} - func TestResolveDeviceRejectsMissingOrInvalidOwnership(t *testing.T) { resolver, _ := testResolver(Collections{})