mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-08 00:03:07 +00:00
fix(frontend): answer the username policy instead of rejecting it
Three rounds of this bug moved between call sites because the shared loader rejects when it cannot read `automate_username_creation`, leaving each caller to guess — and both guessed "automated", which hides the username field and posts none to an instance that derives none. `loadUsernamePolicy` now answers "ask for one" in that case, so `SimpleCreateWorkspace` and `CreateWorkspaceInner` both render a field someone can type into rather than submitting a guess. An instance that does automate ignores a username it was sent, so asking is safe either way. The prefill and the policy are settled apart now too: a failed `globalWhoami` costs the suggested name and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012fRjnaHLwjpHN84gNNxah9
This commit is contained in:
co-authored by
Claude Opus 5
parent
c17d5a3bdf
commit
a978b1dbed
@@ -43,30 +43,34 @@
|
||||
/**
|
||||
* Whether the policy is settled, which is what this form may not submit without:
|
||||
* `automateUsername` starts at the common case, and posting that guess to an instance
|
||||
* that derives no usernames sends none where one is required. A load that fails settles
|
||||
* it by handing over to the full form, which asks for a username outright instead of
|
||||
* inferring one — so this is never true while the answer is still a guess.
|
||||
* that derives no usernames sends none where one is required. `loadUsernamePolicy`
|
||||
* always answers — "ask for one" when it cannot read the setting — so this turns true
|
||||
* on a known answer rather than on the attempt finishing.
|
||||
*/
|
||||
let policyLoaded = $state(false)
|
||||
/** Someone typed while the prefill was in flight; their name wins over the suggestion. */
|
||||
let nameEdited = false
|
||||
|
||||
async function load() {
|
||||
try {
|
||||
const [me, policy] = await Promise.all([UserService.globalWhoami(), loadUsernamePolicy()])
|
||||
if (!nameEdited) name = defaultWorkspaceName(me.name, me.email)
|
||||
automateUsername = policy.automate
|
||||
suggestedUsername = policy.suggested
|
||||
if (!policy.automate && !policy.suggested) advanced = true
|
||||
policyLoaded = true
|
||||
} catch (error) {
|
||||
console.error('Could not prefill the workspace name:', error)
|
||||
if (!nameEdited) name = 'My workspace'
|
||||
// The policy is what failed, so there is nothing to submit against. The full form
|
||||
// carries its own username field, which is the version that needs no policy.
|
||||
advanced = true
|
||||
policyLoaded = true
|
||||
// Settled apart: the policy decides whether this form may submit at all, the suggested
|
||||
// name is cosmetic, and neither failure should decide the other.
|
||||
const [me, policy] = await Promise.allSettled([
|
||||
UserService.globalWhoami(),
|
||||
loadUsernamePolicy()
|
||||
])
|
||||
if (!nameEdited) {
|
||||
name =
|
||||
me.status === 'fulfilled'
|
||||
? defaultWorkspaceName(me.value.name, me.value.email)
|
||||
: 'My workspace'
|
||||
}
|
||||
// `loadUsernamePolicy` answers "ask for one" rather than rejecting when the setting
|
||||
// cannot be read, so the fallback here is the same answer by another route.
|
||||
const answer = policy.status === 'fulfilled' ? policy.value : { automate: false }
|
||||
automateUsername = answer.automate
|
||||
suggestedUsername = answer.suggested
|
||||
if (!answer.automate && !answer.suggested) advanced = true
|
||||
policyLoaded = true
|
||||
}
|
||||
void load()
|
||||
|
||||
|
||||
@@ -1,13 +1,22 @@
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
|
||||
// The module reaches the API for the username policy and the workspace list; the name
|
||||
// helper touches neither.
|
||||
vi.mock('./gen', () => ({ SettingService: {}, UserService: {}, WorkspaceService: {} }))
|
||||
// helper touches neither. `getGlobal` is a spy so the policy's failure path can be driven.
|
||||
const getGlobal = vi.fn()
|
||||
vi.mock('./gen', () => ({
|
||||
SettingService: {
|
||||
get getGlobal() {
|
||||
return getGlobal
|
||||
}
|
||||
},
|
||||
UserService: {},
|
||||
WorkspaceService: {}
|
||||
}))
|
||||
vi.mock('./stores', () => ({ usersWorkspaceStore: { set: () => {} } }))
|
||||
vi.mock('./storeUtils', () => ({ switchWorkspace: () => {} }))
|
||||
vi.mock('./cloud', () => ({ isCloudHosted: () => false }))
|
||||
|
||||
import { defaultWorkspaceName, usernameFromName } from './workspaceCreation'
|
||||
import { defaultWorkspaceName, loadUsernamePolicy, usernameFromName } from './workspaceCreation'
|
||||
|
||||
describe('defaultWorkspaceName', () => {
|
||||
it('names the workspace after the person, not the address', () => {
|
||||
@@ -60,3 +69,19 @@ describe('usernameFromName', () => {
|
||||
expect(usernameFromName('a'.repeat(51))).toBeUndefined()
|
||||
})
|
||||
})
|
||||
|
||||
describe('loadUsernamePolicy', () => {
|
||||
// Fail-closed matters because a caller told "automated" hides its username field and
|
||||
// posts none: an instance that derives none refuses that, with nowhere to supply one.
|
||||
it('asks for a username when the setting cannot be read', async () => {
|
||||
getGlobal.mockRejectedValueOnce(new Error('502'))
|
||||
expect(await loadUsernamePolicy()).toEqual({ automate: false })
|
||||
})
|
||||
|
||||
it('automates when the setting says so, and when it is unset', async () => {
|
||||
getGlobal.mockResolvedValueOnce(true)
|
||||
expect(await loadUsernamePolicy()).toEqual({ automate: true })
|
||||
getGlobal.mockResolvedValueOnce(null)
|
||||
expect(await loadUsernamePolicy()).toEqual({ automate: true })
|
||||
})
|
||||
})
|
||||
|
||||
@@ -68,10 +68,20 @@ export function usernameFromName(name: string): string | undefined {
|
||||
* requires one when it does not, so the field only exists in the second case.
|
||||
*/
|
||||
export async function loadUsernamePolicy(): Promise<UsernamePolicy> {
|
||||
const automate =
|
||||
((await SettingService.getGlobal({
|
||||
key: 'automate_username_creation'
|
||||
})) as boolean | null) ?? true
|
||||
let automate: boolean
|
||||
try {
|
||||
automate =
|
||||
((await SettingService.getGlobal({
|
||||
key: 'automate_username_creation'
|
||||
})) as boolean | null) ?? true
|
||||
} catch (error) {
|
||||
// Unreadable is not "automated". A caller told yes hides its username field and posts
|
||||
// none, which an instance that derives none then refuses — with nowhere on screen to
|
||||
// supply what it wanted. Answering no asks for one, which is right either way: an
|
||||
// instance that does automate ignores a username it was sent.
|
||||
console.error('Could not read the username policy; asking for one instead:', error)
|
||||
return { automate: false }
|
||||
}
|
||||
if (automate) return { automate: true }
|
||||
try {
|
||||
const me = await UserService.globalWhoami()
|
||||
|
||||
Reference in New Issue
Block a user