#2304 Add namespace access control at the gateway level#2348
#2304 Add namespace access control at the gateway level#2348hu-ahmed wants to merge 1 commit intoeclipse-ditto:masterfrom
Conversation
8d8ffdb to
af17314
Compare
af17314 to
89f46cc
Compare
thjaeckle
left a comment
There was a problem hiding this comment.
Review of PR #2348 — Namespace Access Control at the Gateway Level
Thanks for this contribution! The architecture is solid — condition evaluation via the existing placeholder/expression resolver system, clean separation of pattern matching and validation, and proper integration at all gateway entry points (HTTP routes, WebSocket, SSE). All core requirements from #2304 are addressed.
Below are the findings from the review, ordered by severity.
Bug
1. Missing Bearer prefix check in NamespaceAccessValidatorFactory.extractJwtFromHeaders
In NamespaceAccessValidatorFactory.java lines 92–94:
private static Optional<JsonWebToken> extractJwtFromHeaders(final DittoHeaders dittoHeaders) {
return Optional.ofNullable(dittoHeaders.get(DittoHeaderDefinition.AUTHORIZATION.getKey()))
.map(ImmutableJsonWebToken::fromAuthorization);
}If the authorization header contains a non-JWT value (e.g. Basic auth or a malformed value), ImmutableJsonWebToken.fromAuthorization will throw JwtInvalidException since it expects exactly "scheme token" format.
Compare with NamespaceAccessEnforcementDirective.extractJwtFromRequest which correctly filters:
.filter(value -> value.toLowerCase().startsWith("bearer "))This factory method is used by ThingSearchRoute and StreamingSessionActor — both code paths would fail on non-Bearer auth.
Code Issues
2. @Nullable on Optional return type
NamespaceAccessEnforcementDirective.java line 301–302:
@Nullable
private static Optional<JsonWebToken> extractJwtFromRequest(final RequestContext ctx) {@Nullable on an Optional return type is contradictory — an Optional should never be null. The annotation should be removed.
3. @since version should be 3.9.0
NamespaceNotAccessibleException.java line 107 has @since 3.8.0. Since this is in the gateway-api module (public API), the tag should be @since 3.9.0.
Design Suggestions
4. Fake ThingId construction for namespace validation
In ThingsRoute.java (lines 541–543, 340–341) and ThingSearchRoute.java:
final ThingId tempThingId = ThingId.of(namespace + ":temp");
namespaceAccessDirective.validateNamespaceAccessForEntityId(ctx, dittoHeaders, tempThingId);Constructing synthetic ThingIds just to extract the namespace is a workaround. The validator already has isNamespaceAccessible(String namespace). It would be cleaner if the directive also offered a validateNamespaceAccess(RequestContext ctx, DittoHeaders dittoHeaders, String namespace) method directly, avoiding the need for fake entity IDs.
5. Performance: NamespacePatternMatcher re-created on every call
NamespaceAccessValidator.isNamespaceAccessible creates a new NamespacePatternMatcher (and re-compiles regex patterns from LikeHelper.convertToRegexSyntax + Pattern.compile) for each config entry on every single call. Since the config is immutable, the matchers could be pre-built once in the NamespaceAccessValidator constructor and stored as a List<NamespacePatternMatcher>.
Similarly, StreamingSessionActor.isNamespaceAccessible creates a new NamespaceAccessValidator (and thus a new ExpressionResolver) per signal. For high-throughput event streams this could add noticeable overhead. Consider caching the validator per session (it only needs to change when the JWT refreshes).
6. Inconsistent JWT extraction — dual code paths
ThingSearchRoute receives NamespaceAccessValidatorFactory directly, while ThingsRoute and PoliciesRoute receive NamespaceAccessEnforcementDirective. This makes sense functionally (search needs filterAllowedNamespaces/getApplicableNamespacePatterns, not entity-level blocking), but it means JWT extraction logic is duplicated:
NamespaceAccessEnforcementDirective.extractJwtFromRequest— correctly filters for"bearer "prefixNamespaceAccessValidatorFactory.extractJwtFromHeaders— no Bearer check (see bug #1)
Consider unifying the JWT extraction into a single shared utility method to avoid this kind of divergence.
Functional Gap
7. Wildcard patterns not injected into search queries
The issue explicitly raises this as an OPEN question. The current getApplicableNamespacePatterns() silently returns Optional.empty() when only wildcard allowed-namespaces are configured. This means a search with no explicit namespaces parameter will not be restricted when the config uses patterns like "org.eclipse.*".
This is likely the most common configuration pattern, so search results would not be restricted by namespace access control at all in that case. Worth documenting this limitation explicitly (e.g. in the HOCON config comments), or addressing as a follow-up.
Additional Observations
8. Config supports list of rules (undocumented)
The implementation supports multiple namespace-access rules (list of objects with OR semantics across rules). The issue only describes a single config block. This is a reasonable extension, but the HOCON example in gateway.conf only shows the single-object format — consider adding a comment or example showing the list variant and explaining the OR semantics across rules.
Test Coverage
Good coverage overall (NamespacePatternMatcherTest: 8 tests, NamespaceAccessValidatorTest: 16 tests, DefaultNamespaceAccessConfigTest: 6 tests, StreamingSessionActorTest: 2 new tests). Some gaps worth considering:
- No tests for the error case when
extractJwtFromHeadersencounters non-Bearer auth (relates to bug #1) - No tests for
ThingSearchRoute.applyNamespaceAccessControlbehavior - No tests for SSE/WebSocket event filtering with namespace access control enabled
Resolves: #2304