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.

Sign up for GitHub

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

Jump to bottom

feat: make the password policy more configurable #2118

Merged
merged 11 commits into from
Jan 18, 2022
Merged

feat: make the password policy more configurable #2118

merged 11 commits into from
Jan 18, 2022

Conversation

RamiBerm
Copy link
Contributor

@RamiBerm RamiBerm commented Jan 7, 2022 •
edited
Loading

Added two new password config fields: min_password_length and identifier_similarity_check_enabled.

Related issue(s)

#970

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@RamiBerm RamiBerm changed the title Make the password policy more configurable feat: make the password policy more configurable Jan 11, 2022
aeneasr
aeneasr requested changes Jan 15, 2022
View reviewed changes
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Sorry for the late review! This looks great! I think we should still enforce a lower limit for password length (e.g. 6).

Also could you please add a guide that explains how to use this feature in the docs/docs/guide directory? :)

zepatrik
zepatrik requested changes Jan 17, 2022
View reviewed changes
embedx/config.schema.json Outdated
"title": "Minimum Password Length",
"description": "Defines the minimum length of the password.",
"type": "integer",
"default": 6
Copy link
Member

Choose a reason for hiding this comment

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

Please make the default 8. We changed the default to be 8 in #2009, and it had quite some implications especially for automated tests and the login UI, so I would like to keep that default value.

driver/config/config.go Outdated
HaveIBeenPwnedEnabled: p.p.BoolF(ViperKeyPasswordHaveIBeenPwnedEnabled, true),
MaxBreaches: uint(p.p.Int(ViperKeyPasswordMaxBreaches)),
IgnoreNetworkErrors: p.p.BoolF(ViperKeyIgnoreNetworkErrors, true),
MinPasswordLength: uint(p.p.IntF(ViperKeyPasswordMinLength, 6)),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
MinPasswordLength: uint(p.p.IntF(ViperKeyPasswordMinLength, 6)),
MinPasswordLength: uint(p.p.IntF(ViperKeyPasswordMinLength, 8)),

docs/docs/guides/password-policy.mdx Outdated
enabled: true
config:
haveibeenpwned_enabled: true
min_password_length: 6
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
min_password_length: 6
min_password_length: 8

docs/docs/guides/password-policy.mdx Outdated

#### `min_password_length`

The minimum length of the password. The default value is `6`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The minimum length of the password. The default value is `6`.
The minimum length of the password. The default value is `8`.

zepatrik
zepatrik approved these changes Jan 17, 2022
View reviewed changes
Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Nice, thank you 👍

aeneasr
aeneasr approved these changes Jan 18, 2022
View reviewed changes
Copy link
Member

@aeneasr aeneasr 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 so much!

@aeneasr aeneasr merged commit 70c627b into ory:master Jan 18, 2022
@kszafran
Copy link
Contributor

Thanks from me, too ❤️. We were just discussing a need for this functionality internally.

@RamiBerm RamiBerm deleted the more_configurable_password_policy branch January 18, 2022 14:39
@vinckr
Copy link
Member

vinckr commented Jan 18, 2022 •
edited
Loading

Hello @RamiBerm
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community.
Please drop me an email to claim your Ory swag!

Edit: sorry tagged the wrong person the first time

This was referenced Mar 9, 2022
Merged
Merged
@vinckr vinckr mentioned this pull request Mar 18, 2022
Merged
RamiBerm added a commit to RamiBerm/kratos that referenced this pull request Mar 20, 2022
@RamiBerm
feat: make the password policy more configurable (ory#2118)
44f406e 
Closes ory#970

Co-authored-by: aeneasr <[email protected]>
Conflicts:
	docs/sidebar.json
@vinckr vinckr mentioned this pull request Apr 22, 2022
Merged
7 tasks
peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023
@RamiBerm @aeneasr
feat: make the password policy more configurable (ory#2118)
a8855a1 
Closes ory#970

Co-authored-by: aeneasr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeneasr aeneasr aeneasr approved these changes

@zepatrik zepatrik zepatrik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RamiBerm @kszafran @vinckr @aeneasr @zepatrik