Feature: OpenID Connect (OIDC) authentication - #733
Open
Plattenspatz wants to merge 2 commits into
Open
Conversation
Owner
|
I am reviewing the PR, hope it is good and could be merged without much change. thanks a lot |
|
I am happy to see somebody trying to bring forward this 👍. For once this does NOT seem to be an AI SLOP / complete Rewrite of the Codebase, so that's already a good start 😉. I don't know much at all about Typescript, so I cannot provide much Feedback on that Side. I just flew very quickly over the Changes and I didn't see alarming red Flags such as Code Obfuscation, base64 encoded Values, etc, so it should be safe. Unsure about package-lock.json, though package.json only added
Not sure about this Change here and why Is this related to the Comment |
rajnandan1
requested changes
Jun 11, 2026
Owner
There was a problem hiding this comment.
Thanks for the PR. fundamentals are right, PKCE + state + nonce, OIDC and local accounts kept separate by sub, email collisions rejected instead of merged. good work
Blockers
- remove
src/lib/server/db/repositories/users.ts.save, backup file got committed - test connection poisons the live config cache.
TestOidcConnectionclears the cache thengetOidcConfig(settings)writes the unsaved client submitted settings intocachedConfig. after admin tests an edited config all real logins use those unsaved credentials. build a local Configuration for test, do not touch the shared cache - cache key is only issuer_url. changing client_id/secret for the same issuer does not invalidate. also clearing depends on the browser making a second
clearOidcCachecall afterstoreSiteData, if that fails we keep stale credentials silently. clear the cache server side in the storeSiteData handler when key is oidcSettings - KENER_BASE_PATH breaks the redirect_uri.
${url.origin}/account/oidc/callbackomits the base path but the state cookies are set with path = KENER_BASE_PATH. same for the hardcodedhref="/account/oidc/login"on signin page - users table header has 7 columns now but loading/empty rows still have
colspan={6}
Security
- client_secret goes to the browser via getSiteDataByKey. anyone with settings.read can see the IdP secret. mask it on read, update only when a new value is submitted
- allow_local_login false + broken IdP = nobody can sign in, no escape hatch. always allow local login for owner or add an env var override. this is needed before release
- testOidcConnection fetches a client supplied url server side. settings.write gated so ok, but require https in non dev
- auto_create_users true + default role member means every IdP account can self provision into kener. make deafult false imo
Minor
- updateUserOidcSub is dead code, never called
- email/name never refreshed from IdP on next logins, worth syncing along with roles noh?
- openid-client v6 enforces https for discovery, http issuer in local dev will fail with an unhelpful error
- migration drops NOT NULL on password_hash for pg/mysql but code always inserts
""anyway. pick one. down() does not restore NOT NULL - no server side validation in upsertOidcGroupRoleMapping, empty group or bad role_id surfaces a raw db error
- RoleRecord imported from
$lib/server/types/db.jsin a client component, shared types go insrc/lib/types - == vs === in users page and a stray whitespace edit in
(manage)/+layout.svelte, runnpm run prettify - callback checks cookies before the IdP error param, an error after the 10 min cookie expiry shows a generic 400
also please add a docs page under src/routes/(docs)/docs/content, setup + redirect uri + group mapping + the lockout caveat
fix the blockers and we can merge
rajnandan1
reviewed
Jun 11, 2026
| </div> | ||
| <!-- Resend Invitation --> | ||
| {#if !toEditUser.has_password} | ||
| {#if !toEditUser.has_password && toEditUser.auth_provider !== "oidc"} |
Owner
There was a problem hiding this comment.
can we move this global-constant file "oidc"
rajnandan1
reviewed
Jun 11, 2026
| /** | ||
| * Generate a cryptographically random string for state/nonce parameters. | ||
| */ | ||
| function generateRandomString(length: number = 32): string { |
Owner
There was a problem hiding this comment.
we can add this src/lib/server/tool.ts and export from there
|
Just thought I'd add some encouragement for this feature. Once it's in, we plan to properly trial kener. Good luck! |
Add OIDC as a login option with provider configuration, group-to-role mapping, and automatic user provisioning. Relates to rajnandan1#388
Plattenspatz
force-pushed
the
feature/oidc-support
branch
from
June 22, 2026 10:46
d5f32f0 to
0a9c881
Compare
📝 WalkthroughWalkthroughAdds full OpenID Connect (OIDC) SSO support: a DB migration and repository layer for ChangesOIDC Authentication Feature
Sequence Diagram(s)sequenceDiagram
actor User
participant SignInPage
participant LoginRoute as /account/oidc/login
participant OidcController
participant IdentityProvider
participant CallbackRoute as /account/oidc/callback
participant UsersRepository
User->>SignInPage: clicks "Sign in with {Provider}"
SignInPage->>LoginRoute: GET /account/oidc/login
LoginRoute->>OidcController: GetOidcSettings()
LoginRoute->>OidcController: BuildAuthorizationUrl(settings, callbackUrl)
OidcController->>IdentityProvider: discovery(issuer_url)
IdentityProvider-->>OidcController: provider metadata
OidcController-->>LoginRoute: authUrl, state, nonce, codeVerifier
LoginRoute-->>User: 302 redirect + sets oidc-state/nonce/code-verifier cookies
User->>IdentityProvider: authenticates
IdentityProvider->>CallbackRoute: GET /account/oidc/callback?code=...&state=...
CallbackRoute->>OidcController: HandleCallback(settings, callbackUrl, url, state, nonce, codeVerifier)
OidcController->>IdentityProvider: authorizationCodeGrant(code, PKCE)
IdentityProvider-->>OidcController: tokens + ID token claims
OidcController-->>CallbackRoute: sub, email, name, groups
CallbackRoute->>OidcController: FindOrCreateOidcUser(settings, oidcData)
OidcController->>UsersRepository: getUserByOidcSub(sub)
OidcController->>UsersRepository: getOidcRoleIdsForGroups(groups)
OidcController->>UsersRepository: insertUser or SyncOidcUserRoles
OidcController-->>CallbackRoute: UserRecordPublic
CallbackRoute->>OidcController: GenerateOidcSession(user)
OidcController-->>CallbackRoute: token + cookieConfig
CallbackRoute-->>User: 302 redirect to /manage/app/site-configurations + session cookie
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)src/routes/(docs)/docs/content/v4/oidc.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
| Filename | Overview |
|---|---|
| src/lib/server/db/repositories/users.ts | Adds getUserByOidcSub and OIDC group-role mapping CRUD. insertUser silently drops is_active/is_verified fields passed by the OIDC provisioning caller (P1). |
| src/lib/server/controllers/oidcController.ts | Core OIDC logic: discovery caching, PKCE auth-code flow, user provisioning, and role sync. Email-update unique-constraint race (pre-existing P1 thread) remains open; otherwise flow is sound. |
| src/routes/(account)/account/oidc/callback/+server.ts | OIDC callback handler: validates state/nonce/PKCE cookies, calls FindOrCreateOidcUser, sets session cookie. KENER_BASE_PATH mismatch flagged in previous thread; redirect/error handling looks correct. |
| src/routes/(account)/account/oidc/login/+server.ts | Initiates OIDC auth-code flow with PKCE; KENER_BASE_PATH mismatch flagged in previous thread. Cookie security attributes (httpOnly, sameSite=lax, 10-min maxAge) are appropriate. |
| src/routes/(manage)/manage/api/+server.ts | Adds OIDC group-role mapping CRUD and testOidcConnection actions. Missing ID validation on deleteOidcGroupRoleMapping flagged in previous thread; client_secret preservation logic in storeSiteData is correct. |
| migrations/20260610120000_add_oidc_support.ts | Adds auth_provider/oidc_sub columns and oidc_group_role_mappings table. Uses hasColumn/hasTable guards for idempotency; raw SQL ALTER TABLE concern flagged in previous thread. |
| src/routes/(manage)/manage/app/oidc/+page.svelte | Admin OIDC configuration UI. Correctly uses getOidcSettingsMasked and strips unchanged client_secret before saving. Group-role mapping CRUD with confirmation dialog is well-implemented. |
| src/routes/(account)/account/signin/+page.server.ts | Loads OIDC settings for the login page and blocks local login for OIDC users. Logic is clean; KENER_FORCE_LOCAL_LOGIN escape hatch is a good safeguard. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant U as User Browser
participant K as Kener (login route)
participant IdP as Identity Provider
participant CB as Kener (callback route)
participant DB as Database
U->>K: GET /account/oidc/login
K->>K: Generate state, nonce, PKCE verifier
K->>U: Set oidc-state/nonce/code-verifier cookies
K->>U: 302 → IdP authorization URL
U->>IdP: Authenticate
IdP->>U: "302 → /account/oidc/callback?code=…&state=…"
U->>CB: GET /account/oidc/callback
CB->>CB: Validate state cookie, nonce cookie
CB->>IdP: Exchange code for tokens (PKCE)
IdP->>CB: ID token + access token
CB->>CB: Extract sub, email, name, groups claims
CB->>DB: getUserByOidcSub(sub)
alt User exists
CB->>DB: SyncOidcUserRoles(userId, groups)
CB->>DB: updateUserProfile(userId, name, email)
else "New user (auto_create_users=true)"
CB->>DB: getOidcRoleIdsForGroups(groups)
CB->>DB: insertUser(email, name, roles, oidc_sub)
end
CB->>CB: GenerateOidcSession → JWT
CB->>U: Set session cookie
CB->>U: 302 → /manage/app/site-configurations
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant U as User Browser
participant K as Kener (login route)
participant IdP as Identity Provider
participant CB as Kener (callback route)
participant DB as Database
U->>K: GET /account/oidc/login
K->>K: Generate state, nonce, PKCE verifier
K->>U: Set oidc-state/nonce/code-verifier cookies
K->>U: 302 → IdP authorization URL
U->>IdP: Authenticate
IdP->>U: "302 → /account/oidc/callback?code=…&state=…"
U->>CB: GET /account/oidc/callback
CB->>CB: Validate state cookie, nonce cookie
CB->>IdP: Exchange code for tokens (PKCE)
IdP->>CB: ID token + access token
CB->>CB: Extract sub, email, name, groups claims
CB->>DB: getUserByOidcSub(sub)
alt User exists
CB->>DB: SyncOidcUserRoles(userId, groups)
CB->>DB: updateUserProfile(userId, name, email)
else "New user (auto_create_users=true)"
CB->>DB: getOidcRoleIdsForGroups(groups)
CB->>DB: insertUser(email, name, roles, oidc_sub)
end
CB->>CB: GenerateOidcSession → JWT
CB->>U: Set session cookie
CB->>U: 302 → /manage/app/site-configurations
Reviews (3): Last reviewed commit: "Address review feedback: security, cache..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@migrations/20260610120000_add_oidc_support.ts`:
- Around line 21-29: The migration is missing error handling for the raw SQL
statements on lines 26 and 28, and the down() function does not restore the NOT
NULL constraint on password_hash, leaving the database schema in an inconsistent
state after rollback. Wrap the raw SQL ALTER TABLE and MODIFY statements in a
try/catch block to handle errors properly, or refactor to use
knex.schema.alterTable() following the pattern demonstrated in lines 7-19.
Additionally, implement the down() function to explicitly restore the NOT NULL
constraint for both PostgreSQL (using ALTER COLUMN password_hash SET NOT NULL)
and MySQL/MySQL2 (using MODIFY password_hash VARCHAR(255) NOT NULL) to ensure
schema symmetry on rollback.
In `@src/lib/server/controllers/oidcController.ts`:
- Around line 24-35: In the GetOidcSettings function, remove the unnecessary
`typeof raw === "string"` type guard check on line 29 since GetSiteDataByKey
already returns parsed JSON objects when data_type is "object". Simply assign
raw directly to settings without the conditional JSON.parse, as raw will always
be an object and the string branch is dead code.
- Around line 141-149: Add a guard condition before calling client.fetchUserInfo
in the oidcController.ts file to check that tokens.access_token exists. If
access_token is missing or undefined, throw an error with a meaningful message
that explains the OIDC provider did not return an access_token (e.g., some
configurations only issue an id_token). Only proceed with the fetchUserInfo call
if the access_token is confirmed to be present, removing the non-null assertion
from the method call.
In `@src/lib/server/db/repositories/users.ts`:
- Around line 32-33: The getUsersByRoleId method is missing the auth_provider
and oidc_sub columns in its query selection. Update the column projection in the
getUsersByRoleId method to include both auth_provider and oidc_sub so that the
returned objects conform to the UserRecordPublic type contract, matching the
updated projections added in the nearby code.
In `@src/routes/`(account)/account/oidc/callback/+server.ts:
- Line 38: The callbackUrl constant definition is missing the KENER_BASE_PATH
prefix, which will cause incorrect callback URL generation when the application
is deployed with a non-root base path. Update the callbackUrl construction to
include the KENER_BASE_PATH prefix (following the same pattern used in the login
route) so that the full callback URL includes the base path when present,
ensuring proper token exchange during the OIDC callback flow.
In `@src/routes/`(account)/account/oidc/login/+server.ts:
- Line 11: The callbackUrl variable in both the OIDC login route handler and
OIDC callback route handler does not include the KENER_BASE_PATH environment
variable prefix, causing incorrect redirect URIs when the application is
deployed with a base path. In both locations where callbackUrl is constructed,
first extract the KENER_BASE_PATH environment variable (defaulting to an empty
string if not set), then include it in the URL construction by concatenating it
between url.origin and the route path. This ensures the OIDC provider redirects
to the correct endpoint regardless of whether a base path is configured.
In `@src/routes/`(account)/account/signin/+page.svelte:
- Around line 83-91: The Button component with href="/account/oidc/login" and
the forgot password link with href="/account/forgot" use hardcoded paths that
don't account for the KENER_BASE_PATH environment variable, causing 404 errors
when a base path is configured. Import the resolve function from "$app/paths" at
the top of the file, then apply it to both href attributes by wrapping the
hardcoded paths with the resolve function, following the clientResolver(resolve,
path) pattern used elsewhere in the codebase.
In `@src/routes/`(manage)/manage/app/oidc/+page.svelte:
- Around line 2-12: Update all the shadcn-svelte UI component import paths in
this file to follow project conventions. Remove the `/index.js` suffix from each
import statement for Button, Input, Label, Switch, Spinner, Badge, Separator,
Card, Table, Select, and AlertDialog, changing them from
`$lib/components/ui/{component-name}/index.js` to just
`$lib/components/ui/{component-name}`. This applies to all import statements
from line 2 through line 12.
- Around line 94-101: The apiCall function returns parsed JSON without checking
the HTTP response status code, allowing callers to continue with invalid data
even when the server returns an error. After the fetch call in the apiCall
function, check the response status (using response.ok or response.status) and
throw an error if the response indicates failure (non-2xx status). Only parse
and return the JSON response if the HTTP status indicates success.
In `@src/routes/`(manage)/manage/app/users/+page.svelte:
- Line 413: The new Auth column added at line 413 increases the table from 6 to
7 columns, but the loading and empty state rows still have colspan set to 6,
breaking table alignment. Find the loading state row and empty state row (likely
Table.Row components with colspan attributes) in the same file and update both
colspan values from 6 to 7 to match the new table column count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a7e74222-142e-4416-8058-ef39beb021c5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
migrations/20260610120000_add_oidc_support.tspackage.jsonsrc/lib/allPerms.tssrc/lib/server/controllers/controller.tssrc/lib/server/controllers/oidcController.tssrc/lib/server/controllers/siteDataKeys.tssrc/lib/server/db/dbimpl.tssrc/lib/server/db/repositories/users.tssrc/lib/server/db/repositories/users.ts.savesrc/lib/server/db/seedSiteData.tssrc/lib/server/types/db.tssrc/lib/types/site.tssrc/routes/(account)/account/oidc/callback/+server.tssrc/routes/(account)/account/oidc/login/+server.tssrc/routes/(account)/account/signin/+page.server.tssrc/routes/(account)/account/signin/+page.sveltesrc/routes/(manage)/+layout.sveltesrc/routes/(manage)/manage/api/+server.tssrc/routes/(manage)/manage/app/oidc/+page.sveltesrc/routes/(manage)/manage/app/users/+page.svelte
Comment on lines
+24
to
+35
| export async function GetOidcSettings(): Promise<OidcSettings | null> { | ||
| const raw = await GetSiteDataByKey("oidcSettings"); | ||
| if (!raw) return null; | ||
|
|
||
| try { | ||
| const settings: OidcSettings = typeof raw === "string" ? JSON.parse(raw) : raw; | ||
| if (!settings.enabled) return null; | ||
| return settings; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Potential double JSON.parse when data_type is "object".
GetSiteDataByKey already parses JSON when data_type == "object" (see siteDataController.ts:146-155). If oidcSettings is stored with data_type: "object", raw will already be an object, not a string. The current code handles this correctly with the typeof raw === "string" check on line 29, so no bug exists—but the string branch may be dead code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/server/controllers/oidcController.ts` around lines 24 - 35, In the
GetOidcSettings function, remove the unnecessary `typeof raw === "string"` type
guard check on line 29 since GetSiteDataByKey already returns parsed JSON
objects when data_type is "object". Simply assign raw directly to settings
without the conditional JSON.parse, as raw will always be an object and the
string branch is dead code.
Comment on lines
+2
to
+12
| import { Button } from "$lib/components/ui/button/index.js"; | ||
| import { Input } from "$lib/components/ui/input/index.js"; | ||
| import { Label } from "$lib/components/ui/label/index.js"; | ||
| import { Switch } from "$lib/components/ui/switch/index.js"; | ||
| import { Spinner } from "$lib/components/ui/spinner/index.js"; | ||
| import { Badge } from "$lib/components/ui/badge/index.js"; | ||
| import { Separator } from "$lib/components/ui/separator/index.js"; | ||
| import * as Card from "$lib/components/ui/card/index.js"; | ||
| import * as Table from "$lib/components/ui/table/index.js"; | ||
| import * as Select from "$lib/components/ui/select/index.js"; | ||
| import * as AlertDialog from "$lib/components/ui/alert-dialog/index.js"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use route-level shadcn import paths required by project conventions.
These imports should use $lib/components/ui/{component-name} instead of /index.js paths.
Example import-path adjustments
-import { Button } from "$lib/components/ui/button/index.js";
-import { Input } from "$lib/components/ui/input/index.js";
-import { Label } from "$lib/components/ui/label/index.js";
+import { Button } from "$lib/components/ui/button";
+import { Input } from "$lib/components/ui/input";
+import { Label } from "$lib/components/ui/label";As per coding guidelines, src/routes/**/*.svelte: Import shadcn-svelte UI components from "$lib/components/ui/{component-name}".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { Button } from "$lib/components/ui/button/index.js"; | |
| import { Input } from "$lib/components/ui/input/index.js"; | |
| import { Label } from "$lib/components/ui/label/index.js"; | |
| import { Switch } from "$lib/components/ui/switch/index.js"; | |
| import { Spinner } from "$lib/components/ui/spinner/index.js"; | |
| import { Badge } from "$lib/components/ui/badge/index.js"; | |
| import { Separator } from "$lib/components/ui/separator/index.js"; | |
| import * as Card from "$lib/components/ui/card/index.js"; | |
| import * as Table from "$lib/components/ui/table/index.js"; | |
| import * as Select from "$lib/components/ui/select/index.js"; | |
| import * as AlertDialog from "$lib/components/ui/alert-dialog/index.js"; | |
| import { Button } from "$lib/components/ui/button"; | |
| import { Input } from "$lib/components/ui/input"; | |
| import { Label } from "$lib/components/ui/label"; | |
| import { Switch } from "$lib/components/ui/switch"; | |
| import { Spinner } from "$lib/components/ui/spinner"; | |
| import { Badge } from "$lib/components/ui/badge"; | |
| import { Separator } from "$lib/components/ui/separator"; | |
| import * as Card from "$lib/components/ui/card"; | |
| import * as Table from "$lib/components/ui/table"; | |
| import * as Select from "$lib/components/ui/select"; | |
| import * as AlertDialog from "$lib/components/ui/alert-dialog"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(manage)/manage/app/oidc/+page.svelte around lines 2 - 12, Update
all the shadcn-svelte UI component import paths in this file to follow project
conventions. Remove the `/index.js` suffix from each import statement for
Button, Input, Label, Switch, Spinner, Badge, Separator, Card, Table, Select,
and AlertDialog, changing them from
`$lib/components/ui/{component-name}/index.js` to just
`$lib/components/ui/{component-name}`. This applies to all import statements
from line 2 through line 12.
Source: Coding guidelines
Author
|
@rajnandan1
Just running final tests before pushing. Update coming soon. |
Author
Done. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(account)/account/signin/+page.svelte (1)
162-169:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winForgot password link needs base path resolution.
The
href="/account/forgot"is hardcoded and will break whenKENER_BASE_PATHis configured. Applyresolve()consistently as done for the OIDC login link.Suggested fix
<Button variant="link" size="sm" class="text-muted-foreground absolute top-0 right-0 h-auto p-0 text-xs" - href="/account/forgot" + href={resolve("/account/forgot")} > Forgot? </Button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/`(account)/account/signin/+page.svelte around lines 162 - 169, The href attribute in the Button component that displays "Forgot?" text is hardcoded to "/account/forgot" and does not account for the KENER_BASE_PATH configuration. Apply the resolve() function to the href value in this Button component, consistent with how it is applied to the OIDC login link elsewhere in the page, to ensure the path is correctly resolved when a base path is configured.
♻️ Duplicate comments (1)
src/lib/server/controllers/oidcController.ts (1)
137-141: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRemove redundant non-null assertion.
The
!ontokens.access_token!at line 140 is unnecessary since the guard on lines 137-139 ensuresaccess_tokenexists.Suggested fix
if (!tokens.access_token) { throw new Error("No email in ID token and no access_token available for userinfo lookup"); } - const userinfo = await client.fetchUserInfo(config, tokens.access_token!, sub); + const userinfo = await client.fetchUserInfo(config, tokens.access_token, sub);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/server/controllers/oidcController.ts` around lines 137 - 141, In the oidcController's token handling block, the guard condition at lines 137-139 already validates that tokens.access_token exists by throwing an error if it is falsy. Therefore, the non-null assertion operator (!) on tokens.access_token in the subsequent client.fetchUserInfo call is redundant. Remove the exclamation mark from tokens.access_token! in the client.fetchUserInfo invocation to clean up the code, as the guard ensures the value is defined and TypeScript will recognize this.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/server/tool.ts`:
- Around line 538-541: The GenerateRandomHexString function generates excessive
random bytes and discards half of them through slicing. Instead of generating
length random bytes, calculate the minimum bytes needed to produce length hex
characters by using Math.ceil(length / 2) as the argument to crypto.randomBytes.
This ensures only the necessary entropy is generated while still producing the
correct number of hex characters after conversion and slicing.
In `@src/routes/`(account)/account/signin/+page.server.ts:
- Around line 11-13: The file has duplicate imports from $lib/global-constants
on lines 11 and 13, importing the module as both `constants` and `GC`. Remove
the duplicate import statement on line 11 that imports as `constants`, keeping
only the import on line 13 that uses the `GC` alias per coding guidelines. Then
locate and update any usage of the `constants` alias throughout the file
(including line 57) to use `GC` instead.
In `@src/routes/`(manage)/manage/app/oidc/+page.svelte:
- Around line 37-42: The RoleRecord interface defined locally in this component
duplicates the type definition that already exists in $lib/server/types/db.js.
Remove the local RoleRecord interface definition and instead import it from the
shared database types file using an import statement at the top of the file.
This ensures a single source of truth and prevents type drift between the
component and the shared types.
---
Outside diff comments:
In `@src/routes/`(account)/account/signin/+page.svelte:
- Around line 162-169: The href attribute in the Button component that displays
"Forgot?" text is hardcoded to "/account/forgot" and does not account for the
KENER_BASE_PATH configuration. Apply the resolve() function to the href value in
this Button component, consistent with how it is applied to the OIDC login link
elsewhere in the page, to ensure the path is correctly resolved when a base path
is configured.
---
Duplicate comments:
In `@src/lib/server/controllers/oidcController.ts`:
- Around line 137-141: In the oidcController's token handling block, the guard
condition at lines 137-139 already validates that tokens.access_token exists by
throwing an error if it is falsy. Therefore, the non-null assertion operator (!)
on tokens.access_token in the subsequent client.fetchUserInfo call is redundant.
Remove the exclamation mark from tokens.access_token! in the
client.fetchUserInfo invocation to clean up the code, as the guard ensures the
value is defined and TypeScript will recognize this.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ae5f68ee-d9ef-423f-bc17-fab2c0cfdeb5
📒 Files selected for processing (18)
.gitignoremigrations/20260610120000_add_oidc_support.tssrc/lib/allPerms.tssrc/lib/global-constants.tssrc/lib/server/controllers/oidcController.tssrc/lib/server/db/dbimpl.tssrc/lib/server/db/repositories/users.tssrc/lib/server/db/seedSiteData.tssrc/lib/server/tool.tssrc/routes/(account)/account/oidc/callback/+server.tssrc/routes/(account)/account/oidc/login/+server.tssrc/routes/(account)/account/signin/+page.server.tssrc/routes/(account)/account/signin/+page.sveltesrc/routes/(docs)/docs.jsonsrc/routes/(docs)/docs/content/v4/oidc.mdsrc/routes/(manage)/manage/api/+server.tssrc/routes/(manage)/manage/app/oidc/+page.sveltesrc/routes/(manage)/manage/app/users/+page.svelte
Comment on lines
+37
to
+42
| interface RoleRecord { | ||
| id: string; | ||
| role_name: string; | ||
| readonly: number; | ||
| status: string; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Import RoleRecord from DB types instead of redefining it.
The RoleRecord interface duplicates the type definition from $lib/server/types/db.js. Import it instead to prevent drift.
♻️ Refactor to use imported type
import clientResolver from "$lib/client/resolver.js";
import type { OidcSettings } from "$lib/types/site.js";
+ import type { RoleRecord } from "$lib/server/types/db.js";
// ============ Types ============
interface GroupRoleMapping {
id: number;
oidc_group: string;
role_id: string;
created_at: string;
updated_at: string;
}
- interface RoleRecord {
- id: string;
- role_name: string;
- readonly: number;
- status: string;
- }
-
// ============ State ============🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(manage)/manage/app/oidc/+page.svelte around lines 37 - 42, The
RoleRecord interface defined locally in this component duplicates the type
definition that already exists in $lib/server/types/db.js. Remove the local
RoleRecord interface definition and instead import it from the shared database
types file using an import statement at the top of the file. This ensures a
single source of truth and prevents type drift between the component and the
shared types.
- Fix test connection cache poisoning (local config for test) - Cache key includes client_id/secret, server-side invalidation - Mask client_secret on read, preserve on unchanged save - Add KENER_BASE_PATH to all redirect URIs - Add KENER_FORCE_LOCAL_LOGIN env var for lockout recovery - Remove dead code (updateUserOidcSub, users.ts.save) - Sync email/name from IdP on subsequent logins - Server-side validation for group-role mappings - Move generateRandomString to tool.ts, oidc to global-constants - Fix colspan, strict equality, RoleRecord import, callback order - Guard against missing access_token - Fix getUsersByRoleId missing auth_provider/oidc_sub columns - Default auto_create_users to false - Add OIDC documentation page - Add *.save to .gitignore
Plattenspatz
force-pushed
the
feature/oidc-support
branch
from
June 23, 2026 10:49
47fbbe7 to
c96fb89
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/server/controllers/oidcController.ts`:
- Around line 10-15: The imports in oidcController.ts do not follow the
project's established conventions for server-side code. Replace the relative
path import of db from "../db/db.js" with the standardized alias path import db
from "$lib/server/db/db" to maintain consistency across server operations.
Additionally, replace the $lib alias import for global constants (GC from
"$lib/global-constants.js") with a relative path import
(../../global-constants.js) as per the server-layer guidelines. These changes
align the file with the documented import conventions for src/lib/server/**/*.ts
files.
- Around line 230-235: Before calling db.updateUserProfile with the
oidcData.email, add a guard to check if that email is already owned by another
user account, similar to the validation done in the provisioning path. If
oidcData.email is owned by a different user (compare against user.id), either
skip updating the email field or handle the conflict appropriately to prevent
duplicate account identifiers and maintain the "no account merging" contract.
This check should happen before the updateUserProfile call and should protect
against the case where the IdP's email for the user changes to point to another
account's email.
In `@src/routes/`(account)/account/signin/+page.server.ts:
- Around line 59-71: The current code checks if a user exists in the database
before validating if local login is globally disabled, which enables account
enumeration attacks since different error messages are returned for non-existent
vs existing users. Move the OIDC local-login validation check (the condition
checking oidcSettings.allow_local_login and forceLocalLogin) to execute before
the GetUserByEmail call, so that the disabled local login error is returned
consistently regardless of whether the email exists in the database.
In `@src/routes/`(manage)/manage/api/+server.ts:
- Around line 692-699: The code in the getOidcSettingsMasked action is mutating
the settings object returned by GetSiteDataByKey and the masking logic likely
treats any falsy client_secret as missing (meaning it preserves the old secret),
when instead it should only preserve the secret if the field is completely
omitted. Create a clone of the settings object before applying the MaskString
operation on the client_secret field, and modify the logic that handles
client_secret updates (around line 723) to check for undefined/omitted fields
rather than falsy values, so that an empty string is treated as an intentional
clear of the secret rather than a request to keep the existing one.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0dba185a-576a-4aba-8cfa-173f03fda8ac
📒 Files selected for processing (18)
.gitignoremigrations/20260610120000_add_oidc_support.tssrc/lib/allPerms.tssrc/lib/global-constants.tssrc/lib/server/controllers/oidcController.tssrc/lib/server/db/dbimpl.tssrc/lib/server/db/repositories/users.tssrc/lib/server/db/seedSiteData.tssrc/lib/server/tool.tssrc/routes/(account)/account/oidc/callback/+server.tssrc/routes/(account)/account/oidc/login/+server.tssrc/routes/(account)/account/signin/+page.server.tssrc/routes/(account)/account/signin/+page.sveltesrc/routes/(docs)/docs.jsonsrc/routes/(docs)/docs/content/v4/oidc.mdsrc/routes/(manage)/manage/api/+server.tssrc/routes/(manage)/manage/app/oidc/+page.sveltesrc/routes/(manage)/manage/app/users/+page.svelte
Comment on lines
+10
to
+15
| import db from "../db/db.js"; | ||
| import { GenerateToken, CookieConfig } from "./commonController.js"; | ||
| import type { OidcSettings } from "$lib/types/site.js"; | ||
| import type { UserRecordPublic } from "../types/db.js"; | ||
| import { GetSiteDataByKey } from "./siteDataController.js"; | ||
| import GC from "$lib/global-constants.js"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Align server imports with the project import conventions.
oidcController.ts is under src/lib/server/**/*.ts, but Line 10 imports the db singleton by relative path and Line 15 imports global constants through $lib. Use the documented paths for this layer.
Proposed import adjustment
-import db from "../db/db.js";
+import db from "$lib/server/db/db";
import { GenerateToken, CookieConfig } from "./commonController.js";
import type { OidcSettings } from "$lib/types/site.js";
import type { UserRecordPublic } from "../types/db.js";
import { GetSiteDataByKey } from "./siteDataController.js";
-import GC from "$lib/global-constants.js";
+import GC from "../../global-constants.js";As per coding guidelines, "src/lib/server/**/*.ts: Use the db singleton instance via import db from "$lib/server/db/db" for all database operations" and "src/lib/server/**/*.ts: Import status constants using relative path from src/lib/global-constants.js in server code."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import db from "../db/db.js"; | |
| import { GenerateToken, CookieConfig } from "./commonController.js"; | |
| import type { OidcSettings } from "$lib/types/site.js"; | |
| import type { UserRecordPublic } from "../types/db.js"; | |
| import { GetSiteDataByKey } from "./siteDataController.js"; | |
| import GC from "$lib/global-constants.js"; | |
| import db from "$lib/server/db/db"; | |
| import { GenerateToken, CookieConfig } from "./commonController.js"; | |
| import type { OidcSettings } from "$lib/types/site.js"; | |
| import type { UserRecordPublic } from "../types/db.js"; | |
| import { GetSiteDataByKey } from "./siteDataController.js"; | |
| import GC from "../../global-constants.js"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/server/controllers/oidcController.ts` around lines 10 - 15, The
imports in oidcController.ts do not follow the project's established conventions
for server-side code. Replace the relative path import of db from "../db/db.js"
with the standardized alias path import db from "$lib/server/db/db" to maintain
consistency across server operations. Additionally, replace the $lib alias
import for global constants (GC from "$lib/global-constants.js") with a relative
path import (../../global-constants.js) as per the server-layer guidelines.
These changes align the file with the documented import conventions for
src/lib/server/**/*.ts files.
Source: Coding guidelines
Comment on lines
+230
to
+235
| // Sync roles and update profile data from the IdP | ||
| await SyncOidcUserRoles(user.id, oidcData.groups, settings); | ||
| await db.updateUserProfile(user.id, { | ||
| name: oidcData.name, | ||
| email: oidcData.email, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Check email ownership before syncing an existing OIDC profile.
The provisioning path rejects oidcData.email when another account already owns it, but the existing-user sync path updates the email without the same guard. If the IdP email changes to another user's email, this can create duplicate account identifiers or fail on a DB constraint instead of preserving the “no account merging” contract.
Proposed guard before `updateUserProfile`
// Sync roles and update profile data from the IdP
await SyncOidcUserRoles(user.id, oidcData.groups, settings);
+ const existingByEmail = await db.getUserByEmail(oidcData.email);
+ if (existingByEmail && existingByEmail.id !== user.id) {
+ throw new Error(
+ `Another account with the email "${oidcData.email}" already exists. ` +
+ "OIDC and local accounts are kept separate. " +
+ "Please contact an administrator.",
+ );
+ }
await db.updateUserProfile(user.id, {
name: oidcData.name,
email: oidcData.email,
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/server/controllers/oidcController.ts` around lines 230 - 235, Before
calling db.updateUserProfile with the oidcData.email, add a guard to check if
that email is already owned by another user account, similar to the validation
done in the provisioning path. If oidcData.email is owned by a different user
(compare against user.id), either skip updating the email field or handle the
conflict appropriately to prevent duplicate account identifiers and maintain the
"no account merging" contract. This check should happen before the
updateUserProfile call and should protect against the case where the IdP's email
for the user changes to point to another account's email.
Comment on lines
59
to
+71
| const userDB = await GetUserByEmail(email); | ||
| if (!userDB) { | ||
| return fail(401, { error: "User does not exist", values: { email } }); | ||
| } | ||
| // Local login can be enabled by setting Env-Variable "KENER_FORCE_LOCAL_LOGIN" == "true". | ||
| // This prevents lockout when the IdP is misconfigured or unreachable. | ||
| const forceLocalLogin = process.env.KENER_FORCE_LOCAL_LOGIN === "true"; | ||
| if (oidcSettings && !oidcSettings.allow_local_login && !forceLocalLogin) { | ||
| return fail(403, { | ||
| error: "Local login is disabled. Please use SSO.", | ||
| values: { email }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce disabled local login before looking up the user.
When local login is disabled, this still returns different responses for unknown vs existing emails, leaving the password endpoint usable for account enumeration. Move the global OIDC local-login gate before GetUserByEmail().
🔒 Proposed fix
- const userDB = await GetUserByEmail(email);
- if (!userDB) {
- return fail(401, { error: "User does not exist", values: { email } });
- }
// Local login can be enabled by setting Env-Variable "KENER_FORCE_LOCAL_LOGIN" == "true".
// This prevents lockout when the IdP is misconfigured or unreachable.
const forceLocalLogin = process.env.KENER_FORCE_LOCAL_LOGIN === "true";
if (oidcSettings && !oidcSettings.allow_local_login && !forceLocalLogin) {
return fail(403, {
error: "Local login is disabled. Please use SSO.",
values: { email },
});
}
+
+ const userDB = await GetUserByEmail(email);
+ if (!userDB) {
+ return fail(401, { error: "User does not exist", values: { email } });
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(account)/account/signin/+page.server.ts around lines 59 - 71,
The current code checks if a user exists in the database before validating if
local login is globally disabled, which enables account enumeration attacks
since different error messages are returned for non-existent vs existing users.
Move the OIDC local-login validation check (the condition checking
oidcSettings.allow_local_login and forceLocalLogin) to execute before the
GetUserByEmail call, so that the disabled local login error is returned
consistently regardless of whether the email exists in the database.
Comment on lines
+692
to
+699
| } else if (action == "getOidcSettingsMasked") { | ||
| const raw = await GetSiteDataByKey("oidcSettings"); | ||
| if (raw && typeof raw === "object") { | ||
| const settings = raw as Record<string, unknown>; | ||
| if (settings.client_secret && typeof settings.client_secret === "string") { | ||
| settings.client_secret = MaskString(settings.client_secret); | ||
| } | ||
| resp = settings; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the secret only when the field is omitted.
Line 723 treats any falsy client_secret as missing, so saving client_secret: "" silently keeps the old secret. The UI only deletes the field when unchanged, so an empty string should be respected as an intentional clear. Also mask a cloned settings object to avoid mutating the value returned by GetSiteDataByKey.
Proposed fix
} else if (action == "getOidcSettingsMasked") {
const raw = await GetSiteDataByKey("oidcSettings");
if (raw && typeof raw === "object") {
- const settings = raw as Record<string, unknown>;
+ const settings = { ...(raw as Record<string, unknown>) };
if (settings.client_secret && typeof settings.client_secret === "string") {
settings.client_secret = MaskString(settings.client_secret);
}
resp = settings;
@@
if (key === "oidcSettings" && typeof element === "string") {
try {
const newSettings = JSON.parse(element);
- if (!newSettings.client_secret) {
+ if (!Object.prototype.hasOwnProperty.call(newSettings, "client_secret")) {
const existing = await GetSiteDataByKey("oidcSettings");
if (existing && typeof existing === "object") {
newSettings.client_secret = (existing as Record<string, unknown>).client_secret;
element = JSON.stringify(newSettings);
}Also applies to: 719-728
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(manage)/manage/api/+server.ts around lines 692 - 699, The code
in the getOidcSettingsMasked action is mutating the settings object returned by
GetSiteDataByKey and the masking logic likely treats any falsy client_secret as
missing (meaning it preserves the old secret), when instead it should only
preserve the secret if the field is completely omitted. Create a clone of the
settings object before applying the MaskString operation on the client_secret
field, and modify the logic that handles client_secret updates (around line 723)
to check for undefined/omitted fields rather than falsy values, so that an empty
string is treated as an intentional clear of the secret rather than a request to
keep the existing one.
Author
|
Thanks for the detailed review! I've addressed all feedback in the second commit:
Security:
Minor:
Added:
Ready for re-review when you have time. |
|
Commenting here to just say that this is a crucial (and blocking) feature for us. We want to test this out once OIDC support is available. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #388
This adds OpenID Connect as a login option for Kener. Users can sign in via an external identity provider (Keycloak, Authentik, Azure AD, Okta, etc.) instead of — or in addition to — local username/password.
What it does
What it doesn't do (yet)
Screenshots
Changes overview
New files:
migrations/20260610120000_add_oidc_support.ts— addsauth_providerandoidc_subcolumns tousers, createsoidc_group_role_mappingstablesrc/lib/server/controllers/oidcController.ts— OIDC discovery, token exchange, user provisioning, role syncsrc/routes/(account)/account/oidc/login/+server.ts— redirects to the OIDC providersrc/routes/(account)/account/oidc/callback/+server.ts— handles the callback, creates/updates the user, sets the session cookiesrc/routes/(manage)/manage/app/oidc/+page.svelte— admin settings page with provider config and group-role mappingModified files:
package.json— addedopenid-clientdependencysrc/lib/types/site.ts—OidcSettingsinterfacesrc/lib/server/types/db.ts— extended user types withauth_providerandoidc_sub, added mapping typessrc/lib/server/db/repositories/users.ts— OIDC user lookup and group-role mapping CRUDsrc/lib/server/db/dbimpl.ts— exposed new repository methodssrc/lib/server/controllers/siteDataKeys.ts— registeredoidcSettingssrc/lib/server/db/seedSiteData.ts— default OIDC settingssrc/lib/server/controllers/controller.ts— barrel exportsrc/lib/allPerms.ts— registered OIDC actions in permission maps and route mapsrc/routes/(account)/account/signin/+page.server.ts— loads OIDC settings, passes them to the page, blocks local login for OIDC userssrc/routes/(account)/account/signin/+page.svelte— OIDC login button, separator, error displaysrc/routes/(manage)/manage/api/+server.ts— OIDC management API actionssrc/routes/(manage)/+layout.svelte— sidebar navigation linksrc/routes/(manage)/manage/app/users/+page.svelte— auth provider badge in users tableHow to test
https://your-kener/account/oidc/callbackSummary by CodeRabbit
Release Notes
New Features
Documentation