diff --git a/apps/files_sharing/src/mixins/SharesMixin.js b/apps/files_sharing/src/mixins/SharesMixin.js index 0fdedd6fad0..7aa1d7ccbd1 100644 --- a/apps/files_sharing/src/mixins/SharesMixin.js +++ b/apps/files_sharing/src/mixins/SharesMixin.js @@ -179,7 +179,10 @@ export default { async set(enabled) { if (enabled) { this.passwordProtectedState = true - this.$set(this.share, 'newPassword', await GeneratePassword(true)) + const generatedPassword = await GeneratePassword(true) + if (!this.share.newPassword) { + this.$set(this.share, 'newPassword', generatedPassword) + } } else { this.passwordProtectedState = false this.$set(this.share, 'newPassword', '') diff --git a/apps/files_sharing/src/views/SharingDetailsTab.spec.ts b/apps/files_sharing/src/views/SharingDetailsTab.spec.ts index 07aacb2e365..cf7168febe2 100644 --- a/apps/files_sharing/src/views/SharingDetailsTab.spec.ts +++ b/apps/files_sharing/src/views/SharingDetailsTab.spec.ts @@ -5,6 +5,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +const mockGeneratePassword = vi.fn().mockResolvedValue('generated-password-123') + vi.mock('../services/ConfigService.ts', () => ({ default: vi.fn().mockImplementation(() => ({ enableLinkPasswordByDefault: false, @@ -24,180 +26,202 @@ vi.mock('../services/ConfigService.ts', () => ({ })) vi.mock('../utils/GeneratePassword.ts', () => ({ - default: vi.fn().mockResolvedValue('generated-password-123'), + default: (...args: unknown[]) => mockGeneratePassword(...args), })) +/** + * Simulates the isPasswordProtected getter from SharesMixin.js + */ +function getIsPasswordProtected(state: { + enforcePasswordForPublicLink: boolean + passwordProtectedState: boolean | undefined + newPassword: string | undefined + password: string | undefined +}): boolean { + if (state.enforcePasswordForPublicLink) { + return true + } + if (state.passwordProtectedState !== undefined) { + return state.passwordProtectedState + } + return typeof state.newPassword === 'string' + || typeof state.password === 'string' +} + +/** + * Simulates the isPasswordProtected setter from SharesMixin.js + * Returns the resulting share state after the async operation completes. + */ +async function setIsPasswordProtected( + enabled: boolean, + share: { newPassword?: string }, +): Promise<{ passwordProtectedState: boolean, share: { newPassword?: string } }> { + if (enabled) { + const generatedPassword = await mockGeneratePassword(true) + if (!share.newPassword) { + share.newPassword = generatedPassword + } + return { passwordProtectedState: true, share } + } else { + share.newPassword = '' + return { passwordProtectedState: false, share } + } +} + describe('SharingDetailsTab - Password State Management Logic', () => { - describe('isPasswordProtected getter logic', () => { + beforeEach(() => { + mockGeneratePassword.mockClear() + mockGeneratePassword.mockResolvedValue('generated-password-123') + }) + + describe('isPasswordProtected getter', () => { + it('returns true when enforcePasswordForPublicLink is true regardless of other state', () => { + expect(getIsPasswordProtected({ + enforcePasswordForPublicLink: true, + passwordProtectedState: false, + newPassword: undefined, + password: undefined, + })).toBe(true) + }) + it('returns true when passwordProtectedState is explicitly true', () => { - const passwordProtectedState: boolean | undefined = true - const enforcePasswordForPublicLink = false - const newPassword: string | undefined = undefined - const password: string | undefined = undefined - - const isPasswordProtected = (() => { - if (enforcePasswordForPublicLink) { - return true - } - if (passwordProtectedState !== undefined) { - return passwordProtectedState - } - return typeof newPassword === 'string' - || typeof password === 'string' - })() - - expect(isPasswordProtected).toBe(true) + expect(getIsPasswordProtected({ + enforcePasswordForPublicLink: false, + passwordProtectedState: true, + newPassword: undefined, + password: undefined, + })).toBe(true) }) it('returns false when passwordProtectedState is explicitly false', () => { - const passwordProtectedState: boolean | undefined = false - const enforcePasswordForPublicLink = false - const newPassword: string | undefined = 'some-password' - const password: string | undefined = undefined - - const isPasswordProtected = (() => { - if (enforcePasswordForPublicLink) { - return true - } - if (passwordProtectedState !== undefined) { - return passwordProtectedState - } - return typeof newPassword === 'string' - || typeof password === 'string' - })() - - expect(isPasswordProtected).toBe(false) + expect(getIsPasswordProtected({ + enforcePasswordForPublicLink: false, + passwordProtectedState: false, + newPassword: 'some-password', + password: undefined, + })).toBe(false) }) - it('returns true when enforcePasswordForPublicLink is true regardless of other state', () => { - const passwordProtectedState: boolean | undefined = false - const enforcePasswordForPublicLink = true - const newPassword: string | undefined = undefined - const password: string | undefined = undefined - - const isPasswordProtected = (() => { - if (enforcePasswordForPublicLink) { - return true - } - if (passwordProtectedState !== undefined) { - return passwordProtectedState - } - return typeof newPassword === 'string' - || typeof password === 'string' - })() - - expect(isPasswordProtected).toBe(true) + it('falls back to inferring from newPassword when passwordProtectedState is undefined', () => { + expect(getIsPasswordProtected({ + enforcePasswordForPublicLink: false, + passwordProtectedState: undefined, + newPassword: 'some-password', + password: undefined, + })).toBe(true) }) it('falls back to inferring from password when passwordProtectedState is undefined', () => { - const passwordProtectedState: boolean | undefined = undefined - const enforcePasswordForPublicLink = false - const newPassword: string | undefined = 'some-password' - const password: string | undefined = undefined - - const isPasswordProtected = (() => { - if (enforcePasswordForPublicLink) { - return true - } - if (passwordProtectedState !== undefined) { - return passwordProtectedState - } - return typeof newPassword === 'string' - || typeof password === 'string' - })() - - expect(isPasswordProtected).toBe(true) + expect(getIsPasswordProtected({ + enforcePasswordForPublicLink: false, + passwordProtectedState: undefined, + newPassword: undefined, + password: 'existing-password', + })).toBe(true) }) it('returns false when passwordProtectedState is undefined and no passwords exist', () => { - const passwordProtectedState: boolean | undefined = undefined - const enforcePasswordForPublicLink = false - const newPassword: string | undefined = undefined - const password: string | undefined = undefined + expect(getIsPasswordProtected({ + enforcePasswordForPublicLink: false, + passwordProtectedState: undefined, + newPassword: undefined, + password: undefined, + })).toBe(false) + }) - const isPasswordProtected = (() => { - if (enforcePasswordForPublicLink) { - return true - } - if (passwordProtectedState !== undefined) { - return passwordProtectedState - } - return typeof newPassword === 'string' - || typeof password === 'string' - })() + it('checkbox remains checked when passwordProtectedState is true even if password is cleared', () => { + expect(getIsPasswordProtected({ + enforcePasswordForPublicLink: false, + passwordProtectedState: true, + newPassword: '', + password: undefined, + })).toBe(true) + }) + }) - expect(isPasswordProtected).toBe(false) + describe('isPasswordProtected setter (race condition fix)', () => { + it('generated password does NOT overwrite user-typed password', async () => { + const share = { newPassword: 'user-typed-password' } + const result = await setIsPasswordProtected(true, share) + + expect(mockGeneratePassword).toHaveBeenCalledWith(true) + expect(result.passwordProtectedState).toBe(true) + expect(result.share.newPassword).toBe('user-typed-password') + }) + + it('generated password IS applied when user has not typed anything', async () => { + const share: { newPassword?: string } = {} + const result = await setIsPasswordProtected(true, share) + + expect(mockGeneratePassword).toHaveBeenCalledWith(true) + expect(result.passwordProtectedState).toBe(true) + expect(result.share.newPassword).toBe('generated-password-123') + }) + + it('generated password IS applied when newPassword is empty string (user cleared input)', async () => { + const share = { newPassword: '' } + const result = await setIsPasswordProtected(true, share) + + expect(result.share.newPassword).toBe('generated-password-123') + }) + + it('disabling password clears newPassword and sets state to false', async () => { + const share = { newPassword: 'some-password' } + const result = await setIsPasswordProtected(false, share) + + expect(result.passwordProtectedState).toBe(false) + expect(result.share.newPassword).toBe('') }) }) describe('initializeAttributes sets passwordProtectedState', () => { - it('should set passwordProtectedState to true when enableLinkPasswordByDefault is true', async () => { - const config = { - enableLinkPasswordByDefault: true, - enforcePasswordForPublicLink: false, - } + it('should set passwordProtectedState when enableLinkPasswordByDefault is true for new public share', () => { + const config = { enableLinkPasswordByDefault: true, enforcePasswordForPublicLink: false } const isNewShare = true const isPublicShare = true let passwordProtectedState: boolean | undefined - if (isNewShare) { - if ((config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) { - passwordProtectedState = true - } + if (isNewShare && (config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) { + passwordProtectedState = true } expect(passwordProtectedState).toBe(true) }) - it('should set passwordProtectedState to true when isPasswordEnforced is true', async () => { - const config = { - enableLinkPasswordByDefault: false, - enforcePasswordForPublicLink: true, - } + it('should set passwordProtectedState when isPasswordEnforced is true for new public share', () => { + const config = { enableLinkPasswordByDefault: false, enforcePasswordForPublicLink: true } const isNewShare = true const isPublicShare = true let passwordProtectedState: boolean | undefined - if (isNewShare) { - if ((config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) { - passwordProtectedState = true - } + if (isNewShare && (config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) { + passwordProtectedState = true } expect(passwordProtectedState).toBe(true) }) - it('should not set passwordProtectedState for non-public shares', async () => { - const config = { - enableLinkPasswordByDefault: true, - enforcePasswordForPublicLink: false, - } + it('should not set passwordProtectedState for non-public shares', () => { + const config = { enableLinkPasswordByDefault: true, enforcePasswordForPublicLink: false } const isNewShare = true const isPublicShare = false let passwordProtectedState: boolean | undefined - if (isNewShare) { - if ((config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) { - passwordProtectedState = true - } + if (isNewShare && (config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) { + passwordProtectedState = true } expect(passwordProtectedState).toBe(undefined) }) - it('should not set passwordProtectedState for existing shares', async () => { - const config = { - enableLinkPasswordByDefault: true, - enforcePasswordForPublicLink: false, - } + it('should not set passwordProtectedState for existing shares', () => { + const config = { enableLinkPasswordByDefault: true, enforcePasswordForPublicLink: false } const isNewShare = false const isPublicShare = true let passwordProtectedState: boolean | undefined - if (isNewShare) { - if ((config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) { - passwordProtectedState = true - } + if (isNewShare && (config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) { + passwordProtectedState = true } expect(passwordProtectedState).toBe(undefined) @@ -215,10 +239,8 @@ describe('SharingDetailsTab - Password State Management Logic', () => { const newPassword = '' let passwordError = false - if (isPasswordProtected) { - if (isNewShare && !isValidShareAttribute(newPassword)) { - passwordError = true - } + if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) { + passwordError = true } expect(passwordError).toBe(true) @@ -230,10 +252,8 @@ describe('SharingDetailsTab - Password State Management Logic', () => { const newPassword = undefined let passwordError = false - if (isPasswordProtected) { - if (isNewShare && !isValidShareAttribute(newPassword)) { - passwordError = true - } + if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) { + passwordError = true } expect(passwordError).toBe(true) @@ -245,10 +265,8 @@ describe('SharingDetailsTab - Password State Management Logic', () => { const newPassword = 'valid-password-123' let passwordError = false - if (isPasswordProtected) { - if (isNewShare && !isValidShareAttribute(newPassword)) { - passwordError = true - } + if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) { + passwordError = true } expect(passwordError).toBe(false) @@ -260,10 +278,8 @@ describe('SharingDetailsTab - Password State Management Logic', () => { const newPassword = '' let passwordError = false - if (isPasswordProtected) { - if (isNewShare && !isValidShareAttribute(newPassword)) { - passwordError = true - } + if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) { + passwordError = true } expect(passwordError).toBe(false) @@ -275,57 +291,11 @@ describe('SharingDetailsTab - Password State Management Logic', () => { const newPassword = '' let passwordError = false - if (isPasswordProtected) { - if (isNewShare && !isValidShareAttribute(newPassword)) { - passwordError = true - } + if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) { + passwordError = true } expect(passwordError).toBe(false) }) }) - - describe('checkbox persistence after clearing password', () => { - it('checkbox remains checked when passwordProtectedState is explicitly true even if password is cleared', () => { - let passwordProtectedState: boolean | undefined = true - const enforcePasswordForPublicLink = false - let newPassword: string | undefined = 'initial-password' - - newPassword = '' - - const isPasswordProtected = (() => { - if (enforcePasswordForPublicLink) { - return true - } - if (passwordProtectedState !== undefined) { - return passwordProtectedState - } - return typeof newPassword === 'string' - || false - })() - - expect(isPasswordProtected).toBe(true) - }) - - it('checkbox unchecks incorrectly if passwordProtectedState was never set (bug scenario)', () => { - let passwordProtectedState: boolean | undefined = undefined - const enforcePasswordForPublicLink = false - let newPassword: string | undefined = 'initial-password' - - newPassword = undefined - - const isPasswordProtected = (() => { - if (enforcePasswordForPublicLink) { - return true - } - if (passwordProtectedState !== undefined) { - return passwordProtectedState - } - return typeof newPassword === 'string' - || false - })() - - expect(isPasswordProtected).toBe(false) - }) - }) }) diff --git a/apps/files_sharing/src/views/SharingDetailsTab.vue b/apps/files_sharing/src/views/SharingDetailsTab.vue index 10f1ab0c06e..a2d21def25f 100644 --- a/apps/files_sharing/src/views/SharingDetailsTab.vue +++ b/apps/files_sharing/src/views/SharingDetailsTab.vue @@ -898,7 +898,10 @@ export default { if (this.isNewShare) { if ((this.config.enableLinkPasswordByDefault || this.isPasswordEnforced) && this.isPublicShare) { this.passwordProtectedState = true - this.$set(this.share, 'newPassword', await GeneratePassword(true)) + const generatedPassword = await GeneratePassword(true) + if (!this.share.newPassword) { + this.$set(this.share, 'newPassword', generatedPassword) + } this.advancedSectionAccordionExpanded = true } /* Set default expiration dates if configured */