Skip to content
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

fixing password encoding in local storage #1245

Merged
merged 1 commit into from
Jun 19, 2024
Merged

Conversation

DGoiana
Copy link
Collaborator

@DGoiana DGoiana commented Jun 19, 2024

Closes #1244
We were not storing our passwords correctly, so when trying to retrieve it, decode would actually return null

Review checklist

  • Terms and conditions reflect the current change
  • Contains enough appropriate tests
  • If aimed at production, writes a new summary in whatsnew/whatsnew-pt-PT
  • Properly adds an entry in changelog.md with the change
  • If PR includes UI updates/additions, its description has screenshots
  • Behavior is as expected
  • Clean, well-structured code

Copy link

codecov bot commented Jun 19, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 17%. Comparing base (36b0f44) to head (5477255).

Additional details and impacted files
@@           Coverage Diff           @@
##           develop   #1245   +/-   ##
=======================================
+ Coverage       17%     17%   +1%     
=======================================
  Files          229     229           
  Lines         6952    6952           
=======================================
+ Hits          1147    1154    +7     
+ Misses        5805    5798    -7     

@thePeras thePeras requested a review from a team June 19, 2024 20:30
@DGoiana DGoiana merged commit 07e18da into develop Jun 19, 2024
6 checks passed
@DGoiana DGoiana deleted the fix/password-encoding branch June 19, 2024 20:32
try {
return encrypter.decrypt64(base64Text, iv: iv);
} catch (_) {
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is to sentry catch the error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a relevant error for sentry to catch? This happens if the password was encrypted with a different key/IV - we changed it ourselves - or if the store gets miraculously corrupted - unlikely. But in either case, we just want the user to perform login again, no?
Either way, if we wanted the exception to go to sentry, wouldn't it be better to add the code in the catch block to report the exception and return null either way?

Copy link
Member

@LuisDuarte1 LuisDuarte1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of "encrypting" the password (which is not encryption but just obfuscation) we should focus on supporting flutter_secure_storage and remove this step.

@limwa
Copy link
Member

limwa commented Jun 19, 2024

Yep, I agree. I suggested that on Slack and I think it's the way to go! What do the others think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Persistent session not working
5 participants