-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
fix(files_sharing): Use share password policy for password generation #58221
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| /** | ||
| * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
|
|
||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| const axiosGet = vi.hoisted(() => vi.fn()) | ||
| vi.mock('@nextcloud/axios', () => ({ default: { get: axiosGet } })) | ||
|
|
||
| const getCapabilities = vi.hoisted(() => vi.fn()) | ||
| vi.mock('@nextcloud/capabilities', () => ({ getCapabilities })) | ||
|
|
||
| vi.mock('@nextcloud/dialogs', () => ({ | ||
| showError: vi.fn(), | ||
| showSuccess: vi.fn(), | ||
| })) | ||
|
|
||
| describe('GeneratePassword', () => { | ||
| beforeEach(() => { | ||
| vi.resetAllMocks() | ||
| vi.resetModules() | ||
| }) | ||
|
|
||
| it('should pass context=sharing to the API', async () => { | ||
| getCapabilities.mockReturnValue({ | ||
| password_policy: { | ||
| api: { generate: 'https://example.com/api/generate' }, | ||
| }, | ||
| }) | ||
| axiosGet.mockResolvedValue({ | ||
| data: { ocs: { data: { password: 'generated-password' } } }, | ||
| }) | ||
|
|
||
| const { default: generatePassword } = await import('./GeneratePassword.ts') | ||
| const password = await generatePassword() | ||
|
|
||
| expect(axiosGet).toHaveBeenCalledWith( | ||
| 'https://example.com/api/generate', | ||
| { params: { context: 'sharing' } }, | ||
| ) | ||
| expect(password).toBe('generated-password') | ||
| }) | ||
|
|
||
| it('should use sharing policy minLength in fallback', async () => { | ||
| getCapabilities.mockReturnValue({ | ||
| password_policy: { | ||
| policies: { | ||
| sharing: { minLength: 15, enforceSpecialCharacters: false }, | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| const { default: generatePassword } = await import('./GeneratePassword.ts') | ||
| const password = await generatePassword() | ||
|
|
||
| expect(password.length).toBeGreaterThanOrEqual(15) | ||
| }) | ||
|
|
||
| it('should include special characters when policy requires it', async () => { | ||
| getCapabilities.mockReturnValue({ | ||
| password_policy: { | ||
| policies: { | ||
| sharing: { minLength: 10, enforceSpecialCharacters: true }, | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| const { default: generatePassword } = await import('./GeneratePassword.ts') | ||
| const password = await generatePassword() | ||
|
|
||
| expect(password).toMatch(/[!@#$%^&*]/) | ||
| }) | ||
|
|
||
| it('should fallback to default 10 chars when no policy', async () => { | ||
| getCapabilities.mockReturnValue({}) | ||
|
|
||
| const { default: generatePassword } = await import('./GeneratePassword.ts') | ||
| const password = await generatePassword() | ||
|
|
||
| expect(password.length).toBeGreaterThanOrEqual(10) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -10,8 +10,12 @@ import Config from '../services/ConfigService.ts' | |||||
| import logger from '../services/logger.ts' | ||||||
|
|
||||||
| const config = new Config() | ||||||
| // note: some chars removed on purpose to make them human friendly when read out | ||||||
| const passwordSet = 'abcdefgijkmnopqrstwxyzABCDEFGHJKLMNPQRSTWXYZ23456789' | ||||||
| // Character sets for password generation | ||||||
| const CHARS_LOWER = 'abcdefgijkmnopqrstwxyz' | ||||||
| const CHARS_UPPER = 'ABCDEFGHJKLMNPQRSTWXYZ' | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| const CHARS_DIGITS = '23456789' | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing 1?
Suggested change
|
||||||
| const CHARS_SPECIAL = '!@#$%^&*' | ||||||
| const CHARS_HUMAN_READABLE = CHARS_LOWER + CHARS_UPPER + CHARS_DIGITS | ||||||
|
|
||||||
| /** | ||||||
| * Generate a valid policy password or request a valid password if password_policy is enabled | ||||||
|
|
@@ -22,7 +26,9 @@ export default async function(verbose = false): Promise<string> { | |||||
| // password policy is enabled, let's request a pass | ||||||
| if (config.passwordPolicy.api && config.passwordPolicy.api.generate) { | ||||||
| try { | ||||||
| const request = await axios.get(config.passwordPolicy.api.generate) | ||||||
| const request = await axios.get(config.passwordPolicy.api.generate, { | ||||||
| params: { context: 'sharing' }, | ||||||
| }) | ||||||
| if (request.data.ocs.data.password) { | ||||||
| if (verbose) { | ||||||
| showSuccess(t('files_sharing', 'Password created successfully')) | ||||||
|
|
@@ -37,14 +43,39 @@ export default async function(verbose = false): Promise<string> { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| const array = new Uint8Array(10) | ||||||
| const ratio = passwordSet.length / 255 | ||||||
| getRandomValues(array) | ||||||
| // Fallback: generate password based on sharing policy from capabilities | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is a fallback needed? I think this will only cause problems down the line when the implementations in the frontend and backend get out of sync. |
||||||
| const sharingPolicy = config.passwordPolicy?.policies?.sharing | ||||||
| const minLength = Math.max(sharingPolicy?.minLength ?? config.passwordPolicy?.minLength ?? 10, 8) | ||||||
| const needsSpecialChars = sharingPolicy?.enforceSpecialCharacters ?? config.passwordPolicy?.enforceSpecialCharacters ?? false | ||||||
| const needsUpperLower = sharingPolicy?.enforceUpperLowerCase ?? config.passwordPolicy?.enforceUpperLowerCase ?? false | ||||||
| const needsNumeric = sharingPolicy?.enforceNumericCharacters ?? config.passwordPolicy?.enforceNumericCharacters ?? false | ||||||
|
|
||||||
| let password = '' | ||||||
| let chars = CHARS_HUMAN_READABLE | ||||||
|
|
||||||
| // Add required character types | ||||||
| if (needsUpperLower) { | ||||||
| password += getRandomChar(CHARS_UPPER) | ||||||
| password += getRandomChar(CHARS_LOWER) | ||||||
| } | ||||||
| if (needsNumeric) { | ||||||
| password += getRandomChar(CHARS_DIGITS) | ||||||
| } | ||||||
| if (needsSpecialChars) { | ||||||
| password += getRandomChar(CHARS_SPECIAL) | ||||||
| chars += CHARS_SPECIAL | ||||||
| } | ||||||
|
|
||||||
| // Fill remaining length | ||||||
| const remainingLength = Math.max(minLength - password.length, 0) | ||||||
| const array = new Uint8Array(remainingLength) | ||||||
| getRandomValues(array) | ||||||
| for (let i = 0; i < array.length; i++) { | ||||||
| password += passwordSet.charAt(array[i] * ratio) | ||||||
| password += chars.charAt(Math.floor(array[i] / 256 * chars.length)) | ||||||
| } | ||||||
| return password | ||||||
|
|
||||||
| // Shuffle to randomize character positions | ||||||
| return shuffleString(password) | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -65,3 +96,32 @@ function getRandomValues(array: Uint8Array): void { | |||||
| array[len] = Math.floor(Math.random() * 256) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Get a random character from the given character set. | ||||||
| * | ||||||
| * @param chars - The character set to choose from. | ||||||
| */ | ||||||
| function getRandomChar(chars: string): string { | ||||||
| const array = new Uint8Array(1) | ||||||
| getRandomValues(array) | ||||||
| return chars.charAt(Math.floor(array[0] / 256 * chars.length)) | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Shuffle a string randomly using Fisher-Yates algorithm. | ||||||
| * | ||||||
| * @param str - The string to shuffle. | ||||||
| */ | ||||||
| function shuffleString(str: string): string { | ||||||
| const arr = str.split('') | ||||||
| for (let i = arr.length - 1; i > 0; i--) { | ||||||
| const array = new Uint8Array(1) | ||||||
| getRandomValues(array) | ||||||
| const j = Math.floor(array[0] / 256 * (i + 1)) | ||||||
| const temp = arr[i] | ||||||
| arr[i] = arr[j] | ||||||
| arr[j] = temp | ||||||
| } | ||||||
| return arr.join('') | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.