Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Make password policy a standard feature#39213

Merged
evict merged 27 commits into
mainfrom
vincent/password-policy-standard-feature
Jul 26, 2022
Merged

Make password policy a standard feature#39213
evict merged 27 commits into
mainfrom
vincent/password-policy-standard-feature

Conversation

@evict

@evict evict commented Jul 21, 2022

Copy link
Copy Markdown
Contributor

This PR will add the, now experimental, password policy feature as a standard feature.

Test plan

  • Test changing password in usersettings
  • Test setting password on signup page
  • Test setting password on password reset

@cla-bot cla-bot Bot added the cla-signed label Jul 21, 2022
@evict evict changed the title Make password policy a standard feature Draft: Make password policy a standard feature Jul 21, 2022
@evict evict force-pushed the vincent/password-policy-standard-feature branch from b91c026 to 64af14b Compare July 21, 2022 09:05
@evict evict requested a review from eseliger July 21, 2022 09:15
@evict evict marked this pull request as draft July 21, 2022 09:15
@evict evict self-assigned this Jul 21, 2022
@evict evict requested a review from a team July 21, 2022 09:16
@evict evict force-pushed the vincent/password-policy-standard-feature branch 3 times, most recently from 01cb735 to 88e93dd Compare July 21, 2022 16:30

@eseliger eseliger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

did a pass, looks good so far, I think the linter / formatter will have a couple opinions on the indentation and such, did you configure eslint and prettier in your editor? Otherwise I'd strongly recommend using them as they make your life much easier :)

Other than that, I added a bunch of comments, mostly questions and some ideas thrown around, feel free to ignore them if you don't agree.

Comment thread schema/site.schema.json Outdated
Comment thread schema/site.schema.json Outdated
Comment thread schema/site.schema.json Outdated
Comment thread internal/conf/computed.go Outdated
Comment thread client/web/src/jscontext.ts Outdated
Comment thread client/web/src/user/settings/auth/UserSettingsSecurityPage.tsx Outdated
Comment thread internal/security/config_test.go Outdated
Comment thread internal/security/config.go Outdated
Comment thread client/web/src/util/security.ts Outdated
Comment thread client/web/src/util/security.ts Outdated
@evict evict marked this pull request as ready for review July 22, 2022 12:17
@evict evict changed the title Draft: Make password policy a standard feature Make password policy a standard feature Jul 22, 2022
@evict evict force-pushed the vincent/password-policy-standard-feature branch from 442d79d to 5bcba80 Compare July 22, 2022 12:52
@evict evict requested a review from eseliger July 22, 2022 13:03
@evict evict force-pushed the vincent/password-policy-standard-feature branch 3 times, most recently from 48327de to b47118b Compare July 25, 2022 11:53
@evict evict requested review from a team July 25, 2022 11:53

@mrnugget mrnugget left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 Only reviewed the Go parts. Left some suggestions, some of them nitpicky, but I think you should do another round on the tests to make them a bit more consistent with the rest of our codebase.

Comment thread internal/security/security_test.go Outdated
Comment thread internal/security/security_test.go Outdated
Comment thread internal/security/security_test.go Outdated
Comment thread internal/security/security_test.go Outdated
Comment thread internal/security/security_test.go Outdated
Comment thread internal/security/security_test.go Outdated
Comment thread internal/conf/computed.go Outdated
Comment thread internal/conf/computed.go Outdated
Comment thread internal/conf/computed.go Outdated
Comment thread cmd/frontend/internal/app/jscontext/jscontext.go Outdated
@evict evict force-pushed the vincent/password-policy-standard-feature branch from 4633c61 to da4da25 Compare July 26, 2022 12:31

@eseliger eseliger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, great work! :)

@evict evict merged commit 91724a1 into main Jul 26, 2022
@evict evict deleted the vincent/password-policy-standard-feature branch July 26, 2022 16:04
@unknwon

unknwon commented Jul 27, 2022

Copy link
Copy Markdown
Contributor

Congrats! I think this is CHANGELOG-worth 😁

@evict evict added the SSDLC label Oct 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants