Skip to content

Commit 8fc941a

Browse files
committed
fix(unified_bridge): authorize against canonical entity type, not bridge alias
Codex P2 review on PR #363 caught a real semantic bug: authorize_read / authorize_write / authorize_act were passing the bridge-side `public_name` to Policy::evaluate instead of the canonical OGIT entity type that `bridge.entity()` had just resolved to. Concrete failure mode (per Codex's example): WoaBridge maps public name "WorkOrder" → canonical `ogit.WorkOrder:Order`. A policy granting access to "Order" (canonical) was being evaluated against "WorkOrder" (alias) and denying access. Conversely, alias-specific grants could diverge from canonical policy in either direction. Fix: - Switch from `bridge.entity(public_name)` to `bridge.row(public_name)` in all three authorize_* methods. `row()` returns a `MappingRow` carrying the canonical OGIT URI in `row.ogit_uri`; the local-name part (`OgitUri::name()`) is the canonical entity type. - New `canonical_entity_type(row, public_name)` helper extracts the canonical name; falls back to `public_name` if the URI somehow lacks a name part (defensive — a malformed URI shouldn't break authorization). - EntityRef constructed directly from `row.schema_ptr` (same pointer `bridge.entity()` would have returned). Cost: `bridge.row()` calls `bridge.entity()` internally and then enumerates the namespace to find the row — O(n) over rows in the namespace, n ~50 typical. Acceptable on the auth-decision path; the sub-microsecond hot path is the OwlIdentity bitmask predicate one layer down (DataFusion plan-rewrite via PolicyRewriter), which this function gates but does not run on every row. Two new regression tests: - `unified_bridge_evaluates_policy_against_canonical_entity_type` — policy keyed on "Order" allows the call when caller uses alias "WorkOrder" - `unified_bridge_does_not_honor_alias_keyed_policy` — inverse: a policy keyed on the alias "WorkOrder" does NOT grant access through the canonical-name evaluation path (proves the decoupling) Both tests use a real OntologyRegistry seeded with a single MappingProposal where public_name ("WorkOrder") differs from ogit_uri.name() ("Order"). 6/6 unified_bridge tests pass. Architectural note: this means a Policy authored against canonical OGIT entity types is honored regardless of which bridge / public alias the caller used. Consumer-facing aliases stay decoupled from policy authorship — Foundry-parity-style tenant policies write canonical OGIT names once and any bridge that resolves to them honors the grant. Module docs (//! header on authorize_read) call this out so future readers don't re-introduce the bug. Refs: PR #363 review by chatgpt-codex-connector
1 parent be472bf commit 8fc941a

1 file changed

Lines changed: 133 additions & 9 deletions

File tree

crates/lance-graph-callcenter/src/unified_bridge.rs

Lines changed: 133 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,23 @@ use std::sync::Arc;
3838

3939
use lance_graph_contract::property::PrefetchDepth;
4040
use lance_graph_ontology::bridge::{BridgeError, EntityRef, NamespaceBridge};
41+
use lance_graph_ontology::proposal::MappingRow;
4142
use lance_graph_rbac::access::AccessDecision;
4243
use lance_graph_rbac::policy::{Operation, Policy};
4344

45+
/// Extract the canonical ontology entity type name from a resolved
46+
/// [`MappingRow`], for use as the [`Policy::evaluate`] key.
47+
///
48+
/// `row.ogit_uri` carries the canonical OGIT URI (e.g.
49+
/// `ogit.WorkOrder:Order`); `OgitUri::name()` is the local part
50+
/// (`Order`). Falls back to `public_name` if the URI somehow lacks a
51+
/// name part — a malformed URI shouldn't silently bypass the alias
52+
/// resolution, but it shouldn't break authorization either.
53+
fn canonical_entity_type<'a>(row: &'a MappingRow, public_name: &'a str) -> &'a str
54+
{
55+
row.ogit_uri.name().unwrap_or(public_name)
56+
}
57+
4458
// ═══════════════════════════════════════════════════════════════════════════
4559
// OgitFamily — Level-2 basin pointer (§3.1 of super-domain-rbac-tenancy-v1)
4660
// ═══════════════════════════════════════════════════════════════════════════
@@ -240,36 +254,50 @@ impl<B: NamespaceBridge> UnifiedBridge<B> {
240254
/// Resolve `public_name` through the bridge then evaluate read access at
241255
/// `depth`. Returns the resolved `EntityRef` on `Allow`, `AuthError` on
242256
/// `Deny` / `Escalate` / bridge resolution failure.
257+
///
258+
/// **Policy evaluation keys on the canonical ontology entity type
259+
/// (e.g. `Order` for `ogit.WorkOrder:Order`), not the bridge-side
260+
/// `public_name` alias.** This means a `Policy` authored against
261+
/// canonical OGIT names is honored regardless of which bridge / public
262+
/// alias the caller used, and consumer-facing aliases stay decoupled
263+
/// from policy authorship.
243264
pub fn authorize_read(
244265
&self,
245266
public_name: &str,
246267
depth: PrefetchDepth,
247268
) -> Result<EntityRef, AuthError> {
248-
let entity = self.bridge.entity(public_name)?;
249-
self.evaluate(public_name, Operation::Read { depth })?;
250-
Ok(entity)
269+
let row = self.bridge.row(public_name)?;
270+
let canonical = canonical_entity_type(&row, public_name);
271+
self.evaluate(canonical, Operation::Read { depth })?;
272+
Ok(EntityRef { schema_ptr: row.schema_ptr })
251273
}
252274

253275
/// Resolve `public_name` then evaluate write access on `predicate`.
276+
/// Policy keys on the canonical ontology entity type — see
277+
/// [`Self::authorize_read`] for details.
254278
pub fn authorize_write(
255279
&self,
256280
public_name: &str,
257281
predicate: &str,
258282
) -> Result<EntityRef, AuthError> {
259-
let entity = self.bridge.entity(public_name)?;
260-
self.evaluate(public_name, Operation::Write { predicate })?;
261-
Ok(entity)
283+
let row = self.bridge.row(public_name)?;
284+
let canonical = canonical_entity_type(&row, public_name);
285+
self.evaluate(canonical, Operation::Write { predicate })?;
286+
Ok(EntityRef { schema_ptr: row.schema_ptr })
262287
}
263288

264289
/// Resolve `public_name` then evaluate action access on `action`.
290+
/// Policy keys on the canonical ontology entity type — see
291+
/// [`Self::authorize_read`] for details.
265292
pub fn authorize_act(
266293
&self,
267294
public_name: &str,
268295
action: &str,
269296
) -> Result<EntityRef, AuthError> {
270-
let entity = self.bridge.entity(public_name)?;
271-
self.evaluate(public_name, Operation::Act { action })?;
272-
Ok(entity)
297+
let row = self.bridge.row(public_name)?;
298+
let canonical = canonical_entity_type(&row, public_name);
299+
self.evaluate(canonical, Operation::Act { action })?;
300+
Ok(EntityRef { schema_ptr: row.schema_ptr })
273301
}
274302

275303
fn evaluate(&self, entity_type: &str, op: Operation<'_>) -> Result<(), AuthError> {
@@ -348,4 +376,100 @@ mod tests {
348376
.unwrap_err();
349377
assert!(matches!(err, AuthError::Bridge(_)));
350378
}
379+
380+
// ── Alias vs canonical-name resolution (Codex P2 review fix) ──────────────
381+
382+
/// Bridge that locks to a real namespace + uses a real registry, so
383+
/// `bridge.row()` resolves and returns a `MappingRow` carrying the
384+
/// canonical OGIT URI. Used by the alias-resolution tests below.
385+
struct WoaLikeBridge {
386+
registry: Arc<OntologyRegistry>,
387+
g_lock: lance_graph_ontology::namespace::NamespaceId,
388+
}
389+
390+
impl NamespaceBridge for WoaLikeBridge {
391+
fn bridge_id(&self) -> &'static str {
392+
"woa"
393+
}
394+
fn registry(&self) -> &OntologyRegistry {
395+
&self.registry
396+
}
397+
fn g_lock(&self) -> lance_graph_ontology::namespace::NamespaceId {
398+
self.g_lock
399+
}
400+
}
401+
402+
/// Build a registry with a single mapping where `public_name` ("WorkOrder",
403+
/// the bridge-side alias) differs from the canonical OGIT URI's name part
404+
/// ("Order"). Returns the bridge that resolves it.
405+
fn alias_test_bridge() -> Arc<WoaLikeBridge> {
406+
use lance_graph_contract::property::{Marking, Schema};
407+
use lance_graph_ontology::namespace::OgitUri;
408+
use lance_graph_ontology::proposal::{MappingProposal, MappingProposalKind};
409+
410+
let registry = Arc::new(OntologyRegistry::new_in_memory());
411+
let canonical_uri = OgitUri::parse("ogit.WorkOrder:Order").unwrap();
412+
let proposal = MappingProposal {
413+
public_name: "WorkOrder".to_string(),
414+
bridge_id: "woa".to_string(),
415+
ogit_uri: canonical_uri,
416+
namespace: "WorkOrder".to_string(),
417+
kind: MappingProposalKind::Entity {
418+
schema: Schema::builder("Order").required("id").build(),
419+
},
420+
marking: Marking::Internal,
421+
confidence: 1.0,
422+
source_uri: "test://woa-alias".to_string(),
423+
checksum: "checksum-woa-alias".to_string(),
424+
created_by: "test".to_string(),
425+
};
426+
registry.append_mapping(proposal).unwrap();
427+
let g_lock = registry.namespace_id("WorkOrder").unwrap();
428+
Arc::new(WoaLikeBridge { registry, g_lock })
429+
}
430+
431+
fn policy_with_role(role_name: &'static str, entity_type: &'static str) -> Policy {
432+
use lance_graph_rbac::permission::PermissionSpec;
433+
use lance_graph_rbac::role::Role;
434+
Policy::new("alias-test")
435+
.with_role(
436+
Role::new(role_name)
437+
.with_permission(PermissionSpec::read_at(entity_type, PrefetchDepth::Detail)),
438+
)
439+
}
440+
441+
#[test]
442+
fn unified_bridge_evaluates_policy_against_canonical_entity_type() {
443+
// Caller invokes `authorize_read("WorkOrder", ...)` — the bridge-side
444+
// alias. Policy is authored against the canonical OGIT entity type
445+
// "Order" (e.g. shared across multiple bridges that all resolve to
446+
// the same canonical type). The fix: policy must see "Order", not
447+
// "WorkOrder".
448+
let bridge = alias_test_bridge();
449+
let policy = Arc::new(policy_with_role("clerk", "Order"));
450+
let unified = UnifiedBridge::new(bridge, policy, "clerk", TenantId(1));
451+
452+
let entity = unified
453+
.authorize_read("WorkOrder", PrefetchDepth::Detail)
454+
.expect("policy keyed on canonical 'Order' should grant alias 'WorkOrder' access");
455+
assert_eq!(entity.schema_ptr.namespace_id().raw(), 1, "g-lock honored");
456+
}
457+
458+
#[test]
459+
fn unified_bridge_does_not_honor_alias_keyed_policy() {
460+
// Inverse case: a policy keyed on the bridge-side alias "WorkOrder"
461+
// does NOT grant access through the canonical-name evaluation path.
462+
// This is the deliberate decoupling — consumer-facing aliases stay
463+
// separate from policy authorship; policy authors write canonical
464+
// OGIT names once and any bridge that resolves to them honors the
465+
// grant.
466+
let bridge = alias_test_bridge();
467+
let policy = Arc::new(policy_with_role("clerk", "WorkOrder"));
468+
let unified = UnifiedBridge::new(bridge, policy, "clerk", TenantId(1));
469+
470+
let err = unified
471+
.authorize_read("WorkOrder", PrefetchDepth::Detail)
472+
.expect_err("policy keyed on alias 'WorkOrder' should NOT grant access; canonical is 'Order'");
473+
assert!(matches!(err, AuthError::Denied(_)), "got: {err:?}");
474+
}
351475
}

0 commit comments

Comments
 (0)