feat: implement OIDC authentication#237
Conversation
- Added login and callback routes - Added automatic user account creation - Added role and group syncing - Fixed issuer URL and SSL certificate issues
| import { randomUUID } from 'crypto'; | ||
|
|
||
| // Allow self-signed certs (Delete in production if using real SSL) | ||
| process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| currentUrl, | ||
| { | ||
| idTokenExpected: true, | ||
| expectedState: client.skipStateCheck |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const clientUrl = req.headers.referer || req.headers.origin || `http://${req.headers.host}`; | ||
| const redirectUrl = new URL(clientUrl); | ||
| redirectUrl.searchParams.set('token', token); | ||
|
|
||
| res.writeHead(302, { Location: redirectUrl.toString() }); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| redirect_uri: config.oidc.redirectUrl | ||
| }, | ||
| { | ||
| [client.allowInsecureRequests as unknown as string]: true |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| if (!hasRequiredGroup) { | ||
| res.writeHead(403); | ||
| res.end(`Access Denied: Missing required group '${config.oidc.requiredGroup}'`); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Wait ... no your completely right ... I committed the wrong version ... This was my old oidc.ts where I was trying to diagnose some IdP configuration issues. I'll send the correctly implemented (and cleaner one) here soon. |
|
@Giggitybyte just pushed the actual commit. |
| if (targetRoleIds.length > 0) { | ||
| await db.delete(userRoles).where(eq(userRoles.userId, userId)); | ||
| await db.insert(userRoles).values( |
There was a problem hiding this comment.
It seems roles are only added if at least one OIDC group matches a mapped role. If zero groups match, the function does nothing and the user keeps whatever roles they already have.
Consider this scenario:
- A user logs in via OIDC with group
Administrator, which maps to the localAdminrole - They get the
Adminrole in the Sharkord database - Later, the OIDC admin removes the user from the
Administratorgroup - The user logs in again; their groups is now empty or doesn't include
Administrator targetRoleIdsends up empty and this block is skipped entirely- The user keeps their
Adminrole in Sharkord
This means role revocation at the OIDC provider is never actually propagated to Sharkord. The fix is straightforward; always sync roles when a mapping is configured, even if the result is zero roles.
However, there is another problem with this: OIDC login wipes locally assigned roles.
Every role the user has is deleted, then only the OIDC mapped roles are inserted. There's no distinction between "this role was assigned by OIDC" and "this role was assigned by a Sharkord admin manually."
The root issue is that the system doesn't track where a role assignment came from. It'd be a good idea to add a source column to userRoles (e.g. 'oidc' vs 'local') and only delete/replace source = 'oidc' roles during sync, or maintain a separate oidcUserRoles table that gets synced, while leaving the main userRoles table alone for manual assignments.
There was a problem hiding this comment.
It seems roles are only added if at least one OIDC group matches a mapped role. If zero groups match, the function does nothing and the user keeps whatever roles they already have.
Consider this scenario:
- A user logs in via OIDC with group
Administrator, which maps to the localAdminrole- They get the
Adminrole in the Sharkord database- Later, the OIDC admin removes the user from the
Administratorgroup- The user logs in again; their groups is now empty or doesn't include
AdministratortargetRoleIdsends up empty and this block is skipped entirely- The user keeps their
Adminrole in SharkordThis means role revocation at the OIDC provider is never actually propagated to Sharkord. The fix is straightforward; always sync roles when a mapping is configured, even if the result is zero roles.
That is something that I had been thinking about right before I committed it, but my thought was that they wouldn't loose the role until the next login flow ... So I was in-between revoking at login or leaving it for the user to remove...
I ended up deciding to leave it as is until I could figure out a better method of (hopefully live) updating the roles.
However, there is another problem with this: OIDC login wipes locally assigned roles.
Every role the user has is deleted, then only the OIDC mapped roles are inserted. There's no distinction between "this role was assigned by OIDC" and "this role was assigned by a Sharkord admin manually."
The root issue is that the system doesn't track where a role assignment came from. It'd be a good idea to add a
sourcecolumn touserRoles(e.g. 'oidc' vs 'local') and only delete/replacesource = 'oidc'roles during sync, or maintain a separateoidcUserRolestable that gets synced, while leaving the mainuserRolestable alone for manual assignments.
I really do like this more detailed idea. I will start working on this flow later when I get the time.
At the moment for live role updates the only feasible idea that comes to mind is to write a bot for specifically authentik to live update the roles via using a service account but again that would only be for authentik and not for any other OIDC IdP.
There was a problem hiding this comment.
The root issue is that the system doesn't track where a role assignment came from. It'd be a good idea to add a source column to userRoles (e.g. 'oidc' vs 'local') and only delete/replace source = 'oidc' roles during sync, or maintain a separate oidcUserRoles table that gets synced, while leaving the main userRoles table alone for manual assignments.
This seems to add unnecessary complexity to me. I'm trying to think of a scenario where this behavior would be desired, but I can't think of one. At best it is a solution in search of a problem, at worse it's a security flaw.
If you have enabled an Identity Provider then the assumption is you will be managing claims fully through that integration. I would not want to have a different set of permissions applied outside of what has been configured at the IDP. If they truly are a local user, then of course that makes sense, but I wouldn't appreciate having to worry about whether or not a new or existing user has roles defined outside of my identity platform.
I will say, in my experience, this is the pattern I've seen enterprise platforms use because no one wants to go digging through every app (or writing automations to do so), exploring whether or not a user has the correct permissions applied.
There was a problem hiding this comment.
I will say, in my experience, this is the pattern I've seen enterprise platforms use because no one wants to go digging through every app (or writing automations to do so), exploring whether or not a user has the correct permissions applied.
Was thinking about this during the implementation ... I do agree. I will add a server config like ENFORCE_OIDC_ROLES that will allow for oidc to remove roles that are in the oidc role map json that are not given by oidc.
| logo: settings.logo, | ||
| allowNewUsers: settings.allowNewUsers | ||
| allowNewUsers: settings.allowNewUsers, | ||
| oidcEnabled: !!config.oidc.issuer && !!config.oidc.clientId |
There was a problem hiding this comment.
This should also take into account the value of config.oidc.oidcEnabled. The OIDC login button could appear on the frontend even when the admin has explicitly set oidcEnabled: false.
|
There's one more thing I'd personally want if you're willing to implement it here: a mechanism to have clients automatically redirected to the OIDC flow for authorization instead of requiring users to click "Login with OIDC". This would allow for a seamless single sign on experience. Perhaps a simple configuration value that does this, like |
|
I'll need to think about if this makes sense to add at this stage. |
|
Given the scope and target audience of Sharkord (friend groups and small communities that selfhost their own infrastructure) I believe OIDC authentication is particularly valuable. In the selfhosting community, it's quite common for people who run services for friends and family to also maintain a central SSO/OIDC provider (like Authentik, Authelia, or Keycloak). This allows them to manage access across all their services in one place rather than juggling separate credentials for each app. More importantly, OIDC would help solve a practical onboarding problem for small, related communities. Consider a scenario where several friend groups each run their own Sharkord instance but occasionally want to connect across instances. With OIDC, they could agree on a common identity provider, allowing users to seamlessly join any of those instances with their existing credentials; no password resets, no remembering different logins. This would significantly lower the friction for users moving between related communities. |
|
I see, it's probably a nice to have. Not gonna merge right now because I need to study the implementation and the spec, know nothing about it. |
|
This is single-handedly the one feature I am waiting for. It will make the friction of migrating individuals off of public services over to an instance way easier. |
|
Have most of the auth flow fixed. Just need to find some time either tomorrow or the day after to finish the rest. |
|
@zigzatuzoo so this is not fully implemented yet? In that case please change this to a draft PR so it's easier for me to keep track of things. |
- Add 'role added by method' metadata - Change allowedOrigins to csv instead of broken ini List - OIDC role mapping does not overwrite manually given roles - redirectUrl now using clients host but only if withing allowed origins - Fix loadDb to include the schema import, typing the BunSQLiteDatabase instance for full relational API support. - If OIDC is the only allowed login method then it automatically redirects to the oidc provider instead of waiting for user input
|
@Giggitybyte how does it look? |
|
@zigzatuzoo I'm not noticing any glaring issues in the code. Would you mind resolving the merge conflicts so I can pull and build this with the latest version of Sharkord to test it out? |
|
Okay this should be ready to merge. |
|
I haven't had the time to look at this yet. In the meantime I'm trying code rabbit help me review PRs so I'm gonna give it a try here. @coderabbitai review this PR. Make sure you also always check if the committed code follows the same code design as the rest of the code base to make things consistent. Save this command and use it all the time you review PRs in this repo. |
|
✏️ Learnings added
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis pull request introduces OpenID Connect (OIDC) authentication support to the application. Changes include server-side OIDC login and callback handlers, configuration schema extensions, database schema updates for role provenance tracking, client-side OIDC flow integration, and shared type updates to expose OIDC enablement status. Changes
Sequence DiagramssequenceDiagram
participant User
participant Client
participant Server
participant OIDCProvider as OIDC Provider
participant Database
User->>Client: Visit login page / trigger OIDC
Client->>Server: GET /auth/login (validate origin, generate PKCE)
Server->>Server: Validate OIDC enabled & referer
Server->>Server: Generate state + code_challenge
Server->>Client: Set temporary session cookie, redirect
Client->>OIDCProvider: Redirect to authorization endpoint
OIDCProvider->>User: Present login/consent form
User->>OIDCProvider: Authenticate & consent
OIDCProvider->>Client: Redirect with authorization code
Client->>Server: GET /auth/callback?code=...&state=...
Server->>Server: Validate session cookie & state
Server->>OIDCProvider: Exchange code for tokens
OIDCProvider->>Server: Return ID token + access token
Server->>Server: Validate ID token claims
Server->>Server: Extract user info & groups
Server->>Database: Check if user exists
alt User not found
Server->>Database: Create new user
Server->>Database: Assign default role (addedBy: 'oidc')
Server->>Database: Publish user
else User exists
Server->>Database: Sync roles from OIDC groups
end
Server->>Database: Apply role mappings
Server->>Server: Issue app token
Server->>Client: Clear OIDC session, set app token cookie
Client->>Client: Store token in session storage
Client->>User: Redirect with oidc_status=success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
apps/server/src/http/oidc.ts (2)
15-18:⚠️ Potential issue | 🟠 MajorAvoid defaulting OIDC callback URL scheme to
http.Line 16 falls back to
http, so missing/incorrect proxy headers can produce an insecureredirect_uriand leak auth codes over plaintext.🔒 Safer base URL derivation
const getBaseUrl = (req: http.IncomingMessage) => { - const protocol = (req.headers['x-forwarded-proto'] as string) || 'http'; + const forwardedProto = req.headers['x-forwarded-proto'] as string | undefined; + const protocol = forwardedProto || (req.socket.encrypted ? 'https' : 'http'); const host = req.headers.host; return `${protocol}://${host}`; };Also applies to: 89-90, 135-137
174-177:⚠️ Potential issue | 🔴 CriticalAuth token cookie must be
HttpOnly.Line 174 sets the session token cookie without
HttpOnly, which allows JavaScript/XSS to read and exfiltrate it.🍪 Harden cookie flags
- const authCookie = `sharkord_token=${appToken}; Path=/; SameSite=Lax; Secure; Max-Age=86400`; + const authCookie = `__Host-sharkord_token=${appToken}; Path=/; HttpOnly; SameSite=Lax; Secure; Max-Age=86400`;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d4a2e6d-a32d-46e4-a964-0fb48ba4e979
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
apps/client/src/screens/connect/index.tsxapps/server/package.jsonapps/server/src/config.tsapps/server/src/db/index.tsapps/server/src/db/migrations/0007_roles_added_by.sqlapps/server/src/db/migrations/meta/_journal.jsonapps/server/src/db/schema.tsapps/server/src/http/index.tsapps/server/src/http/info.tsapps/server/src/http/login.tsapps/server/src/http/oidc.tspackage.jsonpackages/shared/src/types.ts
| useStrictEffect(() => { | ||
| const token = getSessionStorageItem(SessionStorageKey.TOKEN); | ||
| if (info && info.oidcEnabled && !info.allowNewUsers && !token) { | ||
| onOidcLoginClick(); | ||
| } |
There was a problem hiding this comment.
Don’t couple OIDC-only login behavior to allowNewUsers.
Line 122 uses signup policy (allowNewUsers) to force OIDC redirect. This also suppresses local credential login in the UI (Lines 209 and 268), which can lock out existing local accounts.
| config = applyEnvOverrides(config, { | ||
| 'server.port': 'SHARKORD_PORT', | ||
| 'server.debug': 'SHARKORD_DEBUG', | ||
| 'server.autoupdate': 'SHARKORD_AUTOUPDATE', | ||
| 'server.disableLocalSignup': 'SHARKORD_DISABLE_LOCAL_SIGNUP', | ||
|
|
||
| 'oidc.oidcEnabled': 'OIDC_ENABLED', | ||
| 'oidc.enforceOidcRoles': 'OIDC_ENFORCE_ROLES', | ||
| 'oidc.issuer': 'OIDC_ISSUER', | ||
| 'oidc.clientId': 'OIDC_CLIENT_ID', | ||
| 'oidc.clientSecret': 'OIDC_CLIENT_SECRET', | ||
| 'oidc.rolesMapping': 'OIDC_ROLES_MAPPING', | ||
| 'oidc.requiredGroup': 'OIDC_REQUIRED_GROUP', | ||
| 'oidc.allowedOrigins': 'OIDC_ALLOWED_ORIGINS', | ||
| 'oidc.caCertPath': 'OIDC_CA_CERT_PATH', | ||
|
|
||
| 'webRtc.port': 'SHARKORD_WEBRTC_PORT', | ||
| 'webRtc.announcedAddress': 'SHARKORD_WEBRTC_ANNOUNCED_ADDRESS', | ||
| 'webRtc.maxBitrate': 'SHARKORD_WEBRTC_MAX_BITRATE' | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect override coercion logic and current parse order
fd -a "apply-env-overrides.ts"
sed -n '1,240p' "$(fd -a "apply-env-overrides.ts" | head -n1)"
rg -n "zConfig\\.parse|applyEnvOverrides\\(" apps/server/src/config.tsRepository: Sharkord/sharkord
Length of output: 1203
🏁 Script executed:
# Find and inspect the zConfig schema definition
fd -a "config\\.ts|schema" apps/server/src/ | head -20
rg -n "zConfig|RolesMapping|AllowedOrigins" apps/server/src/config.ts | head -30Repository: Sharkord/sharkord
Length of output: 329
🏁 Script executed:
# Read the schema definition from config.ts
sed -n '33,150p' apps/server/src/config.tsRepository: Sharkord/sharkord
Length of output: 3286
🏁 Script executed:
# Find jsonTransform and commaSeparatedTransform definitions
rg -n "jsonTransform|commaSeparatedTransform" apps/server/src/config.ts -A 8Repository: Sharkord/sharkord
Length of output: 1086
🏁 Script executed:
# Check what happens after applyEnvOverrides and if there's any validation
sed -n '152,180p' apps/server/src/config.tsRepository: Sharkord/sharkord
Length of output: 917
Re-validate config schema after applying environment overrides to prevent type mismatches.
The current code applies environment overrides after schema parsing but does not re-validate. This causes two critical fields to potentially violate their schema types:
oidc.rolesMappingexpectsRecord<string, string>but can be set to a string if the env var is invalid JSONoidc.allowedOriginsexpectsstring[](viacommaSeparatedTransform) but applyEnvOverrides sets a raw string instead of parsing it with the schema's comma-separated transformer
At runtime, code expecting these as objects/arrays will fail with type errors. Wrap the result with zConfig.parse() to re-apply all schema transformers:
Required fix
-config = applyEnvOverrides(config, {
+config = zConfig.parse(applyEnvOverrides(config, {
'server.port': 'SHARKORD_PORT',
'server.debug': 'SHARKORD_DEBUG',
'server.autoupdate': 'SHARKORD_AUTOUPDATE',
'server.disableLocalSignup': 'SHARKORD_DISABLE_LOCAL_SIGNUP',
'oidc.oidcEnabled': 'OIDC_ENABLED',
'oidc.enforceOidcRoles': 'OIDC_ENFORCE_ROLES',
'oidc.issuer': 'OIDC_ISSUER',
'oidc.clientId': 'OIDC_CLIENT_ID',
'oidc.clientSecret': 'OIDC_CLIENT_SECRET',
'oidc.rolesMapping': 'OIDC_ROLES_MAPPING',
'oidc.requiredGroup': 'OIDC_REQUIRED_GROUP',
'oidc.allowedOrigins': 'OIDC_ALLOWED_ORIGINS',
'oidc.caCertPath': 'OIDC_CA_CERT_PATH',
'webRtc.port': 'SHARKORD_WEBRTC_PORT',
'webRtc.announcedAddress': 'SHARKORD_WEBRTC_ANNOUNCED_ADDRESS',
'webRtc.maxBitrate': 'SHARKORD_WEBRTC_MAX_BITRATE'
-});
+}));📝 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.
| config = applyEnvOverrides(config, { | |
| 'server.port': 'SHARKORD_PORT', | |
| 'server.debug': 'SHARKORD_DEBUG', | |
| 'server.autoupdate': 'SHARKORD_AUTOUPDATE', | |
| 'server.disableLocalSignup': 'SHARKORD_DISABLE_LOCAL_SIGNUP', | |
| 'oidc.oidcEnabled': 'OIDC_ENABLED', | |
| 'oidc.enforceOidcRoles': 'OIDC_ENFORCE_ROLES', | |
| 'oidc.issuer': 'OIDC_ISSUER', | |
| 'oidc.clientId': 'OIDC_CLIENT_ID', | |
| 'oidc.clientSecret': 'OIDC_CLIENT_SECRET', | |
| 'oidc.rolesMapping': 'OIDC_ROLES_MAPPING', | |
| 'oidc.requiredGroup': 'OIDC_REQUIRED_GROUP', | |
| 'oidc.allowedOrigins': 'OIDC_ALLOWED_ORIGINS', | |
| 'oidc.caCertPath': 'OIDC_CA_CERT_PATH', | |
| 'webRtc.port': 'SHARKORD_WEBRTC_PORT', | |
| 'webRtc.announcedAddress': 'SHARKORD_WEBRTC_ANNOUNCED_ADDRESS', | |
| 'webRtc.maxBitrate': 'SHARKORD_WEBRTC_MAX_BITRATE' | |
| }); | |
| config = zConfig.parse(applyEnvOverrides(config, { | |
| 'server.port': 'SHARKORD_PORT', | |
| 'server.debug': 'SHARKORD_DEBUG', | |
| 'server.autoupdate': 'SHARKORD_AUTOUPDATE', | |
| 'server.disableLocalSignup': 'SHARKORD_DISABLE_LOCAL_SIGNUP', | |
| 'oidc.oidcEnabled': 'OIDC_ENABLED', | |
| 'oidc.enforceOidcRoles': 'OIDC_ENFORCE_ROLES', | |
| 'oidc.issuer': 'OIDC_ISSUER', | |
| 'oidc.clientId': 'OIDC_CLIENT_ID', | |
| 'oidc.clientSecret': 'OIDC_CLIENT_SECRET', | |
| 'oidc.rolesMapping': 'OIDC_ROLES_MAPPING', | |
| 'oidc.requiredGroup': 'OIDC_REQUIRED_GROUP', | |
| 'oidc.allowedOrigins': 'OIDC_ALLOWED_ORIGINS', | |
| 'oidc.caCertPath': 'OIDC_CA_CERT_PATH', | |
| 'webRtc.port': 'SHARKORD_WEBRTC_PORT', | |
| 'webRtc.announcedAddress': 'SHARKORD_WEBRTC_ANNOUNCED_ADDRESS', | |
| 'webRtc.maxBitrate': 'SHARKORD_WEBRTC_MAX_BITRATE' | |
| })); |
| '/info': (req, res) => infoRouteHandler(req, res) | ||
| '/info': (req, res) => infoRouteHandler(req, res), | ||
| '/auth/login': (req, res) => oidcLogin(req, res), | ||
| 'auth/callback': (req, res) => oidcCallback(req, res) |
There was a problem hiding this comment.
Fix callback route key to include the leading slash.
Line 44 uses 'auth/callback', but request pathnames are slash-prefixed (/auth/callback), so exact matching fails.
Patch
- 'auth/callback': (req, res) => oidcCallback(req, res)
+ '/auth/callback': (req, res) => oidcCallback(req, res)📝 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.
| 'auth/callback': (req, res) => oidcCallback(req, res) | |
| '/auth/callback': (req, res) => oidcCallback(req, res) |
There was a problem hiding this comment.
@zigzatuzoo I seem to be encountering this issue here; /auth/callback/ returns HTTP 404, leading to an incomplete flow.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| if (!settings.allowNewUsers) { | ||
| const inviteError = await isInviteValid(data.invite); | ||
| } |
There was a problem hiding this comment.
Remove the unused invite validation call.
Line 170 executes isInviteValid but discards the result, then Line 175 calls it again for the real flow. This is redundant work on the signup path.
♻️ Suggested cleanup
- if (!settings.allowNewUsers) {
- const inviteError = await isInviteValid(data.invite);
- }
-
let inviteRoleId: number | null = null;
const result = await isInviteValid(data.invite);📝 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.
| if (!settings.allowNewUsers) { | |
| const inviteError = await isInviteValid(data.invite); | |
| } | |
| let inviteRoleId: number | null = null; | |
| const result = await isInviteValid(data.invite); |
| if (config.oidc.oidcEnabled === false) { | ||
| return res.writeHead(404); | ||
| } |
There was a problem hiding this comment.
Complete the 404 responses with end().
Line 62 and Line 110 return after writeHead(404) without ending the response, which can leave requests hanging.
✅ Minimal fix
if (config.oidc.oidcEnabled === false) {
- return res.writeHead(404);
+ res.writeHead(404);
+ return res.end();
} if (config.oidc.oidcEnabled === false) {
- return res.writeHead(404);
+ res.writeHead(404);
+ return res.end();
}Also applies to: 110-112
| for (const [oidcRole, localRole] of Object.entries(rolesMapping)) { | ||
| if (oidcGroups.includes(oidcRole.toLowerCase())) { | ||
| const dbRole = allDbRoles.find( | ||
| (r: { id: number; name: string; }) => r.name.toLowerCase() === localRole.toLowerCase() | ||
| ); | ||
| if (dbRole) targetRoleIds.push(dbRole.id); | ||
| } |
There was a problem hiding this comment.
Deduplicate mapped role IDs before insertion.
If multiple OIDC groups map to the same local role, targetRoleIds can contain duplicates, and Line 275 may attempt duplicate inserts that violate the (user_id, role_id) PK.
🧩 Prevent duplicate role inserts
- const targetRoleIds: number[] = [];
+ const targetRoleIds = new Set<number>();
...
- if (dbRole) targetRoleIds.push(dbRole.id);
+ if (dbRole) targetRoleIds.add(dbRole.id);
...
- const rolesToAdd = targetRoleIds.filter((id) => !userCurrentRoles.some((r) => r.roleId === id));
+ const targetRoleIdList = [...targetRoleIds];
+ const rolesToAdd = targetRoleIdList.filter(
+ (id) => !userCurrentRoles.some((r) => r.roleId === id)
+ );Also applies to: 273-282
Same, this would be huge and is the final puzzle piece needed for me to make the switch for my community. Especially if we can optionally disable/skip the local login step like @Giggitybyte brought up. |
| }); | ||
|
|
||
| const claims = tokenResponse.claims(); | ||
| if (!claims || !claims.sub || !claims.email) { |
There was a problem hiding this comment.
@zigzatuzoo It seems the /auth/callback was not registered properly, leading to an incomplete flow. I fixed that locally and rebuilt the Docker container, but the code here is throwing.
The error occurs because the code above incorrectly expects both sub AND email in the ID token, when only sub is guaranteed to be present according to the OIDC spec.
I am using Authelia, which tries to follow the OIDC spec closely, and it does not provide the email claim in the JWT ID token. This information must be fetched from the UserInfo endpoint. See Authelia docs here, and OIDC docs here.
So, this validation should only require the sub claim, and email needs to be optional. For compatibility with other OIDC providers which don't implement the spec correctly, check to see if the email claim was included with the ID token, and call the UserInfo endpoint as a fallback.
| } | ||
| } | ||
|
|
||
| const identity = (claims.email || claims.sub) as string; |
There was a problem hiding this comment.
Also just noticed this: using email is not a smart idea to uniquely identify a user, only sub should be used. If a user changes their email address in their IDP, they'd lose access to their Sharkord account despite logging in with the same IDP user account.
From the OIDC spec:
... other Claims such as email, phone_number, preferred_username, and name MUST NOT be used as unique identifiers for the End-User, whether obtained from the ID Token or the UserInfo Endpoint.
|
|
||
| const parameters: Record<string, string> = { | ||
| redirect_uri: redirectUri, | ||
| scope: 'openid profile email groups', |
There was a problem hiding this comment.
Important to note: groups is not a standard scope. While I believe most IDPs use groups, some use roles or memberOf, and others use even more non-standard stuff like nested objects (e.g. resource_access.client_name.roles).
There should be a configuration value that allows the end-user to specify the name of the scope/claim for groups, with groups as the default.
| const [insertedUser] = await db.insert(users).values({ | ||
| identity: identity, | ||
| password: randomPassword, | ||
| name: (claims.name as string) ?? identity.split('@')[0], |
There was a problem hiding this comment.
Along with all the other comments about the different claims needing to be configurable, this one likely should be too. with my current keycloak setup this would end up having all users use their full name as the name, i.e. in my case Ryan Voots rather than the name i'd likely want here, simcop2387 which is provided under the preferred_username claim by default. Also this assumes that the identity is an email address and can be split like that, which may or may not be the case even with the existing code. And it'll mean that two users could have the same name, i.e. john@example.com and john@otherdomain.com which is likely to cause some confusing issues.
There was a problem hiding this comment.
Very good catch. Authelia works the same way; name would be the full given name of a user and preferred_username is the actual login name for a user, which would be what their Sharkord username should be. An email should never be used.
|
Okay, will have some time later today to work on all this. Thank you all for catching these issues! |
… bug fixes - Only require sub in ID token per OIDC spec; fetch UserInfo endpoint as fallback for email, groups, username, and display name claims - Add oidcSub column to users table (migration 0008) as stable IdP identifier so email/username changes on the IdP don't create duplicate accounts - Look up users by oidcSub first, fall back to identity for pre-existing users; backfill oidcSub on first login after migration - Sync identity and display name on every OIDC login; always update lastLoginAt - Add migration 0009 for user_roles.added_by column required by role mapping - Fix /auth/callback route missing leading slash (route was unreachable) - Fix cookie parsing to correctly handle values containing = characters - Cache OIDC discovery document for 5 minutes to avoid hitting IdP on every request - Add groupsClaim config (env: OIDC_GROUPS_CLAIM, default: groups) - Add usernameClaim config (env: OIDC_USERNAME_CLAIM, default: preferred_username) - Add displayNameClaim config (env: OIDC_DISPLAY_NAME_CLAIM) and enforceOidcDisplayName (env: OIDC_ENFORCE_DISPLAY_NAME, default: true) - Add additionalScopes config (env: OIDC_ADDITIONAL_SCOPES) for custom claim scopes - Change requiredGroup to requiredGroups (env: OIDC_REQUIRED_GROUPS, comma-separated); user needs membership in any one of the listed groups - Include displayNameClaim in needsUserInfo check - Deduplicate targetRoleIds before insert to prevent PK violations - Clear noisy example defaults for rolesMapping, requiredGroups, allowedOrigins - Remove redundant isInviteValid call in login route
|
Great work! I merged and tested this PR on my branch. It works really well. Thanks for your effort! A few minor things I noticed:
|
This should be handled by a back channel logout process when supported by the oidc provider. This is the exact spec so it's basically useless for understanding it but it's a good reference at least, https://openid.net/specs/openid-connect-backchannel-1_0.html . But basically the idea is that Sharkord will use the auth token that it has to call an endpoint on the OIDC provider to do the logout, resulting in the user session being logged out for that oidc client on the provider. There's also a front-channel logout where you'd redirect the user to a url on the provider to do the same thing but back-channel is preferred because it means that you're not relying on the client to do the actual logout and not just pretending to have done it and then using the token anyway.
Yea usually there's a different interstitial page for the callback that does exactly that. |
|
I'll probably set this up myself tonight and play with it, should hopefully find any other potential issues with it and i'll leave more reviews/comments as necessary. Might take a look at dealing with the logout stuff to enhance that for doing oidc session logouts with both the back-channel setup and front-channel versions for better security (it helps prevent token theft and session hijacking on the oidc provider). |
|
Eagerly Awaiting this PR!!! 👀 |
|
I'm sorry, this has been here for a long time and I haven't looked into it yet. |
|
@zigzatuzoo Hello, could you resolve the merge conflicts so I can pull this PR with the latest Sharkord changes? |
|
This feature would be an absolutely amazing addition. I just installed Sharkord last night and i just want to say you do great work. Its so simple and clean. Bravo. |
Yeah I can get to it in a bit... |
|
Guys, I'm really sorry I haven't looked at this yet. Lately I'm not having too much time to dedicate to Sharkord. |
9 participants
Closes #41