Skip to content

Commit ee19464

Browse files
committed
fix(security): scope collection/index/function ownership by database
Owner lookups previously keyed only on tenant_id, so a same-named collection in another database within the same tenant would incorrectly resolve to whichever owner happened to be recorded first. Thread database_id through permission checks, namespace authorization, and ownership DDL so ownership is correctly scoped per database.
1 parent 0c6137a commit ee19464

9 files changed

Lines changed: 268 additions & 54 deletions

File tree

nodedb/src/control/security/permission/check.rs

Lines changed: 227 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,10 @@ use crate::control::security::audit::{AuditEmitContext, AuditEmitter, AuditEvent
99
use crate::control::security::identity::{self, AuthenticatedIdentity, Permission};
1010
use crate::control::security::role::RoleStore;
1111

12-
use crate::types::TenantId;
12+
use crate::types::{DatabaseId, TenantId};
1313

1414
use super::store::PermissionStore;
15-
use super::types::{Grant, collection_target, function_target, tenant_target};
15+
use super::types::{Grant, collection_target, function_target, owner_key, tenant_target};
1616

1717
impl PermissionStore {
1818
/// Does any grant on `target` confer `permission` to this identity —
@@ -83,6 +83,7 @@ impl PermissionStore {
8383
&self,
8484
identity: &AuthenticatedIdentity,
8585
permission: Permission,
86+
database_id: DatabaseId,
8687
collection: &str,
8788
role_store: &RoleStore,
8889
emitter: &dyn AuditEmitter,
@@ -91,11 +92,18 @@ impl PermissionStore {
9192
return true;
9293
}
9394

94-
let target = collection_target(identity.tenant_id, collection);
95-
if self.is_owner(&target, &identity.username) {
95+
if self.is_owner(
96+
"collection",
97+
database_id,
98+
identity.tenant_id,
99+
collection,
100+
&identity.username,
101+
) {
96102
return true;
97103
}
98104

105+
let target = collection_target(identity.tenant_id, collection);
106+
99107
for role in &identity.roles {
100108
if identity::role_grants_permission(role, permission) {
101109
return true;
@@ -139,6 +147,7 @@ impl PermissionStore {
139147
pub fn check_function(
140148
&self,
141149
identity: &AuthenticatedIdentity,
150+
database_id: DatabaseId,
142151
function_name: &str,
143152
role_store: &RoleStore,
144153
emitter: &dyn AuditEmitter,
@@ -147,12 +156,18 @@ impl PermissionStore {
147156
return true;
148157
}
149158

150-
let target = function_target(identity.tenant_id, function_name);
151-
152-
if self.is_owner(&target, &identity.username) {
159+
if self.is_owner(
160+
"function",
161+
database_id,
162+
identity.tenant_id,
163+
function_name,
164+
&identity.username,
165+
) {
153166
return true;
154167
}
155168

169+
let target = function_target(identity.tenant_id, function_name);
170+
156171
for role in &identity.roles {
157172
if identity::role_grants_permission(role, Permission::Execute) {
158173
return true;
@@ -227,16 +242,36 @@ impl PermissionStore {
227242
false
228243
}
229244

230-
/// Lookup helper: is `username` recorded as the owner of `target`?
231-
pub(super) fn is_owner(&self, target: &str, username: &str) -> bool {
245+
/// Lookup helper: is `username` recorded as the owner of the object?
246+
///
247+
/// The owners map is keyed by [`owner_key`] —
248+
/// `{object_type}:{database_id}:{tenant_id}:{object_name}` — which is a
249+
/// different shape from the `{object_type}:{tenant_id}:{object_name}`
250+
/// target strings used for *grants*. The two must not be interchanged:
251+
/// passing a grant target here silently never matches, which reads as
252+
/// "nobody owns anything" rather than as an error.
253+
pub(super) fn is_owner(
254+
&self,
255+
object_type: &str,
256+
database_id: DatabaseId,
257+
tenant_id: TenantId,
258+
object_name: &str,
259+
username: &str,
260+
) -> bool {
261+
let key = owner_key(
262+
object_type,
263+
database_id.as_u64(),
264+
tenant_id.as_u64(),
265+
object_name,
266+
);
232267
let owners = match self.owners.read() {
233268
Ok(o) => o,
234269
Err(p) => {
235270
tracing::error!("owner store lock poisoned — recovering data");
236271
p.into_inner()
237272
}
238273
};
239-
owners.get(target).is_some_and(|o| o == username)
274+
owners.get(&key).is_some_and(|o| o == username)
240275
}
241276
}
242277

@@ -272,7 +307,14 @@ mod tests {
272307
let store = PermissionStore::new();
273308
let roles = RoleStore::new();
274309
let id = identity("admin", vec![], true);
275-
assert!(store.check(&id, Permission::Write, "secret", &roles, NOOP));
310+
assert!(store.check(
311+
&id,
312+
Permission::Write,
313+
DatabaseId::DEFAULT,
314+
"secret",
315+
&roles,
316+
NOOP
317+
));
276318
}
277319

278320
#[test]
@@ -284,9 +326,68 @@ mod tests {
284326
.unwrap();
285327

286328
let id = identity("alice", vec![], false);
287-
assert!(store.check(&id, Permission::Read, "users", &roles, NOOP));
288-
assert!(store.check(&id, Permission::Write, "users", &roles, NOOP));
289-
assert!(store.check(&id, Permission::Drop, "users", &roles, NOOP));
329+
assert!(store.check(
330+
&id,
331+
Permission::Read,
332+
DatabaseId::DEFAULT,
333+
"users",
334+
&roles,
335+
NOOP
336+
));
337+
assert!(store.check(
338+
&id,
339+
Permission::Write,
340+
DatabaseId::DEFAULT,
341+
"users",
342+
&roles,
343+
NOOP
344+
));
345+
assert!(store.check(
346+
&id,
347+
Permission::Drop,
348+
DatabaseId::DEFAULT,
349+
"users",
350+
&roles,
351+
NOOP
352+
));
353+
}
354+
355+
/// Owner rows are keyed by database. A check against the database the
356+
/// row was written to must recognise the owner, and a check against any
357+
/// other database must not — a same-named collection elsewhere belongs
358+
/// to whoever owns it there, not to this user.
359+
#[test]
360+
fn ownership_is_scoped_to_its_database() {
361+
let store = PermissionStore::new();
362+
let roles = RoleStore::new();
363+
let db = DatabaseId::new(7);
364+
store
365+
.set_owner_in_database(
366+
"collection",
367+
db.as_u64(),
368+
TenantId::new(1),
369+
"users",
370+
"alice",
371+
None,
372+
)
373+
.unwrap();
374+
375+
let id = identity("alice", vec![], false);
376+
assert!(
377+
store.check(&id, Permission::Read, db, "users", &roles, NOOP),
378+
"owner must hold implicit permissions in their own database"
379+
);
380+
assert!(
381+
!store.check(
382+
&id,
383+
Permission::Read,
384+
DatabaseId::DEFAULT,
385+
"users",
386+
&roles,
387+
NOOP
388+
),
389+
"ownership must not leak into a same-named collection in another database"
390+
);
290391
}
291392

292393
#[test]
@@ -298,7 +399,14 @@ mod tests {
298399
.unwrap();
299400

300401
let id = identity("bob", vec![], false);
301-
assert!(!store.check(&id, Permission::Write, "users", &roles, NOOP));
402+
assert!(!store.check(
403+
&id,
404+
Permission::Write,
405+
DatabaseId::DEFAULT,
406+
"users",
407+
&roles,
408+
NOOP
409+
));
302410
}
303411

304412
#[test]
@@ -311,8 +419,22 @@ mod tests {
311419
.unwrap();
312420

313421
let id = identity("bob", vec![], false);
314-
assert!(store.check(&id, Permission::Read, "orders", &roles, NOOP));
315-
assert!(!store.check(&id, Permission::Write, "orders", &roles, NOOP));
422+
assert!(store.check(
423+
&id,
424+
Permission::Read,
425+
DatabaseId::DEFAULT,
426+
"orders",
427+
&roles,
428+
NOOP
429+
));
430+
assert!(!store.check(
431+
&id,
432+
Permission::Write,
433+
DatabaseId::DEFAULT,
434+
"orders",
435+
&roles,
436+
NOOP
437+
));
316438
}
317439

318440
#[test]
@@ -325,7 +447,14 @@ mod tests {
325447
.unwrap();
326448

327449
let id = identity("viewer", vec![Role::Custom("readonly".into())], false);
328-
assert!(store.check(&id, Permission::Read, "reports", &roles, NOOP));
450+
assert!(store.check(
451+
&id,
452+
Permission::Read,
453+
DatabaseId::DEFAULT,
454+
"reports",
455+
&roles,
456+
NOOP
457+
));
329458
}
330459

331460
#[test]
@@ -342,7 +471,14 @@ mod tests {
342471
.unwrap();
343472

344473
let id = identity("alice", vec![Role::Custom("analyst".into())], false);
345-
assert!(perm_store.check(&id, Permission::Read, "data", &role_store, NOOP));
474+
assert!(perm_store.check(
475+
&id,
476+
Permission::Read,
477+
DatabaseId::DEFAULT,
478+
"data",
479+
&role_store,
480+
NOOP
481+
));
346482
}
347483

348484
#[test]
@@ -360,17 +496,45 @@ mod tests {
360496

361497
let roles = RoleStore::new();
362498
let id = identity("bob", vec![], false);
363-
assert!(!store.check(&id, Permission::Read, "users", &roles, NOOP));
499+
assert!(!store.check(
500+
&id,
501+
Permission::Read,
502+
DatabaseId::DEFAULT,
503+
"users",
504+
&roles,
505+
NOOP
506+
));
364507
}
365508

366509
#[test]
367510
fn builtin_role_still_works() {
368511
let store = PermissionStore::new();
369512
let roles = RoleStore::new();
370513
let id = identity("writer", vec![Role::ReadWrite], false);
371-
assert!(store.check(&id, Permission::Read, "anything", &roles, NOOP));
372-
assert!(store.check(&id, Permission::Write, "anything", &roles, NOOP));
373-
assert!(!store.check(&id, Permission::Drop, "anything", &roles, NOOP));
514+
assert!(store.check(
515+
&id,
516+
Permission::Read,
517+
DatabaseId::DEFAULT,
518+
"anything",
519+
&roles,
520+
NOOP
521+
));
522+
assert!(store.check(
523+
&id,
524+
Permission::Write,
525+
DatabaseId::DEFAULT,
526+
"anything",
527+
&roles,
528+
NOOP
529+
));
530+
assert!(!store.check(
531+
&id,
532+
Permission::Drop,
533+
DatabaseId::DEFAULT,
534+
"anything",
535+
&roles,
536+
NOOP
537+
));
374538
}
375539

376540
#[test]
@@ -382,7 +546,14 @@ mod tests {
382546
let emitter = CapturingEmitter::new();
383547
let id = identity("eve", vec![], false);
384548

385-
let allowed = store.check(&id, Permission::Write, "secrets", &roles, &emitter);
549+
let allowed = store.check(
550+
&id,
551+
Permission::Write,
552+
DatabaseId::DEFAULT,
553+
"secrets",
554+
&roles,
555+
&emitter,
556+
);
386557
assert!(!allowed);
387558

388559
let recorded = emitter.recorded();
@@ -399,7 +570,14 @@ mod tests {
399570
let emitter = CapturingEmitter::new();
400571
let id = identity("admin", vec![], true);
401572

402-
let allowed = store.check(&id, Permission::Write, "anything", &roles, &emitter);
573+
let allowed = store.check(
574+
&id,
575+
Permission::Write,
576+
DatabaseId::DEFAULT,
577+
"anything",
578+
&roles,
579+
&emitter,
580+
);
403581
assert!(allowed);
404582
assert!(emitter.recorded().is_empty());
405583
}
@@ -417,10 +595,31 @@ mod tests {
417595
let id = identity("bob", vec![], false);
418596
// A tenant-wide grant confers the permission on any collection in
419597
// the tenant, with no per-collection grant.
420-
assert!(store.check(&id, Permission::Read, "orders", &roles, NOOP));
421-
assert!(store.check(&id, Permission::Read, "invoices", &roles, NOOP));
598+
assert!(store.check(
599+
&id,
600+
Permission::Read,
601+
DatabaseId::DEFAULT,
602+
"orders",
603+
&roles,
604+
NOOP
605+
));
606+
assert!(store.check(
607+
&id,
608+
Permission::Read,
609+
DatabaseId::DEFAULT,
610+
"invoices",
611+
&roles,
612+
NOOP
613+
));
422614
// It does not widen to permissions that were not granted.
423-
assert!(!store.check(&id, Permission::Write, "orders", &roles, NOOP));
615+
assert!(!store.check(
616+
&id,
617+
Permission::Write,
618+
DatabaseId::DEFAULT,
619+
"orders",
620+
&roles,
621+
NOOP
622+
));
424623
}
425624

426625
#[test]

0 commit comments

Comments
 (0)