Repository navigation
Refactor SCRAM-SHA-1 unit tests for reuse, readability, and future algorithm coverage - #3334
Merged
Merged
Conversation
This introduces a test fixture that defines stable, shared test data, canonical protocol constants, and repeated values with semantic meaning. Usage of constants from this new fixture avoids using hard-coded values in tests. This makes intent clearer (by referring to data by a meaningful name, rather than by a 'magic value').
This replaces repeated construction patterns and protocol-building patterns.
By replacing most invocations of `new ScramSha1SaslServer(isPlusMechanism, new HashMap<>(), new ChannelBindingProviderManager(), ScramSha1TestFixtures.SUPPORTED_MECHANISMS);` with a factory method (`newServer(isPlusMechanism)`), a lot of boilerplate code with low informational value has been reduced.
The client `initialMessage` (`"n,,n=" + USER + ",r=" + nonce`) was constructed manually in various variants. By extracting this to a factory method tests become more readable: `final byte[] initialMessage = createClientInitialMessage("n,,", USER, CLIENT_NONCE);` This improves readability a lot, because the SCRAM message construction becomes 'named'. Tests now focus on intent rather than string concatenation.
Similarly, the client finalMessage, which was also constructed with string concatenation in various places, has been extracted to a factory method (`createClientFinalMessage(channelBinding, serverNonce, proof)`). Its usage now becomes declarative (`createClientFinalMessage("biws", nonce, proof)`) rather than protocol-string spaghetti ('"c=biws,r=" + serverNonce + ",p=" + proof').
The 'first exchange' helper (`String doFirstExchange(server)`) has been improved. Instead of returning just the nonce from the first result, it now returns a structure that holds all data from that response.
The old code repeatedly did `Pattern.compile(...); Matcher ...; group(...);` which was getting repetitive. This has been replaced by another centralized parse method (`FirstExchangeResult#fromFirstServerResponse(firstServerResponse)`).
Tests used various mocks of `AuthFactory`, which each set up a partial factory, based on subsets of the same canonical data. These partials (`setupSaltAndIterations()` and `setupSaltAndIterationsAndKeys()`) have been replaced with one method that sets up everything: salt, iterations, password and keys. The new method name is `setupCanonicalAuthData()`. This encodes 'valid SCRAM credentials', avoids subtle divergence between tests, and reduces fragile setup duplication.
All in all, this does not introduce functional changes, but upgrades the code to be better self-describing, and easier to adapt to future use-cases.
|
No actionable comments were generated in the recent review. ð âđïļ Recent review infoâïļ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ð Files selected for processing (10)
ðĪ Files with no reviewable changes (2)
ð§ Files skipped from review as they are similar to previous changes (6)
ð WalkthroughWalkthroughThis PR introduces ScramSaslServer (abstract base) with timing-attack/user-enumeration-safe contracts and a byte-level GS2 header extractor, changes ScramSha1SaslServer to extend the new base and convert credential hooks to protected overrides, and consolidates tests by adding AbstractScramCredentialObfuscationTest and AbstractScramSaslServerTest plus FirstExchangeResult and ScramSha1TestFixtures. SCRAM-SHA-1 concrete tests were refactored to use the new abstract test bases and shared fixtures. Possibly related PRs
Suggested reviewers
ðĨ Pre-merge checks | â 3â Passed checks (3 passed)
âïļ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the SCRAM-SHA-1 unit test suite to reduce duplication and to enable reuse by future SCRAM mechanisms (e.g. SCRAM-SHA-256). It introduces shared test fixtures, helper factory methods, parsing utilities, and abstract base test classes, plus a new abstract ScramSaslServer parent class so that test polymorphism is possible. No protocol behavior is changed.
Changes:
- Adds a new abstract
ScramSaslServersuperclass and pullsgetOrFakeServerKey,getOrFakeStoredKey,getOrCreateSalt, andextractRawGS2Headerinto it. - Centralizes shared SCRAM test logic in
AbstractScramSaslServerTestandAbstractScramCredentialObfuscationTest, plus the parsing helperFirstExchangeResultand constants inScramSha1TestFixtures. - Replaces inline test setup/helpers in the SHA-1 tests with overrides of the new base classes; deletes the standalone
ScramSha1SaslServerSaltTestandScramSha1SaslServerFakeKeyTestin favor ofScramSha1CredentialObfuscationTest.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| xmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSaslServer.java | New abstract base for SCRAM SASL servers; hosts shared GS2 helper and abstract credential hooks |
| xmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServer.java | Extends new abstract class; methods promoted to overrides |
| xmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSaslServer (test files) | Adds shared fixtures and abstract test bases; deletes per-algorithm duplication |
| xmppserver/src/test/java/.../ScramSha1TestFixtures.java | Canonical SCRAM-SHA-1 test constants |
| xmppserver/src/test/java/.../FirstExchangeResult.java | Parsing helper for the SCRAM first server response |
| xmppserver/src/test/java/.../AbstractScramSaslServerTest.java | Reusable suite of SCRAM SASL contract/protocol tests |
| xmppserver/src/test/java/.../AbstractScramCredentialObfuscationTest.java | Reusable suite for fake-key/salt obfuscation tests |
| xmppserver/src/test/java/.../ScramSha1SaslServerTest.java | Now extends abstract test base; algorithm-specific overrides only |
| xmppserver/src/test/java/.../ScramSha1CredentialObfuscationTest.java | Replaces deleted SaltTest/FakeKeyTest by extending the new abstract base |
| xmppserver/src/test/java/.../ScramSha1SaslServerSaltTest.java | Deleted (consolidated) |
| xmppserver/src/test/java/.../ScramSha1SaslServerFakeKeyTest.java | Deleted (consolidated) |
ðĄ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ð§đ Nitpick comments (1)
xmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSaslServer.java (1)
17-40: ⥠Quick winMake the abstract SCRAM hooks
protected.Lines 17, 27, and 40 currently declare package-private abstract methods. Since
ScramSaslServeris itself a public abstract class, these package-private hooks limit extensibility to the same package. The existing subclass (ScramSha1SaslServer) already declares these methods asprotected, indicating the intended visibility. Align the base class declarations toprotected abstractto properly express the extensibility contract.âŧïļ Proposed fix
- abstract byte[] getOrFakeServerKey(String username); + protected abstract byte[] getOrFakeServerKey(String username); - abstract byte[] getOrFakeStoredKey(final String username); + protected abstract byte[] getOrFakeStoredKey(final String username); - abstract byte[] getOrCreateSalt(final String username); + protected abstract byte[] getOrCreateSalt(final String username);ðĪ 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/sasl/ScramSaslServer.java` around lines 17 - 40, The three abstract hook methods in ScramSaslServer (getOrFakeServerKey, getOrFakeStoredKey, getOrCreateSalt) are currently package-private; change their declarations to protected abstract so subclasses outside the package (e.g., ScramSha1SaslServer) can override them and the class's extensibility contract is explicitâupdate the method modifiers in ScramSaslServer accordingly.
ðĪ 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
`@xmppserver/src/test/java/org/jivesoftware/openfire/sasl/AbstractScramSaslServerTest.java`:
- Line 573: The test is passing an already-prefixed proof string to
createClientFinalMessage which itself prefixes "p=", producing malformed
client-final-message; update the three calls to createClientFinalMessage (the
one assigning clientFinalMessage and the other two usages referenced) to pass
the raw Base64 proof bytes (e.g., "dGVzdA==") instead of "p=..." so the helper
will produce a correctly formed "p=<base64>" field.
---
Nitpick comments:
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSaslServer.java`:
- Around line 17-40: The three abstract hook methods in ScramSaslServer
(getOrFakeServerKey, getOrFakeStoredKey, getOrCreateSalt) are currently
package-private; change their declarations to protected abstract so subclasses
outside the package (e.g., ScramSha1SaslServer) can override them and the
class's extensibility contract is explicitâupdate the method modifiers in
ScramSaslServer accordingly.
ðŠ 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: 00df861a-a06e-44fd-9631-de75ed563a64
ð Files selected for processing (10)
xmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSaslServer.javaxmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServer.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/AbstractScramCredentialObfuscationTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/AbstractScramSaslServerTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/FirstExchangeResult.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1CredentialObfuscationTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerFakeKeyTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerSaltTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1TestFixtures.java
ðĪ Files with no reviewable changes (2)
- xmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerFakeKeyTest.java
- xmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerSaltTest.java
The goal of this change is to prepare the unit test structure for SHA-256 re-use. A new abstract test class is created that now contains tests for: - state machine - malformed input - negotiated properties - channel binding behavior Tests (and helpers) for protocol semantics, SASL contract behavior and algorithm-independent SCRAM behavior now live in that abstract class. SHA-1-specific cryptographic behavior, mechanism naming and success path details remain in the SCRAM-SHA-1 specific class. The fake salt determinism, fake key generation and user enumeration protection responsibilities are deliberately not folded into the same abstract class, as that would cause us to end up with a "god test class". There is some duplication in those that is worth removing, but that will be the subject of a future commit.
The fake salt determinism, fake key generation and user enumeration protection tests for SCRAM-SHA-1 are consolidated into one class, which is then split up in an abstract base class, and a SCRAM-SHA-1 specific subclass. This is expected to make it easier to add test coverage for other algorithms. Of note is that this change introduces a test-driven rather than domain-driven change in the system under test (`ScramSaslServer` and `ScramSha1SaslServer`): methods were lifted into the abstract superclass primarily because the tests needed polymorphism. This needs improving, which should go in a future commit.
guusdk
force-pushed
the
SCRAM_refactor_unittests
branch
from
May 19, 2026 11:05
7fb92e9 to
434e1c2
Compare
This was referenced Jul 2, 2026
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.
This PR refactors the SCRAM-SHA-1 unit test suite to improve readability, reduce duplication, and prepare the test structure for reuse by additional SCRAM mechanisms (such as SCRAM-SHA-256).
The changes are intentionally non-functional: they reorganize and consolidate test infrastructure without changing protocol behavior.
Highlights include
Please refer to the individual commit messages for more details.
Please note: Part of this refactoring introduces a test-driven rather than domain-driven change in the implementation under test (ScramSaslServer and ScramSha1SaslServer). Some methods were moved into abstract superclasses primarily to enable polymorphic test reuse. This is a compromise and will be revisited in a future change (that I've got ready to go, but in a different PR).