✅ Done - merged in 2.10.0. Sandy worked through these notes in commit f8d57cf0, and the branch was merged on 30 September 2026. How the finished feature works, and the few things adjusted at merge time, are in SSO auto-create and profile sync - Developer Guide. This page is kept as the record of the review.

Review notes on the feat/differentiated-jit-sso branch from discussion #155. For each file it covers what I'd do differently, the exact change and the reason.

Written for Santhosh Srinivasan (Sandy), who built it, and for the AI coding assistant he works with. Every change below names the file, shows the code and explains why, so it can be worked through top to bottom.

• Branch: srinivasansanthosh/freeitsm → feat/differentiated-jit-sso
• Based on: 2.7.1 (801019c9)
• For how SSO works today: Single Sign-On (SSO)

The idea is right, and it fills a real gap. Right now one checkbox, auto_create_users, means "create an analyst" on the analyst login and "create a requester" on the portal login. That's confusing, and it makes it impossible to say "customers yes, analysts no". Splitting it in two is the correct fix.

Things the branch already does well, which should stay exactly as they are:

• The confirm screen has proper CSRF. A token goes into the session, is checked with hash_equals() and is thrown away after one use.
• The schema change is in both places. database/freeitsm.sql and includes/db_verify_schema.php agree, including the ENUM values.
• ID-token claims win over userinfo claims. array_merge($userInfo, $claims) puts the ID token last, so a signed claim is never overwritten by an unsigned one. That's the right way round.
• The analyst-side lock is enforced on the server, not just hidden in the browser. api/myaccount/save_profile.php refuses the save.
• The "Analyst console" link in the portal menu fills a gap: the analyst side has had a link to the portal since #81, but there was no way back.

The changes below are mostly about one thing: someone upgrading must see no difference until they choose to use the new options. FreeITSM has real installs, and a sign-in change that quietly alters behaviour on upgrade is the worst kind of surprise.

The branch is based on 2.7.1, and main is now 2.8.0. Please don't rebase or merge main into the branch. I'll do that myself when it comes in. There are only four small overlaps, and three of them disappear once the changes in section 6 are made.

Just make the changes on the branch as it is.

File: api/system/db_verify.php

Before this branch, auto_create_users = 1 did two jobs:

The branch moves the analyst half into a new column, auto_create_analysts, which starts at 0. It also points LDAP at it (includes/ldap.php, ldapResolveAnalyst()). That's correct, but on its own it means that after upgrading:

• anyone relying on analyst JIT through OIDC or LDAP stops getting new analysts, with no warning
• instead, new staff land on the "continue to the portal?" screen.

DB Verify already has a pattern for exactly this: "was this column missing before this run? If so, fill it in once." Look for $analystIsAdminColWasMissing in api/system/db_verify.php. Do the same thing.

Step 1 - near the other ...ColWasMissing probes (around line 135-160 of api/system/db_verify.php), add:

    // SSO JIT split (discussion #155): was auth_providers.auto_create_analysts
    // absent before this run? Before the split, auto_create_users ALSO meant
    // "create an analyst" on the analyst login, so on the run that adds the new
    // column we copy the old value across. Only this once: after that the two
    // switches are set deliberately and must never be re-synced.
    $providerAutoAnalystsColWasMissing = false;
    try {
        $aaProbe = $conn->prepare("SELECT COUNT(*) FROM information_schema.columns WHERE table_schema = ? AND table_name = 'auth_providers' AND column_name = 'auto_create_analysts'");
        $aaProbe->execute([$dbName]);
        $providerAutoAnalystsColWasMissing = ((int)$aaProbe->fetchColumn() === 0);
    } catch (Exception $e) {}

Step 2 - next to the if ($analystIsAdminColWasMissing) { ... } backfill (after the schema loop has added the columns), add:

    // One-time SSO JIT split (discussion #155). Keeps every existing provider
    // behaving exactly as it did before the upgrade:
    //  - analyst JIT stays on wherever the old single switch was on
    //  - someone with no analyst account is refused, as before, rather than
    //    being offered the portal; admins can choose 'confirm' deliberately
    if ($providerAutoAnalystsColWasMissing) {
        $copied = $conn->exec("UPDATE auth_providers SET auto_create_analysts = auto_create_users");
        $conn->exec("UPDATE auth_providers SET analyst_fallback_mode = 'block'");
        $results[] = [
            'table'   => 'auth_providers',
            'status'  => 'updated',
            'details' => ['Kept the auto-create setting for ' . (int)$copied . ' sign-in provider(s) (one-time upgrade)']
        ];
    }

Before the upgrade, someone with no analyst account got an error. block keeps that. New providers still default to confirm, which is a good default for someone setting SSO up fresh. Existing ones only change when an admin chooses to.

1. On a 2.7.1 database, set one OIDC provider to auto_create_users = 1 and one to 0.
2. Run System → Database Verification.
3. Both providers should now have auto_create_analysts equal to their old auto_create_users, and analyst_fallback_mode = 'block'.
4. Change one by hand, then run verification again. It must not be overwritten. This is the "only this once" part.

Files: database/freeitsm.sql, includes/db_verify_schema.php, api/system/save_sso_provider.php, api/system/get_sso_providers.php, system/sso/index.php, api/auth/oidc_callback.php, api/myaccount/*.php, includes/users.php

profile_sync_mode defaults to 'always'. When the column is added on upgrade, every existing provider becomes "always", which means that the moment the upgrade is done:

• every analyst's full_name, job title, department and phones are overwritten on each sign-in (someone who deliberately renamed themselves loses it)
• every SSO user's My Account fields become locked
• LDAP and CardDAV providers are included too, even though sync only runs for OIDC. That breaks CardDAV write-back editing (see section 4).

Make never the default everywhere the value is read or written, so nothing changes until an admin switches it on.

database/freeitsm.sql and includes/db_verify_schema.php:

`profile_sync_mode`      ENUM('always','initial','never') NOT NULL DEFAULT 'never',

'profile_sync_mode'      => "ENUM('always','initial','never') NOT NULL DEFAULT 'never'",

api/system/save_sso_provider.php:

$profileSyncMode = in_array($data['profile_sync_mode'] ?? '', ['always', 'initial', 'never'], true)
                   ? $data['profile_sync_mode'] : 'never';

Every ?? 'always' fallback in the branch becomes ?? 'never'. Search for them all; there are more than a dozen:

git grep -n "profile_sync_mode'\] ?? 'always'\|profile_sync_mode\"\] ?? \"always\"\|profile_sync_mode || 'always'\|: 'always'"

In system/sso/index.php, the modal's default and the save payload:

document.getElementById('fProfileSyncMode').value = (p && p.profile_sync_mode) ? p.profile_sync_mode : 'never';
// ...
profile_sync_mode: document.getElementById('fProfileSyncMode').value || 'never',

and the table pill (const mode = p.profile_sync_mode || 'never';).

Also put never first in the <select>, so the safe choice is the one at the top:

<option value="never">...profile_sync_never...</option>
<option value="initial">...profile_sync_initial...</option>
<option value="always">...profile_sync_always...</option>

File: api/auth/oidc_callback.php

The same ~20-line "copy claims onto the record" block appears four times (branch lines ~165, ~202, ~400, ~437). Two of those copies run before the account has been checked.

This is the analyst email-match path as the branch has it (simplified):

if ($analyst) {
    $analystId = (int)$analyst['id'];
    // ... profile sync runs here: UPDATE analysts SET full_name = ?, job_title = ? ...
    if ((int)$analyst['is_active'] !== 1) {
        ssoBail('Your account is inactive. Contact an administrator.');   // too late - already updated
    }
    $assigned = (int)($analyst['auth_provider_id'] ?? 0);
    if ($assigned !== 0 && $assigned !== $providerId) {
        ssoBail('This account is not set up to sign in with this provider.');   // too late
    }

The requester path in completeSelfServiceSso() has the same shape: sync, then elseif ($assigned !== $providerId) ssoBail(...).

So a provider that is about to be refused has already rewritten the person's name, title and phone numbers. With multi-tenancy that matters: a provider pinned to Company A can assert the email address of someone at Company B and edit their record, even though the sign-in itself fails.

The rule: nothing about an account is written until the account has passed every check.

Step 1 - one helper, next to oidcExtractProfileAttributes():

/**
 * Copy profile details from the identity provider onto a local account.
 *
 * 🔴 Call this ONLY after the account has passed every access check (active,
 * assigned to this provider). Writing first and refusing afterwards lets a
 * provider that is about to be turned away edit someone else's record.
 *
 * Only runs when the provider's profile_sync_mode is 'always'. 'initial' is
 * handled at creation time (oidcCreateAnalyst / the requester INSERT), and
 * 'never' does nothing.
 *
 * @param string $table 'analysts' or 'users' - never user input
 */
function oidcSyncProfile(PDO $conn, array $provider, string $table, array $record, array $claims, string $name): void {
    if (($provider['profile_sync_mode'] ?? 'never') !== 'always') return;

    if ($table === 'analysts') {
        $nameCol = 'full_name';
        $cols    = ['job_title', 'department', 'phone', 'mobile'];            // analysts has no office column
        $stamp   = ', last_modified_datetime = UTC_TIMESTAMP()';
    } elseif ($table === 'users') {
        $nameCol = 'display_name';
        $cols    = ['job_title', 'department', 'office', 'phone', 'mobile'];
        $stamp   = '';
    } else {
        return;
    }

    $attrs   = oidcExtractProfileAttributes($claims);
    $updates = [];
    $params  = [];

    // A requester's own preferred_name wins over the directory's display name.
    $nameLocked = ($table === 'users' && !empty($record['preferred_name']));
    if ($name !== '' && !$nameLocked && ($record[$nameCol] ?? null) !== $name) {
        $updates[] = "$nameCol = ?";
        $params[]  = $name;
    }
    foreach ($cols as $col) {
        if (!empty($attrs[$col]) && ($record[$col] ?? null) !== $attrs[$col]) {
            $updates[] = "$col = ?";
            $params[]  = $attrs[$col];
        }
    }
    if (!$updates) return;

    $params[] = (int)$record['id'];
    $conn->prepare("UPDATE $table SET " . implode(', ', $updates) . "$stamp WHERE id = ?")
         ->execute($params);
}

Step 2 - delete all four inline copies.

Step 3 - call the helper once per side, just before the success block, after all three paths (linked, email match, JIT) have finished and passed their checks.

Analyst side, just above // --- Success: set the session directly (SSO bypasses local MFA) ---:

    // Every path above has now passed its access checks, so it is safe to write.
    oidcSyncProfile($conn, $provider, 'analysts', $analyst, $claims, $name);
    $analyst = oidcLoadAnalyst($conn, $analystId);   // pick up any new full_name for the session

    // --- Success: set the session directly (SSO bypasses local MFA) ---

Requester side, inside completeSelfServiceSso(), just above // --- Success: set the self-service session (SSO bypasses local TOTP) ---:

    oidcSyncProfile($conn, $provider, 'users', $user, $claims, $name);
    $user = ssLoadUser($conn, $userId);

    // --- Success: set the self-service session (SSO bypasses local TOTP) ---

For a brand-new JIT account the helper finds nothing to change (the INSERT already wrote the values), so calling it on every path is harmless and keeps the code to one call site each.

Files: includes/users.php (portalProfileAccess()), api/myaccount/get_signatures.php, api/myaccount/save_profile.php

In the branch, portalProfileAccess() became:

$managed = ((int)($row['is_managed'] ?? 0) === 1)
        || (!empty($row['protocol']) && ($row['profile_sync_mode'] ?? 'always') === 'always');

!empty($row['protocol']) is true for LDAP and CardDAV providers too, not just OIDC. The profile sync in this branch only runs for OIDC, so:

• LDAP users get locked fields that nothing in this feature ever updates
• CardDAV write-back breaks. $managed now comes out true for a CardDAV user, which feeds into $locked, and the carefully balanced write-back logic below it (read the 🔴 comments there) stops letting people edit.

It also locks through userDirectoryOwnedFields(), which for a non-CardDAV protocol returns the full USER_DIRECTORY_OWNED list, including employee_id and manager_id. OIDC sync never writes those, so they'd be locked for no reason.

Leave $managed exactly as it was. It means "a directory import owns this record", and other code relies on that meaning. Add a separate OIDC lock alongside it:

    $fields  = portalProfileEditableFields($conn);
    $managed = (int)($row['is_managed'] ?? 0) === 1;          // unchanged
    $isCardDav = strtolower((string)($row['protocol'] ?? '')) === 'carddav';

    // ... $addressBook and $locked exactly as before ...

    // SSO profile sync (discussion #155): an OIDC provider set to 'always'
    // rewrites these on every sign-in, so an edit here would be undone at the
    // next one. Only the five it actually writes - never employee_id or
    // manager_id, which OIDC sync does not touch.
    $oidcSynced = strtolower((string)($row['protocol'] ?? '')) === 'oidc'
               && ($row['profile_sync_mode'] ?? 'never') === 'always';
    if ($oidcSynced) {
        $locked = array_values(array_unique(array_merge(
            $locked,
            array_intersect($fields, USER_CARDDAV_OWNED)   // the same five: job_title, department, office, phone, mobile
        )));
    }

(Or add a constant USER_OIDC_SYNCED with the same five fields beside USER_CARDDAV_OWNED if you prefer the name to say what it is. Either is fine.)

The analyst side has the same issue. In both api/myaccount/get_signatures.php and api/myaccount/save_profile.php, the check should be:

$oidcSynced = strtolower((string)($row['protocol'] ?? '')) === 'oidc'
           && ($row['profile_sync_mode'] ?? 'never') === 'always';

api/myaccount/save_profile.php currently only selects auth_provider_id and profile_sync_mode, so add p.protocol to its SELECT.

Files: includes/oidc.php (oidcFetchUserInfo()), api/auth/oidc_callback.php (branch line ~100)

Three small things.

a) Skip the call when sync is never. It's an extra HTTP round trip on every sign-in, and its result is only used for profile attributes:

    if (($provider['profile_sync_mode'] ?? 'never') !== 'never'
        && !empty($disco['userinfo_endpoint']) && !empty($tokens['access_token'])) {
        $userInfo = oidcFetchUserInfo($disco['userinfo_endpoint'], $tokens['access_token']);
        // ...

b) Check that userinfo is about the same person. The OpenID Connect spec (Core 1.0, §5.3.2) says the sub in the userinfo response must be checked against the ID token's sub, and the response thrown away if they differ:

        if (!empty($userInfo) && isset($userInfo['sub']) && hash_equals((string)$claims['sub'], (string)$userInfo['sub'])) {
            $claims = array_merge($userInfo, $claims);   // ID token still wins
        }

c) Don't follow redirects while carrying a bearer token. Remove CURLOPT_FOLLOWLOCATION => true from oidcFetchUserInfo(). A userinfo endpoint comes from the provider's discovery document and has no reason to redirect.

A note for the help page: Microsoft Entra ID's userinfo endpoint only returns the basic profile (name, email and so on), not job title or department. For Entra, those have to come in the ID token as optional claims. The help text in system/help/sso.php already goes in this direction; it's worth being explicit that for Entra, userinfo won't fill the gap.

Files: auth/login.php, lang/en/auth.php, lang/en/self-service.php, lang/en/system.php

Please revert the footer restyle in auth/login.php (the .login-links block and the CSS around it). Two reasons:

• The layout was deliberate. The portal link sits below a rule so it reads as "you're on the wrong page" rather than as another way to sign in. The comment that explained this (discussion #82) was removed with it.
• The branch moves Forgot password? outside the if it lived in. It now shows even when local sign-in is switched off, where there's no password to forget.

Keep the three lines that clear the pending fallback. They're needed:

if (isset($_GET['cancel_sso'])) {
    unset($_SESSION['sso_pending_portal'], $_SESSION['sso_portal_csrf']);
}

FreeITSM is translated into 20+ languages. The translations are keyed by the English string's key, so when the English text for an existing key changes, every other language quietly carries on showing the old meaning. Please revert these to what they were:

When the meaning genuinely changes, use a new key. system.sso.cb_autocreate and cb_autocreate_desc used to mean "create an analyst" and now mean "create a portal requester". Leave the old two keys unused and add new ones, for example:

'cb_autocreate_users'      => 'Auto-create portal requesters on first login (JIT)',
'cb_autocreate_users_desc' => 'Create a self-service portal account automatically the first time an end user signs in via this provider.',

and point system/sso/index.php at the new keys. The field_default_modules wording change ("users" to "analysts") is a clarification with the same meaning, so that one is fine either way.

Adding brand-new keys is always fine. I translate them into the other languages in batches.

File: system/sso/index.php (branch lines ~800-830)

At the moment the modal hides the new fields only for CardDAV:

$('autoCreateAnalystsField').style.display = isCardDav ? 'none' : '';
$('analystFallbackField').style.display   = isCardDav ? 'none' : '';

But only the OIDC callback does anything with analyst_fallback_mode and profile_sync_mode. LDAP signs in through a password form, has no redirect to fall back from, and keeps its details up to date through its own directory sync. Showing the controls for LDAP offers settings that do nothing.

const isOidc = protocol === 'oidc';
$('autoCreateAnalystsField').style.display = isCardDav ? 'none' : '';   // LDAP uses this one too - keep it
$('analystFallbackField').style.display    = isOidc ? '' : 'none';
$('profileSyncModeField').style.display    = isOidc ? '' : 'none';

Do the same for the "Profile sync" pill in the providers table: show "Not applicable" (the existing system.sso.jit_na string) for anything that isn't OIDC.

Files: includes/waffle-menu.php, self-service/includes/user-menu.php

The branch hides Change password, MFA and Trusted device for anyone whose current session came through SSO. It's a reasonable idea, but it isn't mentioned in the discussion, and it has edge cases I'd like to think through separately:

• An admin who has a local password and an SSO link would lose the Change password option just because of how they signed in today.
• An account flagged must_change_password (see the long comment in api/auth/oidc_callback.php about why SSO still honours it) needs to be able to change the password.

Please take these hunks out of this branch. If you'd like to take the idea further, a separate discussion would be the best place.

File: self-service/includes/user-menu.php

The link is a good addition. One change: the analyst side's link to the portal (#81, in includes/waffle-menu.php) builds its URL from BASE_URL, and FreeITSM avoids relative links because pages can be served at "pretty" URLs deeper than their file. Match it:

<button class="ss-menu-item" onclick="window.open('<?php echo BASE_URL; ?>', '_blank', 'noopener');">

The rest of the check (an active analyst with the same email) mirrors $__portalAccount nicely. It only shows a link and doesn't sign anyone in, so matching on email is fine.

File: system/preferences/index.php

.pref-managed-note uses var(--accent-subtle, ...), which isn't defined in assets/css/theme.css, so the fallback is always used. The other variables it uses (--sys-accent, --surface-2, --text-muted) all exist. Either use one of those, or keep the rgba() value directly. Check it in dark mode either way.

For the coding assistant: run these before opening the PR.

# 1. Every changed PHP file still parses
git diff --name-only 801019c9 -- '*.php' | xargs -n1 php -l

# 2. No 'always' defaults left for profile sync
git grep -n "profile_sync_mode" | grep "always"     # should only show the ENUM list and the option/label

# 3. The four inline sync blocks are gone - only the helper writes these
git grep -n "UPDATE analysts SET \" . implode\|UPDATE users SET \" . implode" api/auth/oidc_callback.php   # expect none
git grep -n "oidcSyncProfile(" api/auth/oidc_callback.php                                                # expect 1 definition + 2 calls

# 4. No existing English wording changed
git diff 801019c9 -- lang/en/ | grep "^-[^-]"   # every removed line should be a key you meant to retire, not a reworded one

And by hand:

1. Upgrade test: start from a 2.7.1 database with a provider that has auto-create on, and run DB Verify. Analyst JIT should still work exactly as before.
2. Fresh install: a new provider should default to sync never and fallback confirm.
3. Walk through the table in section 3 and the one in section 4.
4. CardDAV with write-back - make sure My Account still lets people edit.

Push the changes to the same branch and open the PR from it as it is. I'll handle bringing it up to date with 2.8.0, the changelog, the translations and the release notes. None of that is expected from a contributor.

If anything here doesn't make sense, or you disagree with a call, say so on discussion #155. Several of these are judgement calls, and I'd rather hear the argument than have it quietly changed.

Thank you again, Sandy.