Conversation
… additional_config
…onfigured providers
Summary of ChangesHello @nikpcenicni, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the authentication system by allowing comprehensive configuration of OpenID Connect (OIDC) providers and global authentication settings through environment variables. This change streamlines deployments, particularly for single Identity Provider (IdP) setups, by automating OIDC provider registration and enabling dynamic control over login options and user role synchronization based on IdP groups. The update also includes necessary database schema adjustments and frontend UI improvements to reflect environment-managed settings. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-structured feature for configuring OIDC entirely through environment variables. The changes span the backend, frontend, database, and documentation, providing a complete solution. The frontend updates to indicate environment-managed settings are a great user experience improvement. I've identified a critical issue in how OIDC configuration is saved to the database which would likely prevent the feature from working as intended, a high-severity security concern in the new JWT parsing logic, and a medium-severity opportunity for refactoring to improve maintainability. After addressing these points, this will be an excellent addition to the project.
| endpoints.userInfoEndpoint, | ||
| config, | ||
| oidcConfig | ||
| JSON.stringify(oidcConfig) |
There was a problem hiding this comment.
Using JSON.stringify(oidcConfig) here will store a JSON string in the oidc_config JSONB column, rather than a JSON object. The better-auth OIDC plugin expects a parsed object, and this was the motivation for migrating the column to JSONB. Storing it as a string will likely cause the OIDC flow to fail with an invalid_provider error.
The node-postgres (pg) driver automatically handles serializing JavaScript objects to JSON when the target column is JSON or JSONB. You should pass the oidcConfig object directly.
| JSON.stringify(oidcConfig) | |
| oidcConfig |
| endpoints.userInfoEndpoint, | ||
| config, | ||
| oidcConfig, | ||
| JSON.stringify(oidcConfig), |
There was a problem hiding this comment.
Similar to the createOidcProvider function, using JSON.stringify(oidcConfig) here is incorrect for a JSONB column. This will store a JSON string instead of an object, which will likely break the OIDC integration with better-auth. Please pass the oidcConfig object directly to the query; the pg driver will handle serialization.
| JSON.stringify(oidcConfig), | |
| oidcConfig, |
| const idToken = oidcAccount.id_token; | ||
| const parts = idToken.split('.'); | ||
| if (parts.length < 2) return; | ||
|
|
||
| const payloadBase64 = parts[1]; | ||
| if (payloadBase64) { | ||
| // Use Buffer to decode base64url or base64 | ||
| const payload = JSON.parse(Buffer.from(payloadBase64, 'base64').toString()); |
There was a problem hiding this comment.
Manually parsing the JWT payload is unsafe and not robust.
- Security Risk: The token's signature is not verified. An attacker who can tamper with the
id_tokenin the database could escalate their privileges. - Incorrect Decoding: JWTs use Base64Url encoding.
Buffer.from(..., 'base64')expects standard Base64 and can fail.
Please use a dedicated library like jwt-decode for safe decoding. You will need to add it to your package.json. For production, you should consider full signature verification using a library like jsonwebtoken.
Example with jwt-decode:
const { jwtDecode } = require('jwt-decode');
// ... inside syncUserGroups ...
if (oidcAccount?.id_token) {
try {
const payload = jwtDecode(oidcAccount.id_token);
const groups = payload.groups || [];
// ... rest of your logic
} catch (e) {
console.error('Failed to decode token', e);
}
}| const oidcConfig = JSON.stringify({ | ||
| issuer: providerData.issuer_url, | ||
| // Construct native oidcConfig for Better Auth (object for JSONB column; same base as auth.baseURL) | ||
| const baseUrl = (process.env.SPARKY_FITNESS_FRONTEND_URL?.startsWith('http') ? process.env.SPARKY_FITNESS_FRONTEND_URL : `https://${process.env.SPARKY_FITNESS_FRONTEND_URL || 'http://localhost:8080'}`).replace(/\/$/, ''); |
There was a problem hiding this comment.
This complex line for constructing the baseUrl is duplicated in updateOidcProvider (line 240). To improve maintainability and reduce redundancy, consider extracting this logic into a shared helper function.
For example:
function getBaseUrl() {
const frontendUrl = process.env.SPARKY_FITNESS_FRONTEND_URL || 'http://localhost:8080';
const urlWithProtocol = frontendUrl.startsWith('http') ? frontendUrl : `https://${frontendUrl}`;
return urlWithProtocol.replace(/\/$/, '');
}…r OIDC provider repository to use getBaseUrl function for URL handling
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for configuring OIDC authentication via environment variables, which significantly improves deployment flexibility and automation. The implementation correctly handles startup provider upserts, global settings overrides, and adds a useful group-based admin role synchronization feature. The changes are well-integrated into the existing architecture, following established repository and service patterns. I have identified a few issues regarding security and robustness in the group synchronization logic and environment variable parsing that should be addressed.
| if (oidcAccount?.id_token) { | ||
| try { | ||
| const payload = jwtDecode(oidcAccount.id_token); | ||
| const groups = payload.groups || []; |
There was a problem hiding this comment.
The current implementation of the group check is vulnerable to substring matching if the groups claim in the ID token is returned as a string instead of an array. For example, if the adminGroup is set to 'admin' and the user's groups claim is 'superadmin', the includes method on a string will return true, leading to unintended privilege escalation. Ensuring the claim is treated as an array (or an element of one) prevents this issue.
| const groups = payload.groups || []; | |
| const groupsClaim = payload.groups || []; | |
| const groups = Array.isArray(groupsClaim) ? groupsClaim : [groupsClaim]; |
| 'SELECT provider_id, id_token FROM "account" WHERE user_id = $1', | ||
| [userId] |
There was a problem hiding this comment.
When a user has multiple OIDC accounts linked, the current logic picks the first one found in the database to perform group synchronization. This can lead to inconsistent behavior if different providers have different group claims. It is recommended to sort the accounts by their last update time to ensure the synchronization is based on the most recently used (and likely currently active) OIDC provider.
const { rows: allAccounts } = await pool.query(
'SELECT provider_id, id_token FROM "account" WHERE user_id = $1 ORDER BY updated_at DESC',| token_endpoint_auth_method: tokenAuthMethod || 'client_secret_post', | ||
| signing_algorithm: idTokenAlg || 'RS256', | ||
| profile_signing_algorithm: userInfoAlg || 'none', | ||
| timeout: timeout ? parseInt(timeout, 10) : 30000, |
There was a problem hiding this comment.
The parseInt function can return NaN if the environment variable contains an invalid numeric string. Storing NaN in the configuration could lead to unexpected behavior or crashes in downstream components (e.g., network request timeouts). Providing a fallback to the default value when parsing fails improves the robustness of the configuration loading.
| timeout: timeout ? parseInt(timeout, 10) : 30000, | |
| timeout: Number.parseInt(timeout, 10) || 30000, |
…nt and properly parse group claims, and improve OIDC timeout handling.
…idation for OIDC group synchronization.
|
What hap? Will you be resubmitting again? |
|
Accidentally closed it, and realized there were still a few issues. Negative side effects of coding too late at night. Will resubmit this Pr this evening, Or reopen it depending on your preference |
…gging in `oidcGroupSync.js`, and simplify OIDC provider updates in `oidcEnvConfig.js`.
|
/gemini review |
|
Hello @nikpcenicni, I see you've requested a new review and summary for this pull request. The system will process these commands shortly and provide updated feedback based on the current state of your changes. |
|
@CodeWithCJ Should be good to go now, let me know if there's any changes you want implemented or if you have any questions |
|
@nikpcenicni Could you check this. 
|
…ted SSO providers post-database initialization in `SparkyFitnessServer.js`
…js` to prevent potential errors during SSO synchronization Co-authored-by: Cursor <cursoragent@cursor.com>
…les in `4.environment-variables.md`
…nment-variables.md` for improved readability and consistency
…ronment-variables.md`, including password change implications and new database host/port settings
…on to improve readability and maintainability
…rigins and database hooks to enhance readability and maintainability
…itnessServer.js`, including improved sync handling for trusted providers
|
IDE had messed up the formatting which was causing some messy conflicts, took a bit to resolve them. |
2 participants
Environment Variables for OIDC Setup
Overview
This PR adds support for configuring OIDC (OpenID Connect) authentication entirely via environment variables, so deployments can use a single IdP without requiring an initial email/password signup or manual admin configuration. Auth behavior (enable/disable email login, enable OIDC, auto-redirect) and an optional env-configured OIDC provider are driven by env vars, with optional OIDC group-based admin role sync.
Features
1. OIDC Provider from Environment
SPARKY_FITNESS_OIDC_ISSUER_URL,SPARKY_FITNESS_OIDC_CLIENT_ID,SPARKY_FITNESS_OIDC_CLIENT_SECRET, andSPARKY_FITNESS_OIDC_PROVIDER_SLUGare set, the server creates or updates an OIDC provider at startup and registers it with the existing SSO/Better Auth flow.utils/oidcEnvConfig.jsreads env, normalizes issuer URL, and callsoidcProviderRepositoryto upsert the provider;SparkyFitnessServer.jsruns this after migrations.is_env_configured: trueinadditional_configso the admin UI can show a “Managed by Env” badge and restrict deletion/editing of credentials.2. Global Auth Settings and Env Overrides
globalSettingsRepository.getGlobalSettings()applies env overrides before returning:SPARKY_FITNESS_DISABLE_EMAIL_LOGIN=true→enable_email_password_login = falseSPARKY_FITNESS_FORCE_EMAIL_LOGIN=true→enable_email_password_login = true(fail-safe; wins over disable)SPARKY_FITNESS_OIDC_AUTH_ENABLED=true→is_oidc_active = trueis_email_login_env_configuredandis_oidc_active_env_configuredindicate when the value came from env so the admin UI can reflect that.3. Public Auth Settings and Auto-Redirect
GET /api/auth/settingsreturns login options for the frontend: email enabled/disabled, OIDC enabled, list of active OIDC providers (id, display_name, logo_url, auto_register), andauto_redirect.SPARKY_FITNESS_OIDC_AUTO_REDIRECT=truesetsoidc.auto_redirectin the response. The login page auto-redirects to the single OIDC provider only whenauto_redirectis true, email login is disabled, and exactly one provider is active.4. OIDC Group Sync and Admin Role
auth.js, the Better Authsession.create.afterhook runs after session creation. WhenSPARKY_FITNESS_OIDC_ADMIN_GROUPis set, it callsoidcGroupSync.syncUserGroups()with the user id and that group name.utils/oidcGroupSync.jsreads the user’s OIDC accountid_token, decodes thegroupsclaim, and promotes the user to admin if the admin group is present, or revokes admin if not. This keeps SparkyFitness admin role in sync with IdP groups.SPARKY_FITNESS_OIDC_USER_GROUPis documented for IdP configuration (e.g. which group denotes standard users); role sync currently uses only the admin group.5. Database and Better Auth Adapter
20260221210000_oidc_config_jsonb.sqlchangessso_provider.oidc_configfromtexttoJSONBso the Better Auth adapter receives an object and avoids “invalid_provider” when using the OIDC plugin.oidcProviderRepositorycreates/updates providers with a JSONBoidc_configobject (issuer, clientId, clientSecret, scopes, discoveryEndpoint, redirectURI, endpoints, etc.) and continues to store display/UI options inadditional_config(includingis_env_configured).6. Frontend
LoginSettingsintypes/auth.d.tsincludesoidc.auto_redirect?: booleanandOidcProviderincludesauto_register.GET /api/auth/settings; whenoidc.auto_redirectis true, email is disabled, and there is exactly one OIDC provider, it auto-redirects to that provider after a short delay.is_env_configured. The admin table shows a “Managed by Env” badge for such providers and hides the delete button for them (edit remains for display/logo/behavior; credentials are managed via env).Environment Variables
SPARKY_FITNESS_DISABLE_EMAIL_LOGINtrueto disable email/password login (overridden bySPARKY_FITNESS_FORCE_EMAIL_LOGIN).SPARKY_FITNESS_FORCE_EMAIL_LOGINtrueto force email login on (fail-safe if OIDC is misconfigured).SPARKY_FITNESS_OIDC_AUTH_ENABLEDtrueto enable OIDC login (overrides DB admin setting).SPARKY_FITNESS_OIDC_ISSUER_URLhttps://auth.example.com). Discovery URL is derived as issuer +/.well-known/openid-configuration. Required for env provider upsert.SPARKY_FITNESS_OIDC_CLIENT_IDSPARKY_FITNESS_OIDC_CLIENT_SECRETSPARKY_FITNESS_OIDC_PROVIDER_SLUGmy-idp). Required for env provider upsert.SPARKY_FITNESS_OIDC_PROVIDER_NAMESPARKY_FITNESS_OIDC_AUTO_REDIRECTtrueto allow auto-redirect to the single OIDC provider when email login is disabled.SPARKY_FITNESS_OIDC_ADMIN_GROUPSPARKY_FITNESS_OIDC_USER_GROUPSPARKY_FITNESS_OIDC_SCOPEopenid email profile).SPARKY_FITNESS_OIDC_AUTO_REGISTERtrueto auto-create user on first OIDC login (optional; defaulttrue).SPARKY_FITNESS_OIDC_LOGO_URLSPARKY_FITNESS_OIDC_DOMAIN{slug}.env).SPARKY_FITNESS_OIDC_TOKEN_AUTH_METHODclient_secret_post).SPARKY_FITNESS_OIDC_ID_TOKEN_SIGNED_ALGRS256).SPARKY_FITNESS_OIDC_USERINFO_SIGNED_ALGnone).SPARKY_FITNESS_OIDC_TIMEOUT30000).Note: The issue requested
OIDC_CONFIGURATION_URL; this implementation usesOIDC_ISSUER_URLand derives the discovery URL as{issuer}/.well-known/openid-configuration, which is standard and avoids redundancy.Database Migrations
20260221210000_oidc_config_jsonb.sql: Convertssso_provider.oidc_configfromtexttoJSONBfor Better Auth OIDC adapter compatibility.API Changes
Modified Endpoints
GET /api/auth/settings(public): Now returnsemail.enabled,oidc.enabled,oidc.providers(id, display_name, logo_url, auto_register), andoidc.auto_redirect(fromSPARKY_FITNESS_OIDC_AUTO_REDIRECT).Unchanged
additional_config(e.g.is_env_configuredwhen set by env upsert).Frontend Changes
src/types/auth.d.ts:LoginSettings.oidcextended withauto_redirect?: boolean.src/api/Auth/auth.ts:getLoginSettings()fetches/api/auth/settings; response typing matches new shape.src/pages/Auth/Auth.tsx: Auto-redirect only whenloginSettings.oidc.auto_redirectis true, email login is disabled, and exactly one OIDC provider is active.src/pages/Admin/OidcSettings.tsx: Shows “Managed by Env” badge for providers withis_env_configured; delete button hidden for those providers.src/api/Admin/oidcSettingsService.ts:OidcProvidertype includesis_env_configured?: boolean.Testing
SparkyFitnessServer/tests/oidcEnvConfig.test.js: Tests forgetEnvOidcConfig()(required vars, optional vars, issuer normalization).SparkyFitnessServer/tests/oidcGroupSync.test.js: Tests for group sync (admin grant/revoke based ongroupsclaim).SparkyFitnessServer/tests/globalSettingsRepository.test.js: Updated for env overrides ingetGlobalSettings().SparkyFitnessServer/tests/oidcProviderRepository.test.js: Updated for JSONBoidc_configandis_env_configuredin config.Documentation
docker/.env.example: Commented block for OIDC (DISABLE_EMAIL_LOGIN, OIDC_AUTH_ENABLED, ISSUER_URL, CLIENT_ID, CLIENT_SECRET, PROVIDER_SLUG/NAME, SCOPE, AUTO_REGISTER, LOGO_URL, DOMAIN, AUTO_REDIRECT, ADMIN_GROUP, USER_GROUP, and advanced options).docs/content/1.install/4.environment-variables.md: OIDC section updated to list all env vars and describe env-configured provider upsert, auth overrides, auto-redirect, and group-based admin sync.Breaking Changes
None. Existing behavior is preserved:
sso_provider.oidc_configmigration is backward-compatible (existing text values are cast to JSONB where applicable).Migration Notes
20260221210000_oidc_config_jsonb.sqlbefore or with this release.SPARKY_FITNESS_OIDC_ISSUER_URL,SPARKY_FITNESS_OIDC_CLIENT_ID,SPARKY_FITNESS_OIDC_CLIENT_SECRET,SPARKY_FITNESS_OIDC_PROVIDER_SLUG, and optionallySPARKY_FITNESS_OIDC_AUTH_ENABLED=true,SPARKY_FITNESS_DISABLE_EMAIL_LOGIN=true,SPARKY_FITNESS_OIDC_AUTO_REDIRECT=true,SPARKY_FITNESS_OIDC_ADMIN_GROUP=your_admin_group.SPARKY_FITNESS_FORCE_EMAIL_LOGIN=trueduring testing if you need a fallback to email login.Related Issues
Closes #598