-
Notifications
You must be signed in to change notification settings - Fork 7
Clear storage on logout #34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
gjwgit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ensure there is a final new line in the files - This is indicated by the red circled dash in git. All files should finish with a new line.
|
This needs a linked issue and an explanation as to it's purpose - why was it implemented and what issue does it solve? |
Hi Graham, To clarify, this PR is specifically for clearing all local storage/cache upon logout. It fixes a bug where guest users could potentially see the previous user's data because the cache wasn't purged after logout. Also, it's a prerequisite for our GeoPod preloading strategy. We need a clean slate to ensure the sync logic doesn't clash with stale data. And this change is fully backward-compatible. Note on the "Login Success" bug: I initially thought the issue where "Login Successfully" popped up prematurely was a cache problem, but I’ve since found the root cause in solidpod and solidui, the handler was mistakenly routing to SolidDefaultLogin (which is just a static UI) instead of the actual login form. Although the "fix" for that specific UI routing is in the other repos, the storage clearing logic here is essential to make sure the logout flow is complete and secure. I didn't link the issue earlier because it was a quick fix during the sync, but I'll be more careful with the workflow next time. Let me know if you need anything else! |
|
cdawei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lambadajester50-byte, thanks for the PR.
I can run the solid_auth example app without any obvious problems, so this is a good sign.
However, it is unclear what the exact problem this PR solves (the high level description in comment #34 (comment) seems to imply a problem related to cached auth data). Do you have an issue in GeoPod that provides more details of the problem?
lib/solid_auth_client.dart
Outdated
| authManager.clearLocalStorage(); | ||
| } catch (e) { | ||
| debugPrint('logout() => Warning: Failed to clear storage: $e'); | ||
| // Continue anyway - the important part is clearing auth data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand this comment -- if there is an exception from Line 326, how do we know the auth data has been cleared?
When testing the solid_auth example app on Linux, I got the following message:
logout() => Warning: Failed to clear storage: Unsupported operation: Cannot create a keyfinder without the packages dart:html or package:shared_preferences
| try { | ||
| windowLoc.localStorage.clear(); | ||
| } catch (e) { | ||
| throw 'Failed to clear localStorage: $e'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good reason to throw a string object instead of an instance of Exception or Error? (as documented here https://dart.dev/language/error-handling)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @cdawei ,
Sorry for the confusion earlier. To clarify, this PR is specifically intended to clear SharedPreferences (and local storage) data upon logout.
I have updated the code to address your concerns:
Platform-specific fixes: I have refactored the logic to properly handle cross-platform scenarios. On Web, it clears localStorage; on native platforms (Linux, Windows, macOS, iOS, Android), it clears SharedPreferences. This ensures the Unsupported operation error no longer occurs.
Error Handling: I’ve updated the throw statements to use proper Exception objects instead of strings, following the Dart style guide.
Motivation: Within GeoPod, I am using local caching after a successful login to improve performance. This PR ensures that all cached auth and session data are securely cleared when a user logs out. This implementation also provides a reusable pattern for clearing other pod-specific caches in the future.
I 've pushed the updated commits. Thanks for the feedback!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Platform-specific fixes: I have refactored the logic to properly handle cross-platform scenarios. On Web, it clears localStorage; on native platforms (Linux, Windows, macOS, iOS, Android), it clears SharedPreferences. This ensures the Unsupported operation error no longer occurs.
Thanks for the updates, @lambadajester50-byte. I no longer see the error message mentioned here with the latest commits.
Error Handling: I’ve updated the throw statements to use proper Exception objects instead of strings, following the Dart style guide.
Great!
Motivation: Within GeoPod, I am using local caching after a successful login to improve performance. This PR ensures that all cached auth and session data are securely cleared when a user logs out. This implementation also provides a reusable pattern for clearing other pod-specific caches in the future.
Unfortunately, I am still struggling to understand the exact problem. As far as I can tell, there are three different places that the authentication data might be cached, i.e., GeoPod, solid-auth, and the POD server.
- GeoPod uses solidpod which caches auth data through flutter secure storage (related to PR fix login issue solidpod#499 but not this PR).
- I suspect this PR is related to how auth data is cached in
solid-auth, but I am not aware of the exact problem... We probably need @anushkavidanage's help. - Then there is another issue related to the POD server (and also
solid-auth?), as described in SOLID POD: Logout properly solidpod#132
| try { | ||
| windowLoc.localStorage.clear(); | ||
| } catch (e) { | ||
| throw 'Failed to clear localStorage: $e'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Platform-specific fixes: I have refactored the logic to properly handle cross-platform scenarios. On Web, it clears localStorage; on native platforms (Linux, Windows, macOS, iOS, Android), it clears SharedPreferences. This ensures the Unsupported operation error no longer occurs.
Thanks for the updates, @lambadajester50-byte. I no longer see the error message mentioned here with the latest commits.
Error Handling: I’ve updated the throw statements to use proper Exception objects instead of strings, following the Dart style guide.
Great!
Motivation: Within GeoPod, I am using local caching after a successful login to improve performance. This PR ensures that all cached auth and session data are securely cleared when a user logs out. This implementation also provides a reusable pattern for clearing other pod-specific caches in the future.
Unfortunately, I am still struggling to understand the exact problem. As far as I can tell, there are three different places that the authentication data might be cached, i.e., GeoPod, solid-auth, and the POD server.
- GeoPod uses solidpod which caches auth data through flutter secure storage (related to PR fix login issue solidpod#499 but not this PR).
- I suspect this PR is related to how auth data is cached in
solid-auth, but I am not aware of the exact problem... We probably need @anushkavidanage's help. - Then there is another issue related to the POD server (and also
solid-auth?), as described in SOLID POD: Logout properly solidpod#132
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
| // SOFTWARE. | ||
| /// | ||
| /// Authors: Anushka Vidanage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be your name?
|
Hi @cdawei , Thank you very much for your patient guidance and for pointing this out. I sincerely apologize for the confusion. After a thorough re-examination of the entire cache handling logic, I now realize that this implementation is indeed redundant. This code was originally intended to address geopod/issues/13, but I've confirmed that the registerLogoutCacheCallback mechanism in solidpod already provides the standard and correct way to clear app-specific private data. I will close this PR now to avoid further clutter. Thank you again for your time and for helping me better understand the architecture. I'm sorry for any inconvenience this may have caused! Best regards, Miduo |
Thank you for investigating this! |
Pull Request Details
What issue does this PR address
Associated Issue
Type of Change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist
Complete the check-list below to ensure your branch is ready for PR.
make preporflutter analyze lib)dart testoutput or screenshot included in issue #Finalising
Once PR discussion is complete and reviewers have approved: