TF-3348 Move access token from persistent to memory#3404
TF-3348 Move access token from persistent to memory#3404tddang-linagora wants to merge 6 commits intomasterfrom
Conversation
tddang-linagora
temporarily deployed
to
PR-3404
GitHub Actions
Inactive
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/3404. |
dab246
left a comment
dab246
left a comment
There was a problem hiding this comment.
- Test more on browser 'Firefox',
Safari
|
tddang-linagora
temporarily deployed
to
PR-3404
GitHub Actions
Inactive
Screen.Recording.2025-01-07.at.15.12.51.mov
Screen.Recording.2025-01-07.at.15.21.50.mov |
|
Unit test will suffice. |
lib/features/login/data/local/web_token_oidc_cache_manager.dart
Outdated
Show resolved
Hide resolved
tddang-linagora
temporarily deployed
to
PR-3404
GitHub Actions
Inactive
Token refreshed fine Screen.Recording.2025-01-07.at.16.33.04.mov |
tddang-linagora
force-pushed
the
fixbug/TF-3348-move-access-token-from-persistent-to-memory
branch
from
e4a4267 to
8c7c051
Compare
tddang-linagora
dismissed
dab246’s stale review
The base branch was changed.
|
@tddang-linagora please cary over work on this. |
tddang-linagora
force-pushed
the
fixbug/TF-3348-move-access-token-from-persistent-to-memory
branch
from
8c7c051 to
2ed4011
Compare
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds platform-aware OIDC token caching for web by introducing 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/login/data/local/web_token_oidc_cache_manager.dart`:
- Around line 24-25: Remove raw token values from logs in
WebTokenOidcCacheManager::getTokenOidc(): do not log tokenSessionStorageCache,
tokenHiveCache or tokenOIDC.token directly; instead log non-sensitive metadata
(e.g., whether cache is present, timestamps, token type or masked token like
first/last 4 chars, or token length) and ensure any serialization of tokenOIDC
excludes token fields before logging. Locate uses of tokenSessionStorageCache,
tokenHiveCache, and tokenOIDC within getTokenOidc() and replace direct value/log
statements with sanitized or presence-only messages.
- Around line 53-54: The method deleteTokenOidc() in WebTokenOidcCacheManager is
annotated with `@override` but no parent declares it; remove the `@override`
annotation from deleteTokenOidc() in WebTokenOidcCacheManager, or alternatively
declare deleteTokenOidc() in the parent contract (e.g., add its signature to
TokenOidcCacheManager or CacheManagerInteraction) so the override is valid;
reference the deleteTokenOidc() method and the classes WebTokenOidcCacheManager,
TokenOidcCacheManager, and CacheManagerInteraction when making the change.
In `@test/main/bindings/local/local_bindings_test.dart`:
- Line 57: The test currently asserts expect(cacheManager,
isInstanceOf<TokenOidcCacheManager>()), which is a false-positive because
WebTokenOidcCacheManager is a subtype; update the assertion to ensure the
concrete non-web type is used by either asserting the runtime type equals
TokenOidcCacheManager (e.g. expect(cacheManager.runtimeType,
equals(TokenOidcCacheManager))) or asserting it is not the web implementation
(e.g. expect(cacheManager, isNot(isInstanceOf<WebTokenOidcCacheManager>()))),
referencing the cacheManager variable and the TokenOidcCacheManager and
WebTokenOidcCacheManager classes.
- Around line 22-27: The tests register globals in setUp (LocalBindings, Get.put
of MockFlutterSecureStorage/MockSharedPreferences/MockFileUtils) but never clear
them, causing cross-test leakage; add a tearDown block that resets Get and
clears/reset any shared state (e.g., call Get.reset() or delete the specific
registrations for MockFlutterSecureStorage, MockSharedPreferences,
MockFileUtils) and null/cleanup LocalBindings to ensure isolation between tests
and avoid relying on other test lines to perform cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df637b08-0101-4d43-8dd4-20335e4b9fc4
📒 Files selected for processing (7)
lib/features/caching/interaction/cache_manager_interaction.dartlib/features/login/data/extensions/token_oidc_extension.dartlib/features/login/data/local/token_oidc_cache_manager.dartlib/features/login/data/local/web_token_oidc_cache_manager.dartlib/main/bindings/local/local_bindings.dartlib/main/bindings/local/local_isolate_bindings.darttest/main/bindings/local/local_bindings_test.dart
lib/features/login/data/local/web_token_oidc_cache_manager.dart
Outdated
Show resolved
Hide resolved
lib/features/login/data/local/web_token_oidc_cache_manager.dart
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/features/login/data/local/web_token_oidc_cache_manager.dart (1)
24-24:⚠️ Potential issue | 🟠 MajorAvoid logging cached token objects directly.
Line 24 logs
tokenHiveCacheas a whole object. Even if currently safe, object/stringify changes can expose sensitive fields (notably refresh token). Prefer presence-only or non-sensitive metadata logging here.🔒 Proposed fix
- log('WebTokenOidcCacheManager::getTokenOidc(): tokenHiveCache: $tokenHiveCache'); + log( + 'WebTokenOidcCacheManager::getTokenOidc(): tokenHiveCache: ${tokenHiveCache != null ? "[present]" : "[missing]"}', + );Based on learnings: For the tmail-flutter repository, verify logs do not expose sensitive data in production logging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/login/data/local/web_token_oidc_cache_manager.dart` at line 24, In WebTokenOidcCacheManager::getTokenOidc replace the direct logging of the cached object (tokenHiveCache) with a presence-only or sanitized metadata log: log whether a cache entry exists and non-sensitive fields such as masked token presence (e.g., hasAccessToken/hasRefreshToken), expiry or scope, but do not stringify or print token values; update the log call to only emit that minimal metadata from tokenHiveCache (or null) instead of the full object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/features/login/data/local/web_token_oidc_cache_manager.dart`:
- Line 24: In WebTokenOidcCacheManager::getTokenOidc replace the direct logging
of the cached object (tokenHiveCache) with a presence-only or sanitized metadata
log: log whether a cache entry exists and non-sensitive fields such as masked
token presence (e.g., hasAccessToken/hasRefreshToken), expiry or scope, but do
not stringify or print token values; update the log call to only emit that
minimal metadata from tokenHiveCache (or null) instead of the full object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ecc4242-02b4-4739-b0aa-dca9432c09ad
📒 Files selected for processing (2)
lib/features/login/data/local/web_token_oidc_cache_manager.darttest/main/bindings/local/local_bindings_test.dart
✅ Files skipped from review due to trivial changes (1)
- test/main/bindings/local/local_bindings_test.dart
tddang-linagora
had a problem deploying
to
PR-3404
GitHub Actions
Failure
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lib/features/login/data/local/web_token_oidc_cache_manager.dart
Outdated
Show resolved
Hide resolved
lib/features/login/data/local/web_token_oidc_cache_manager.dart
Outdated
Show resolved
Hide resolved
tddang-linagora
had a problem deploying
to
PR-3404
GitHub Actions
Failure
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
4 participants
Issue
Demo
Should not refresh token if token is still valid and browser is refresh
Screen.Recording.2025-01-07.at.10.50.20.mov
Should not force user to login and refresh token if refresh token is still valid and new tab is open
Screen.Recording.2025-01-07.at.10.50.57.mov
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation