diff --git a/backend/helpers/authDebug.ts b/backend/helpers/authDebug.ts new file mode 100644 index 000000000..d3991f840 --- /dev/null +++ b/backend/helpers/authDebug.ts @@ -0,0 +1,66 @@ +/** + * What an authentication module has to say about one attempt, while the `authDebug` flag is on. + * + * A failed login deliberately tells whoever is at the login screen almost nothing: every refusal + * arrives as one of a handful of coded errors (`ERR_LOGIN_FAILED`, `ERR_STRATEGY_MISCONFIGURED`, + * `ERR_PROVIDER_REQUEST_FAILED`), because that form is open to whoever can reach the wiki and a + * message naming the check that refused is a message telling an attacker what to change. This is the + * other half of that arrangement: the detail goes to the server log, where the administrator setting + * the strategy up can read it and nobody else can. + * + * Which is what makes it the difference between a directory that cannot be reached, the wiki's own + * bind credentials being wrong, a search base with a typo in it, a filter matching two people and a + * password that is simply not the right one — five different things to go and fix, all of which reach + * the user as the same `ERR_LOGIN_FAILED`. + * + * **Never a credential, whatever is being diagnosed**: not the password being tried, not the + * strategy's own bind credentials or client secret, not a token a provider issued. Configuration is + * quoted freely — a search filter or an issuer URL is what the message is *for* — and secrets are + * described instead: whether one is set, never what it is. + * + * @param strategy The module instance. Every one carries `strategyId`, and `models/authentication.ts` + * sets `module` on it right after construction, so a message says which of several + * strategies of the same kind it is about. + */ +export function strategyDebug( + strategy: { strategyId: string; module?: string }, + message: string +): void { + WIKI.models.flags.authDebug( + `${strategy.module ?? 'unknown'} strategy ${strategy.strategyId}: ${message}` + ) +} + +/** + * Which of a set of settings are empty, by the names the admin area shows them under. + * + * The titles rather than the config keys, because these names are only ever read in a log message + * about a strategy that will not work, and the point of such a message is to name the field to go and + * fill in — "Client Secret is empty", not `clientSecret`. + */ +export function missingSettings(settings: Record): string { + const missing = Object.entries(settings) + .filter(([, value]) => !value) + .map(([title]) => title) + return `${missing.join(', ')} ${missing.length > 1 ? 'are' : 'is'} empty` +} + +/** + * What a directory, a provider or the network said, as far as it can be put on one line. + * + * Every field of it earns its place, and each comes from a different kind of failure: + * + * - the **class name**, because `ldapts` raises a result-code error whose message is often only the + * code's own name — and `InvalidCredentialsError` on the wiki's own search connection is what + * says the strategy's bind DN is wrong rather than the person's password; + * - the **code**, which is an LDAP result code (49, 32) that a directory's documentation is indexed + * by, or a socket error's (`ECONNREFUSED`, `ETIMEDOUT`) that is the whole answer on its own; + * - the OAuth2 **`error` / `error_description`**, which `openid-client` attaches when a provider + * refuses something and is the provider's own account of why — `invalid_client` for a rotated + * secret, `invalid_grant` for a redirect URI it does not have registered. + */ +export function describeAuthError(err: any): string { + const code = err?.code === undefined ? '' : ` [${err.code}]` + const detail = [err?.error, err?.error_description].filter(Boolean).join(': ') + return `${err?.name ?? 'Error'}${code}: ${err?.message ?? err}${detail ? ` (${detail})` : ''}` +} diff --git a/backend/models/flags.ts b/backend/models/flags.ts index c8781d1f7..3884dcbeb 100644 --- a/backend/models/flags.ts +++ b/backend/models/flags.ts @@ -8,7 +8,11 @@ export const FLAGS = { /** Consumed by the frontend, which reveals unfinished features when it is on. */ experimental: 'Unfinished features are offered in the interface.', - /** Consumed by `models/users.ts` and `api/authentication.ts` via `authDebug()` below. */ + /** + * Consumed by `models/users.ts` and `api/authentication.ts` via `authDebug()` below, and by every + * authentication module through `helpers/authDebug.ts` — which is the half that says *why* a + * strategy refused somebody, since what reaches the login screen is only ever a coded error. + */ authDebug: 'Login and account creation attempts are logged in detail.', /** Consumed by the query logger in `core/db.ts`. */ sqlLog: 'Every database query is logged.' diff --git a/backend/models/users.ts b/backend/models/users.ts index aa8edd2d4..beb37e503 100644 --- a/backend/models/users.ts +++ b/backend/models/users.ts @@ -919,11 +919,15 @@ class Users { const unmatched = profile.groups.filter( (name) => !all.some((grp) => grp.name.toLowerCase() === name.toLowerCase()) ) - if (unmatched.length > 0) { - WIKI.models.flags.authDebug( - `Strategy ${strategy.id} named ${unmatched.length} group(s) this wiki does not have, for user ${userId}: ${unmatched.join(', ')}` - ) - } + /* + Both halves of the answer, and logged even when the provider named nothing: an empty claim is + the commonest reason a group mapping appears not to work, and it is silent everywhere else — the + wanted set below then simply equals the current one. What the provider said about this person is + logged by the module; this is what the wiki could do with it. + */ + WIKI.models.flags.authDebug( + `Strategy ${strategy.id} named ${profile.groups.length} group(s) for user ${userId}: ${matched.length} matched a wiki group (${matched.map((grp) => grp.name).join(', ') || 'none'})${unmatched.length > 0 ? `, ${unmatched.length} did not (${unmatched.join(', ')})` : ''}` + ) const current = await this.getUserGroupIds(userId) const autoEnroll = strategy.autoEnrollGroups ?? [] diff --git a/backend/modules/authentication/discord/authentication.ts b/backend/modules/authentication/discord/authentication.ts index cddb6f272..486aedc02 100644 --- a/backend/modules/authentication/discord/authentication.ts +++ b/backend/modules/authentication/discord/authentication.ts @@ -1,3 +1,4 @@ +import { missingSettings, strategyDebug } from '../../../helpers/authDebug.ts' import type { AuthFlow, AuthFlowCallback, ProviderProfile } from '../../../models/authentication.ts' /** Where a person signs in. Not under `/api`, unlike everything else Discord answers. */ @@ -66,6 +67,10 @@ export default class DiscordAuthentication { const clientSecret = this.conf.clientSecret || '' const serverId = (this.conf.serverId || '').trim() if (!clientId || !clientSecret) { + strategyDebug( + this, + `is not configured: ${missingSettings({ 'Client ID': clientId, 'Client Secret': clientSecret })}` + ) throw new Error('ERR_STRATEGY_MISCONFIGURED') } if (this.conf.mapGroups === true && !serverId) { @@ -115,6 +120,12 @@ export default class DiscordAuthentication { WIKI.logger.warn( `Discord strategy ${this.strategyId} asked for ${path} and the API answered ${resp.status}.` ) + // -> The body as well as the status, under the flag: Discord's error payload names the scope + // that was not granted or the intent the bot is missing, which the status alone does not + strategyDebug( + this, + `GET ${path} answered ${resp.status}: ${await resp.text().catch(() => '(no body)')}` + ) throw new Error('ERR_PROVIDER_REQUEST_FAILED') } return resp.json() @@ -172,6 +183,10 @@ export default class DiscordAuthentication { let names = await this.roleNames(serverId) if (names.size < 1) { // -> No bot token. The IDs are the whole of what Discord will say about these roles + strategyDebug( + this, + `no Bot Token is configured, so the ${roleIds.length} role(s) held can only be matched by ID: ${roleIds.join(', ') || 'none'}` + ) return roleIds } if (roleIds.some((id) => !names.has(id))) { @@ -249,15 +264,26 @@ export default class DiscordAuthentication { try { token = (await tokenResp.json()) as Record } catch { + strategyDebug( + this, + `the token exchange answered ${tokenResp.status} with something that is not JSON — is something else answering for ${API}?` + ) throw new Error('ERR_TOKEN_EXCHANGE_FAILED') } if (!tokenResp.ok || token.error || !token.access_token) { + // -> Discord's own account of the refusal, which names the cause: `invalid_client` for a Client + // Secret that has been reset, `invalid_grant` for a Redirect URI it does not have registered + strategyDebug( + this, + `the token exchange answered ${tokenResp.status}: ${[token.error, token.error_description].filter(Boolean).join(': ') || 'no access token'}` + ) throw new Error('ERR_TOKEN_EXCHANGE_FAILED') } const bearer = `Bearer ${token.access_token}` const account = await this.api('/users/@me', bearer) if (!account?.id) { + strategyDebug(this, 'the API answered with no account for this token') throw new Error('ERR_NO_PROVIDER_ACCOUNT') } /* @@ -266,6 +292,10 @@ export default class DiscordAuthentication { by, so it is refused rather than trusted. */ if (!account.email || account.verified !== true) { + strategyDebug( + this, + `${account.username} ${account.email ? 'has not confirmed their address with Discord' : 'gave no address — was the `email` scope granted?'}` + ) throw new Error('ERR_NO_VERIFIED_EMAIL_FROM_PROVIDER') } @@ -283,6 +313,12 @@ export default class DiscordAuthentication { bearer ) if (!member) { + // -> 404, which Discord uses for both cases. Said as both, since a mistyped Server ID and a + // person who is not in the server are one answer here and two different things to fix + strategyDebug( + this, + `${account.username} is not in server ${serverId}, or there is no such server` + ) throw new Error('ERR_ACCOUNT_NOT_ALLOWED') } roleIds = Array.isArray(member.roles) @@ -290,6 +326,13 @@ export default class DiscordAuthentication { : [] } + const groups = + this.conf.mapGroups === true ? await this.groupsFor(serverId, roleIds) : undefined + strategyDebug( + this, + `${account.username} (${account.id}) signs in as <${account.email}>${groups ? `, holding ${groups.length} mapped role(s): ${groups.join(', ') || 'none'}` : ', groups not mapped'}` + ) + return { id: String(account.id), email: account.email, @@ -297,9 +340,9 @@ export default class DiscordAuthentication { // has not set one has name: account.global_name || account.username, picture: this.pictureFor(account), - ...(this.conf.mapGroups === true + ...(groups ? { - groups: await this.groupsFor(serverId, roleIds), + groups, groupsExclusive: this.conf.unassignMissingGroups === true } : {}) diff --git a/backend/modules/authentication/entra/authentication.ts b/backend/modules/authentication/entra/authentication.ts index 0dc0b1984..9f3672276 100644 --- a/backend/modules/authentication/entra/authentication.ts +++ b/backend/modules/authentication/entra/authentication.ts @@ -1,4 +1,5 @@ import * as client from 'openid-client' +import { describeAuthError, missingSettings, strategyDebug } from '../../../helpers/authDebug.ts' import type { AuthFlow, AuthFlowCallback, ProviderProfile } from '../../../models/authentication.ts' /** Where a tenant's OpenID Connect metadata lives. `{tenant}` is the one thing configurable about it. */ @@ -52,16 +53,30 @@ export default class EntraAuthentication { } const { tenantId, clientId, clientSecret } = this.conf if (!tenantId || !clientId || !clientSecret) { + strategyDebug( + this, + `is not configured: ${missingSettings({ 'Directory (tenant) ID': tenantId, 'Application (client) ID': clientId, 'Client Secret': clientSecret })}` + ) throw new Error('ERR_STRATEGY_MISCONFIGURED') } if (MULTI_TENANT.includes(String(tenantId).toLowerCase())) { + strategyDebug( + this, + `has \`${tenantId}\` as its Directory (tenant) ID, which is a multi-tenant placeholder — this module needs the tenant's own ID, since it accepts tokens from that directory alone (see MULTI_TENANT)` + ) throw new Error('ERR_STRATEGY_MISCONFIGURED') } - this.config = await client.discovery( - new URL(ISSUER_TEMPLATE.replace('{tenant}', encodeURIComponent(tenantId))), - clientId, - clientSecret - ) + const issuer = ISSUER_TEMPLATE.replace('{tenant}', encodeURIComponent(tenantId)) + strategyDebug(this, `reading the tenant's metadata from ${issuer}`) + try { + this.config = await client.discovery(new URL(issuer), clientId, clientSecret) + } catch (err: any) { + strategyDebug( + this, + `could not read the tenant's metadata from ${issuer}: ${describeAuthError(err)}` + ) + throw err + } return this.config } @@ -86,13 +101,26 @@ export default class EntraAuthentication { codeVerifier }: AuthFlowCallback): Promise { const config = await this.configuration() - const tokens = await client.authorizationCodeGrant(config, new URL(currentUrl), { - expectedState: state, - expectedNonce: nonce, - pkceCodeVerifier: codeVerifier - }) + let tokens + try { + tokens = await client.authorizationCodeGrant(config, new URL(currentUrl), { + expectedState: state, + expectedNonce: nonce, + pkceCodeVerifier: codeVerifier + }) + } catch (err: any) { + // -> The state, the PKCE verifier, the code exchange and the ID token's signature, issuer, + // audience and nonce are all checked in there, so this covers a redirect URI the app + // registration does not have, an expired client secret, and a mismatched tenant alike + strategyDebug(this, `the tenant's answer did not check out: ${describeAuthError(err)}`) + throw err + } const claims = tokens.claims() if (!claims?.sub) { + strategyDebug( + this, + 'the tenant returned no ID token, so there is nothing signed saying who signed in' + ) throw new Error('ERR_NO_ID_TOKEN') } @@ -103,16 +131,37 @@ export default class EntraAuthentication { */ let info: Record = claims if (config.serverMetadata().userinfo_endpoint) { - info = { - ...claims, - ...(await client.fetchUserInfo(config, tokens.access_token, claims.sub)) + try { + info = { + ...claims, + ...(await client.fetchUserInfo(config, tokens.access_token, claims.sub)) + } + } catch (err: any) { + strategyDebug(this, `the userinfo endpoint could not be read: ${describeAuthError(err)}`) + throw err } } + strategyDebug(this, `the tenant says: ${Object.keys(info).join(', ')}`) - const email = info[this.conf.emailClaim || 'email'] + const emailClaim = this.conf.emailClaim || 'email' + const email = info[emailClaim] if (!email || typeof email !== 'string') { + /* + The single most common way an Entra strategy does not work, which is why the log says what to + do about it: `email` is only emitted for an account with a Mail attribute or a tenant that + maps the optional claim, and `preferred_username` is where the address is otherwise. + */ + strategyDebug( + this, + `the \`${emailClaim}\` claim carries no address — set Email Claim to \`preferred_username\`, or map the \`email\` optional claim on the app registration` + ) throw new Error('ERR_NO_EMAIL_FROM_PROVIDER') } + const groups = this.conf.mapGroups === true ? this.groupsFrom(info) : undefined + strategyDebug( + this, + `${claims.sub} signs in as <${email}>${groups ? `, in ${groups.length} tenant group(s)` : ', groups not mapped'}` + ) return { // -> `oid` is the account's identifier within the tenant and `sub` is its identifier for this // one application. `sub` is the one to link by: it is what the ID token was verified as @@ -121,9 +170,9 @@ export default class EntraAuthentication { email, name: (info[this.conf.displayNameClaim || 'name'] as string) || email, picture: this.pictureFrom(info), - ...(this.conf.mapGroups === true + ...(groups ? { - groups: this.groupsFrom(info), + groups, groupsExclusive: this.conf.unassignMissingGroups === true } : {}) @@ -147,10 +196,22 @@ export default class EntraAuthentication { /** The group names — or, as Entra usually has it, the group object IDs — the claim carries. */ private groupsFrom(info: Record): string[] { - const value = info[this.conf.groupsClaim || 'groups'] + const claim = this.conf.groupsClaim || 'groups' + const value = info[claim] const raw = typeof value === 'string' ? [value] : Array.isArray(value) ? value : [] - return raw + const names = raw .filter((entry) => typeof entry === 'string' && entry.trim().length > 0) .map((entry) => entry.trim()) + /* + Logged even when it is empty, and especially then: a tenant emits this claim only for an app + registration configured to ask for it, and an empty answer is not distinguishable on the wiki + side from somebody genuinely being in no group. The values are worth seeing too — object IDs + where an administrator expected names is the other half of why a mapping matches nothing. + */ + strategyDebug( + this, + `the \`${claim}\` claim names ${names.length} group(s): ${names.join(', ') || 'none'}` + ) + return names } } diff --git a/backend/modules/authentication/github/authentication.ts b/backend/modules/authentication/github/authentication.ts index a33b948c4..11dcf3bf7 100644 --- a/backend/modules/authentication/github/authentication.ts +++ b/backend/modules/authentication/github/authentication.ts @@ -1,3 +1,4 @@ +import { missingSettings, strategyDebug } from '../../../helpers/authDebug.ts' import type { AuthFlow, AuthFlowCallback, ProviderProfile } from '../../../models/authentication.ts' /** How many pages of a hundred teams are read before the answer is treated as unusable. */ @@ -48,6 +49,10 @@ export default class GitHubAuthentication { const clientSecret = this.conf.clientSecret || '' const organization = (this.conf.allowedOrganization || '').trim() if (!clientId || !clientSecret) { + strategyDebug( + this, + `is not configured: ${missingSettings({ 'Client ID': clientId, 'Client Secret': clientSecret })}` + ) throw new Error('ERR_STRATEGY_MISCONFIGURED') } if (this.conf.mapGroups === true && !organization) { @@ -99,6 +104,12 @@ export default class GitHubAuthentication { headers: this.apiHeaders(accessToken) }) if (!resp.ok) { + // -> The body as well as the status: GitHub's error payload names the scope that was not + // granted or the resource that is not visible, which the status alone does not + strategyDebug( + this, + `GET ${path} answered ${resp.status}: ${await resp.text().catch(() => '(no body)')}` + ) throw new Error(`ERR_PROVIDER_REQUEST_FAILED`) } return resp.json() @@ -128,9 +139,14 @@ export default class GitHubAuthentication { headers: this.apiHeaders(accessToken) }) if (resp.status === 204) { + strategyDebug(this, `${login} is a member of ${org}`) return true } if (resp.status === 404) { + strategyDebug( + this, + `${login} is not a member of ${org} as far as this token can see — a private membership needs the OAuth app approved by the organization` + ) return false } WIKI.logger.warn( @@ -176,6 +192,7 @@ export default class GitHubAuthentication { } } if (batch.length < 100) { + strategyDebug(this, `${names.length} team(s) in ${org}: ${names.join(', ') || 'none'}`) return names } } @@ -233,14 +250,25 @@ export default class GitHubAuthentication { try { token = (await tokenResp.json()) as Record } catch { + strategyDebug( + this, + `the token exchange answered ${tokenResp.status} with something that is not JSON — is something else answering for ${this.hosts.web}?` + ) throw new Error('ERR_TOKEN_EXCHANGE_FAILED') } if (!tokenResp.ok || token.error || !token.access_token) { + // -> GitHub's own account of the refusal, which names the cause: `bad_verification_code` for a + // code already spent, `incorrect_client_credentials` for a Client Secret that has been reset + strategyDebug( + this, + `the token exchange answered ${tokenResp.status}: ${[token.error, token.error_description].filter(Boolean).join(': ') || 'no access token'}` + ) throw new Error('ERR_TOKEN_EXCHANGE_FAILED') } const account = await this.api('/user', token.access_token) if (!account?.id) { + strategyDebug(this, 'the API answered with no account for this token') throw new Error('ERR_NO_PROVIDER_ACCOUNT') } @@ -251,6 +279,13 @@ export default class GitHubAuthentication { const emails: any[] = await this.api('/user/emails', token.access_token) const email = emails?.find((entry) => entry.primary && entry.verified)?.email if (!email) { + // -> Counts rather than the addresses themselves, which are not needed to tell the two cases + // apart: no addresses at all is the `user:email` scope missing, and addresses with no + // verified primary among them is an account that has to confirm one at GitHub first + strategyDebug( + this, + `${account.login} has no verified primary address (${emails?.length ?? 0} address(es) readable, ${emails?.filter((entry) => entry.verified).length ?? 0} verified)` + ) throw new Error('ERR_NO_VERIFIED_EMAIL_FROM_PROVIDER') } @@ -260,13 +295,22 @@ export default class GitHubAuthentication { } } + const groups = + this.conf.mapGroups === true + ? await this.teamsIn(organization, token.access_token) + : undefined + strategyDebug( + this, + `${account.login} (${account.id}) signs in as <${email}>${groups ? `, on ${groups.length} team(s)` : ', groups not mapped'}` + ) + return { id: String(account.id), email, name: account.name || account.login, - ...(this.conf.mapGroups === true + ...(groups ? { - groups: await this.teamsIn(organization, token.access_token), + groups, groupsExclusive: this.conf.unassignMissingGroups === true } : {}) diff --git a/backend/modules/authentication/google/authentication.ts b/backend/modules/authentication/google/authentication.ts index 261513146..8bc639ad5 100644 --- a/backend/modules/authentication/google/authentication.ts +++ b/backend/modules/authentication/google/authentication.ts @@ -1,4 +1,5 @@ import * as client from 'openid-client' +import { describeAuthError, missingSettings, strategyDebug } from '../../../helpers/authDebug.ts' import type { AuthFlow, AuthFlowCallback, ProviderProfile } from '../../../models/authentication.ts' /** Google's issuer, from which every endpoint and signing key is discovered. */ @@ -58,13 +59,26 @@ export default class GoogleAuthentication { return this.config } if (!this.conf.clientId || !this.conf.clientSecret) { + strategyDebug( + this, + `is not configured: ${missingSettings({ 'Client ID': this.conf.clientId, 'Client Secret': this.conf.clientSecret })}` + ) throw new Error('ERR_STRATEGY_MISCONFIGURED') } - this.config = await client.discovery( - new URL(ISSUER), - this.conf.clientId, - this.conf.clientSecret - ) + try { + this.config = await client.discovery( + new URL(ISSUER), + this.conf.clientId, + this.conf.clientSecret + ) + } catch (err: any) { + // -> Google's own metadata, so this is the wiki's outbound connectivity rather than a setting + strategyDebug( + this, + `could not read Google's metadata from ${ISSUER}: ${describeAuthError(err)}` + ) + throw err + } return this.config } @@ -84,6 +98,7 @@ export default class GoogleAuthentication { url.searchParams.set('pageToken', pageToken) } let resp: Response + strategyDebug(this, `asking the Cloud Identity API for group memberships: ${query}`) try { resp = await fetch(url, { headers: { Authorization: `Bearer ${accessToken}`, Accept: 'application/json' } @@ -98,6 +113,12 @@ export default class GoogleAuthentication { WIKI.logger.warn( `Google strategy ${this.strategyId} asked the Cloud Identity API for group memberships and it answered ${resp.status}.` ) + // -> The body as well as the status, under the flag: Google's error payload names the API that + // is not enabled or the permission that is missing, which the status alone does not + strategyDebug( + this, + `the Cloud Identity API answered ${resp.status}: ${await resp.text().catch(() => '(no body)')}` + ) throw new Error('ERR_PROVIDER_REQUEST_FAILED') } return resp.json() @@ -122,6 +143,10 @@ export default class GoogleAuthentication { accessToken: string ): Promise { if (!hostedDomain) { + strategyDebug( + this, + `<${email}> is in no Workspace (no \`hd\` claim), so it is in no groups either` + ) return [] } /* @@ -144,6 +169,10 @@ export default class GoogleAuthentication { } pageToken = body?.nextPageToken if (!pageToken) { + strategyDebug( + this, + `the Cloud Identity API names ${names.length} group(s) for <${email}>, read by ${byName ? 'display name' : 'group key'}: ${names.join(', ') || 'none'}` + ) return names } } @@ -189,34 +218,66 @@ export default class GoogleAuthentication { codeVerifier }: AuthFlowCallback): Promise { const config = await this.configuration() - const tokens = await client.authorizationCodeGrant(config, new URL(currentUrl), { - expectedState: state, - expectedNonce: nonce, - pkceCodeVerifier: codeVerifier - }) + let tokens + try { + tokens = await client.authorizationCodeGrant(config, new URL(currentUrl), { + expectedState: state, + expectedNonce: nonce, + pkceCodeVerifier: codeVerifier + }) + } catch (err: any) { + strategyDebug(this, `Google's answer did not check out: ${describeAuthError(err)}`) + throw err + } const claims = tokens.claims() as Record | undefined if (!claims?.sub) { + strategyDebug( + this, + 'Google returned no ID token, so there is nothing signed saying who signed in' + ) throw new Error('ERR_NO_ID_TOKEN') } const email = claims.email if (!email || typeof email !== 'string') { + strategyDebug( + this, + `the ID token carries no email claim. It carries: ${Object.keys(claims).join(', ')}` + ) throw new Error('ERR_NO_EMAIL_FROM_PROVIDER') } if (claims.email_verified === false && this.conf.allowUnverifiedEmail !== true) { + strategyDebug( + this, + `Google has not verified <${email}>, and this strategy does not allow unverified addresses` + ) throw new Error('ERR_EMAIL_NOT_VERIFIED') } if (this.conf.hostedDomain && claims.hd !== this.conf.hostedDomain) { + // -> Which domain it IS is the whole of the diagnosis: a personal account has no `hd` at all, + // and a Workspace account has the one it is in, misspelled setting or not + strategyDebug( + this, + `<${email}> is in ${claims.hd ? `the \`${claims.hd}\` Workspace` : 'no Workspace'}, and this strategy is restricted to \`${this.conf.hostedDomain}\`` + ) throw new Error('ERR_ACCOUNT_NOT_ALLOWED') } + const groups = + this.conf.mapGroups === true + ? await this.groupsFor(email, claims.hd, tokens.access_token) + : undefined + strategyDebug( + this, + `${claims.sub} signs in as <${email}>${groups ? `, in ${groups.length} Workspace group(s)` : ', groups not mapped'}` + ) return { id: claims.sub, email, name: (claims.name as string) || email, - ...(this.conf.mapGroups === true + ...(groups ? { - groups: await this.groupsFor(email, claims.hd, tokens.access_token), + groups, groupsExclusive: this.conf.unassignMissingGroups === true } : {}) diff --git a/backend/modules/authentication/ldap/authentication.ts b/backend/modules/authentication/ldap/authentication.ts index 6d3faa13d..b8a624811 100644 --- a/backend/modules/authentication/ldap/authentication.ts +++ b/backend/modules/authentication/ldap/authentication.ts @@ -2,6 +2,7 @@ import fs from 'node:fs/promises' import type { ConnectionOptions } from 'node:tls' import { Client, Filter, InvalidCredentialsError } from 'ldapts' import type { Entry, SearchOptions } from 'ldapts' +import { describeAuthError, missingSettings, strategyDebug } from '../../../helpers/authDebug.ts' import type { ProviderProfile } from '../../../models/authentication.ts' /** What a form module is handed for one attempt. `login()` in `models/users.ts` assembles it. */ @@ -61,9 +62,17 @@ export default class LdapAuthentication { async profile({ username, password }: FormCredential): Promise { const { url, bindDn, searchBase, searchFilter } = this.conf if (!url || !bindDn || !searchBase || !searchFilter) { + strategyDebug( + this, + `is not configured: ${missingSettings({ 'LDAP URL': url, 'Admin Bind DN': bindDn, 'Search Base': searchBase, 'Search Filter': searchFilter })}, so no login can be attempted` + ) throw new Error('ERR_STRATEGY_MISCONFIGURED') } if (!searchFilter.includes('{{username}}')) { + strategyDebug( + this, + `cannot look anybody up: the Search Filter \`${searchFilter}\` has no {{username}} placeholder for the typed username to go in` + ) throw new Error('ERR_STRATEGY_MISCONFIGURED') } /* @@ -72,16 +81,46 @@ export default class LdapAuthentication { into an LDAP-backed application and it must never reach the wire. */ if (!username || !password) { + strategyDebug( + this, + `refused an attempt with ${username ? 'an empty password' : 'no username'} without asking the directory` + ) throw new Error('ERR_LOGIN_FAILED') } - const search = await this.connect() + /* + Opening the connection is inside the same reporting as everything after it: with StartTLS + configured, `connect` upgrades the connection there and then, so a directory that cannot be + reached fails HERE rather than on the bind below — and left outside, the socket error escaped + `asLoginError` and became the code the login screen showed somebody. + */ + let search: Client try { - await search.bind(bindDn, this.conf.bindCredentials ?? '') + search = await this.connect('the user search') + } catch (err: any) { + throw this.asLoginError(err) + } + try { + try { + await search.bind(bindDn, this.conf.bindCredentials ?? '') + } catch (err: any) { + /* + The wiki's own account and not the person signing in — so this refuses every login until it + is fixed, and it is worth saying apart from a bad password. Whether the credentials are set + at all is said, since an empty one is what a directory that allows anonymous search hides. + */ + strategyDebug( + this, + `the directory refused the wiki's own bind as \`${bindDn}\` (Admin Bind Credentials ${this.conf.bindCredentials ? 'set' : 'empty'}): ${describeAuthError(err)}` + ) + throw err + } + const filter = searchFilter.replaceAll('{{username}}', Filter.escape(username)) + strategyDebug(this, `searching \`${searchBase}\` (scope sub) for \`${filter}\``) const found = await search.search(searchBase, { scope: 'sub', - filter: searchFilter.replaceAll('{{username}}', Filter.escape(username)), + filter, sizeLimit: 2, ...this.attributeOptions() }) @@ -91,28 +130,55 @@ export default class LdapAuthentication { directory happened to return first. */ if (found.searchEntries.length !== 1) { + strategyDebug( + this, + found.searchEntries.length < 1 + ? `nothing under \`${searchBase}\` matched \`${filter}\` — check the Search Base and the Search Filter against the directory's own tree` + : `more than one entry matched \`${filter}\`, so it does not identify one person: ${found.searchEntries.map((one) => one.dn).join(', ')}` + ) throw new Error('ERR_LOGIN_FAILED') } const entry = found.searchEntries[0] + strategyDebug(this, `"${username}" is \`${entry.dn}\``) await this.verifyPassword(entry.dn, password) - const id = this.attr(entry, this.conf.mappingUID || 'uid') + const uidField = this.conf.mappingUID || 'uid' + const id = this.attr(entry, uidField) if (!id) { + strategyDebug( + this, + `\`${entry.dn}\` has no \`${uidField}\` to be identified by. Its attributes are: ${this.attributeNames(entry)}` + ) throw new Error('ERR_NO_PROVIDER_ACCOUNT') } - const email = this.attr(entry, this.conf.mappingEmail || 'mail') + const emailField = this.conf.mappingEmail || 'mail' + const email = this.attr(entry, emailField) if (!email) { + strategyDebug( + this, + `\`${entry.dn}\` has no address in \`${emailField}\`, and an account here is matched by address. Its attributes are: ${this.attributeNames(entry)}` + ) throw new Error('ERR_NO_EMAIL_FROM_PROVIDER') } + // -> Read before the answer rather than in it, so what the directory said about this person's + // groups is logged as part of the attempt and not only once a membership actually changes + const groups = this.conf.mapGroups === true ? await this.groupsFor(search, entry) : undefined + // -> Before the line below rather than in the answer, so that the log reads in the order the + // work happened and "signs in" is the last thing said about the attempt + const pictureData = this.pictureFrom(entry) + strategyDebug( + this, + `\`${entry.dn}\` signs in as <${email}> with id \`${id}\`${groups ? `, in ${groups.length} directory group(s)` : ', groups not mapped'}` + ) return { id, email, name: this.attr(entry, this.conf.mappingDisplayName || 'displayName') || email, - pictureData: this.pictureFrom(entry), - ...(this.conf.mapGroups === true + pictureData, + ...(groups ? { - groups: await this.groupsFor(search, entry), + groups, groupsExclusive: this.conf.unassignMissingGroups === true } : {}) @@ -131,12 +197,19 @@ export default class LdapAuthentication { * opens in the clear and upgrades before anything is sent, which is a request of its own and so a * second round trip. Both end up at the same place, and which one a directory offers is not this * module's business — the URL says. + * + * @param purpose What this connection is for, for the log: there are two per login, and a failure + * on the second one is a different thing from a failure on the first */ - private async connect(): Promise { + private async connect(purpose: string): Promise { const secure = this.conf.url.toLowerCase().startsWith('ldaps://') // -> StartTLS on an `ldaps://` URL would be upgrading a connection that is already encrypted const startTls = this.conf.tlsEnabled === true && !secure const tlsOptions = secure || startTls ? await this.tlsOptions() : undefined + strategyDebug( + this, + `opening a connection for ${purpose} to ${this.conf.url} (${this.protection(secure, startTls)})` + ) const conn = new Client({ url: this.conf.url, timeout: OPERATION_TIMEOUT_MS, @@ -156,6 +229,24 @@ export default class LdapAuthentication { return conn } + /** + * How the connection this login is being made over is protected, as a phrase for the log. + * + * Worth saying on every attempt because it is derived rather than configured: the URL's scheme + * decides it, and "Use StartTLS" is silently ignored on an `ldaps://` URL that is encrypted + * already. A wiki whose directory is being talked to in the clear should be able to see that here. + */ + private protection(secure: boolean, startTls: boolean): string { + if (!secure && !startTls) { + return 'unencrypted' + } + const scheme = secure ? 'ldaps' : 'StartTLS' + if (this.conf.verifyTLSCertificate === false) { + return `${scheme}, certificate NOT verified` + } + return `${scheme}, certificate verified${this.conf.tlsCertPath ? ` against ${this.conf.tlsCertPath}` : ''}` + } + /** * How the directory's certificate is treated. * @@ -181,13 +272,28 @@ export default class LdapAuthentication { * a connection bound as somebody else has no further use here. */ private async verifyPassword(dn: string, password: string): Promise { - const asUser = await this.connect() + const asUser = await this.connect('the password check') try { await asUser.bind(dn, password) + strategyDebug(this, `the directory accepted the password for \`${dn}\``) } catch (err: any) { if (err instanceof InvalidCredentialsError) { + /* + Whatever the directory said with it: Active Directory reports a locked, disabled or expired + account as invalid credentials too, and names which in a `data` code inside the message that + `describe` prints. So this line is what separates "wrong password" from "this account cannot + sign in at all", neither of which the login screen is told apart. + */ + strategyDebug( + this, + `the directory refused the password for \`${dn}\`: ${describeAuthError(err)}` + ) throw new Error('ERR_LOGIN_FAILED') } + strategyDebug( + this, + `the directory could not check the password for \`${dn}\`: ${describeAuthError(err)}` + ) throw err } finally { await this.release(asUser) @@ -205,23 +311,44 @@ export default class LdapAuthentication { private async groupsFor(search: Client, entry: Entry): Promise { const { groupSearchBase, groupSearchFilter } = this.conf if (!groupSearchBase || !groupSearchFilter) { + strategyDebug( + this, + `maps groups but ${missingSettings({ 'Group Search Base': groupSearchBase, 'Group Search Filter': groupSearchFilter })}, so there is nothing to search` + ) throw new Error('ERR_STRATEGY_MISCONFIGURED') } const nameField = this.conf.groupNameField || 'name' const dnProperty = this.conf.groupDnProperty || 'dn' const dnValue = dnProperty === 'dn' ? entry.dn : this.attr(entry, dnProperty) if (!dnValue) { + strategyDebug( + this, + `maps groups by the \`${dnProperty}\` of \`${entry.dn}\`, which the entry does not have. Its attributes are: ${this.attributeNames(entry)}` + ) throw new Error('ERR_STRATEGY_MISCONFIGURED') } + const scope = (this.conf.groupSearchScope || 'sub') as SearchOptions['scope'] + const filter = groupSearchFilter.replaceAll('{{dn}}', Filter.escape(dnValue)) + strategyDebug(this, `searching \`${groupSearchBase}\` (scope ${scope}) for \`${filter}\``) const found = await search.search(groupSearchBase, { - scope: (this.conf.groupSearchScope || 'sub') as SearchOptions['scope'], - filter: groupSearchFilter.replaceAll('{{dn}}', Filter.escape(dnValue)), + scope, + filter, attributes: [nameField] }) - return found.searchEntries + const names = found.searchEntries .map((grp) => this.attr(grp, nameField)) .filter((name): name is string => Boolean(name)) + /* + Both numbers, because they differ for a reason worth seeing: an entry counted here but not named + is a group whose `groupNameField` is not the attribute this is reading, which reads on the wiki + side as a membership the directory did not grant. + */ + strategyDebug( + this, + `${found.searchEntries.length} group entr${found.searchEntries.length === 1 ? 'y' : 'ies'} matched, ${names.length} named by \`${nameField}\`: ${names.join(', ') || 'none'}` + ) + return names } /** @@ -264,7 +391,14 @@ export default class LdapAuthentication { } const value = entry[name] const first = Array.isArray(value) ? value[0] : value - return Buffer.isBuffer(first) && first.length > 0 ? first : undefined + if (!Buffer.isBuffer(first) || first.length < 1) { + strategyDebug( + this, + `\`${entry.dn}\` carries no image in \`${name}\`, so no avatar was taken from the directory` + ) + return undefined + } + return first } /** @@ -279,14 +413,25 @@ export default class LdapAuthentication { if (typeof err?.message === 'string' && err.message.startsWith('ERR_')) { return err } - // -> The class name as well as the message: `ldapts` raises a result-code error whose message is - // only the code, and "InvalidCredentialsError" is what says the wiki's own bind DN is wrong WIKI.logger.warn( - `LDAP strategy ${this.strategyId} could not complete a login: ${err.name}: ${err.message}` + `LDAP strategy ${this.strategyId} could not complete a login: ${describeAuthError(err)}` ) return new Error('ERR_PROVIDER_REQUEST_FAILED') } + /** + * The attribute names an entry came back with. + * + * Names only — a directory holds a person's password hash and rather more besides, and none of the + * values are anybody's business here. What the list answers is the question a failed mapping raises: + * the search asks for every attribute, so this is exactly what the four Field Mapping settings have + * to be chosen from. + */ + private attributeNames(entry: Entry): string { + const names = Object.keys(entry).filter((key) => key !== 'dn') + return names.length > 0 ? names.join(', ') : 'none' + } + /** Close a connection without letting the close itself fail a login that already succeeded. */ private async release(conn: Client): Promise { try { diff --git a/backend/modules/authentication/local/authentication.ts b/backend/modules/authentication/local/authentication.ts index ba803deaa..3cdd25462 100644 --- a/backend/modules/authentication/local/authentication.ts +++ b/backend/modules/authentication/local/authentication.ts @@ -1,5 +1,5 @@ -/* global WIKI */ import bcrypt from 'bcryptjs' +import { strategyDebug } from '../../../helpers/authDebug.ts' // ------------------------------------ // Local Account @@ -15,25 +15,55 @@ export default class LocalAuthentication { this.conf = conf } + /** + * Whether this is the account's own password, and whether the account may use it. + * + * Each refusal is logged under the auth debug flag, because the login screen is told apart only two + * of these six: an account that does not exist, one that exists but signs in through some other + * strategy, and one whose password is simply wrong all reach the user as the same failure. Which it + * was is what an administrator being told "it will not let me in" needs. + */ async authenticate({ username, password }: { username: string; password: string }): Promise { - const user = await WIKI.models.users.getByEmail(username.toLowerCase()) - if (user) { - const authStrategyData = (user.auth as Record)[this.strategyId] - if (!authStrategyData) { - throw new Error('ERR_INVALID_STRATEGY') - } else if ((await bcrypt.compare(password, authStrategyData.password)) !== true) { - throw new Error('ERR_LOGIN_FAILED') - } else if (!user.isActive) { - throw new Error('ERR_INACTIVE_USER') - } else if (authStrategyData.restrictLogin) { - throw new Error('ERR_LOGIN_RESTRICTED') - } else if (!user.isVerified) { - throw new Error('ERR_USER_NOT_VERIFIED') - } else { - return user - } - } else { + const email = username.toLowerCase() + const user = await WIKI.models.users.getByEmail(email) + if (!user) { + strategyDebug(this, `no account here has the address <${email}>`) throw new Error('ERR_LOGIN_FAILED') } + const authStrategyData = (user.auth as Record)[this.strategyId] + if (!authStrategyData) { + strategyDebug( + this, + `user ${user.id} <${user.email}> has no password for this strategy — the account signs in through another one` + ) + throw new Error('ERR_INVALID_STRATEGY') + } + if ((await bcrypt.compare(password, authStrategyData.password)) !== true) { + strategyDebug(this, `user ${user.id} <${user.email}> gave the wrong password`) + throw new Error('ERR_LOGIN_FAILED') + } + if (!user.isActive) { + strategyDebug( + this, + `user ${user.id} <${user.email}> gave the right password, but the account is deactivated` + ) + throw new Error('ERR_INACTIVE_USER') + } + if (authStrategyData.restrictLogin) { + strategyDebug( + this, + `user ${user.id} <${user.email}> gave the right password, but the account is barred from signing in` + ) + throw new Error('ERR_LOGIN_RESTRICTED') + } + if (!user.isVerified) { + strategyDebug( + this, + `user ${user.id} <${user.email}> gave the right password, but the address has never been confirmed` + ) + throw new Error('ERR_USER_NOT_VERIFIED') + } + strategyDebug(this, `user ${user.id} <${user.email}> gave the right password`) + return user } } diff --git a/backend/modules/authentication/oidc/authentication.ts b/backend/modules/authentication/oidc/authentication.ts index 43abc7b50..c8769464d 100644 --- a/backend/modules/authentication/oidc/authentication.ts +++ b/backend/modules/authentication/oidc/authentication.ts @@ -1,4 +1,5 @@ import * as client from 'openid-client' +import { describeAuthError, missingSettings, strategyDebug } from '../../../helpers/authDebug.ts' import type { AuthFlow, AuthFlowCallback, ProviderProfile } from '../../../models/authentication.ts' /** @@ -47,12 +48,36 @@ export default class OidcAuthentication { } const { clientId, clientSecret, issuer } = this.conf if (!clientId || !clientSecret || !issuer) { + strategyDebug( + this, + `is not configured: ${missingSettings({ 'Client ID': clientId, 'Client Secret': clientSecret, Issuer: issuer })}` + ) throw new Error('ERR_STRATEGY_MISCONFIGURED') } if (this.conf.useDiscovery !== false) { - this.config = await client.discovery(new URL(issuer), clientId, clientSecret) + strategyDebug(this, `reading the provider's metadata from ${issuer}`) + try { + this.config = await client.discovery(new URL(issuer), clientId, clientSecret) + } catch (err: any) { + // -> The first thing to fail on a new strategy, and it fails for reasons the log has to + // carry: an issuer that is not a URL, one publishing no discovery document, TLS + strategyDebug( + this, + `could not read the provider's metadata from ${issuer}: ${describeAuthError(err)}` + ) + throw err + } + const meta = this.config.serverMetadata() + strategyDebug( + this, + `the provider is ${meta.issuer} — authorization at ${meta.authorization_endpoint}, token at ${meta.token_endpoint}, userinfo at ${meta.userinfo_endpoint ?? 'nowhere (it publishes none)'}` + ) } else { if (!this.conf.authorizationURL || !this.conf.tokenURL || !this.conf.jwksURL) { + strategyDebug( + this, + `has discovery turned off and is not configured: ${missingSettings({ 'Authorization Endpoint URL': this.conf.authorizationURL, 'Token Endpoint URL': this.conf.tokenURL, 'JWKS Endpoint URL': this.conf.jwksURL })}` + ) throw new Error('ERR_STRATEGY_MISCONFIGURED') } this.config = new client.Configuration( @@ -146,13 +171,29 @@ export default class OidcAuthentication { codeVerifier }: AuthFlowCallback): Promise { const config = await this.configuration() - const tokens = await client.authorizationCodeGrant(config, new URL(currentUrl), { - expectedState: state, - expectedNonce: nonce, - pkceCodeVerifier: codeVerifier - }) + let tokens + try { + tokens = await client.authorizationCodeGrant(config, new URL(currentUrl), { + expectedState: state, + expectedNonce: nonce, + pkceCodeVerifier: codeVerifier + }) + } catch (err: any) { + /* + Everything the flow is checked by is in here — the state, the PKCE verifier, the code + exchange, and the ID token's signature, issuer, audience and nonce — so this one line covers a + redirect URI the provider does not have registered, a client secret that has been rotated, a + clock that is out, and a token signed by a key the issuer does not publish. + */ + strategyDebug(this, `the provider's answer did not check out: ${describeAuthError(err)}`) + throw err + } const claims = tokens.claims() if (!claims?.sub) { + strategyDebug( + this, + 'the provider returned no ID token, so there is nothing signed saying who signed in' + ) throw new Error('ERR_NO_ID_TOKEN') } // -> Before the userinfo round trip: no point spending one on a login already being refused @@ -166,25 +207,46 @@ export default class OidcAuthentication { */ let info: Record = claims if (config.serverMetadata().userinfo_endpoint) { - info = { - ...claims, - ...(await client.fetchUserInfo(config, tokens.access_token, claims.sub)) + try { + info = { + ...claims, + ...(await client.fetchUserInfo(config, tokens.access_token, claims.sub)) + } + } catch (err: any) { + strategyDebug(this, `the userinfo endpoint could not be read: ${describeAuthError(err)}`) + throw err } } + strategyDebug(this, `the provider says: ${Object.keys(info).join(', ')}`) /* `sub` is the identifier OIDC guarantees is stable and never reassigned, and is what this reads unless an administrator names another claim. Whichever it is, it is what the account is linked by from here on — the ID token's own subject stays what the library verified the answer against. */ - const id = info[this.conf.idClaim || 'sub'] + const idClaim = this.conf.idClaim || 'sub' + const id = info[idClaim] if (!id || typeof id !== 'string') { + strategyDebug( + this, + `the \`${idClaim}\` claim carries no identifier for this account (Unique ID Claim)` + ) throw new Error('ERR_NO_PROVIDER_ACCOUNT') } - const email = info[this.conf.emailClaim || 'email'] + const emailClaim = this.conf.emailClaim || 'email' + const email = info[emailClaim] if (!email || typeof email !== 'string') { + strategyDebug( + this, + `the \`${emailClaim}\` claim carries no address, and an account here is matched by address (Email Claim)` + ) throw new Error('ERR_NO_EMAIL_FROM_PROVIDER') } + const groups = this.conf.mapGroups === true ? this.groupsFrom(info) : undefined + strategyDebug( + this, + `${id} signs in as <${email}>${groups ? `, in ${groups.length} provider group(s)` : ', groups not mapped'}` + ) return { id, email, @@ -192,9 +254,9 @@ export default class OidcAuthentication { picture: this.pictureFrom(info), // -> Absent rather than empty when groups are not mapped: an empty list is the provider saying // this person is in none, which with `unassignMissingGroups` on takes memberships away - ...(this.conf.mapGroups === true + ...(groups ? { - groups: this.groupsFrom(info), + groups, groupsExclusive: this.conf.unassignMissingGroups === true } : {}) @@ -278,11 +340,23 @@ export default class OidcAuthentication { * since a name that matches nothing is a membership silently not granted. */ private groupsFrom(info: Record): string[] { - const value = info[this.conf.groupsClaim || 'groups'] + const claim = this.conf.groupsClaim || 'groups' + const value = info[claim] const raw = typeof value === 'string' ? [value] : Array.isArray(value) ? value : [] - return raw + const names = raw .filter((entry) => typeof entry === 'string' && entry.trim().length > 0) .map((entry) => entry.trim()) + /* + Said even when it is empty, and especially then: a provider that emits the groups claim only for + a client asking for the right scope — or only once the claim is configured on the application — + answers with nothing here, which is not distinguishable on the wiki side from somebody genuinely + being in no group. + */ + strategyDebug( + this, + `the \`${claim}\` claim names ${names.length} group(s): ${names.join(', ') || 'none'}` + ) + return names } /** diff --git a/backend/modules/authentication/saml/authentication.ts b/backend/modules/authentication/saml/authentication.ts index 9450a9ea7..d88387d96 100644 --- a/backend/modules/authentication/saml/authentication.ts +++ b/backend/modules/authentication/saml/authentication.ts @@ -1,5 +1,6 @@ import { SAML, ValidateInResponseTo } from '@node-saml/node-saml' import type { SamlConfig } from '@node-saml/node-saml' +import { missingSettings, strategyDebug } from '../../../helpers/authDebug.ts' import { CustomError } from '../../../helpers/common.ts' import type { AuthFlow, @@ -51,6 +52,10 @@ export default class SamlAuthentication { private saml(callbackUrl: string): SAML { const { entryPoint, issuer, cert } = this.conf if (!entryPoint || !issuer || !cert) { + strategyDebug( + this, + `is not configured: ${missingSettings({ 'Login URL': entryPoint, 'Issuer / Entity ID': issuer, "Identity Provider's Certificate": cert })}` + ) throw new Error('ERR_STRATEGY_MISCONFIGURED') } const idpCert = String(cert) @@ -59,8 +64,16 @@ export default class SamlAuthentication { .filter((one) => one.length > 0) .slice(0, MAX_CERTS) if (idpCert.length < 1) { + strategyDebug( + this, + "the Identity Provider's Certificate holds no certificate, so no assertion can be verified" + ) throw new Error('ERR_STRATEGY_MISCONFIGURED') } + strategyDebug( + this, + `assertions are expected from ${entryPoint} for audience \`${this.conf.audience || issuer}\`, verified against ${idpCert.length} certificate(s), posted back to ${callbackUrl}` + ) const options: SamlConfig = { callbackUrl, @@ -127,6 +140,10 @@ export default class SamlAuthentication { */ async profile({ redirectUri, body }: AuthFlowCallback): Promise { if (!body?.SAMLResponse) { + strategyDebug( + this, + `the callback carried no SAMLResponse. It carried: ${Object.keys(body ?? {}).join(', ') || 'nothing'}` + ) throw new Error('ERR_NO_PROVIDER_ACCOUNT') } const saml = this.saml(redirectUri) @@ -145,8 +162,16 @@ export default class SamlAuthentication { throw new Error('ERR_LOGIN_FAILED') } if (!profile) { + strategyDebug( + this, + 'the assertion verified but carried no subject, so there is nobody in it to sign in' + ) throw new Error('ERR_NO_PROVIDER_ACCOUNT') } + // -> The names and not the values: which attributes an identity provider actually asserts is what + // the four Field Mapping settings have to be chosen from, and is never quite what its + // documentation says — AD FS and Entra both send URI-shaped ones + strategyDebug(this, `the assertion carries: ${Object.keys(profile).join(', ')}`) /* Attributes are read off the profile, where `node-saml` puts each of them under its own name @@ -156,20 +181,33 @@ export default class SamlAuthentication { */ const id = this.attr(profile, this.conf.mappingUID) ?? profile.nameID if (!id) { + strategyDebug( + this, + `neither \`${this.conf.mappingUID || '(no Unique ID mapping)'}\` nor the NameID identifies this account` + ) throw new Error('ERR_NO_PROVIDER_ACCOUNT') } const email = this.attr(profile, this.conf.mappingEmail) if (!email) { + strategyDebug( + this, + `\`${this.conf.mappingEmail || '(no Email mapping)'}\` carries no address, and an account here is matched by address` + ) throw new Error('ERR_NO_EMAIL_FROM_PROVIDER') } + const groups = this.conf.mapGroups === true ? this.groupsFrom(profile) : undefined + strategyDebug( + this, + `${id} signs in as <${email}>${groups ? `, in ${groups.length} asserted group(s)` : ', groups not mapped'}` + ) return { id, email, name: this.attr(profile, this.conf.mappingDisplayName) || email, picture: this.attr(profile, this.conf.mappingPicture), - ...(this.conf.mapGroups === true + ...(groups ? { - groups: this.groupsFrom(profile), + groups, groupsExclusive: this.conf.unassignMissingGroups === true } : {}) @@ -233,10 +271,21 @@ export default class SamlAuthentication { * string, and both forms mean the same thing here. */ private groupsFrom(profile: Record): string[] { - const value = profile[this.conf.mappingGroups || 'memberOf'] + const attribute = this.conf.mappingGroups || 'memberOf' + const value = profile[attribute] const raw = typeof value === 'string' ? [value] : Array.isArray(value) ? value : [] - return raw + const names = raw .filter((entry) => typeof entry === 'string' && entry.trim().length > 0) .map((entry) => entry.trim()) + /* + Logged even when it is empty, and especially then: a provider asserts group membership only for + a relying party configured to receive it, and an empty answer is not distinguishable on the + wiki side from somebody genuinely being in no group. + */ + strategyDebug( + this, + `\`${attribute}\` names ${names.length} group(s): ${names.join(', ') || 'none'}` + ) + return names } } diff --git a/frontend/src/components/GroupEditOverlay.vue b/frontend/src/components/GroupEditOverlay.vue index 3ff835a11..49f67fb75 100644 --- a/frontend/src/components/GroupEditOverlay.vue +++ b/frontend/src/components/GroupEditOverlay.vue @@ -196,7 +196,7 @@ {{ t('admin.groups.exportRules') }} @@ -204,7 +204,7 @@