Skip to content

Commit 706a0d8

Browse files
otaviogustavosbarreto
authored andcommitted
fix(api): enforce unique namespace names in postgres
The mongo to postgres migration dropped mongo's unique index on namespaces.name, so duplicate names could coexist. Device lookup resolves a namespace by name and scans a single row, so duplicates made SSH resolve to the wrong tenant and return "device not found". Add migration 004 that renames pre-existing duplicates non-destructively (oldest row per case-insensitive group keeps its name) and creates a unique index on lower(name), and map store.ErrDuplicate to ErrNamespaceDuplicated in both the create and rename service paths. Fixes: #6438
1 parent e786c11 commit 706a0d8

8 files changed

Lines changed: 665 additions & 0 deletions

File tree

api/services/namespace.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,14 @@ func (s *service) CreateNamespace(ctx context.Context, req *requests.NamespaceCr
9494
ns.MaxDevices = -1
9595
}
9696

97+
// The NamespaceConflicts pre-check above is the fast path; store.ErrDuplicate
98+
// here means a concurrent insert raced past it. Map it to ErrNamespaceDuplicated
99+
// so callers get a consistent duplicate signal regardless of timing.
97100
if _, err := s.store.NamespaceCreate(ctx, ns); err != nil {
101+
if errors.Is(err, store.ErrDuplicate) {
102+
return nil, NewErrNamespaceDuplicated(err)
103+
}
104+
98105
return nil, NewErrNamespaceCreateStore(err)
99106
}
100107

@@ -196,7 +203,14 @@ func (s *service) EditNamespace(ctx context.Context, req *requests.NamespaceEdit
196203
namespace.Settings.ConnectionAnnouncement = *req.Settings.ConnectionAnnouncement
197204
}
198205

206+
// NamespaceUpdate returns store.ErrDuplicate when the new name collides with an
207+
// existing namespace. Map it to ErrNamespaceDuplicated so callers get a
208+
// consistent duplicate signal regardless of timing.
199209
if err := s.store.NamespaceUpdate(ctx, namespace); err != nil {
210+
if errors.Is(err, store.ErrDuplicate) {
211+
return nil, NewErrNamespaceDuplicated(err)
212+
}
213+
200214
return nil, err
201215
}
202216

api/services/namespace_test.go

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -653,6 +653,83 @@ func TestCreateNamespace(t *testing.T) {
653653
err: NewErrNamespaceCreateStore(errors.New("error")),
654654
},
655655
},
656+
{
657+
// store.ErrDuplicate from NamespaceCreate means a concurrent insert raced past the
658+
// NamespaceConflicts pre-check (case-sensitive name=? queries rely on lowercased
659+
// writes). The service must map it to ErrNamespaceDuplicated, not ErrNamespaceCreateStore.
660+
description: "fails when NamespaceCreate returns store.ErrDuplicate",
661+
req: &requests.NamespaceCreate{
662+
UserID: "000000000000000000000000",
663+
Name: "namespace",
664+
TenantID: "00000000-0000-4000-0000-000000000000",
665+
},
666+
requiredMocks: func() {
667+
storeMock.
668+
On("UserGetInfo", ctx, "000000000000000000000000").
669+
Return(
670+
&models.UserInfo{
671+
OwnedNamespaces: []models.Namespace{{}},
672+
AssociatedNamespaces: []models.Namespace{},
673+
},
674+
nil,
675+
).
676+
Once()
677+
storeMock.
678+
On("UserResolve", ctx, store.UserIDResolver, "000000000000000000000000").
679+
Return(&models.User{
680+
ID: "000000000000000000000000",
681+
MaxNamespaces: 3,
682+
}, nil).
683+
Once()
684+
storeMock.
685+
On("NamespaceConflicts", ctx, &models.NamespaceConflicts{Name: "namespace"}).
686+
Return(nil, false, nil).
687+
Once()
688+
// envs.IsCommunity() calls SHELLHUB_CLOUD then SHELLHUB_ENTERPRISE.
689+
envMock.
690+
On("Get", "SHELLHUB_CLOUD").
691+
Return("false").
692+
Once()
693+
envMock.
694+
On("Get", "SHELLHUB_ENTERPRISE").
695+
Return("false").
696+
Once()
697+
// envs.IsCloud() calls SHELLHUB_CLOUD once more.
698+
envMock.
699+
On("Get", "SHELLHUB_CLOUD").
700+
Return("false").
701+
Once()
702+
storeMock.
703+
On(
704+
"NamespaceCreate",
705+
ctx,
706+
&models.Namespace{
707+
TenantID: "00000000-0000-4000-0000-000000000000",
708+
Name: "namespace",
709+
Owner: "000000000000000000000000",
710+
Type: models.TypeTeam,
711+
Members: []models.Member{
712+
{
713+
ID: "000000000000000000000000",
714+
Role: authorizer.RoleOwner,
715+
AddedAt: now,
716+
},
717+
},
718+
Settings: &models.NamespaceSettings{
719+
SessionRecord: true,
720+
ConnectionAnnouncement: models.DefaultAnnouncementMessage,
721+
},
722+
MaxDevices: -1,
723+
},
724+
).
725+
Return("", store.ErrDuplicate).
726+
Once()
727+
},
728+
expected: Expected{
729+
ns: nil,
730+
err: NewErrNamespaceDuplicated(store.ErrDuplicate),
731+
},
732+
},
656733
{
657734
description: "succeeds to create a namespace",
658735
req: &requests.NamespaceCreate{
@@ -1106,6 +1183,33 @@ func TestEditNamespace(t *testing.T) {
11061183
errors.New("error"),
11071184
},
11081185
},
1186+
{
1187+
description: "fails with ErrNamespaceDuplicated when store returns ErrDuplicate",
1188+
tenantID: "xxxxx",
1189+
namespaceName: "newname",
1190+
requiredMocks: func() {
1191+
namespace := &models.Namespace{
1192+
TenantID: "xxxxx",
1193+
Name: "oldname",
1194+
Settings: &models.NamespaceSettings{SessionRecord: false},
1195+
}
1196+
storeMock.
1197+
On("NamespaceResolve", ctx, store.NamespaceTenantIDResolver, "xxxxx").
1198+
Return(namespace, nil).
1199+
Once()
1200+
1201+
expectedNamespace := *namespace
1202+
expectedNamespace.Name = "newname"
1203+
storeMock.
1204+
On("NamespaceUpdate", ctx, &expectedNamespace).
1205+
Return(store.ErrDuplicate).
1206+
Once()
1207+
},
1208+
expected: Expected{
1209+
nil,
1210+
NewErrNamespaceDuplicated(store.ErrDuplicate),
1211+
},
1212+
},
11091213
{
11101214
description: "succeeds changing the name to lowercase",
11111215
namespaceName: "newName",
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
DROP INDEX IF EXISTS namespaces_name_unique;
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
-- Step a: deduplicate existing rows non-destructively by renaming duplicates.
2+
-- For each case-insensitive name group the oldest row (by created_at, ties broken
3+
-- by id ASC) keeps its original name; every other row in that group is renamed to
4+
-- "<base>-<first-8-chars-of-id>" where <base> is the lower-cased original name
5+
-- truncated so the full result never exceeds 63 characters.
6+
-- Trailing hyphens are stripped from the base before the suffix is appended, so
7+
-- the resulting name always starts and ends with an alphanumeric character.
8+
-- Re-running this statement is safe (idempotent): rows that were already renamed
9+
-- by a previous run will not match the duplicate-detection predicate again.
10+
WITH winners AS (
11+
-- One winner per lower(name) group: oldest created_at, smallest id on ties.
12+
SELECT DISTINCT ON (lower(name)) id
13+
FROM namespaces
14+
ORDER BY lower(name), created_at ASC, id ASC
15+
),
16+
duplicates AS (
17+
-- Every row that is NOT the winner of its group.
18+
SELECT n.id,
19+
lower(n.name) AS lower_name
20+
FROM namespaces n
21+
WHERE n.id NOT IN (SELECT id FROM winners)
22+
)
23+
UPDATE namespaces n
24+
SET name = (
25+
-- Build "<base>-<suffix>" where suffix = first 8 hex chars of the UUID.
26+
-- Base = lower(name) truncated to at most 54 chars, then right-stripped of
27+
-- hyphens so the final name never starts/ends with '-'.
28+
rtrim(
29+
left(lower(n.name), 54),
30+
'-'
31+
) || '-' || left(replace(n.id::text, '-', ''), 8)
32+
)
33+
FROM duplicates d
34+
WHERE n.id = d.id;
35+
36+
--bun:split
37+
38+
-- Step b: create a case-insensitive unique index on lower(name).
39+
CREATE UNIQUE INDEX namespaces_name_unique ON namespaces USING btree (lower(name));

0 commit comments

Comments
 (0)