Skip to content

#2849 - Generate random password for users #2866

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
+260 −64
Open

#2849 - Generate random password for users #2866

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
cf4c8de
some initial work
SebinSong Jul 6, 2025
a757a8b
build a auto-password field UI
SebinSong Jul 7, 2025
b4754ba
implement copy / add feedback tooltip
SebinSong Jul 8, 2025
f49da1d
signup call uses the auto-gen PW / update validation logic / add the …
SebinSong Jul 8, 2025
65df525
update cypress test according to the updated user-flow
SebinSong Jul 8, 2025
7a98167
Merge remote-tracking branch 'origin/master' into sebin/task/#2849-ra…
SebinSong Jul 16, 2025
ec8e31b
Merge remote-tracking branch 'origin/master' into sebin/task/#2849-ra…
SebinSong Jul 22, 2025
197b783
Merge remote-tracking branch 'origin/master' into sebin/task/#2849-ra…
SebinSong Aug 4, 2025
3a35173
install multiformats and use it for random-pw / typo fix
SebinSong Aug 4, 2025
a96c8b6
update per PR feedbacks
SebinSong Oct 3, 2025
ff4a644
copy btn design update
SebinSong Oct 6, 2025
d9d765f
update UI per Greg request
SebinSong Oct 6, 2025
4a692d1
update cypress test according to the new user-flow
SebinSong Oct 6, 2025
b7092b1
resolve LLM suggestions
SebinSong Oct 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions frontend/assets/style/_mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,21 @@ $payment-table-desktop-bp: 1290px;
width: $dimensions;
}

@mixin tooltip-style-common {
position: absolute;
top: 0;
left: 0;
min-width: 3rem;
max-width: 12rem;
border-radius: 3px;
padding: 0.5rem;
z-index: 50; // $zindex-tooltip: 50;
pointer-events: none;
background-color: var(--text_0);
opacity: 0.95;
color: var(--background_0);
}

@mixin overflow-touch {
-webkit-overflow-scrolling: touch;
}
Expand Down
13 changes: 1 addition & 12 deletions frontend/views/components/Tooltip.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,18 +375,7 @@ export default ({
.c-tooltip,
.c-anchored-tooltip {
position: absolute;
top: 0;
left: 0;
min-width: 3rem;
max-width: 12rem;
border-radius: $radius;
padding: 0.5rem;
z-index: $zindex-tooltip;
pointer-events: none;
background-color: $text_0;
opacity: 0.95;
color: $background_0;
@include tooltip-style-common;
&.has-text-center {
text-align: center;
Expand Down
128 changes: 119 additions & 9 deletions frontend/views/containers/access/PasswordForm.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,29 @@
<template lang='pug'>
label.field
component.field(:is='mode === "manual" ? "label" : "div"')
.label(v-if='label') {{ label }}

.inputgroup.c-mode-auto(
v-if='mode === "auto"'
v-error:[name]=''
)
.input.width-with-single-addon.has-ellipsis.c-auto-password(
:data-test='name'
) {{ ephemeral.randomPassword }}

.addons
button.is-success.c-copy-btn(
type='button'
@click.stop='copyPassword'
)
i18n Copy

i18n.c-feedback(
v-if='ephemeral.showCopyFeedback'
) Copied to clipboard!

.inputgroup(
v-error:[name]='{ attrs: { "data-test": "badPassword" }}'
v-else
v-error:[name]=''
)
input.input.with-single-addon(
:type='isLock ? "password" : "text"'
Expand All @@ -17,7 +38,6 @@ label.field
.addons
button.is-icon(
type='button'
v-if='hasIconRight'
:aria-label='L("Toggle password visibility")'
:aria-pressed='!isLock'
@click.prevent='isLock = !isLock'
Expand All @@ -26,13 +46,22 @@ label.field
</template>

<script>
import Tooltip from '@components/Tooltip.vue'
import validationsDebouncedMixins from '@view-utils/validationsDebouncedMixins.js'
import { L } from '@common/common.js'

export default ({
name: 'PasswordForm',
components: {
Tooltip
},
data () {
return {
isLock: true
isLock: true,
ephemeral: {
randomPassword: '',
showCopyFeedback: false
}
}
},
mixins: [validationsDebouncedMixins],
Expand All @@ -42,6 +71,11 @@ export default ({
required: false,
default: 'password'
},
mode: {
type: String,
requried: false,
default: 'manual' // 'manual' | 'auto'
},
label: {
type: String,
required: false
Expand All @@ -50,10 +84,6 @@ export default ({
type: Object,
required: true
},
hasIconRight: {
type: Boolean,
default: true
},
showPlaceholder: {
type: Boolean,
default: false
Expand All @@ -67,15 +97,95 @@ export default ({
required: false
}
},
methods: {
generateRandomPassword (pwLen = 32) {
let genPassword = ''

if (window?.Cypress) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd use process.env.CI here. If this code makes it to production and someone is using Cypress or some extension that defines Cypress, we wouldn't want to accidentally use a weak password.

Also, not sure why window?, since it should always be defined.

Copy link
Collaborator Author

@SebinSong SebinSong Aug 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

will remove ? typo here. Correct me if I'm wrong but process.env.CI is only true when tested as part of GH action workflow?

Copy link
Member

Choose a reason for hiding this comment

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

Correct me if I'm wrong but process.env.CI is only true when tested as part of GH action workflow?

I wouldn't say 'only', but yes, you'd need to manually set it when running grunt dev (e.g., CI=true grunt dev) if you want it locally (something that could be useful, as it will expose some exceptions / error conditions that are swallowed otherwise).

After thinking about it again, I've changed my mind and think that instead of using process.env.CI, this branch should just be removed entirely (see 3 below).

Still, these are the reasons why I don't think the current check is a good idea:

  1. Unlike CI=true, this will be a runtime check. If window.Cypress happens to be defined (browser extension, someone running Cypress to access GI, etc.), it can result in accidentally weak passwords. I don't see a compelling reason to ship this check into production. See point 4 below for a compromise.
  2. For the most part, Cypress doesn't use this new default value, except for the change you made to [signup-and-login.spec.js](https://github.com/okTurtles/group-income/pull/2866/files#diff-14ca527d56d62af00f45c89deb5f3456657bcf6e23a2c2f284422781d2e02a22)
  3. That change to signup-and-login IMHO isn't the best way to test this feature, since the code path taken in production and Cypress will be slightly different. If this something we want to test, I'd say it's best to give it the same behaviour as users will see, or as close to it as possible. For example, what if generateBase58Password didn't work for some reason (e.g., it throws an exception)? Then we wouldn't learn about it from the automated tests. I'd suggest removing the branch entirely and, in the test, checking that the auto-generated password has the expected length.
  4. If you still feel we should have this special case for Cypress, consider maybe changing it to process.env.CI || (process.env.NODE_ENV !== 'production' && window.Cypress), so that the chance of this generating a weak password in production is 0.

Copy link
Collaborator Author

@SebinSong SebinSong Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@corrideat I think my original inline comment was not clear enough.

One thing I do quite often for debugging is,
I run a specific cypress test-suite to save time for creating a group/adding members/setting income-details info etc.
(eg. When I need to debug something in payment area but the scenario is not simple to reproduce, I start by runninf group-paying.spect.js first so that all the payment-related stuffs automatically get ready and I don't have to manually create them all by myself). Then I would launch multiple browser tabs and log in for each users and debug things etc..)
If this auto-generated password is enabled, I would have to know the different passwords of each users created, and this wouldn't help easier/faster development or debugging.

So, using your suggestion in 4. let me update this to if (process.env.CI || process.env.NODE_ENV !== 'production') . So we can still use this default password while running the app locally for development.

Copy link
Member

@corrideat corrideat Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

TL;DR: Please fix the tests so that grunt dev (without CI=true) works. In the last part of this comment I also suggest changing the if condition to something else (starts at 'May I suggest then rewriting...').

I see, sometimes I do that too, and agree that the having a different password for each user, especially a random one, would be a hassle. But it doesn't seem like these "random" (123456789) passwords are being used in tests.

I noticed you also added a Math.random() > 1 condition, which will cause the branch to never be taken during local development (unless CI=true is set).

Was this the intent? If so:

  1. The comment seems to be slightly wrong. It should probably mention removing this part rather than the NODD_ENV part.
  2. What is the point of having this branch at all? Is it so that you can comment out the check for local development and not have to type a password (but still have it be predictable)? This could actually be quite useful, but see below.

If I understood this correctly, I suggest the following changes:

  1. Remove the CI check, as now the hardcoded password isn't used in tests (actually, it'd be nice to also add a short test to ensure the random password is working correctly; since it's random, it could be something simple like checking the length).
  2. I like the idea of having a pre-filled default password for development, but I'm not a big fan of having to change the source code for this, since it's tedious and error prone (by this I mean it's easy to forget about this change and commit it).
  3. Actually, tests now fail locally (group-proposals, user6 registers…) unless you set CI=true. This is currently withholding my approval.

May I suggest then rewriting the condition to something like if (process.env.NODE_ENV !== 'production' && process.env.UNSAFE_HARDCODED_TEST_PASSWORD)?

I think that this has the advantages of:

  1. Still providing a way to quickly create an account
  2. Not needing to change the source code for doing it, which can avoid accidental changes
  3. Having tests run against the 'real' code that users will have
  4. Providing a way to test the real code

Otherwise, this PR is looking great. I'll give my approval once tests pass without needing to set CI=true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Math.random() thing here is something I added for testing... it means nothing. am gonna remove it

Copy link
Collaborator Author

@SebinSong SebinSong Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

FYI again, Math.random() > 1 was something I added to test and see locally if the auto-generated password works when process.env.NODE_ENV === production. It seems some part of the previous comment was written thinking this is something meaningful. It's just I forgot to remove it after checking (My bad!). So let me skip that part.

BTW let me think about doing UNSAFE_HARDCODED_TEST_PASSWORD stuff though..
As a front-end dev who will have to do create users -> repeatedly log in/out pretty much as an every day task, I would always want to disable this auto-generated password feature during local development(Unless I'm working on this very feature itself..) So I would want this as a default instead of optional thing for development environment.

EDIT: I ended up adopting your suggestion for UNSAFE_HARDCODED_TEST_PASSWORD. Running UNSAFE_HARDCODED_TEST_PASSWORD=true grunt dev each time during development wouldn't be much hassle I guess!

// For easier debugging, use the common default password in Cypress test.
genPassword = '123456789'
} else {
const bytes = new Uint8Array(Math.ceil(pwLen / 2))
crypto.getRandomValues(bytes)

genPassword = Array.from(bytes).map(b => b.toString(36) // [0-9a-z] => 36 characters
.padStart(2, '0')).join('')

if (pwLen % 2 === 1) {
genPassword = genPassword.slice(1)
}
}

this.ephemeral.randomPassword = genPassword
this.$v.form[this.name].$model = genPassword
},
copyPassword () {
const pw = this.ephemeral.randomPassword
const copyToClipBoard = () => {
navigator.clipboard.writeText(pw)
this.ephemeral.showCopyFeedback = true

setTimeout(() => {
this.ephemeral.showCopyFeedback = false
}, 1500)
}

if (navigator.share) {
navigator.share({
title: L('Your password'),
text: pw
}).catch((error) => {
console.error('navigator.share failed with:', error)
copyToClipBoard()
})
} else {
copyToClipBoard()
}
}
},
created () {
this.isLock = !this.showPassword
if (this.mode === 'auto') {
this.generateRandomPassword()
} else {
this.isLock = !this.showPassword
}
}
}: Object)
</script>

<style lang="scss" scoped>
@import "@assets/style/_variables.scss";

.c-mode-auto {
.c-auto-password {
display: block;
line-height: 2.75rem;
padding-right: 5.5rem;
}

.addons {
align-items: center;
right: 0.5rem;
}
}

.icon {
cursor: pointer;
pointer-events: initial !important;
}

button.c-copy-btn {
min-height: unset;
height: 1.75rem;
border-radius: 3px;
padding-left: 1rem;
padding-right: 1rem;
}

.c-feedback {
@include tooltip-style-common;
top: calc(100% + 0.5rem);
left: 50%;
transform: translateX(-50%);
}
</style>
3 changes: 0 additions & 3 deletions frontend/views/containers/access/PasswordModal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ modal-template(class='is-centered is-left-aligned' back-on-mobile=true ref='moda
:value='form'
:$v='$v'
@enter='changePassword'
:hasIconRight='true'
:showPlaceholder='false'
:showPassword='false'
size='is-large'
Expand All @@ -26,7 +25,6 @@ modal-template(class='is-centered is-left-aligned' back-on-mobile=true ref='moda
:value='form'
:$v='$v'
@enter='changePassword'
:hasIconRight='true'
:showPlaceholder='false'
:showPassword='false'
size='is-large'
Expand All @@ -38,7 +36,6 @@ modal-template(class='is-centered is-left-aligned' back-on-mobile=true ref='moda
:value='form'
:$v='$v'
@enter='changePassword'
:hasIconRight='true'
:showPlaceholder='false'
:showPassword='false'
size='is-large'
Expand Down
66 changes: 45 additions & 21 deletions frontend/views/containers/access/SignupForm.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,36 @@ form(data-test='signup' @submit.prevent='')
v-error:username='{ attrs: { "data-test": "badUsername" } }'
)

.c-password-fields-container
password-form(:label='L("Password")' name='password' :$v='$v')
.c-auto-password-field-container
password-form(
mode='auto'
:label='L("This is your password. Save it now.")'
name='password'
:$v='$v'
)

password-form(:label='L("Confirm Password")' name='passwordConfirm' :$v='$v')
.c-confirm-check-container
label.checkbox
input.input(
type='checkbox'
name='savedPassword'
v-model='form.savedPassword'
data-test='savedPassword'
@click.stop=''
)
i18n I have saved the password

label.checkbox
input.input(
type='checkbox'
name='terms'
v-model='form.terms'
data-test='signTerms'
@click.stop=''
)
i18n(
:args='{ a_: `<a class="link" target="_blank" href="${linkToTerms}">`, _a: "</a>"}'
) I agree to the {a_}terms and conditions{_a}
label.checkbox
input.input(
type='checkbox'
name='terms'
v-model='form.terms'
data-test='signTerms'
@click.stop=''
)
i18n(
:args='{ a_: `<a class="link" target="_blank" href="${linkToTerms}">`, _a: "</a>"}'
) I agree to the {a_}terms and conditions{_a}

banner-scoped(ref='formMsg' allow-a)

Expand All @@ -44,7 +58,7 @@ form(data-test='signup' @submit.prevent='')
<script>
import sbp from '@sbp/sbp'
import { L } from '@common/common.js'
import { maxLength, minLength, required, sameAs } from 'vuelidate/lib/validators'
import { maxLength, minLength, required } from 'vuelidate/lib/validators'
import { validationMixin } from 'vuelidate'
import PasswordForm from '@containers/access/PasswordForm.vue'
import BannerScoped from '@components/banners/BannerScoped.vue'
Expand Down Expand Up @@ -107,7 +121,7 @@ export default ({
form: {
username: '',
password: '',
passwordConfirm: '',
savedPassword: false,
terms: false,
pictureBase64: ''
},
Expand Down Expand Up @@ -175,13 +189,11 @@ export default ({
[L('A password is required.')]: required,
[L('Your password must be at least {minChars} characters long.', { minChars: passwordMinChars })]: minLength(passwordMinChars)
},
passwordConfirm: {
[L('Passwords do not match.')]: sameAs('password')
savedPassword: {
[L('Please save the password.')]: (value) => Boolean(value)
},
terms: {
[L('You need to agree to the terms and conditions.')]: (value) => {
return Boolean(value)
}
[L('You need to agree to the terms and conditions.')]: (value) => Boolean(value)
}
}
}
Expand All @@ -208,4 +220,16 @@ export default ({
margin-bottom: 1.5rem;
}
}

.c-auto-password-field-container {
margin: 1.5rem 0;
}

.c-confirm-check-container {
position: relative;
display: flex;
flex-direction: column;
row-gap: 0.5rem;
margin-bottom: 2rem;
}
</style>
1 change: 0 additions & 1 deletion frontend/views/containers/user-settings/AccountRemovalModal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
:label='L("Password")'
:value='form'
:$v='$v'
:hasIconRight='true'
:showPlaceholder='false'
:showPassword='false'
size='is-large'
Expand Down
16 changes: 10 additions & 6 deletions test/cypress/integration/signup-and-login.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,24 +53,28 @@ describe('Signup, Profile and Login', () => {
cy.giLogout({ bypassUI: true })
})

it('sign up button remains disabled if passwords are not the same or terms are not agreed', () => {
it('sign up button remains disabled if terms are not agreed or you have not saved the password', () => {
const user2 = `user2-${userId}`
const password = '123456789'
const wrongPassword = 'wRoNgPaSsWoRd123'

cy.getByDT('signupBtn').click()

cy.getByDT('signName').type(user2)
cy.getByDT('password').type(password)
cy.getByDT('passwordConfirm').type(wrongPassword)
cy.getByDT('signTerms').check({ force: true }).should('be.checked')
cy.getByDT('password').should('have.text', password)
cy.getByDT('signSubmit').should('be.disabled')

cy.getByDT('passwordConfirm').type('{selectall}{del}' + password)
cy.getByDT('savedPassword').check({ force: true }).should('be.checked')
cy.getByDT('signTerms').check({ force: true }).should('be.checked')
cy.getByDT('signSubmit').should('not.be.disabled')

cy.getByDT('signTerms').uncheck({ force: true }).should('not.be.checked')
cy.getByDT('signSubmit').should('be.disabled')

cy.getByDT('signTerms').check({ force: true }).should('be.checked')
cy.getByDT('savedPassword').uncheck({ force: true }).should('not.be.checked')
cy.getByDT('signSubmit').should('be.disabled')

cy.getByDT('savedPassword').check({ force: true }).should('be.checked')
cy.getByDT('signSubmit').should('not.be.disabled')

cy.closeModal()
Expand Down
Loading
Loading