Repository navigation
OF-3322: Move SCRAM credential storage into a dedicated ofUserScram table - #3415
Merged
guusdk merged 10 commits intoJul 3, 2026
Merged
Conversation
âĶable Openfire stores SCRAM credentials (storedKey, serverKey, salt, iterations) as columns on the ofUser table. This hard-codes the assumption that a user has exactly one SCRAM credential (which is SCRAM-SHA-1). Supporting additional mechanisms (SCRAM-SHA-256, SCRAM-SHA-512) requires storing more than one credential per user. This change relocates SCRAM credentials to a new ofUserScram table keyed by (`username`, `mechanism`), without adding any new mechanism. Behavior is intended to be identical to before: SCRAM-SHA-1 remains the only mechanism written or read, and credentials are still regenerated from a stored plaintext/encrypted password when absent. This is a storage refactor only; it is a precursor to a follow-up change that adds the additional SCRAM mechanisms, kept separate so that the schema migration and the new-mechanism logic can be reviewed independently. This commit introduces a new ofUserScram table added to all base install scripts, and a two-step migration: 1. upgrade script 39 creates `ofUserScram` and copies existing SHA-1 credentials from `ofUser` 2. upgrade script 40 drops the old columns from ofUser. `User`'s `getSalt`/`getStoredKey`/`getServerKey`/`getIterations` accessors (and their setters) expose only the single SCRAM-SHA-1 credential and have no role in the per-mechanism model. They are marked `@Deprecated` and retained for backwards compatibility with third-party integrations. `AuthProvider`, `AuthMultiProvider` and `AuthFactory` gain mechanism-aware SCRAM accessors. the existing single-argument methods are retained and delegate to SCRAM-SHA-1, and marked as `@Deprecated`.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ð WalkthroughWalkthroughThis PR moves SCRAM credential storage out of Possibly related PRs
Suggested reviewers: ðĨ Pre-merge checks | â 3â Passed checks (3 passed)
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
There was a problem hiding this comment.
Pull request overview
This PR implements OF-3322 by refactoring SCRAM credential storage out of ofUser and into a new, mechanism-keyed ofUserScram table (username, mechanism), while keeping runtime behavior limited to SCRAM-SHA-1 (for now) and introducing mechanism-aware SCRAM accessors in the authentication APIs.
Changes:
- Introduces
ofUserScramacross base install scripts and adds DB upgrade steps 39 (create/copy) and 40 (drop old columns). - Updates
DefaultUserProvider/DefaultAuthProviderto load and persist SCRAM material viaofUserScramand adds mechanism-aware SCRAM API surface area. - Adds/updates tests to validate SCRAM storage loading and mechanism-aware API usage.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| xmppserver/src/test/java/org/jivesoftware/openfire/user/DefaultUserProviderScramStorageTest.java | New test verifying DefaultUserProvider.loadUser reads SCRAM from ofUserScram. |
| xmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerTest.java | Updates mocks to use mechanism-aware AuthFactory SCRAM accessors. |
| xmppserver/src/test/java/org/jivesoftware/openfire/sasl/AbstractScramCredentialObfuscationTest.java | Updates test setup to use mechanism-aware SCRAM salt retrieval. |
| xmppserver/src/test/java/org/jivesoftware/openfire/auth/DefaultAuthProviderScramStorageTest.java | New test verifying DefaultAuthProvider consults ofUserScram for SCRAM verification. |
| xmppserver/src/main/java/org/jivesoftware/openfire/user/User.java | Deprecates legacy SCRAM-SHA-1 accessors on User (backwards compatibility). |
| xmppserver/src/main/java/org/jivesoftware/openfire/user/DefaultUserProvider.java | Loads descriptive fields from ofUser and SCRAM-SHA-1 from ofUserScram. |
| xmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServer.java | Adds MECHANISM_NAME constant for consistent SCRAM-SHA-1 naming. |
| xmppserver/src/main/java/org/jivesoftware/openfire/auth/ScramCredentialData.java | New value type for per-user, per-mechanism SCRAM credential data. |
| xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.java | Moves SCRAM persistence/loading to ofUserScram; adds mechanism-aware getScramCredential. |
| xmppserver/src/main/java/org/jivesoftware/openfire/auth/AuthProvider.java | Adds default mechanism-aware SCRAM accessors; deprecates legacy ones. |
| xmppserver/src/main/java/org/jivesoftware/openfire/auth/AuthMultiProvider.java | Delegates mechanism-aware SCRAM accessors to mapped providers; deprecates legacy ones. |
| xmppserver/src/main/java/org/jivesoftware/openfire/auth/AuthFactory.java | Adds mechanism-aware SCRAM accessors and delegates legacy methods to SCRAM-SHA-1. |
| xmppserver/src/main/java/org/jivesoftware/database/SchemaManager.java | Bumps schema version to 40. |
| documentation/database-guide.html | Documents new ofUserScram table. |
| distribution/src/database/upgrade/39/openfire_sybase.sql | Upgrade step 39 (Sybase): create/copy SCRAM credentials into ofUserScram. |
| distribution/src/database/upgrade/39/openfire_sqlserver.sql | Upgrade step 39 (SQL Server): create/copy SCRAM credentials into ofUserScram. |
| distribution/src/database/upgrade/39/openfire_postgresql.sql | Upgrade step 39 (PostgreSQL): create/copy SCRAM credentials into ofUserScram. |
| distribution/src/database/upgrade/39/openfire_oracle.sql | Upgrade step 39 (Oracle): create/copy SCRAM credentials into ofUserScram. |
| distribution/src/database/upgrade/39/openfire_mysql.sql | Upgrade step 39 (MySQL): create/copy SCRAM credentials into ofUserScram. |
| distribution/src/database/upgrade/39/openfire_mariadb.sql | Upgrade step 39 (MariaDB): create/copy SCRAM credentials into ofUserScram. |
| distribution/src/database/upgrade/39/openfire_hsqldb.sql | Upgrade step 39 (HSQLDB): create/copy SCRAM credentials into ofUserScram. |
| distribution/src/database/upgrade/39/openfire_firebird.sql | Upgrade step 39 (Firebird): create/copy SCRAM credentials into ofUserScram. |
| distribution/src/database/upgrade/39/openfire_db2.sql | Upgrade step 39 (DB2): create/copy SCRAM credentials into ofUserScram. |
| distribution/src/database/upgrade/39/openfire_cockroachdb.sql | Upgrade step 39 (CockroachDB): create/copy SCRAM credentials into ofUserScram. |
| distribution/src/database/upgrade/40/openfire_sybase.sql | Upgrade step 40 (Sybase): drop legacy SCRAM columns from ofUser. |
| distribution/src/database/upgrade/40/openfire_sqlserver.sql | Upgrade step 40 (SQL Server): drop legacy SCRAM columns from ofUser. |
| distribution/src/database/upgrade/40/openfire_postgresql.sql | Upgrade step 40 (PostgreSQL): drop legacy SCRAM columns from ofUser. |
| distribution/src/database/upgrade/40/openfire_oracle.sql | Upgrade step 40 (Oracle): drop legacy SCRAM columns from ofUser. |
| distribution/src/database/upgrade/40/openfire_mysql.sql | Upgrade step 40 (MySQL): drop legacy SCRAM columns from ofUser. |
| distribution/src/database/upgrade/40/openfire_mariadb.sql | Upgrade step 40 (MariaDB): drop legacy SCRAM columns from ofUser. |
| distribution/src/database/upgrade/40/openfire_hsqldb.sql | Upgrade step 40 (HSQLDB): drop legacy SCRAM columns from ofUser. |
| distribution/src/database/upgrade/40/openfire_firebird.sql | Upgrade step 40 (Firebird): drop legacy SCRAM columns from ofUser. |
| distribution/src/database/upgrade/40/openfire_db2.sql | Upgrade step 40 (DB2): drop legacy SCRAM columns from ofUser. |
| distribution/src/database/upgrade/40/openfire_cockroachdb.sql | Upgrade step 40 (CockroachDB): drop legacy SCRAM columns from ofUser. |
| distribution/src/database/openfire_sybase.sql | Base schema: remove SCRAM columns from ofUser, add ofUserScram, bump version. |
| distribution/src/database/openfire_sqlserver.sql | Base schema: remove SCRAM columns from ofUser, add ofUserScram, bump version. |
| distribution/src/database/openfire_postgresql.sql | Base schema: remove SCRAM columns from ofUser, add ofUserScram, bump version. |
| distribution/src/database/openfire_oracle.sql | Base schema: remove SCRAM columns from ofUser, add ofUserScram, bump version. |
| distribution/src/database/openfire_mysql.sql | Base schema: remove SCRAM columns from ofUser, add ofUserScram, bump version. |
| distribution/src/database/openfire_mariadb.sql | Base schema: remove SCRAM columns from ofUser, add ofUserScram, bump version. |
| distribution/src/database/openfire_hsqldb.sql | Base schema: remove SCRAM columns from ofUser, add ofUserScram, bump version. |
| distribution/src/database/openfire_firebird.sql | Base schema: remove SCRAM columns from ofUser, add ofUserScram, bump version. |
| distribution/src/database/openfire_db2.sql | Base schema: remove SCRAM columns from ofUser, add ofUserScram, bump version. |
| distribution/src/database/openfire_cockroachdb.sql | Base schema: remove SCRAM columns from ofUser, add ofUserScram, bump version. |
ðĄ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+20
to
+22
| WHERE storedKey IS NOT NULL | ||
| AND serverKey IS NOT NULL | ||
| AND salt IS NOT NULL; |
Member
There was a problem hiding this comment.
I mean... sure? The server would be generating a lot of errors if iterations IS NULL but the other two aren't.
Member
Author
There was a problem hiding this comment.
I was debating if we'd be better off failing fast, but that'd mean we should fail on any scenario where not all of these valus are non-NULL. In the end I've just added the missing NULL check to each of the scripts.
![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
ð§đ Nitpick comments (4)
distribution/src/database/upgrade/39/openfire_mysql.sql (1)
7-15: ð Maintainability & Code Quality | ðĩ Trivial | ⥠Quick winUnnamed primary key breaks naming convention.
Every sibling script (Oracle, SQL Server v39, and the base
ofUserPKs) names the constraint<table>_pk. This MySQL script leaves it unnamed, which will get an auto-generated name and diverge if a future script needs to reference/drop it by name.âŧïļ Proposed fix
- PRIMARY KEY (username, mechanism) + CONSTRAINT ofUserScram_pk PRIMARY KEY (username, mechanism)ðĪ 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 `@distribution/src/database/upgrade/39/openfire_mysql.sql` around lines 7 - 15, The primary key on ofUserScram is unnamed, which breaks the existing `<table>_pk` convention used by the sibling upgrade scripts and base table constraints. Update the CREATE TABLE statement in the openfire_mysql.sql upgrade for ofUserScram to explicitly name the PRIMARY KEY constraint using the same naming pattern as the other scripts, so future schema changes can reference it consistently.distribution/src/database/upgrade/40/openfire_oracle.sql (1)
6-9: ð Performance & Scalability | ðĩ Trivial | ⥠Quick winCombine the 4 column drops into one statement.
Oracle supports dropping multiple columns in a single
ALTER TABLE ... DROP (...)statement, avoiding four separate full-table passes for constraint/data validation onofUser.⥠Proposed fix
-ALTER TABLE ofUser DROP COLUMN storedKey; -ALTER TABLE ofUser DROP COLUMN serverKey; -ALTER TABLE ofUser DROP COLUMN salt; -ALTER TABLE ofUser DROP COLUMN iterations; +ALTER TABLE ofUser DROP (storedKey, serverKey, salt, iterations);ðĪ 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 `@distribution/src/database/upgrade/40/openfire_oracle.sql` around lines 6 - 9, The four separate ALTER TABLE drops on ofUser should be combined into a single Oracle DROP statement to avoid multiple table passes. Update the openfire_oracle.sql migration so the storedKey, serverKey, salt, and iterations removals are handled together in one ALTER TABLE ... DROP (...) statement, using the existing ofUser upgrade block as the place to make the change.distribution/src/database/upgrade/40/openfire_mysql.sql (1)
6-9: ð Performance & Scalability | ðĩ Trivial | ⥠Quick winCombine the 4
DROP COLUMNstatements into oneALTER TABLE.Four separate statements mean up to four passes over
ofUseron MySQL versions/storage engines where DROP COLUMN isn't done via the newer instant metadata-only path; "ALGORITHM=INSTANT, is not supported for DROP COLUMN and it would still need a table rebuild" prior to the multi-column instant support added in 8.0.29. A single combined statement reduces this to one pass.⥠Proposed fix
-ALTER TABLE ofUser DROP COLUMN storedKey; -ALTER TABLE ofUser DROP COLUMN serverKey; -ALTER TABLE ofUser DROP COLUMN salt; -ALTER TABLE ofUser DROP COLUMN iterations; +ALTER TABLE ofUser DROP COLUMN storedKey, DROP COLUMN serverKey, DROP COLUMN salt, DROP COLUMN iterations;Please confirm the minimum supported MySQL/MariaDB version for Openfire to verify this syntax is compatible across all supported targets.
ðĪ 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 `@distribution/src/database/upgrade/40/openfire_mysql.sql` around lines 6 - 9, Combine the four separate ALTER TABLE drops on ofUser into a single ALTER TABLE statement that removes storedKey, serverKey, salt, and iterations together. Update the openfire_mysql.sql migration in the upgrade/40 script so the ofUser table is altered once instead of four times, keeping the same column names but using one multi-drop ALTER TABLE that is compatible with the minimum supported MySQL/MariaDB version.xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.java (1)
61-62: ð Maintainability & Code Quality | ðĩ Trivial | âïļ Poor tradeoffDuplicated SCRAM-credential SQL with divergent column order across files.
LOAD_SCRAM_CREDENTIALhere selectsiterations,salt,storedKey,serverKey, whileDefaultUserProvider.LOAD_SCRAM_CREDENTIALselectssalt, serverKey, storedKey, iterationsfor the same table. Each is internally consistent with its own result-mapping code today, but the duplication with differing column orders is a maintenance hazard: a future edit to one query's column list without updating the matchingrs.get*(index)calls (or the sibling query) could silently corrupt credential data.Consider extracting a shared helper/DAO for reading
ofUserScramrows into aScramCredentialData, used by bothDefaultAuthProviderandDefaultUserProvider.ðĪ 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 `@xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.java` around lines 61 - 62, The SCRAM credential query is duplicated with a different column order, which makes the index-based result mapping in DefaultAuthProvider and DefaultUserProvider fragile. Update the LOAD_SCRAM_CREDENTIAL usage in DefaultAuthProvider and the matching logic in DefaultUserProvider so both rely on a single shared helper/DAO for reading ofUserScram rows into ScramCredentialData, or otherwise ensure one canonical column order is used everywhere with matching rs.get* indexes.
ðĪ 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 `@distribution/src/database/upgrade/39/openfire_cockroachdb.sql`:
- Around line 13-22: The SCRAM backfill into ofUserScram only filters out rows
with missing keys and salt, but it can still select legacy rows where iterations
is NULL and fail against the NOT NULL target. Update the INSERT ... SELECT in
the SCRAM migration to also exclude rows with missing iterations, so only fully
populated ofUser records are copied into ofUserScram.
In `@distribution/src/database/upgrade/39/openfire_db2.sql`:
- Around line 13-22: The SCRAM backfill in the database upgrade script does not
filter out rows with a missing iterations value, even though
ofUserScram.iterations is NOT NULL. Update the INSERT ... SELECT for ofUserScram
to also exclude legacy rows where iterations IS NULL, alongside the existing
storedKey/serverKey/salt checks, so incomplete SCRAM data is skipped during the
upgrade.
In `@distribution/src/database/upgrade/39/openfire_firebird.sql`:
- Around line 14-23: The SCRAM backfill in the ofUserScram insert only filters
on storedKey, serverKey, and salt, so legacy rows with a NULL iterations value
can still violate the NOT NULL target. Update the INSERT...SELECT for
ofUserScram to also exclude rows where iterations is NULL, keeping the filter
aligned with the NOT NULL columns. Locate the change in the SCRAM upgrade SQL
near the ofUserScram backfill statement.
In `@distribution/src/database/upgrade/39/openfire_hsqldb.sql`:
- Around line 13-22: Filter out incomplete legacy SCRAM rows before the backfill
into ofUserScram. In the INSERT ... SELECT for the ofUserScram upgrade script,
add a condition to exclude rows where iterations is NULL alongside the existing
storedKey, serverKey, and salt checks. This keeps the upgrade from failing on
partially populated ofUser data while still copying only complete SCRAM records.
In `@distribution/src/database/upgrade/39/openfire_mariadb.sql`:
- Around line 13-22: The version-39 SCRAM backfill in the ofUserScram insert
only filters on storedKey, serverKey, and salt, so rows with iterations IS NULL
can still fail the NOT NULL target. Update the INSERT ... SELECT in the
openfire_mariadb.sql upgrade script, and apply the same predicate change to the
matching version-39 backend scripts, so the legacy ofUser rows are only copied
when iterations is also present.
In `@distribution/src/database/upgrade/39/openfire_postgresql.sql`:
- Around line 17-22: The backfill in the PostgreSQL upgrade script must also
exclude legacy rows with NULL iterations before inserting into the new mandatory
column. Update the INSERT ... SELECT that populates ofUserScram so it filters on
iterations in addition to storedKey, serverKey, and salt, using the same
upgrade/backfill logic around ofUserScram and ofUser.
In `@distribution/src/database/upgrade/39/openfire_sqlserver.sql`:
- Around line 17-22: The SCRAM backfill in the v39 upgrade script can fail
because it inserts into ofUserScram.iterations, which is NOT NULL, without
filtering out legacy ofUser rows where iterations is missing. Update the
backfill in the ofUserScram insert to also require iterations IS NOT NULL, and
apply the same guard in the other v39 backfill scripts that populate
ofUserScram. Use the ofUserScram INSERT...SELECT block as the reference point
when making the change.
In `@distribution/src/database/upgrade/39/openfire_sybase.sql`:
- Around line 13-23: The backfill into ofUserScram should also exclude legacy
rows where iterations is null, since ofUserScram.iterations is NOT NULL and the
current INSERT from ofUser only filters storedKey, serverKey, and salt. Update
the INSERT...SELECT in openfire_sybase.sql to add an iterations IS NOT NULL ØīØąØ·
alongside the existing SCRAM field checks so upgrade 39 skips incomplete rows
and allows credential regeneration to handle them instead of failing.
In `@distribution/src/database/upgrade/40/openfire_db2.sql`:
- Around line 6-9: Add a Db2 REORG step after the ofUser column drops in the
openfire_db2.sql upgrade script; the ALTER TABLE DROP COLUMN statements for
storedKey, serverKey, salt, and iterations leave the table in reorg-pending
state. Update the database upgrade flow so that, after these drops complete,
ofUser is explicitly reorganized using the appropriate Db2 REORG command to make
the table available again.
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.java`:
- Around line 87-92: `getScramCredential` is regenerating SCRAM credentials too
early, which can change the salt mid-handshake and trigger intermittent auth
failures. Update `DefaultAuthProvider.getScramCredential` to first try
`loadScramCredential` and return an existing credential immediately; only call
`getUserInfo` (and any regeneration path it triggers) when no SCRAM credential
is present. Keep the fix consistent with `checkPassword` and the
`loadScramCredential`/`getUserInfo` flow so `setPassword` is not invoked unless
a credential truly needs to be created.
---
Nitpick comments:
In `@distribution/src/database/upgrade/39/openfire_mysql.sql`:
- Around line 7-15: The primary key on ofUserScram is unnamed, which breaks the
existing `<table>_pk` convention used by the sibling upgrade scripts and base
table constraints. Update the CREATE TABLE statement in the openfire_mysql.sql
upgrade for ofUserScram to explicitly name the PRIMARY KEY constraint using the
same naming pattern as the other scripts, so future schema changes can reference
it consistently.
In `@distribution/src/database/upgrade/40/openfire_mysql.sql`:
- Around line 6-9: Combine the four separate ALTER TABLE drops on ofUser into a
single ALTER TABLE statement that removes storedKey, serverKey, salt, and
iterations together. Update the openfire_mysql.sql migration in the upgrade/40
script so the ofUser table is altered once instead of four times, keeping the
same column names but using one multi-drop ALTER TABLE that is compatible with
the minimum supported MySQL/MariaDB version.
In `@distribution/src/database/upgrade/40/openfire_oracle.sql`:
- Around line 6-9: The four separate ALTER TABLE drops on ofUser should be
combined into a single Oracle DROP statement to avoid multiple table passes.
Update the openfire_oracle.sql migration so the storedKey, serverKey, salt, and
iterations removals are handled together in one ALTER TABLE ... DROP (...)
statement, using the existing ofUser upgrade block as the place to make the
change.
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.java`:
- Around line 61-62: The SCRAM credential query is duplicated with a different
column order, which makes the index-based result mapping in DefaultAuthProvider
and DefaultUserProvider fragile. Update the LOAD_SCRAM_CREDENTIAL usage in
DefaultAuthProvider and the matching logic in DefaultUserProvider so both rely
on a single shared helper/DAO for reading ofUserScram rows into
ScramCredentialData, or otherwise ensure one canonical column order is used
everywhere with matching rs.get* indexes.
ðŠ 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 921be694-8bee-4c1a-8ed5-c30cb9939dd5
ð Files selected for processing (44)
distribution/src/database/openfire_cockroachdb.sqldistribution/src/database/openfire_db2.sqldistribution/src/database/openfire_firebird.sqldistribution/src/database/openfire_hsqldb.sqldistribution/src/database/openfire_mariadb.sqldistribution/src/database/openfire_mysql.sqldistribution/src/database/openfire_oracle.sqldistribution/src/database/openfire_postgresql.sqldistribution/src/database/openfire_sqlserver.sqldistribution/src/database/openfire_sybase.sqldistribution/src/database/upgrade/39/openfire_cockroachdb.sqldistribution/src/database/upgrade/39/openfire_db2.sqldistribution/src/database/upgrade/39/openfire_firebird.sqldistribution/src/database/upgrade/39/openfire_hsqldb.sqldistribution/src/database/upgrade/39/openfire_mariadb.sqldistribution/src/database/upgrade/39/openfire_mysql.sqldistribution/src/database/upgrade/39/openfire_oracle.sqldistribution/src/database/upgrade/39/openfire_postgresql.sqldistribution/src/database/upgrade/39/openfire_sqlserver.sqldistribution/src/database/upgrade/39/openfire_sybase.sqldistribution/src/database/upgrade/40/openfire_cockroachdb.sqldistribution/src/database/upgrade/40/openfire_db2.sqldistribution/src/database/upgrade/40/openfire_firebird.sqldistribution/src/database/upgrade/40/openfire_hsqldb.sqldistribution/src/database/upgrade/40/openfire_mariadb.sqldistribution/src/database/upgrade/40/openfire_mysql.sqldistribution/src/database/upgrade/40/openfire_oracle.sqldistribution/src/database/upgrade/40/openfire_postgresql.sqldistribution/src/database/upgrade/40/openfire_sqlserver.sqldistribution/src/database/upgrade/40/openfire_sybase.sqldocumentation/database-guide.htmlxmppserver/src/main/java/org/jivesoftware/database/SchemaManager.javaxmppserver/src/main/java/org/jivesoftware/openfire/auth/AuthFactory.javaxmppserver/src/main/java/org/jivesoftware/openfire/auth/AuthMultiProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/auth/AuthProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/auth/ScramCredentialData.javaxmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServer.javaxmppserver/src/main/java/org/jivesoftware/openfire/user/DefaultUserProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/user/User.javaxmppserver/src/test/java/org/jivesoftware/openfire/auth/DefaultAuthProviderScramStorageTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/AbstractScramCredentialObfuscationTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/user/DefaultUserProviderScramStorageTest.java
AuthFactory's SCRAM accessors are now mechanism-aware; the server still used the deprecated single-arg forms. Switch the five lookups to the two-arg form.
Also pin the test's AuthFactory mock to eq("SCRAM-SHA-1") so a regression to a PLUS-suffixed lookup fails rather than being masked.
dwd
approved these changes
Jul 2, 2026
dwd
left a comment
dwd
left a comment
Member
There was a problem hiding this comment.
Might as well fix a few things if you've time, but I wouldn't block merging on them.
Comment on lines
+20
to
+22
| WHERE storedKey IS NOT NULL | ||
| AND serverKey IS NOT NULL | ||
| AND salt IS NOT NULL; |
Member
There was a problem hiding this comment.
I mean... sure? The server would be generating a lot of errors if iterations IS NULL but the other two aren't.
The SCRAM fallback in checkPassword() previously obtained credentials through getScramCredential(), which re-queried the user and opened additional database connections even though checkPassword() already held an open connection and had just loaded the ofUser row. On a busy server this roughly doubled the connections needed per authentication.
This commit loads the password and SCRAM columns together in a single statement, reading all credential material from the one result set on the existing
connection.
Also harden the SCRAM derivation: an empty/invalid testPassword can cause createSaltedPassword() to throw IllegalArgumentException ("Empty key") from the HMAC init. Catch IllegalArgumentException alongside the existing SaslException/NoSuchAlgorithmException so such input fails authentication cleanly instead of escaping as an unchecked exception.
There was a problem hiding this comment.
ð§đ Nitpick comments (1)
xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.java (1)
484-518: ðĐš Stability & Availability | ðĩ Trivial | ðĪ Low valueCheck-then-insert in
upsertScramCredentialis racy for concurrent first-time writes.Two concurrent
setPasswordcalls for a brand-new user can both observeTEST_SCRAM_CREDENTIALreturning no row and both take theINSERTbranch, causing a primary-key/unique violation on(username, mechanism)for the losing transaction. The transaction abort path handles it safely (surfaced asUserNotFoundException), so impact is low, but a dialect-native upsert or catching the constraint violation and retrying as an update would be more robust.ðĪ 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 `@xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.java` around lines 484 - 518, The check-then-insert flow in upsertScramCredential is race-prone for concurrent first-time writes, since two setPassword calls can both miss TEST_SCRAM_CREDENTIAL and then collide on INSERT. Replace the existence check plus separate UPDATE/INSERT branching with a single atomic, database-native upsert if possible, or otherwise handle the unique constraint failure and retry as UPDATE. Keep the fix localized to upsertScramCredential and its use of TEST_SCRAM_CREDENTIAL, UPDATE_SCRAM_CREDENTIAL, and INSERT_SCRAM_CREDENTIAL.
ðĪ 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.
Nitpick comments:
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.java`:
- Around line 484-518: The check-then-insert flow in upsertScramCredential is
race-prone for concurrent first-time writes, since two setPassword calls can
both miss TEST_SCRAM_CREDENTIAL and then collide on INSERT. Replace the
existence check plus separate UPDATE/INSERT branching with a single atomic,
database-native upsert if possible, or otherwise handle the unique constraint
failure and retry as UPDATE. Keep the fix localized to upsertScramCredential and
its use of TEST_SCRAM_CREDENTIAL, UPDATE_SCRAM_CREDENTIAL, and
INSERT_SCRAM_CREDENTIAL.
âđïļ Review info
âïļ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 063a6175-5116-4baa-b300-f858fa5ba008
ð Files selected for processing (2)
xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.javaxmppserver/src/test/java/org/jivesoftware/openfire/auth/DefaultAuthProviderScramStorageTest.java
upsertScramCredential() previously did SELECT-then-INSERT/UPDATE, which had a time-of-check/time-of-use race: a concurrent setPassword() could insert the (username, mechanism) row between the existence check and the INSERT, causing a primary-key violation that aborted setPassword(). Switch to UPDATE-first, falling back to INSERT when no row was affected. For a password change the row already exists, so this is also the common single-statement case. If the INSERT loses a race and hits a unique/PK violation, the UPDATE is retried once, since the row exists by then. Constraint-violation detection (isIntegrityConstraintViolation) combines the JDBC 4 SQLIntegrityConstraintViolationException subclass with an SQLState class-23 check, walking both the cause and getNextException() chains, to work across the databases Openfire supports.
The migration inserts into `ofUserScram.iterations` which is defined `NOT NULL`, but the INSERT ... SELECT only filters on storedKey/serverKey/salt. If an existing row has SCRAM columns populated but iterations is `NULL`, this upgrade will fail with a `NOT NULL` constraint violation. This commit adds an explicit `iterations IS NOT NULL` predicate.
SchemaManager executes upgrade scripts by concatenating lines until a line ends with ';' (see SchemaManager.executeSQLScript). The literal 'go' is therefore treated as part of a SQL statement and will cause the upgrade to fail. This script should not use 'go' batch separators. None of the other scripts do, either.
When the database contains incomplete SCRAM credentials, skip that row, rather than stop evaluating any other rows. This prevents scenarios where authentication fails even though credentials exist.
Normalize all SCRAM mechanism names for credentials obtained from the database. This prevents issues with comparing values if the database would contains non-normalized mechanism names (e.g. the `-PLUS` variant for a mechanism). This should not be put in the database, but protecting against this is cheap hardening.
`setPassword()` shouljd verify that the UPDATE on ofUser actually affected a row. If the username does not exist, this method should not silently succeed and still upsert a SCRAM row (creating an orphan record in ofUserScram).
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
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.
Openfire stores SCRAM credentials (
storedKey,serverKey,salt,iterations) as columns on theofUsertable. This hard-codes the assumption that a user has exactly one SCRAM credential (which is SCRAM-SHA-1). Supporting additional mechanisms (SCRAM-SHA-256, SCRAM-SHA-512) requires storing more than one credential per user.This change relocates SCRAM credentials to a new ofUserScram table keyed by (
username,mechanism), without adding any new mechanism. Behavior is intended to be identical to before: SCRAM-SHA-1 remains the only mechanism written or read, and credentials are still regenerated from a stored plaintext/encrypted password when absent. This is a storage refactor only; it is a precursor to a follow-up change that adds the additional SCRAM mechanisms, kept separate so that the schema migration and the new-mechanism logic can be reviewed independently.This commit introduces a new ofUserScram table added to all base install scripts, and a two-step migration:
ofUserScramand copies existing SHA-1 credentials fromofUserUser'sgetSalt/getStoredKey/getServerKey/getIterationsaccessors (and their setters) expose only the single SCRAM-SHA-1 credential and have no role in the per-mechanism model. They are marked@Deprecatedand retained for backwards compatibility with third-party integrations.AuthProvider,AuthMultiProviderandAuthFactorygain mechanism-aware SCRAM accessors. the existing single-argument methods are retained and delegate to SCRAM-SHA-1, and marked as@Deprecated.