Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds HTTP session management support to QuestDB by introducing a new session store interface with implementations, refactoring security context creation to use a PrincipalContext abstraction, refining clock types from generic Clock to specific NanosecondClock/MicrosecondClock, and simplifying HttpServer wiring by moving cookie and header handling into the connection context factory. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HttpServer
participant HttpConnectionContext
participant SessionStore
participant Authenticator
participant SecurityContextFactory
Client->>HttpServer: HTTP request (auth header)
HttpServer->>HttpConnectionContext: handleClientRecv()
HttpConnectionContext->>HttpConnectionContext: parseCookies()
alt Session Cookie Present
HttpConnectionContext->>SessionStore: verifySessionId()
SessionStore-->>HttpConnectionContext: SessionInfo or null
else No Session Cookie
HttpConnectionContext->>Authenticator: authenticate()
Authenticator-->>HttpConnectionContext: PrincipalContext
end
HttpConnectionContext->>SecurityContextFactory: getInstance(PrincipalContext)
SecurityContextFactory-->>HttpConnectionContext: SecurityContext
HttpConnectionContext->>HttpConnectionContext: configureSecurityContext()
alt Cookies Enabled and No Valid Session
HttpConnectionContext->>SessionStore: createSession(PrincipalContext)
SessionStore-->>HttpConnectionContext: sessionId
HttpConnectionContext->>HttpConnectionContext: setSessionCookie(sessionId)
end
HttpConnectionContext-->>Client: HTTP response with session cookie
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Reasoning: The diff spans 50+ files with significant heterogeneity. Changes include:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (47)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.39.6)core/src/main/java/io/questdb/PropServerConfiguration.javaThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
glasstiger
added
the
DO NOT MERGE
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
utils/src/test/java/io/questdb/cliutil/Table2IlpTest.java (1)
199-207: Null‑safe teardown for httpServercreateHttpServer() can return null when HTTP is disabled; tearDown should guard against NPE.
Apply:
- httpServer.close(); + if (httpServer != null) { + httpServer.close(); + }core/src/main/java/io/questdb/cutlass/http/HttpResponseSink.java (1)
604-608: Sanitize cookie name/value to prevent response-splitting/header injection.Cookie fields are written directly to headers without CR/LF checks. If any value contains
\ror\n, a client-controlled header/body split is possible.Apply a central sanitizer and reuse it for both emitters:
@@ public class HttpResponseHeaderImpl implements Utf8Sink, HttpResponseHeader, Mutable { - public void setCookie(CharSequence name, CharSequence value) { + public void setCookie(CharSequence name, CharSequence value) { if (cookiesEnabled) { - put(HEADER_SET_COOKIE).putAscii(": ").put(name).putAscii(COOKIE_VALUE_SEPARATOR).put(value).putEOL(); + final CharSequence safeName = sanitizeCookieToken(name); + final CharSequence safeValue = sanitizeCookieValue(value); + put(HEADER_SET_COOKIE).putAscii(": ").put(safeName).putAscii(COOKIE_VALUE_SEPARATOR).put(safeValue).putEOL(); } } @@ public class SimpleResponseImpl { - private void setCookie(CharSequence name, CharSequence value) { - if (cookiesEnabled) { - headerImpl.put(HEADER_SET_COOKIE).putAscii(": ").put(name).putAscii(COOKIE_VALUE_SEPARATOR).put(!isBlank(value) ? value : "").putEOL(); - } - } + private void setCookie(CharSequence name, CharSequence value) { + // Delegate to a single implementation to keep behavior consistent + headerImpl.setCookie(name, !isBlank(value) ? value : ""); + }Add helpers near HttpResponseSink class scope:
+ private static CharSequence sanitizeCookieToken(CharSequence cs) { + if (cs == null) return ""; + for (int i = 0, n = cs.length(); i < n; i++) { + char c = cs.charAt(i); + if (c <= 0x1F || c == 0x7F || c == '\r' || c == '\n' || c == ';') { + throw new IllegalArgumentException("Invalid cookie token character"); + } + } + return cs; + } + + private static CharSequence sanitizeCookieValue(CharSequence cs) { + if (cs == null) return ""; + for (int i = 0, n = cs.length(); i < n; i++) { + char c = cs.charAt(i); + if (c == '\r' || c == '\n') { + throw new IllegalArgumentException("Invalid cookie value character"); + } + } + return cs; + }Also applies to: 842-846
🧹 Nitpick comments (11)
core/src/main/java/io/questdb/cutlass/http/PrincipalContext.java (1)
1-11: Add documentation and nullability annotations.This new public interface lacks Javadoc and nullability annotations. Consider adding:
- Interface-level Javadoc explaining the purpose and usage
- Method-level documentation, especially for
getAuthType()to clarify expected values@NotNullor@Nullableannotations on return types per the project's usage oforg.jetbrains:annotations:17.0.0Example documentation:
package io.questdb.cutlass.http; import io.questdb.std.ObjList; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +/** + * Represents an authenticated principal with associated metadata. + * Used for session management and security context creation. + */ public interface PrincipalContext { + /** + * @return the authentication type identifier + */ byte getAuthType(); + /** + * @return the list of groups this principal belongs to, or null if none + */ + @Nullable ObjList<CharSequence> getGroups(); + /** + * @return the principal name/identifier + */ + @NotNull CharSequence getPrincipal(); }core/src/main/java/io/questdb/cutlass/http/TokenGenerator.java (1)
1-5: Add documentation and nullability annotations.This new interface lacks documentation and nullability annotations. Consider adding:
- Interface-level Javadoc explaining the purpose (e.g., generates session tokens)
@NotNullannotation on the return type- Copyright header consistent with other project files
Example:
+/******************************************************************************* + * ___ _ ____ ____ + * / _ \ _ _ ___ ___| |_| _ \| __ ) + * | | | | | | |/ _ \/ __| __| | | | _ \ + * | |_| | |_| | __/\__ \ |_| |_| | |_) | + * \__\_\\__,_|\___||___/\__|____/|____/ + * + * Copyright (c) 2014-2019 Appsicle + * Copyright (c) 2019-2024 QuestDB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + ******************************************************************************/ + package io.questdb.cutlass.http; +import org.jetbrains.annotations.NotNull; + +/** + * Generates unique tokens for HTTP session management. + */ public interface TokenGenerator { + /** + * @return a newly generated unique token + */ + @NotNull CharSequence newToken(); }core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessorState.java (1)
1025-1025: Good refactor: preferisEmpty()overlength() == 0.This change improves code readability by using the more idiomatic
isEmpty()method to check for empty strings.core/src/main/java/io/questdb/cutlass/line/tcp/LineTcpConnectionContext.java (1)
53-53: Consider enum for dummy principal context.The
DummyPrincipalContextis a stateless, immutable singleton. Consider using an enum instead of a static inner class for better clarity and guaranteed singleton semantics:- private static final DummyPrincipalContext DUMMY_CONTEXT = new DummyPrincipalContext(); + private static final PrincipalContext DUMMY_CONTEXT = DummyPrincipal.INSTANCE; - private static class DummyPrincipalContext implements PrincipalContext { + private enum DummyPrincipal implements PrincipalContext { + INSTANCE; + @Override public byte getAuthType() { return SecurityContext.AUTH_TYPE_NONE; } - + @Override public ObjList<CharSequence> getGroups() { return null; } - + @Override public CharSequence getPrincipal() { return null; } }Also applies to: 427-442
core/src/main/java/io/questdb/cairo/security/ReadOnlySecurityContextFactory.java (1)
35-37: LGTM: signature aligned with new factory APIMethod now matches the PrincipalContext‑based contract and preserves behavior.
If IDE flags the unused parameter, consider
@SuppressWarnings("unused")on the method.core/src/main/java/io/questdb/cutlass/pgwire/PGConnectionContext.java (1)
174-176: Constructor contract: enforce non‑null tasCachetasCache is required everywhere. Add a non‑null check to fail fast and consider annotating for static analysis.
Apply:
- private final AssociativeCache<TypesAndSelect> tasCache; + private final AssociativeCache<TypesAndSelect> tasCache; @@ - AssociativeCache<TypesAndSelect> tasCache + AssociativeCache<TypesAndSelect> tasCache ) { @@ - this.tasCache = tasCache; + this.tasCache = java.util.Objects.requireNonNull(tasCache, "tasCache");Also applies to: 205-210
core/src/main/java/io/questdb/cutlass/http/HttpCookieHandler.java (1)
29-41: Specify non‑null contract for session resultMake the non‑null sentinel explicit to prevent null handling bugs, and document intent for implementors.
Apply:
package io.questdb.cutlass.http; import io.questdb.cairo.SecurityContext; +import org.jetbrains.annotations.NotNull; public interface HttpCookieHandler { boolean parseCookies(HttpConnectionContext context); boolean processServiceAccountCookie(HttpConnectionContext context, SecurityContext securityContext); - HttpSessionStore.SessionInfo processSessionCookie(HttpConnectionContext context); + @NotNull HttpSessionStore.SessionInfo processSessionCookie(HttpConnectionContext context); default void setServiceAccountCookie(HttpResponseHeader header, SecurityContext securityContext) { } default void setSessionCookie(HttpResponseHeader header, CharSequence sessionId) { } }core/src/main/java/io/questdb/cutlass/http/HttpResponseSink.java (1)
762-770: API polish: collapse cookie overloads around a single path.
sendStatusTextContent(...)andsendStatusWithCookie(...)duplicate intent. Consider keeping only the text method with cookie pairs to avoid future drift.Also applies to: 788-795
core/src/main/java/io/questdb/cutlass/http/HttpConnectionContext.java (1)
79-82: Avoid duplicating TRUE/FALSE literals.Prefer a single source (e.g., constants in
HttpConstants) to reduce drift and improve discoverability.core/src/main/java/io/questdb/cutlass/http/HttpSessionStore.java (2)
64-66: Harden theNO_SESSIONsentinel against mutation.Make it
finaland override mutators to throw to prevent accidental changes during debugging or misuse.Apply this diff:
- public static SessionInfo NO_SESSION = new SessionInfo( - "NO_SESSION", "NO_SESSION", null, SecurityContext.AUTH_TYPE_NONE, -1L, -1L - ); + public static final SessionInfo NO_SESSION = new SessionInfo( + "NO_SESSION", "NO_SESSION", null, SecurityContext.AUTH_TYPE_NONE, -1L, -1L + ) { + @Override public void rotate(String newSessionId, long nextRotationAt) { throw new UnsupportedOperationException("NO_SESSION"); } + @Override public void setExpiresAt(long expiresAt) { throw new UnsupportedOperationException("NO_SESSION"); } + @Override public synchronized void setGroups(@Nullable ObjList<CharSequence> source) { throw new UnsupportedOperationException("NO_SESSION"); } + };Also applies to: 113-121, 122-142
95-103: Avoid exposing a mutable groups list.
getGroups()returns a liveObjListthat callers can mutate, breaking the double‑buffering assumption.Consider returning a defensive copy or an unmodifiable view, or document the method as “read‑only” and enforce immutability at the type level if possible.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
core/src/main/java/io/questdb/DefaultFactoryProvider.java(2 hunks)core/src/main/java/io/questdb/FactoryProvider.java(2 hunks)core/src/main/java/io/questdb/FactoryProviderImpl.java(2 hunks)core/src/main/java/io/questdb/PropHttpServerConfiguration.java(2 hunks)core/src/main/java/io/questdb/PropServerConfiguration.java(1 hunks)core/src/main/java/io/questdb/cairo/security/AllowAllSecurityContextFactory.java(2 hunks)core/src/main/java/io/questdb/cairo/security/ReadOnlySecurityContextFactory.java(1 hunks)core/src/main/java/io/questdb/cairo/security/SecurityContextFactory.java(1 hunks)core/src/main/java/io/questdb/cutlass/Services.java(2 hunks)core/src/main/java/io/questdb/cutlass/auth/Authenticator.java(1 hunks)core/src/main/java/io/questdb/cutlass/http/DefaultHttpContextConfiguration.java(2 hunks)core/src/main/java/io/questdb/cutlass/http/DefaultHttpCookieHandler.java(1 hunks)core/src/main/java/io/questdb/cutlass/http/DefaultHttpSessionStore.java(1 hunks)core/src/main/java/io/questdb/cutlass/http/HttpConnectionContext.java(14 hunks)core/src/main/java/io/questdb/cutlass/http/HttpConstants.java(1 hunks)core/src/main/java/io/questdb/cutlass/http/HttpContextConfiguration.java(2 hunks)core/src/main/java/io/questdb/cutlass/http/HttpCookieHandler.java(1 hunks)core/src/main/java/io/questdb/cutlass/http/HttpRequestProcessor.java(2 hunks)core/src/main/java/io/questdb/cutlass/http/HttpResponseSink.java(5 hunks)core/src/main/java/io/questdb/cutlass/http/HttpServer.java(4 hunks)core/src/main/java/io/questdb/cutlass/http/HttpSessionStore.java(1 hunks)core/src/main/java/io/questdb/cutlass/http/PrincipalContext.java(1 hunks)core/src/main/java/io/questdb/cutlass/http/TokenGenerator.java(1 hunks)core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessor.java(3 hunks)core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessorState.java(1 hunks)core/src/main/java/io/questdb/cutlass/http/processors/RejectProcessorImpl.java(5 hunks)core/src/main/java/io/questdb/cutlass/line/tcp/LineTcpConnectionContext.java(5 hunks)core/src/main/java/io/questdb/cutlass/pgwire/PGConnectionContext.java(2 hunks)core/src/main/java/io/questdb/cutlass/pgwire/ReadOnlyUsersAwareSecurityContextFactory.java(2 hunks)core/src/test/java/io/questdb/test/ServerMainTest.java(1 hunks)core/src/test/java/io/questdb/test/TestServerConfiguration.java(2 hunks)core/src/test/java/io/questdb/test/cutlass/http/HttpErrorHandlingTest.java(5 hunks)core/src/test/java/io/questdb/test/cutlass/http/HttpQueryTestBuilder.java(4 hunks)core/src/test/java/io/questdb/test/cutlass/http/HttpServerConfigurationBuilder.java(4 hunks)core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderLoggingTest.java(1 hunks)utils/src/test/java/io/questdb/cliutil/Table2IlpTest.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: New pull request (Coverage Report Coverage Report)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-x64-zfs)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (Hosted Running tests on windows-other)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-other)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-cairo)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-fuzz)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start X64Zfs Agent)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start ARM Agent)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-griffin)
- GitHub Check: New pull request (SelfHosted Other tests Start X64Zfs Agent)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (SelfHosted Other tests Start ARM Agent)
- GitHub Check: New pull request (Check Changes Check changes)
🔇 Additional comments (28)
core/src/test/java/io/questdb/test/ServerMainTest.java (1)
727-727: LGTM! HTTP cookie property correctly added to expected parameters.The addition of
http.server.cookies.enabledproperty aligns with the PR's HTTP session management feature. The format is consistent with existing properties, and the default value oftrueappropriately enables cookie support by default.core/src/main/java/io/questdb/PropServerConfiguration.java (2)
1122-1138: Min HTTP context is hardcoded read-only — confirm intentThe constructor passes true for readOnlySecurityContext. If this is intentional for the min server, ignore; otherwise, consider making it configurable (min variant of HTTP_SECURITY_READONLY) for parity with the main server.
1118-1120: Property referenced in suggested fix does not exist; manual verification requiredLine 1119 correctly identifies a bug—
minHttpServerCookiesEnabledreadsHTTP_MIN_SERVER_KEEP_ALIVE(line 1118's property). However,HTTP_MIN_SERVER_COOKIES_ENABLEDdoes not exist inPropertyKey.java. OnlyHTTP_SERVER_COOKIES_ENABLED(non-min variant) is defined.Additionally, line 1118 (
minHttpMinServerKeepAlive) appears to have an identical bug with a confusing variable name that duplicates the property key.Clarify the intended behavior:
- Should
HTTP_MIN_SERVER_COOKIES_ENABLEDbe created as a new property?- Or should
minHttpServerCookiesEnabledreadHTTP_SERVER_COOKIES_ENABLEDinstead?core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderLoggingTest.java (1)
126-129: Updated SecurityContextFactory lambda matches new signatureSwitch to (principalContext, interfaceId) looks correct and keeps the test focused on auth error handling.
core/src/test/java/io/questdb/test/cutlass/http/HttpQueryTestBuilder.java (1)
196-197: Tests align with new APIs (TextImportProcessor wiring + NanosecondClock)
- Direct TextImportProcessor(engine, jsonCfg) simplifies the upload route setup.
- Field and builder now use NanosecondClock; builder call propagated.
Looks good.Also applies to: 81-82, 339-342
core/src/main/java/io/questdb/cutlass/auth/Authenticator.java (1)
28-35: Let me verify this finding more thoroughly to ensure no implementations exist:Now let me run one more comprehensive check to ensure no hidden implementations exist:No actionable breaking changes identified — review comment is incorrect
The comprehensive code search found zero direct implementations of the
Authenticatorinterface in the codebase. All references to classes likeAnonymousAuthenticator,EllipticCurveAuthenticator, etc., implement different interfaces (SocketAuthenticator,HttpAuthenticator) rather thanAuthenticator.Since the
Authenticatorinterface now extendsPrincipalContext(which already definesgetPrincipal()), any implementor would automatically inherit this contract without breaking changes. However, no internal implementors exist to break, making the premise of the review comment unfounded.Likely an incorrect or invalid review comment.
core/src/main/java/io/questdb/cutlass/http/HttpConstants.java (1)
77-77: LGTM!The new session parameter constant follows the established naming pattern and integrates cleanly with the existing URL parameter constants.
core/src/main/java/io/questdb/FactoryProvider.java (1)
60-61: LGTM!The new factory method follows the established pattern in the interface and appropriately uses
@NotNullannotation.core/src/main/java/io/questdb/cutlass/http/DefaultHttpContextConfiguration.java (1)
116-118: LGTM!The return type change aligns with the interface update and the implementation already returns a
NanosecondClockinstance.core/src/test/java/io/questdb/test/TestServerConfiguration.java (1)
143-145: LGTM!Test configuration correctly updated to align with the production interface change.
core/src/main/java/io/questdb/FactoryProviderImpl.java (1)
77-80: No thread-safety concerns—DefaultHttpSessionStore is a stateless no-op implementation.DefaultHttpSessionStore is a singleton with zero mutable state and all empty method implementations. Its singleton instance is initialized eagerly and safely by Java's static field semantics. No concurrent operations are performed within this class, making thread-safety guarantees unnecessary. Low test coverage is expected for a no-op implementation.
Likely an incorrect or invalid review comment.
core/src/main/java/io/questdb/cutlass/http/HttpContextConfiguration.java (1)
30-30: Type narrowing from Clock to NanosecondClock is correctly implemented and safe.Verification confirms:
- Both implementations (DefaultHttpContextConfiguration and PropHttpContextConfiguration) return NanosecondClock
- NanosecondClock extends Clock, making this a safe subtype narrowing
- All call sites are compatible with the change (assignments to Clock variables work due to inheritance; no explicit casts found)
- No breaking changes detected
core/src/main/java/io/questdb/cutlass/http/HttpServer.java (2)
117-117: Session store wiring looks correct.The session store is properly threaded through
HttpContextFactorytoHttpConnectionContextconstruction, maintaining consistency with the existing cookie handler pattern.Also applies to: 368-373
85-86: API extension is backward-compatible; no breaking change.The
HttpServerconstructor has been properly extended with a new parameter (HttpSessionStore) while maintaining backward compatibility through a convenience constructor. All call sites are correctly updated:
- The 3-parameter convenience constructor (lines 85-92) delegates to the 6-parameter full constructor with
DefaultHttpSessionStore.INSTANCEand other defaults- All test files correctly use the 3-parameter convenience constructor
- Production code (Services.java, Table2IlpTest.java) uses the 6-parameter full constructor with explicit values
- No broken or incomplete constructor calls detected
This is a standard, non-breaking API extension pattern.
Likely an incorrect or invalid review comment.
core/src/main/java/io/questdb/cutlass/line/tcp/LineTcpConnectionContext.java (1)
247-248: SecurityContextFactory API migration looks correct.The migration from passing individual auth parameters to using
PrincipalContext(via authenticator or DUMMY_CONTEXT) maintains the same authentication semantics while improving API cohesion.Also applies to: 311-312
core/src/main/java/io/questdb/DefaultFactoryProvider.java (1)
70-73: Clean addition following existing patterns.The
getHttpSessionStore()method follows the established factory pattern, returning a singleton instance consistent with other handlers in this provider.core/src/main/java/io/questdb/cutlass/Services.java (1)
101-111: Session store wiring is consistent.The session store is properly retrieved from the factory provider and passed to the
HttpServerconstructor alongside other dependencies.core/src/main/java/io/questdb/cutlass/http/HttpRequestProcessor.java (1)
86-88: The review comment is incorrect—there is no breaking API change.The review claims that
processCookies()was renamed toprocessServiceAccountCookie()inHttpRequestProcessor. However, the verification reveals:
processCookies()does not exist inHttpRequestProcessorand never has been renamed.processCookies()exists in a different, unrelated interface:HttpClientCookieHandler(for HTTP client-side cookie handling).processServiceAccountCookie()is the current, existing method inHttpRequestProcessor(lines 86-88), already implemented by multiple classes (JsonQueryProcessor,DefaultHttpCookieHandler, etc.).- The method is already being used correctly throughout the codebase (e.g.,
HttpConnectionContextline 911).The review conflates two separate APIs. No breaking changes to
HttpRequestProcessorhave occurred.Likely an incorrect or invalid review comment.
core/src/main/java/io/questdb/cairo/security/AllowAllSecurityContextFactory.java (1)
38-40: No action needed - API migration is complete and correct.All implementations of
SecurityContextFactoryhave been properly updated with the new signature requiringPrincipalContextinstead of individual authentication parameters. The two call sites inLineTcpConnectionContextare correctly passing compatible types:
- Line 246-248:
authenticator(typeSocketAuthenticator, which extendsAuthenticatorwhich extendsPrincipalContext)- Line 310-312:
DUMMY_CONTEXT(typeDummyPrincipalContextimplementingPrincipalContext)All three implementations verified:
ReadOnlySecurityContextFactory- updatedAllowAllSecurityContextFactory- updatedReadOnlyUsersAwareSecurityContextFactory- updatedcore/src/test/java/io/questdb/test/cutlass/http/HttpErrorHandlingTest.java (3)
100-102: LGTM: test calls consolidated helperCalling assertExecRequest(httpClient) simplifies the test without changing behavior.
166-185: LGTM: deterministic exec error assertionHardcoded CREATE TABLE and JSON error assertions are clear and stable. Status 500 check is appropriate.
126-138: Returning null from processSessionCookie() is the correct and supported contractThe code at
HttpConnectionContextline 505 explicitly handles null:if (sessionInfo != null && sessionInfo != NO_SESSION). WhenprocessSessionCookie()returns null, the authentication flow correctly treats it as "no valid session" and proceeds to the fallback branch (line 507–509). This is the intended design—both the productionDefaultHttpCookieHandlerand your test mock return null consistently, and the calling code is defensively written to handle this. No changes needed.core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessor.java (3)
317-319: LGTM: cookie processing renameprocessServiceAccountCookie delegation matches the new HttpCookieHandler API.
380-396: LGTM: Java 17 pattern matchingUsing
if (e instanceof CairoException ce)clarifies logging and avoids redundant casts.
879-883: Review comment is incorrect: CharSequence.isEmpty() is available in Java 17.The codebase targets Java 17 (
<javac.target>17</javac.target>in pom.xml). Java 15 introducedisEmpty()as a default method in theCharSequenceinterface, making it available in Java 17. SinceStringSinkimplementsCharSequenceand provides alength()method, theisEmpty()call in the code will work correctly without compilation errors. The suggested diff is unnecessary.Likely an incorrect or invalid review comment.
utils/src/test/java/io/questdb/cliutil/Table2IlpTest.java (1)
491-501: Session store wiring looks correctPassing HttpSessionStore into HttpServer and using constant 1 for worker-bound JSON processor in this test is fine.
Also applies to: 503-520
core/src/main/java/io/questdb/cutlass/pgwire/PGConnectionContext.java (1)
596-603: No issues found. The code properly satisfies the PrincipalContext contract.The review comment's concern is unfounded. SocketAuthenticator extends Authenticator, and Authenticator extends QuietCloseable, Mutable, PrincipalContext. Through this inheritance chain, SocketAuthenticator is a valid PrincipalContext, and Java's type system ensures compatibility at compile time. SecurityContextFactory.getInstance() expects a PrincipalContext parameter, and the authenticator passed at PGConnectionContext.java:598-600 satisfies this contract. Group/role attributes are propagated through the PrincipalContext interface methods (getAuthType(), getGroups()).
core/src/main/java/io/questdb/cutlass/http/processors/RejectProcessorImpl.java (1)
46-48: Migration to multi-cookie looks good.Lists are cleared on reuse, pairing is consistent, and the new send path is correct.
Also applies to: 58-60, 80-81, 112-115
core/src/main/java/io/questdb/cairo/security/SecurityContextFactory.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/questdb/cutlass/http/DefaultHttpCookieHandler.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/questdb/cutlass/http/DefaultHttpCookieHandler.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/questdb/cutlass/http/EmptyHttpSessionStore.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/questdb/cutlass/http/EmptyHttpSessionStore.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/questdb/cutlass/pgwire/ReadOnlyUsersAwareSecurityContextFactory.java
Outdated
Show resolved
Hide resolved
core/src/test/java/io/questdb/test/cutlass/http/HttpServerConfigurationBuilder.java
Show resolved
Hide resolved
ideoma
left a comment
ideoma
left a comment
There was a problem hiding this comment.
I believe authenticationNanos should be set to 0 in HttpConnectoinContext.clear(). This is an existing bug; clear() is called when the request is complete, reset() when the connection is closed. We want to reset the auth time for every request, in case the same connection is used for multiple users, like it can happen in case of a proxy
core/src/main/java/io/questdb/cutlass/http/DefaultHttpSessionStore.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/questdb/cutlass/http/processors/LineHttpProcessorConfiguration.java
Show resolved
Hide resolved
core/src/main/java/io/questdb/cutlass/http/HttpSessionStore.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/questdb/cutlass/http/HttpSessionStoreImpl.java
Outdated
Show resolved
Hide resolved
[PR Coverage check]😍 pass : 282 / 332 (84.94%) file detail
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
4 participants
HTTP session with a session cookie.
required by https://github.com/questdb/questdb-enterprise/pull/744