Skip to content

#1306 Add message password policy#1330

Merged
tomholub merged 16 commits intomasterfrom
feature/issue-1618-message-password-policy
Jan 27, 2022
Merged

#1306 Add message password policy#1330
tomholub merged 16 commits intomasterfrom
feature/issue-1618-message-password-policy

Conversation

@sosnovsky
Copy link
Collaborator

@sosnovsky sosnovsky commented Jan 24, 2022

This PR adds message password policy for checking password strength

close #1306


Tests (delete all except exactly one):

  • Tests added or updated - updated SendEmailToRecipientWithoutPublicKey test

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@sosnovsky
Copy link
Collaborator Author

@tomholub here is a video of current implementation:

Simulator.Screen.Recording.-.iPhone.13.-.2022-01-24.at.14.56.55.mp4

What about notifying users about weak password before tapping Send - for example, we can show warning instead of green Web portal password added message. Do we need it or current implementation is enough for now?

@sosnovsky sosnovsky requested a review from tomholub January 24, 2022 13:01
@tomholub
Copy link
Collaborator

What about notifying users about weak password before tapping Send - for example, we can show warning instead of green Web portal password added message. Do we need it or current implementation is enough for now?

Agree, actually best would be still on the modal where the user inputs it.

Best would be:

  • the modal should describe the requirements for the password
  • there should be visual indication to show if the password is sufficient. Maybe color of the input and the set button
  • user should not be allowed to set it unless it fits the requirements

I don't know if we are trying to do more than what is reasonable to expect from a modal - do you think this needs its own view? Or should modal be enough for this?

@tomholub
Copy link
Collaborator

On browser extension we color it in red while it's non-conforming, plus it shows the requirements above:

image

On Android it evaluates them one by one which is an overkill, I don't want to do that on iOS. But again there is indication if it conforms + list of requirements.

image

@sosnovsky
Copy link
Collaborator Author

I think it's possible to do such check using UIAlertViewController - Set button will be disabled until all requirements are met. And I'll also add all requirements to add password modal

@sosnovsky sosnovsky changed the title #1618 Add message password policy #1306 Add message password policy Jan 24, 2022
@sosnovsky
Copy link
Collaborator Author

I updated implementation to check password strength when user types it in modal, here is video:

Simulator.Screen.Recording.-.iPhone.13.-.2022-01-25.at.16.02.53.mp4

@sosnovsky sosnovsky marked this pull request as ready for review January 25, 2022 14:06
@sosnovsky sosnovsky requested a review from tomholub January 25, 2022 14:07
@tomholub
Copy link
Collaborator

Yup - simple, effective, clear.

tomholub
tomholub previously approved these changes Jan 25, 2022
Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

Code looks good 👍

tomholub
tomholub previously approved these changes Jan 26, 2022
# Conflicts:
#	FlowCrypt/Controllers/Compose/ComposeViewController.swift
tomholub
tomholub previously approved these changes Jan 26, 2022
tomholub
tomholub previously approved these changes Jan 26, 2022
Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

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

👍

@tomholub tomholub merged commit 34cc874 into master Jan 27, 2022
@tomholub tomholub deleted the feature/issue-1618-message-password-policy branch January 27, 2022 19:48
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.

password policy for message passwords

3 participants