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

feat: extended password verification #2134

Merged
merged 4 commits into from
Oct 18, 2023

Conversation

saleniuk
Copy link
Contributor


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

Currently we can only verify if the whole password is valid or not, but some screens require to show which elements are still missing (like "one uppercase character").

Solutions

Change the use case to return the sealed class where Invalid contains multiple fields to know exactly which elements are still missing in the password.

Testing

Test Coverage (Optional)

  • I have added automated test to this contribution

PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 13, 2023

Unit Test Results

   458 files  ±0     458 suites  ±0   2m 17s ⏱️ -5s
2 550 tests +4  2 448 ✔️ +4  102 💤 ±0  0 ±0 

Results for commit 97aab37. ± Comparison against base commit a3963e9.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

Codecov Report

Merging #2134 (d87c7f2) into develop (675404c) will increase coverage by 0.01%.
Report is 1 commits behind head on develop.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             develop    #2134      +/-   ##
=============================================
+ Coverage      57.63%   57.64%   +0.01%     
  Complexity        24       24              
=============================================
  Files           1031     1031              
  Lines          38704    38719      +15     
  Branches        3519     3522       +3     
=============================================
+ Hits           22306    22321      +15     
  Misses         14896    14896              
  Partials        1502     1502              
Files Coverage Δ
...lium/logic/feature/auth/ValidatePasswordUseCase.kt 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 675404c...d87c7f2. Read the comment docs.

@saleniuk saleniuk requested review from a team, alexandreferris, MohamadJaara, vitorhugods, mchenani and Garzas and removed request for a team October 16, 2023 13:14
Copy link
Member

@vitorhugods vitorhugods left a comment

Choose a reason for hiding this comment

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

👨🏻‍🍳🤌 Nice!

Nice overall. Please ignore my comment as it's just food for thought really.

Comment on lines +67 to +71
val missingLowercaseCharacter: Boolean = true,
val missingUppercaseCharacter: Boolean = true,
val missingDigit: Boolean = true,
val missingSpecialCharacter: Boolean = true,
val tooShort: Boolean = true,
Copy link
Member

@vitorhugods vitorhugods Oct 16, 2023

Choose a reason for hiding this comment

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

thought: maybe use a Set<Criteria> instead of multiple fields.

For example:

enum class Criteria {
    LOWERCASE, UPPERCASE, NUMERIC_DIGIT, SPECIAL_CHARACTER, MINIMUM_LENGTH;
}

sealed interface ValidatePasswordResult {
    data object Valid: ValidatePasswordResult()
    data class Invalid(val missingCriteria: Set<Criteria>)
}

In any case, not something I'd change now that it's done. Just food for thought really. Aiming to make the API a tiny bit more scalable.

@saleniuk saleniuk added this pull request to the merge queue Oct 16, 2023
@saleniuk saleniuk removed this pull request from the merge queue due to a manual request Oct 16, 2023
@datadog-wireapp
Copy link

datadog-wireapp bot commented Oct 16, 2023

Datadog Report

All test runs 030d6f5 🔗

2 Total Test Services: 0 Failed, 0 with New Flaky, 2 Passed

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Wall Time Branch View
kalium-ios 0 0 0 1769 30 10.09s Link
kalium-jvm 0 0 0 2448 102 10m 33s Link

@saleniuk saleniuk added this pull request to the merge queue Oct 17, 2023
@saleniuk saleniuk removed this pull request from the merge queue due to a manual request Oct 17, 2023
@saleniuk saleniuk added this pull request to the merge queue Oct 18, 2023
Merged via the queue into develop with commit c82d8aa Oct 18, 2023
@saleniuk saleniuk deleted the feat/extended_password_verification branch October 18, 2023 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants