Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

feat: add password validation #86

Merged
merged 23 commits into from
Oct 18, 2021
Merged

feat: add password validation #86

merged 23 commits into from
Oct 18, 2021

Conversation

KaWaite
Copy link
Member

@KaWaite KaWaite commented Sep 16, 2021

Overview

The passwordModal for changing your password merely disables the submit button unless new password and confirmation password match. This adds validation and feedback to make sure users are using secure passwords.

What I've done

  • Small update to TextBox to allow for feedback messages
  • Added several regex checks for validation (based on Auth0 setup) as a password policy (setup to not need much changed if begin receiving unique policy from backend (aka after auth is opened to other options other than Auth0))
    • Note: passwordPolicy is setup the way it is so in the future we could fetch the passwordPolicy from the backend (when we are auth-agnostic and not relying on Auth0).
  • Some small fixes/improvements
    • language updates
    • link update/opens new page

Please check

  • Please add Japanese translations where appropriate.

Memo

  • Depending on how we implement Authentication locally in the future, we COULD potentially make the PrivacyPolicy have a RegEx AND Text field for each check (which would be passed from the backends Config) and then populate the PasswordModal with the Text to show a detailed description of the password requirements being used to the user. But I have read being too specific about a password policy can be bad and lead to someone figuring out passwords easier.

@netlify
Copy link

netlify bot commented Sep 16, 2021

❌ Deploy Preview for reearth-web failed.

🔨 Explore the source changes: a913b27

🔍 Inspect the deploy log: https://app.netlify.com/sites/reearth-web/deploys/616d167235ee360007a6adf3

@codecov
Copy link

codecov bot commented Sep 16, 2021

Codecov Report

Merging #86 (a913b27) into main (778395e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #86   +/-   ##
=======================================
  Coverage   53.53%   53.53%           
=======================================
  Files          48       48           
  Lines         891      891           
  Branches      119      119           
=======================================
  Hits          477      477           
  Misses        364      364           
  Partials       50       50           

@KaWaite KaWaite marked this pull request as ready for review September 22, 2021 02:26
KaWaite added 2 commits September 22, 2021 11:39
Move regex code out of function.
Copy link
Member

@rot1024 rot1024 left a comment

Choose a reason for hiding this comment

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

It turns out that this PR actually contains a problem that we need to think about.

  • Why do you limit passwords to between 5 and 25 characters? It is usually safe to not limit the maximum number of characters unless it is very long.
  • Is the password policy in sync with the backend implementation and the OAuth provider's settings? If you don't have such a restriction in the backend or the provider, then trying to restrict it only in the frontend will result in a restriction that doesn't make much sense from a security perspective.
  • Don't forget that users can also change the password from the login page provided by the OAuth provider. What if a different password policy is applied there?

Re:Earth is OSS, so we never know who will use it and where will be used it, and even what OAuth2 provider will be used. Considering the possibility that various password policies may be required, it would be desirable to be able to change the password policy in, for example, reearth_config.json.

@KaWaite KaWaite marked this pull request as draft September 22, 2021 03:36
@KaWaite KaWaite marked this pull request as ready for review October 4, 2021 02:01
@KaWaite KaWaite requested a review from rot1024 October 4, 2021 02:02
@rot1024
Copy link
Member

rot1024 commented Oct 4, 2021

Additional implementation: to make password policy changeable by reearth-config.json (https://github.com/reearth/reearth-web/blob/main/src/config.ts)

  • note: if no password policy is set, there is no limitation
  • note: JSON does not support regexp, so Re:Earth should parse string and handle errors (if there is any error, treat as no password policy is set)

Copy link
Member

@HideBa HideBa left a comment

Choose a reason for hiding this comment

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

Thank you for your implementation. 👍
I've left a couple of comments. Plz, check them.

src/components/molecules/Common/ProjectMenu/index.tsx Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/components/atoms/TextBox/index.tsx Outdated Show resolved Hide resolved
@KaWaite KaWaite requested a review from HideBa October 11, 2021 06:13
HideBa
HideBa previously approved these changes Oct 11, 2021
Copy link
Member

@HideBa HideBa left a comment

Choose a reason for hiding this comment

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

Seems good! 👍

src/components/organisms/Settings/Account/hooks.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/components/organisms/Settings/Account/hooks.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
@KaWaite KaWaite merged commit 2017aa3 into main Oct 18, 2021
@KaWaite KaWaite deleted the feat-password-validation branch October 18, 2021 08:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants