Skip to content

# FIX: OIDC missing idp attribute and ensureLocalIdPMetadata method#2616

Merged
monkeyiq merged 1 commit intofilesender:development3from
victoritis:fix/oidc-idp-attribute
Mar 17, 2026
Merged

# FIX: OIDC missing idp attribute and ensureLocalIdPMetadata method#2616
monkeyiq merged 1 commit intofilesender:development3from
victoritis:fix/oidc-idp-attribute

Conversation

@victoritis
Copy link
Copy Markdown

@victoritis victoritis commented Mar 12, 2026

Related issue: #2569
Related PR (partial fix, only idp on development branch): #2600


The problem

When using native OIDC authentication (AuthSPOidc), creating a transfer causes:

  1. PHP Warning: Undefined array key 'idp' in Transfer.class.php (line 658)
  2. PHP Fatal Error: Call to undefined method AuthSPOidc::ensureLocalIdPMetadata() — if idp were set, Transfer.class.php calls this method which doesn't exist in the OIDC class

This happens because AuthSPOidc was added (July 2025) without implementing two things that SAML and Shibboleth already had:

  • Setting the idp attribute in attributes()
  • The ensureLocalIdPMetadata() static method

What was changed

1. classes/auth/AuthSPOidc.class.php

Added idp attribute (after the existing uid, email, name attributes):

$idpAttribute = Config::get('auth_sp_oidc_idp_attribute');
$idp = null;

if ($idpAttribute && isset($rawAttributes->$idpAttribute) && $rawAttributes->$idpAttribute) {
    $idp = trim((string)$rawAttributes->$idpAttribute);
}

if (!$idp) {
    $idp = Config::get('auth_sp_oidc_issuer');
}

$attributes['idp'] = $idp;

How it works:

  • By default, uses auth_sp_oidc_issuer (e.g. https://keycloak.example.com/realms/myrealm). This is always set because oidc.php throws an exception if it's missing.
  • If auth_sp_oidc_idp_attribute is configured (see below), reads the IdP from a claim in the OIDC userinfo response instead. This is useful for identity brokers (see "Optional: identity_provider claim" section below).

Added ensureLocalIdPMetadata() method:

public static function ensureLocalIdPMetadata($entityId, $idp, $force = false)
{
    $idp->saveIfChanged();
}
  • Transfer.class.php calls AuthSP::ensureLocalIdPMetadata() after creating a transfer to persist IdP metadata.
  • Without this method, OIDC transfers would crash with a fatal error.
  • The signature ($entityId, $idp, $force = false) matches AuthSPSaml (line 220) and AuthSPShibboleth (line 174).
  • The body calls $idp->saveIfChanged() which is a method of the IdP class (line 256).

2. classes/data/Transfer.class.php

Line 658 — defensive null coalescing:

// Before:
$entityId = $attrs['idp'];

// After:
$entityId = $attrs['idp'] ?? null;
  • If any authentication provider doesn't set the idp attribute, this prevents the PHP warning.
  • The existing if ($entityId) check on the next line already handles null gracefully (skips IdP storage).

3. includes/ConfigDefaults.php

Added new config default:

'auth_sp_oidc_idp_attribute' => null,
  • Registers the new configuration option with a null default (disabled).
  • Without this, Config::get('auth_sp_oidc_idp_attribute') could emit a PHP notice.
  • Placed alongside the existing OIDC defaults (uid_attribute, email_attribute, name_attribute, groups_claim).

Optional: identity_provider claim for broker setups

By default, auth_sp_oidc_idp_attribute is null and the issuer URL is used as the IdP identifier. This is correct for most setups (single OIDC provider).

However, if your OIDC provider acts as an identity broker (e.g. Keycloak federating Google, Azure AD, SAML IdPs, etc.), you may want each transfer to record which upstream IdP the user actually came from, not just "Keycloak".

In that case, the OIDC provider needs to expose a claim with the upstream IdP name. For example, Keycloak can be configured to include an identity_provider claim in the userinfo response:

{
    "sub": "abc123",
    "email": "user@example.com",
    "name": "User",
    "identity_provider": "google"
}

To enable this in FileSender, add to your config.php:

$config['auth_sp_oidc_idp_attribute'] = 'identity_provider';

The logic is:

  1. If auth_sp_oidc_idp_attribute is set AND the claim exists in the userinfo response, use that claim value (e.g. "google")
  2. Otherwise, fall back to auth_sp_oidc_issuer (e.g. "https://keycloak.example.com/realms/myrealm")

This is entirely optional. If you don't configure it, nothing changes — the issuer URL is used.


Difference with PR #2600

PR #2600 (merged into development on March 5, 2026) only adds one line:

$attributes['idp'] = Config::get('auth_sp_oidc_issuer');

But not merged on development3 and this fix goes further:

Feature PR #2600 This fix
idp = issuer Yes Yes
Optional broker claim (idp_attribute) No Yes
ensureLocalIdPMetadata() method No Yes
Transfer.class.php defensive ?? null No Yes
ConfigDefaults.php registration No Yes

Without ensureLocalIdPMetadata(), Transfer.class.php will still crash with a fatal error when creating transfers with OIDC authentication and the idp attribute is set.

Closes #2569

@monkeyiq monkeyiq merged commit 3496344 into filesender:development3 Mar 17, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants