Login/Signup with OIDC

[KernelDeimos]

let's not merge on a Friday night, but this is ready for review.

These changes implement login and signup for OIDC flows, as well as consequential changes because of how these changes disrupt other parts of authentication, such as:

• user protected endpoint need a new re-auth flow for OIDC-signed-up users
• GUI tokens and http-only tokens need to be separated to support redirect flows safely

[Salazareo]

Artisan, handmade, human review

So a lot of these are more stylistic and code structure changes.

however, please do remove noisy logs, bits that make the code footprint larger ( commented out code, pseudo-blocks and boilerplate classes if they can be avoided) and avoid the this.modules.* syntax from before as it breaks support from language servers.

aside from that. 2 things to note:

• that object outcome success looks suspect

• lets avoid hard coded X provider configs, like Google_Scope, and such, ideally we make this class follow spec and just pull config as needed from some storage, whether it be live or via config redeploy.

lastly I had some concerns about the change with gui token and http-only token.

fully in favour of moving to http-only tokens, and giving granular restrictions to tokens, but we'll need to verify to make sure existing features (like switching acocunts from gui) still function with that.

Additionally, and I'll say to take this with a grain of salt, but running codex 5.3 on this flagged some things too. Some I already mentioned in my artisan, human review, but some others I missed and definitely worth looking at:

LLM slop review

Findings (ordered by severity)

1. Critical: OIDC signup linking uses outcome.success incorrectly, so the link step always executes.
   File: src/backend/src/services/auth/OIDCService.js:237
OutcomeObject exposes a success() method and a failed flag. Checking if (outcome.success) is always truthy (function reference), so linkProviderToUser(...) runs even for failed outcomes. That can attempt linking with user_id = null, turning expected signup validation failures into DB errors/500s.

2. High: Change password request payload is broken for non-OIDC path and can throw in OIDC retry path.
   File: src/gui/src/UI/UIWindowChangePassword.js:174
doSubmit expects an object ({ new_password, current_password }), but code passes a string (doSubmit(current_password)), so both fields serialize as undefined. Also doSubmit() is called with no args on revalidation retry (src/gui/src/UI/UIWindowChangePassword.js:183), which throws due destructuring from undefined. This can break password change for normal users and OIDC users.

3. High: Change email OIDC retry can throw at runtime.
   File: src/gui/src/UI/Settings/UIWindowChangeEmail.js:167
doSubmit destructures { new_email, password }, but retry path calls doSubmit() with no argument. That throws before the request is sent, so OIDC revalidation flow can fail after popup.

4. Medium: Username revalidation flow does not await popup completion before retry.
   File: src/gui/src/UI/UIWindowChangeUsername.js:131
   and src/gui/src/UI/UIWindowChangeUsername.js:151
myOpenRevalidatePopup(...) is invoked without await, and a protected request is sent immediately afterward. The retry can race before cookie issuance and fail spuriously.

5. Medium: Sensitive debug logging was added in auth paths.
   Files: src/backend/src/middleware/configurable_auth.js:126, src/backend/src/routers/auth/oidc.js:31, src/backend/src/routers/logout.js:54
   These log decoded token data, redirect state, and account password/email nullness to stdout. Even if some are gated by DEBUG, this is high-risk auth telemetry and should be removed or replaced with structured/redacted service logs.

6. Low: Wrong rate-limit key used for username changes.
   File: src/backend/src/routers/user-protected/change-username.js:49
   Route checks svc_edgeRateLimit.check('change-email-start') instead of a username-specific key. This ties username throttling behavior to the email flow by mistake and undermines intended controls.

Open Questions / Assumptions

1. I assumed this review target is behavior/correctness/security; I did not run full backend/frontend test suites in this pass.
2. I also noticed some incidental cleanup debt (debug comments, unused imports, duplicated utility additions), but I excluded style-only notes from findings.

Last notes

This was all static analysis, so we'll need to test this in depth before deploying after any changes.

[Salazareo]

nit: can we just do if blocks for clarity?

[KernelDeimos]

Where? Can you give an example? GitHub might be showing the wrong section of code, but this is my argument against what I think you're suggesting:

first rewrite:

let origin;
if ( svc_oidc.global_config?.origin ) {
    origin = origin.replace(/\/$/, '');
} else {
    origin = '';
}

problem: changes a const to a let, fixable by:

cosnt origin = (() => {
    if ( svc_oidc.global_config?.origin ) {
        return origin.replace(/\/$/, '');
    } else {
        return '';
    }
})();

problem: unnecessary IIFE - probably more confusing to read for most

[Salazareo]

this is a return, no need for let nor IIFE

[KernelDeimos]

Not seeing how this would be simpler or easier to read. Can you show me using "Add a suggestion"?

[Salazareo]

Suggested change

	return provider
	if (provider) {
	return {oidc_revalidate_url: `${origin}/auth/oidc/${provider}/start?flow=revalidate&user_id=${req.user.id}`
	}
	return {}

[KernelDeimos]

I see now, GitHub wasn't showing the entire section of the code you were referring to as I suspected earlier. For the same reason your suggestion won't replace the entire expression; it will only replace thee return provider of:

return provider
  ? { oidc_revalidate_url: `${origin}/auth/oidc/${provider}/start?flow=revalidate&user_id=${req.user.id}` }
  : {};

GitHub allows commenting on a selection of lines of code btw - I would recommend doing that for future PR comments like this one.

I made this change because I agree that it's more clear. I still like the ternary expression better because it's easier to move around the expression as a whole because it's not interlaced with control-flow statements.