feat: Allow websocket subscription with PAK/PAT #3203
Conversation
WalkthroughThe pull request introduces HTTP conditional request support for export endpoints via a new ProjectLastModifiedManager component and refactors WebSocket authentication to use a dedicated WebsocketAuthenticationResolver that handles JWT, API Key, and Personal Access Token schemes uniformly across STOMP endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant V2ExportController
participant ProjectLastModifiedManager
participant ProjectTranslationLastModifiedManager
participant WebRequest
Client->>V2ExportController: GET /export (with optional If-Modified-Since)
V2ExportController->>ProjectLastModifiedManager: onlyWhenProjectDataChanged(request, fn)
ProjectLastModifiedManager->>ProjectTranslationLastModifiedManager: getLastModified(projectId)
ProjectTranslationLastModifiedManager-->>ProjectLastModifiedManager: timestamp
ProjectLastModifiedManager->>WebRequest: checkNotModified(timestamp)
alt Data unchanged
WebRequest-->>ProjectLastModifiedManager: true (304 condition met)
ProjectLastModifiedManager-->>V2ExportController: null
V2ExportController-->>Client: 304 Not Modified
else Data changed
WebRequest-->>ProjectLastModifiedManager: false
ProjectLastModifiedManager->>ProjectLastModifiedManager: execute fn(headersBuilder)
Note over ProjectLastModifiedManager: Set Last-Modified header<br/>Set Cache-Control: max-age=0
ProjectLastModifiedManager->>ProjectLastModifiedManager: build StreamingResponseBody
ProjectLastModifiedManager-->>V2ExportController: ResponseEntity with body
V2ExportController-->>Client: 200 OK (with Last-Modified, Cache-Control)
end
sequenceDiagram
participant WebSocket Client
participant WebSocketConfig
participant WebsocketAuthenticationResolver
participant JwtService/ApiKeyService
participant SecurityService
WebSocket Client->>WebSocketConfig: CONNECT frame<br/>(with auth headers)
WebSocketConfig->>WebsocketAuthenticationResolver: resolve(authHeader, apiKeyHeader, legacyJwtHeader)
alt Bearer JWT Token
WebsocketAuthenticationResolver->>JwtService: validateToken(bearer)
JwtService-->>WebsocketAuthenticationResolver: TolgeeAuthentication
else X-API-Key (PAT)
WebsocketAuthenticationResolver->>ApiKeyService: validate PAT key
ApiKeyService-->>WebsocketAuthenticationResolver: TolgeeAuthentication + update last-used async
else X-API-Key (PAK)
WebsocketAuthenticationResolver->>ApiKeyService: validate PAK key
ApiKeyService-->>WebsocketAuthenticationResolver: TolgeeAuthentication + update last-used async
else Legacy jwtToken
WebsocketAuthenticationResolver->>JwtService: validateToken(jwtToken)
JwtService-->>WebsocketAuthenticationResolver: TolgeeAuthentication
else No valid auth
WebsocketAuthenticationResolver-->>WebSocketConfig: null
WebSocketConfig-->>WebSocket Client: DISCONNECT (unauthenticated)
end
WebsocketAuthenticationResolver-->>WebSocketConfig: TolgeeAuthentication
WebSocketConfig->>SecurityService: Check permissions (subscribe path)
SecurityService-->>WebSocketConfig: permitted or forbidden
WebSocketConfig-->>WebSocket Client: CONNECTED or ERROR (403/401)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Rationale: The changes introduce multiple new Spring components (ProjectLastModifiedManager, WebsocketAuthenticationResolver) with non-trivial logic for HTTP caching semantics and multi-scheme authentication, modify constructor signatures across controllers, alter WebSocket authentication flows, and include comprehensive test coverage. While individual changes follow consistent patterns, the heterogeneity of affected areas (export/translation APIs, WebSocket infrastructure, test support) and density of interdependent modifications demand careful review of authentication logic, HTTP conditional request handling, and thread-safety of async operations. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
JanCizmar
force-pushed
the
jancizmar/ws-with-api-key
branch
from
159086b to
cb93702
Compare
|
I am working on it! |
|
This PR is stale because it has been open for 30 days with no activity. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/src/test/kotlin/io/tolgee/websocket/AbstractWebsocketTest.kt (1)
61-64: Stop all WebSockets to avoid cross-test leakage.
anotherUserWebsocketandspyingUserWebsocketaren’t closed; this can cause flakiness and port/socket leaks.Apply teardown and try/finally:
@@ @AfterEach fun after() { - currentUserWebsocket.stop() + if (this::currentUserWebsocket.isInitialized) currentUserWebsocket.stop() + if (this::anotherUserWebsocket.isInitialized) anotherUserWebsocket.stop() }@@ - val spyingUserWebsocket = + val spyingUserWebsocket = WebsocketTestHelper( port, WebsocketTestHelper.Auth(jwtToken = jwtService.emitToken(anotherUser.id)), testData.projectBuilder.self.id, // anotherUser trying to spy on other user's websocket testData.user.id, ) - spyingUserWebsocket.listenForNotificationsChanged() - spyingUserWebsocket.waitForForbidden() - saveNotificationForCurrentUser() - - assertCurrentUserReceivedMessage() - spyingUserWebsocket.receivedMessages.assert.isEmpty() + try { + spyingUserWebsocket.listenForNotificationsChanged() + spyingUserWebsocket.waitForForbidden() + saveNotificationForCurrentUser() + assertCurrentUserReceivedMessage() + spyingUserWebsocket.receivedMessages.assert.isEmpty() + } finally { + spyingUserWebsocket.stop() + }Also applies to: 52-59, 235-253
🧹 Nitpick comments (11)
backend/app/src/test/kotlin/io/tolgee/websocket/AbstractWebsocketTest.kt (3)
48-51: Add coverage for PAK/PAT auth paths.Good refactor to Auth wrapper. Please add tests that subscribe via PAK and PAT (success and forbidden cases), not just JWT, to validate the new resolver end-to-end.
Also applies to: 55-58, 242-246
220-233: Assert the forbidden subscribe explicitly.This negative test should also await the 403 to avoid a false pass due to timing.
fun `doesn't subscribe without permissions`() { currentUserWebsocket.listenForTranslationDataModified() anotherUserWebsocket.listenForTranslationDataModified() + anotherUserWebsocket.waitForForbidden()
215-217: Typo in doc comment.“shell” → “shall”.
backend/api/src/main/kotlin/io/tolgee/controllers/ExportController.kt (1)
49-84: Stream safety and header hygiene.
- Use
use {}to always close streams on error.- Consider adding
Vary: Authorization, X-API-KEYto prevent shared-cache mix-ups when auth differs.- streamingResponseBodyProvider.createStreamingResponseBody { out: OutputStream -> - val zipOutputStream = ZipOutputStream(out) + streamingResponseBodyProvider.createStreamingResponseBody { out: OutputStream -> + val mapper = jacksonObjectMapper() + ZipOutputStream(out).use { zipOutputStream -> val translations = translationService.getTranslations( allLanguages.map { it.tag }.toSet(), null, projectHolder.project.id, '.', ) for ((key, value) in translations) { zipOutputStream.putNextEntry(ZipEntry(String.format("%s.json", key))) - val data = jacksonObjectMapper().writerWithDefaultPrettyPrinter().writeValueAsBytes(value) - val byteArrayInputStream = ByteArrayInputStream(data) - IOUtils.copy(byteArrayInputStream, zipOutputStream) - byteArrayInputStream.close() + val data = mapper.writerWithDefaultPrettyPrinter().writeValueAsBytes(value) + ByteArrayInputStream(data).use { byteArrayInputStream -> + IOUtils.copy(byteArrayInputStream, zipOutputStream) + } zipOutputStream.closeEntry() } - zipOutputStream.close() - } + } + }Also please confirm
authenticationFacade.authenticatedUser.idis always present for API key/PAT flows on this endpoint. If not, derive permissions from the token principal instead.backend/api/src/main/kotlin/io/tolgee/component/ProjectLastModifiedManager.kt (1)
10-17: Clarify semantics and add optional cache headers.
- Update KDoc to mention 304 (GET/HEAD) and 412 (POST/PUT/DELETE) outcomes from
checkNotModified.- Add
Vary: Authorization, X-API-KEYby default to avoid cache poisoning across auth contexts.- Consider allowing a custom
CacheControlto be passed in when needed.@@ - * This manager implements the HTTP conditional request mechanism (If-Modified-Since/Last-Modified headers) + * This manager implements HTTP conditional requests (If-Modified-Since/Last-Modified). + * For GET/HEAD it results in 304 Not Modified; for modifying methods (POST/PUT/DELETE) Spring returns 412. @@ - val headersBuilder = ResponseEntity + val headersBuilder = ResponseEntity .ok() .lastModified(lastModified) .cacheControl(DEFAULT_CACHE_CONTROL_HEADER) + .varyBy("Authorization", "X-API-KEY")Also applies to: 41-67, 69-71
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ExportController/V2ExportControllerCachingTest.kt (3)
48-73: Fix test name and safe header retrieval (GET path).
- Name says POST but the test hits GET.
- Avoid
as Stringon a nullable header; it can throw before the assert.- fun `returns 304 for POST export when data not modified`() { + fun `returns 304 for GET export when data not modified`() { @@ - val lastModifiedHeader = firstResponse.response.getHeaderValue("Last-Modified") as String - Assertions.assertThat(lastModifiedHeader).isNotNull() + val lastModifiedHeader = firstResponse.response.getHeader("Last-Modified") + Assertions.assertThat(lastModifiedHeader).isNotNull() + // Optional: also assert Cache-Control + Assertions.assertThat(firstResponse.response.getHeader("Cache-Control")).isEqualTo("max-age=0")
90-109: Fix misleading comment and assert cache header (POST path).Comment says 304 in a test that asserts 412. Add cache header assertion for parity with GET.
- // Second request with If-Modified-Since header - should return 304 + // Second request with If-Modified-Since header - server returns 412 for POST @@ Assertions.assertThat(secondResponse.response.status).isEqualTo(412) + Assertions.assertThat(secondResponse.response.getHeader("Cache-Control")).isEqualTo("max-age=0")
111-137: Reduce blanket retries and cleanups.The retry matcher swallows NPEs and data integrity issues that may hide real regressions. Consider narrowing exceptions or lowering retries once flakiness is addressed.
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/batch/V2ExportController.kt (2)
171-181: Close streams via use{} to avoid leaks on error.- return ResponseEntity.ok().headers(headers).body( - streamingResponseBodyProvider.createStreamingResponseBody { out: OutputStream -> - IOUtils.copy(stream, out) - stream.close() - out.close() - }, - ) + return ResponseEntity.ok().headers(headers).body( + streamingResponseBodyProvider.createStreamingResponseBody { out: OutputStream -> + stream.use { input -> out.use { IOUtils.copy(input, it) } } + }, + )
184-192: Use use{} for ZipOutputStream and input streams.- return ResponseEntity.ok().headers(httpHeaders).body( - streamingResponseBodyProvider.createStreamingResponseBody { out: OutputStream -> - streamZipResponse(out, exported) - }, - ) + return ResponseEntity.ok().headers(httpHeaders).body( + streamingResponseBodyProvider.createStreamingResponseBody { out: OutputStream -> + streamZipResponse(out, exported) + }, + ) @@ - private fun streamZipResponse( + private fun streamZipResponse( out: OutputStream, exported: Map<String, InputStream>, ) { - val zipOutputStream = ZipOutputStream(out) - - exported.forEach { (fileAbsolutePath, stream) -> - zipOutputStream.putNextEntry(ZipEntry(fileAbsolutePath)) - IOUtils.copy(stream, zipOutputStream) - stream.close() - zipOutputStream.closeEntry() - } - - zipOutputStream.close() + ZipOutputStream(out).use { zipOutputStream -> + exported.forEach { (fileAbsolutePath, stream) -> + zipOutputStream.putNextEntry(ZipEntry(fileAbsolutePath)) + stream.use { IOUtils.copy(it, zipOutputStream) } + zipOutputStream.closeEntry() + } + } }Also applies to: 194-208
backend/app/src/test/kotlin/io/tolgee/websocket/WebsocketAuthenticationTest.kt (1)
79-145: Verify test annotation usage for PAK/PAT tests.Several tests that validate PAK or PAT authentication use the
@ProjectJWTAuthTestMethodannotation (lines 79, 88, 115, 127, 136, 149) rather than@ProjectApiKeyAuthTestMethod. While this might be intentional if the annotations only control REST API authentication setup (like thecreateKey()call), it could be confusing to readers.Please verify that this annotation usage is correct and consider adding a comment explaining why JWT annotations are used for PAK/PAT websocket authentication tests if this is intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/batch/V2ExportController.kt(3 hunks)backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/translation/TranslationsController.kt(3 hunks)backend/api/src/main/kotlin/io/tolgee/component/ProjectLastModifiedManager.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/controllers/ExportController.kt(4 hunks)backend/api/src/main/kotlin/io/tolgee/websocket/WebSocketConfig.kt(6 hunks)backend/api/src/main/kotlin/io/tolgee/websocket/WebsocketAuthenticationResolver.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ExportController/V2ExportAllFormatsTest.kt(2 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ExportController/V2ExportControllerCachingTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ExportController/V2ExportControllerTest.kt(7 hunks)backend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/controllers/ExportControllerTest.kt(2 hunks)backend/app/src/test/kotlin/io/tolgee/websocket/AbstractWebsocketTest.kt(2 hunks)backend/app/src/test/kotlin/io/tolgee/websocket/WebsocketAuthenticationTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/websocket/WebsocketTestHelper.kt(7 hunks)backend/data/src/main/kotlin/io/tolgee/model/Pat.kt(2 hunks)webapp/src/websocket-client/WebsocketClient.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
backend/app/src/test/kotlin/io/tolgee/controllers/ExportControllerTest.kt (2)
backend/testing/src/main/kotlin/io/tolgee/AbstractSpringTest.kt (2)
setForcedDate(272-274)clearForcedDate(276-278)backend/testing/src/main/kotlin/io/tolgee/fixtures/ProjectAuthRequestPerformer.kt (1)
performProjectAuthGet(36-36)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ExportController/V2ExportAllFormatsTest.kt (1)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ExportController/V2ExportControllerTest.kt (1)
parseZip(227-237)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/v2ExportController/V2ExportControllerCachingTest.kt (1)
backend/testing/src/main/kotlin/io/tolgee/AbstractSpringTest.kt (2)
clearCaches(231-236)clearForcedDate(276-278)
🪛 detekt (1.23.8)
backend/api/src/main/kotlin/io/tolgee/websocket/WebSocketConfig.kt
[warning] 94-94: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (11)
backend/data/src/main/kotlin/io/tolgee/model/Pat.kt (1)
3-3: LGTM!The import is necessary for the new
tokenWithPrefixproperty.webapp/src/websocket-client/WebsocketClient.ts (1)
7-15: Fix type inconsistency:jwtTokenis required but treated as optional in code.The type definition declares
jwtTokenas a required string (line 7), but line 99 checksif (options.authentication.jwtToken), treating it as optional. This inconsistency should be resolved by either:
- Making
jwtTokenoptional in the type:jwtToken?: string;, or- Removing the truthiness check and assuming the token is always provided
Additionally, consider whether this field should accept PAK/PAT tokens. If the backend WebSocket authentication handles multiple token types, renaming to a generic
tokenfield or updating the field documentation would improve clarity.backend/app/src/test/kotlin/io/tolgee/websocket/WebsocketTestHelper.kt (1)
198-204: LGTM: Clear authentication wrapper with proper validation.The Auth data class properly enforces the constraint that exactly one authentication method must be provided, preventing misconfiguration in tests.
backend/api/src/main/kotlin/io/tolgee/websocket/WebSocketConfig.kt (2)
67-97: Consider logging the caught exception for debugging.The API key permission logic correctly validates project match and scope. However, the exception handling at lines 92-96 swallows the original exception, which could make debugging permission issues difficult.
While returning a generic "Forbidden" message to websocket clients is appropriate for security, consider logging the original exception for operators:
val user = authentication.principal try { securityService.checkProjectPermissionNoApiKey(projectId = projectId, Scope.KEYS_VIEW, user) } catch (e: Exception) { + logger.debug("Project permission check failed for user ${user.id} on project $projectId", e) throw MessagingException("Forbidden") }This addresses the static analysis warning about the swallowed exception while maintaining security.
99-123: Good security decision to block API keys from user topics.The explicit rejection of API key authentication for user-specific topics (lines 114-117) is correct, as API keys are project-scoped and should not access user-specific resources like notifications.
Minor inconsistency: Line 110 throws "Forbidden" for null authentication, while line 78 (in
checkProjectPathPermissionsAuth) throws "Unauthenticated". Consider using "Unauthenticated" here as well for consistency:if (authentication == null) { - throw MessagingException("Forbidden") + throw MessagingException("Unauthenticated") }backend/app/src/test/kotlin/io/tolgee/controllers/ExportControllerTest.kt (3)
34-40: Good refactoring: centralized test setup and proper cleanup.The new setup/teardown methods eliminate duplication and ensure proper cleanup of both test data and forced dates after each test.
Also applies to: 114-118
96-112: Good test coverage for HTTP conditional requests.The new tests properly verify Last-Modified header generation and 304 Not Modified responses. Using forced dates ensures deterministic test behavior.
120-139: Clean test helpers for conditional request testing.The helper methods properly handle HTTP date header formatting and comparison at second-level precision, which is appropriate since HTTP date headers don't include milliseconds.
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/translation/TranslationsController.kt (1)
165-177: Conditional request handling is properly implemented.Verification confirms
ProjectLastModifiedManager.onlyWhenProjectDataChangedcorrectly:
- Sets Last-Modified header via
.lastModified(lastModified)on 200 responses- Returns null (HTTP 304 Not Modified) when
request.checkNotModified()is true- Handles missing modification timestamps by caching the current time via
ProjectTranslationLastModifiedManager.getLastModified()- Sets Cache-Control header to
max-age=0for request validationThe refactoring safely centralizes conditional request logic across controllers without losing functionality.
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/batch/V2ExportController.kt (2)
76-93: Good: conditional export wrapped via ProjectLastModifiedManager.The flow and header propagation look correct.
Please confirm all external callers/tests are updated for the now-nullable return type.
53-72: Docstrings: solid.Nice API docs on conditional requests (304/412 + Last-Modified/Cache-Control).
Also applies to: 95-120
| fun resolve( | ||
| authorizationHeader: String?, | ||
| xApiKeyHeader: String?, | ||
| legacyJwtHeader: String?, | ||
| ): TolgeeAuthentication? { | ||
| // Authorization: Bearer <jwt> | ||
| val bearer = extractBearer(authorizationHeader) | ||
| if (bearer != null) { | ||
| return runCatching { jwtService.validateToken(bearer) }.getOrNull() | ||
| } | ||
|
|
||
| // X-API-Key: PAT / PAK | ||
| val xApiKey = xApiKeyHeader | ||
| if (!xApiKey.isNullOrBlank()) { | ||
| return when { | ||
| xApiKey.startsWith(PAT_PREFIX) -> runCatching { patAuth(xApiKey) }.getOrNull() | ||
| else -> runCatching { pakAuth(xApiKey) }.getOrNull() | ||
| } | ||
| } | ||
|
|
||
| // Legacy jwtToken header | ||
| if (!legacyJwtHeader.isNullOrBlank()) { | ||
| return runCatching { jwtService.validateToken(legacyJwtHeader) }.getOrNull() | ||
| } | ||
|
|
||
| return null | ||
| } |
There was a problem hiding this comment.
Add logging for authentication failures.
The runCatching { }.getOrNull() pattern silently swallows all exceptions, making it difficult to diagnose authentication failures in production. When websocket connections fail to authenticate, there will be no logs to help operators understand why.
Consider logging authentication failures at debug or warn level:
val bearer = extractBearer(authorizationHeader)
if (bearer != null) {
- return runCatching { jwtService.validateToken(bearer) }.getOrNull()
+ return runCatching { jwtService.validateToken(bearer) }
+ .onFailure { logger.debug("Bearer token validation failed", it) }
+ .getOrNull()
}
// X-API-Key: PAT / PAK
val xApiKey = xApiKeyHeader
if (!xApiKey.isNullOrBlank()) {
return when {
- xApiKey.startsWith(PAT_PREFIX) -> runCatching { patAuth(xApiKey) }.getOrNull()
- else -> runCatching { pakAuth(xApiKey) }.getOrNull()
+ xApiKey.startsWith(PAT_PREFIX) -> runCatching { patAuth(xApiKey) }
+ .onFailure { logger.debug("PAT authentication failed", it) }
+ .getOrNull()
+ else -> runCatching { pakAuth(xApiKey) }
+ .onFailure { logger.debug("PAK authentication failed", it) }
+ .getOrNull()
}
}
// Legacy jwtToken header
if (!legacyJwtHeader.isNullOrBlank()) {
- return runCatching { jwtService.validateToken(legacyJwtHeader) }.getOrNull()
+ return runCatching { jwtService.validateToken(legacyJwtHeader) }
+ .onFailure { logger.debug("Legacy JWT validation failed", it) }
+ .getOrNull()
}You'll also need to add the Logging interface:
@Component
class WebsocketAuthenticationResolver(
@Lazy private val jwtService: JwtService,
@Lazy private val apiKeyService: ApiKeyService,
@Lazy private val patService: PatService,
@Lazy private val userAccountService: UserAccountService,
) : Logging {| fun `forbidden with insufficient scopes on user with JWT`() { | ||
| val user2 = testData.root.addUserAccount { username = "user2" } | ||
| saveTestData() | ||
| testItIsForbiddenWithAuth( | ||
| auth = WebsocketTestHelper.Auth(jwtToken = jwtService.emitToken(user2.self.id)) | ||
| ) |
There was a problem hiding this comment.
Fix inconsistent indentation.
The function declaration and the call to testItIsForbiddenWithAuth have inconsistent indentation, breaking the standard Kotlin formatting.
Apply this diff to fix the indentation:
-fun `forbidden with insufficient scopes on user with JWT`() {
+ fun `forbidden with insufficient scopes on user with JWT`() {
val user2 = testData.root.addUserAccount { username = "user2" }
saveTestData()
- testItIsForbiddenWithAuth(
+ testItIsForbiddenWithAuth(
auth = WebsocketTestHelper.Auth(jwtToken = jwtService.emitToken(user2.self.id))
)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
backend/app/src/test/kotlin/io/tolgee/websocket/WebsocketAuthenticationTest.kt
around lines 61 to 66, the function body and the call to
testItIsForbiddenWithAuth are indented inconsistently; fix by aligning the inner
block consistently (use standard 4-space indentation for the body), so the lines
with saveTestData() and the testItIsForbiddenWithAuth(...) call are indented the
same level under the function declaration and the auth parameter is indented to
match the call's continuation.
| /** for api key we need at least translations.view scope */ | ||
| @Test | ||
| @ProjectApiKeyAuthTestMethod(scopes = []) // No scopes | ||
| fun `forbidden with insufficient scopes on PAT`() { | ||
| saveTestData() | ||
| testItIsForbiddenWithAuth( | ||
| auth = WebsocketTestHelper.Auth(apiKey = apiKey.key) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Test name doesn't match implementation - says PAT but tests PAK.
The test name claims to test "PAT" (Personal Access Token) but actually tests PAK (Project API Key) using apiKey.key. The annotation @ProjectApiKeyAuthTestMethod also confirms this is testing PAK, not PAT.
Apply this diff to fix the test name:
/** for api key we need at least translations.view scope */
@Test
@ProjectApiKeyAuthTestMethod(scopes = []) // No scopes
-fun `forbidden with insufficient scopes on PAT`() {
+ fun `forbidden with insufficient scopes on PAK`() {
saveTestData()
testItIsForbiddenWithAuth(
auth = WebsocketTestHelper.Auth(apiKey = apiKey.key)
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** for api key we need at least translations.view scope */ | |
| @Test | |
| @ProjectApiKeyAuthTestMethod(scopes = []) // No scopes | |
| fun `forbidden with insufficient scopes on PAT`() { | |
| saveTestData() | |
| testItIsForbiddenWithAuth( | |
| auth = WebsocketTestHelper.Auth(apiKey = apiKey.key) | |
| ) | |
| } | |
| /** for api key we need at least translations.view scope */ | |
| @Test | |
| @ProjectApiKeyAuthTestMethod(scopes = []) // No scopes | |
| fun `forbidden with insufficient scopes on PAK`() { | |
| saveTestData() | |
| testItIsForbiddenWithAuth( | |
| auth = WebsocketTestHelper.Auth(apiKey = apiKey.key) | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
backend/app/src/test/kotlin/io/tolgee/websocket/WebsocketAuthenticationTest.kt
around lines 104 to 112, the test method name and description incorrectly
mention "PAT" while the test uses a Project API Key (PAK); rename the test
function and any inlined text from "PAT" to "PAK" (for example change the method
name to `forbiddenWithInsufficientScopesOnPAK`) so the name matches the use of
`apiKey.key` and the `@ProjectApiKeyAuthTestMethod` annotation; keep
implementation and annotation unchanged.
| fun `forbidden with insufficient scopes on user with PAT`() { | ||
| val pat = addInsufficientPatToTestData() | ||
| saveTestData() | ||
| // This test should fail with insufficient permissions - intentionally designed to fail | ||
| testItIsForbiddenWithAuth( | ||
| auth = WebsocketTestHelper.Auth(apiKey = pat.tokenWithPrefix) | ||
| ) |
There was a problem hiding this comment.
Fix inconsistent indentation.
The function declaration has inconsistent indentation, breaking the standard Kotlin formatting.
Apply this diff to fix the indentation:
-fun `forbidden with insufficient scopes on user with PAT`() {
+ fun `forbidden with insufficient scopes on user with PAT`() {
val pat = addInsufficientPatToTestData()
saveTestData()
// This test should fail with insufficient permissions - intentionally designed to fail
testItIsForbiddenWithAuth(
auth = WebsocketTestHelper.Auth(apiKey = pat.tokenWithPrefix)
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun `forbidden with insufficient scopes on user with PAT`() { | |
| val pat = addInsufficientPatToTestData() | |
| saveTestData() | |
| // This test should fail with insufficient permissions - intentionally designed to fail | |
| testItIsForbiddenWithAuth( | |
| auth = WebsocketTestHelper.Auth(apiKey = pat.tokenWithPrefix) | |
| ) | |
| fun `forbidden with insufficient scopes on user with PAT`() { | |
| val pat = addInsufficientPatToTestData() | |
| saveTestData() | |
| // This test should fail with insufficient permissions - intentionally designed to fail | |
| testItIsForbiddenWithAuth( | |
| auth = WebsocketTestHelper.Auth(apiKey = pat.tokenWithPrefix) | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
backend/app/src/test/kotlin/io/tolgee/websocket/WebsocketAuthenticationTest.kt
around lines 150 to 156, the function declaration and its body are indented
inconsistently; reformat the function so the `fun` line aligns with other test
functions and the body lines (val pat..., saveTestData(), comment, and
testItIsForbiddenWithAuth call) are uniformly indented (use the project's Kotlin
indentation style, e.g., 4 spaces), and ensure the closing brace lines up with
the `fun` declaration; run the Kotlin formatter/IDE reformat after making the
indentation fixes.
| fun `forbidden with insufficient scopes on user with PAT`() { | ||
| val pat = addInsufficientPatToTestData() | ||
| saveTestData() | ||
| // This test should fail with insufficient permissions - intentionally designed to fail |
There was a problem hiding this comment.
Clarify misleading comment.
The comment suggests the test itself is "intentionally designed to fail," which is misleading. The test is designed to verify that the system correctly returns a forbidden response when permissions are insufficient. The test should pass when this verification succeeds.
Apply this diff to clarify the comment:
- // This test should fail with insufficient permissions - intentionally designed to fail
+ // This test verifies that the system correctly rejects requests with insufficient permissions📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // This test should fail with insufficient permissions - intentionally designed to fail | |
| // This test verifies that the system correctly rejects requests with insufficient permissions |
🤖 Prompt for AI Agents
In
backend/app/src/test/kotlin/io/tolgee/websocket/WebsocketAuthenticationTest.kt
around line 153, the inline comment "This test should fail with insufficient
permissions - intentionally designed to fail" is misleading; update the comment
to state that the test verifies the system returns a forbidden response when
permissions are insufficient and that the test should pass when that behavior is
observed (e.g., change to "This test verifies that a request with insufficient
permissions returns a forbidden response").
|
|
||
| val tokenWithPrefix get() = "$PAT_PREFIX$token" |
There was a problem hiding this comment.
Handle null token explicitly.
The token field is nullable, but string interpolation with a null value produces the literal string "pat_null" instead of null. This could cause unexpected behavior in authentication flows where the calling code might expect null when the token is not available.
Consider one of these approaches:
- val tokenWithPrefix get() = "$PAT_PREFIX$token"
+ val tokenWithPrefix: String? get() = token?.let { "$PAT_PREFIX$it" }Or, if you want to enforce non-null:
- val tokenWithPrefix get() = "$PAT_PREFIX$token"
+ val tokenWithPrefix: String get() = "$PAT_PREFIX${token ?: error("Token is not available")}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val tokenWithPrefix get() = "$PAT_PREFIX$token" | |
| val tokenWithPrefix: String? get() = token?.let { "$PAT_PREFIX$it" } |
🤖 Prompt for AI Agents
In backend/data/src/main/kotlin/io/tolgee/model/Pat.kt around lines 45-46, token
is nullable but tokenWithPrefix uses string interpolation which yields
"pat_null" when token is null; change the getter to handle null explicitly —
either return null when token is null (e.g., token?.let { PAT_PREFIX + it }) or
if you want to enforce non-null change the token type to non-null and
validate/throw on creation; implement one of these approaches so tokenWithPrefix
never produces the literal "pat_null".
| const headers: Record<string, string> | null = options.authentication | ||
| .jwtToken | ||
| ? { | ||
| jwtToken: options.authentication.jwtToken, | ||
| Authorization: `Bearer ${options.authentication.jwtToken}`, | ||
| } | ||
| : null; | ||
|
|
||
| client.connect(headers, onConnected, onError, onDisconnect); |
There was a problem hiding this comment.
Fix type inconsistency for authentication.jwtToken.
The conditional check options.authentication.jwtToken ? { ... } : null treats jwtToken as optional, but the type definition declares it as a required field (jwtToken: string). This creates a type-safety gap.
Consider one of these solutions:
Solution 1 (Recommended): If jwtToken can be absent or empty, update the type to reflect this:
type WebsocketClientOptions = {
serverUrl?: string;
authentication: {
- jwtToken: string;
+ jwtToken?: string;
};
onConnected?: () => void;
onError?: () => void;
onConnectionClose?: () => void;
};Solution 2: If jwtToken must always be present, remove the conditional check:
-const headers: Record<string, string> | null = options.authentication
- .jwtToken
- ? {
- jwtToken: options.authentication.jwtToken,
- Authorization: `Bearer ${options.authentication.jwtToken}`,
- }
- : null;
+const headers: Record<string, string> = {
+ jwtToken: options.authentication.jwtToken,
+ Authorization: `Bearer ${options.authentication.jwtToken}`,
+};Note: Setting both jwtToken and Authorization headers with the same value appears redundant. Verify if both headers are required by the backend or if this is for backward compatibility.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const headers: Record<string, string> | null = options.authentication | |
| .jwtToken | |
| ? { | |
| jwtToken: options.authentication.jwtToken, | |
| Authorization: `Bearer ${options.authentication.jwtToken}`, | |
| } | |
| : null; | |
| client.connect(headers, onConnected, onError, onDisconnect); | |
| const headers: Record<string, string> = { | |
| jwtToken: options.authentication.jwtToken, | |
| Authorization: `Bearer ${options.authentication.jwtToken}`, | |
| }; | |
| client.connect(headers, onConnected, onError, onDisconnect); |
🤖 Prompt for AI Agents
In webapp/src/websocket-client/WebsocketClient.ts around lines 99 to 107, the
code conditionally builds headers based on options.authentication.jwtToken even
though the type declares jwtToken as required; update the types and code so they
are consistent: if jwtToken can be absent make jwtToken optional in the
authentication type (e.g., jwtToken?: string) and keep the conditional header
creation, or if jwtToken must always be present remove the conditional and
always create the headers; also verify whether both jwtToken and Authorization
headers are required and remove the redundant header if not.
Summary by CodeRabbit
New Features
Tests