feat: Add WebSocket authentication strategy pattern and Email/get batching #4261
Conversation
## WebSocket Authentication Strategy Pattern Implements a flexible strategy pattern for WebSocket authentication to support multiple JMAP servers (James, Stalwart, and reverse proxy setups). ### New Files: - `web_socket_auth_strategy.dart` - Abstract interface for auth strategies - `ticket_auth_strategy.dart` - James server ticket-based authentication - `token_auth_strategy.dart` - OIDC/OAuth2 token authentication (Stalwart) - `basic_auth_strategy.dart` - HTTP Basic authentication for reverse proxies - `no_auth_strategy.dart` - Fallback for unauthenticated connections - `web_socket_auth_strategy_selector.dart` - Automatic strategy selection - `web_socket_capability_provider.dart` - JMAP capability detection - `web_socket_uri_builder.dart` - URI utilities with credential redaction ### Documentation: - `docs/websocket-auth-proxy.md` - Reverse proxy configuration guide ### Why This Is Needed: Browser WebSocket connections cannot set custom HTTP headers (like Authorization), so authentication must be passed via query parameters. Different servers support different authentication methods: - James: Uses proprietary ticket-based auth (jmapWebSocketTicket capability) - Stalwart: Supports OIDC tokens via access_token query parameter - Reverse proxies: Can inject auth headers from query parameters Reference: stalwartlabs/stalwart#2680 ## Email/get Batch Size Fix Implements batched Email/get requests to respect server maxObjectsInGet limits. ### Changes: - Added `getEmailsByIdsBatched` method in `MailApiMixin` for batched fetching - Added `getMaxObjectsInGetMethod` to query server capabilities - Refactored `ThreadApi.getChanges` to use batched requests - Added `defaultMaxObjectsInGet = 50` constant ### Why This Is Needed: Stalwart and other JMAP servers enforce maxObjectsInGet limits. When loading threads with many emails (e.g., after WebSocket push notification), the client must batch requests to avoid exceeding server limits. ## Test Coverage: - 20 tests for authentication strategies (TokenAuth, BasicAuth, NoAuth, TicketAuth) - 5 tests for strategy selector logic - 13 tests for capability provider - 16 tests for URI builder utilities Co-Authored-By: Claude Opus 4.5 <[email protected]>
WalkthroughAdds a WebSocket push-notification strategy system and supporting utilities. Introduces an abstract Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/features/base/mixin/mail_api_mixin.dart`:
- Around line 159-167: The current code unconditionally assigns latestState =
emailResponse?.state which can overwrite a previously valid latestState with
null; change the logic in the batch handling (where emailResponse is processed
alongside allEmails and allNotFoundIds) to only update latestState when
emailResponse?.state is non-null (e.g., check emailResponse?.state != null
before assigning to latestState) so existing state is preserved when a batch
response has no state.
In
`@lib/features/push_notification/data/datasource/strategies/ticket_auth_strategy.dart`:
- Around line 42-50: The buildConnectionUri method in TicketAuthStrategy
currently constructs the URI via string interpolation which breaks when baseUri
already has query parameters; replace that logic by calling
baseUri.replace(queryParameters: {...}) to merge existing query parameters with
the ticket parameter returned from _webSocketApi.getWebSocketTicket(session,
accountId) (i.e., get the webSocketTicket, create a new map copying
baseUri.queryParameters and adding 'ticket': webSocketTicket, then call
baseUri.replace with that map) so the resulting Uri is built safely and
consistently with TokenAuthStrategy/BasicAuthStrategy.
🧹 Nitpick comments (4)
lib/features/push_notification/data/datasource/strategies/token_auth_strategy.dart (1)
44-49: UseWebSocketUriBuilder.addQueryParamfor consistency.This duplicates the URI query parameter logic that
WebSocketUriBuilder.addQueryParamwas created for. Using the utility improves consistency across strategies and centralizes URI manipulation logic.♻️ Suggested refactor
+import 'package:tmail_ui_user/features/push_notification/data/utils/web_socket_uri_builder.dart'; // In buildConnectionUri: if (token != null && token.isNotEmpty) { - return baseUri.replace(queryParameters: { - ...baseUri.queryParameters, - 'access_token': token, - }); + return WebSocketUriBuilder.addQueryParam(baseUri, 'access_token', token); }lib/features/push_notification/data/datasource/strategies/basic_auth_strategy.dart (1)
44-49: UseWebSocketUriBuilder.addQueryParamfor consistency.Same refactor opportunity as in
TokenAuthStrategy. Using the utility centralizes URI manipulation and ensures consistent behavior across all auth strategies.♻️ Suggested refactor
+import 'package:tmail_ui_user/features/push_notification/data/utils/web_socket_uri_builder.dart'; // In buildConnectionUri: if (basicAuth != null && basicAuth.isNotEmpty) { - return baseUri.replace(queryParameters: { - ...baseUri.queryParameters, - 'authorization': 'Basic $basicAuth', - }); + return WebSocketUriBuilder.addQueryParam(baseUri, 'authorization', 'Basic $basicAuth'); }test/features/push_notification/data/datasource/strategies/auth_strategies_test.dart (2)
96-120: Consider adding a test for empty credentials in BasicAuthStrategy.The
TokenAuthStrategygroup includes tests for both null and empty tokens (lines 45-69), butBasicAuthStrategyonly tests for null credentials. For consistency and completeness, consider adding a test case for empty credentials.💡 Suggested additional test case
test('should return base URI when credentials are empty', () async { // Arrange when(mockAuthInterceptors.basicAuthCredentials).thenReturn(''); final session = _createTestSession(); // Act final result = await strategy.buildConnectionUri(baseUri, session, testAccountId); // Assert expect(result.queryParameters.containsKey('authorization'), isFalse); expect(result, equals(baseUri)); });
142-165: Consider adding error handling test for TicketAuthStrategy.The current test only covers the happy path. Consider adding a test case for when
getWebSocketTicketthrows an exception to document the expected error behavior.💡 Suggested additional test case
test('should propagate exception when WebSocket API fails', () async { // Arrange final session = _createTestSession(); when(mockWebSocketApi.getWebSocketTicket(session, testAccountId)) .thenThrow(Exception('Network error')); // Act & Assert expect( () => strategy.buildConnectionUri(baseUri, session, testAccountId), throwsException, ); });
lib/features/push_notification/data/datasource/strategies/ticket_auth_strategy.dart
Show resolved
Hide resolved
The previous implementation used string interpolation which would break
when baseUri already had query parameters (e.g., producing double '?').
Changed to use baseUri.replace(queryParameters: {...}) to safely merge
existing query parameters with the ticket parameter, consistent with
TokenAuthStrategy and BasicAuthStrategy implementations.
Added test case to verify existing query parameters are preserved.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Previously latestState was unconditionally assigned from emailResponse?.state, which could overwrite a valid state from a previous batch with null if a subsequent batch response had no state. Now only updates latestState when emailResponse?.state is non-null, preserving valid state across batches. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/features/base/mixin/mail_api_mixin.dart`:
- Around line 119-127: The code doesn't guard against batchSize (or the result
of getMaxObjectsInGetMethod) being <= 0 and against negative maxEmailsToFetch,
which can cause division-by-zero, infinite loops, or RangeError in idsToFetch;
fix by clamping the effective batch size and requested max emails: compute
maxObjectsInGet = max(1, batchSize ?? getMaxObjectsInGetMethod(session,
accountId)) to ensure at least 1, and when building idsToFetch, if
maxEmailsToFetch != null clamp it to the range 0..emailIds.length before calling
emailIds.sublist(0, clampedValue); apply these changes where maxObjectsInGet and
idsToFetch are defined (referencing batchSize, getMaxObjectsInGetMethod,
maxEmailsToFetch, idsToFetch).
🧹 Nitpick comments (1)
test/features/push_notification/data/datasource/strategies/auth_strategies_test.dart (1)
87-121: Consider adding test for empty credentials and preserving existing query parameters.For consistency with
TokenAuthStrategytests, consider adding:
- A test case for when
basicAuthCredentialsreturns an empty string- A test case verifying existing query parameters are preserved when credentials are added
This would improve coverage parity across strategies.
Suggested additional tests
test('should return base URI when credentials are empty', () async { // Arrange when(mockAuthInterceptors.basicAuthCredentials).thenReturn(''); final session = _createTestSession(); // Act final result = await strategy.buildConnectionUri(baseUri, session, testAccountId); // Assert expect(result.queryParameters.containsKey('authorization'), isFalse); expect(result, equals(baseUri)); }); test('should preserve existing query parameters', () async { // Arrange const testCredentials = 'dXNlcm5hbWU6cGFzc3dvcmQ='; when(mockAuthInterceptors.basicAuthCredentials).thenReturn(testCredentials); final uriWithParams = Uri.parse('wss://example.com/ws?existing=param'); final session = _createTestSession(); // Act final result = await strategy.buildConnectionUri(uriWithParams, session, testAccountId); // Assert expect(result.queryParameters['existing'], equals('param')); expect(result.queryParameters['authorization'], equals('Basic $testCredentials')); });
- Ensure maxObjectsInGet is at least 1 using max(1, ...) to prevent infinite loops or division by zero when batchSize <= 0 - Clamp maxEmailsToFetch to valid range [0, emailIds.length] to prevent RangeError when negative values are passed to sublist() Co-Authored-By: Claude Opus 4.5 <[email protected]>
| int getMaxObjectsInGetMethod(Session session, AccountId accountId) { | ||
| final coreCapability = session.getCapabilityProperties<CoreCapability>( | ||
| accountId, | ||
| CapabilityIdentifier.jmapCore, | ||
| ); | ||
| final maxObjectsInGetMethod = | ||
| coreCapability?.maxObjectsInGet?.value.toInt() ?? | ||
| CapabilityIdentifierExtension.defaultMaxObjectsInGet; | ||
|
|
||
| final minOfMaxObjectsInGetMethod = min( | ||
| maxObjectsInGetMethod, | ||
| CapabilityIdentifierExtension.defaultMaxObjectsInGet, | ||
| ); | ||
| log('$runtimeType::getMaxObjectsInGetMethod:minOfMaxObjectsInGetMethod = $minOfMaxObjectsInGetMethod'); | ||
| return minOfMaxObjectsInGetMethod; | ||
| } |
There was a problem hiding this comment.
Please use getMaxObjectsInGet method in session_extension.dart file
- Replace custom getMaxObjectsInGetMethod with session.getMaxObjectsInGet from session_extension.dart as suggested in PR review - Change emailResponse?.list != null to emailResponse?.list.isNotEmpty == true to avoid adding empty lists Addresses review comments on PR linagora#4261 Co-Authored-By: Claude Opus 4.5 <[email protected]>
| /// | ||
| /// The [maxCreatedEmailsToFetch] parameter enables early termination when only | ||
| /// a limited number of new emails are needed (e.g., for UI preview). | ||
| Future<EmailChangeResponse> getChanges( |
|
So does Twake Mail backend, though we accept high limits on metadata. Please @dab246 @hoangdat this bug would definitly affect us so please consider splitting that fix and integrating it in Twake mail as well. |
|
we didnot use
|
While this is true for users relying on tmail server this is apparently not enough for users of Stallwart servers. IMO we should be open to such a contributions. |
| State? latestState; | ||
|
|
||
| // If maxEmailsToFetch is specified, clamp it to valid range and only fetch up to that many IDs | ||
| final idsToFetch = maxEmailsToFetch != null |
There was a problem hiding this comment.
There is NO continuation logic for ignored IDs. The maxEmailsToFetch parameter creates an irreversible data loss if used .Emails beyond the limit are discarded without tracking, and callers have no mechanism to fetch them later.
| return baseUri.replace(queryParameters: { | ||
| ...baseUri.queryParameters, | ||
| 'access_token': token, |
There was a problem hiding this comment.
What happens if baseUri already have valid, unexpired access_token, but the _authInterceptors.currentToken is expired?
| Properties? propertiesCreated, | ||
| Properties? propertiesUpdated | ||
| Properties? propertiesUpdated, | ||
| int? maxCreatedEmailsToFetch, |
There was a problem hiding this comment.
Where is this maxCreatedEmailsToFetch parameter used?
5 participants
Summary
This PR implements two key features for improved JMAP server compatibility:
1. WebSocket Authentication Strategy Pattern
Implements a flexible strategy pattern for WebSocket authentication to support multiple JMAP servers:
jmapWebSocketTicketcapabilityaccess_tokenquery parameterWhy This Is Needed
Browser WebSocket connections cannot set custom HTTP headers (like
Authorization), so authentication must be passed via query parameters. Different servers support different authentication methods.Reference: stalwartlabs/stalwart#2680
New Strategy Files
web_socket_auth_strategy.dartticket_auth_strategy.darttoken_auth_strategy.dartbasic_auth_strategy.dartno_auth_strategy.dartweb_socket_auth_strategy_selector.dartweb_socket_capability_provider.dartweb_socket_uri_builder.dartDocumentation
docs/websocket-auth-proxy.md- Reverse proxy configuration guide for nginx, Caddy, Traefik, and HAProxy2. Email/get Batch Size Fix
Implements batched
Email/getrequests to respect servermaxObjectsInGetlimits.Changes
getEmailsByIdsBatchedmethod inMailApiMixinfor batched fetchinggetMaxObjectsInGetMethodto query server capabilitiesThreadApi.getChangesto use batched requests with early terminationdefaultMaxObjectsInGet = 50constantWhy This Is Needed
Stalwart and other JMAP servers enforce
maxObjectsInGetlimits. When loading threads with many emails (e.g., after WebSocket push notification), the client must batch requests to avoid exceeding server limits.Test Plan
Test Coverage
auth_strategies_test.dartweb_socket_auth_strategy_selector_test.dartweb_socket_capability_provider_test.dartweb_socket_uri_builder_test.dartBreaking Changes
None. The refactoring maintains backward compatibility:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.