FAST#3447
Draft
dwd wants to merge 53 commits into
Draft
Conversation
# Conflicts: # xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java
…nc SessionManager.bindResource Co-authored-by: Junie <junie@jetbrains.com>
… non-SASL2 sessions The authenticationSuccessful method had two separate if (usingSASL2) blocks, both of which sent a <success/> element in their else branches, causing the element to be sent twice for non-SASL2 (SASL1/ANONYMOUS) sessions. Refactored to a single combined block: the SASL2 path handles bind2 and non-bind2 cases, while the else branch sends the SASL1 <success/> once. Tests use verify(connection).deliverRawText(...) (i.e. times(1)) to assert exactly one write, ensuring a double-write regression would be caught. Co-authored-by: Junie <junie@jetbrains.com>
Co-authored-by: Junie <junie@jetbrains.com>
Co-authored-by: Junie <junie@jetbrains.com>
Co-authored-by: Junie <junie@jetbrains.com>
…zation-identifier form Co-authored-by: Junie <junie@jetbrains.com>
…fier in SASL2 authentication - Remove redundant early SASL2 success block that caused duplicate <success/> elements - Fix anonymous SASL2 authorization-identifier: pass pre-computed bare JID instead of raw username (which was null for anonymous), preventing '@Domain' instead of 'uuid@domain' - Update buildSasl2SuccessElement to accept full authorizationIdentity directly
… in SASL2+Bind2 async path Add a package-private isStartedSASL() accessor to StanzaHandler to allow unit tests to observe the flag state, and add StanzaHandlerTest with a regression test that demonstrates the bug: after a SASL2+Bind2 single-step <authenticate> completes asynchronously, startedSASL is never reset to false. Co-authored-by: Junie <junie@jetbrains.com>
…d regression tests In StanzaHandler.processStanza, the startedSASL flag was only reset for the synchronous SASL2 completion paths (authenticated status). When Bind2 is present, SASLAuthentication.handle() returns authenticatedAwaitingFeatures instead, and startedSASL was never reset, leaving it true indefinitely. Fix: reset startedSASL when saslStatus == authenticatedAwaitingFeatures in both the <authenticate> and <response> branches of processStanza. Also adds StanzaHandlerTest with three regression tests covering: - SASL1 synchronous reset (via initiateSession stream restart) - SASL2 without Bind2 synchronous reset (via <response> path) - SASL2+Bind2 async reset (the bug case, now fixed) Co-authored-by: Junie <junie@jetbrains.com>
The test checks two things: - Log lines from Conversations that inline features were reported as part of resource binding - Conversations UI shows server support for Bind 2 The test is run with the sasl2 config, since enabling sasl2 in config is a prerequisite for bind2
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* We expect features * But not bind!
…(XEP-0386) XEP-0198 was previously supported in isolation but had no integration with the SASL2 / Bind2 inline-feature negotiation that this branch also supports. This commit wires the two together at all three integration points defined by the specifications. 1. Inline feature advertisement (XEP-0388 §6.3.1) SASLAuthentication.getSASLMechanismsElement() now adds an <sm/> element (urn:xmpp:sm:3) inside the <inline/> child of the SASL2 <authentication/> feature, but only when stream management is globally active. 2. SM <enable> inside Bind2 (XEP-0386) A new Bind2InlineHandler implementation, Bind2StreamManagementHandler, handles <enable xmlns='urn:xmpp:sm:3'/> elements that arrive inside the Bind2 <bind/> element. It calls the new StreamManager.enableAndBuildElement() method (which performs the same work as the existing private enable() but returns the <enabled/> element instead of sending it) and adds the result to the <bound/> element in the SASL2 <success/> stanza, so the client receives everything in a single round-trip. The handler is registered in SessionManager.start() and unregistered in SessionManager.stop(). 3. SM <resume> inside SASL2 <authenticate> (XEP-0388 §6.3.2) When a client includes a <resume xmlns='urn:xmpp:sm:3'/> element inside its SASL2 <authenticate/> stanza, SASLAuthentication.handle() stores it on the session. After SASL authentication succeeds, authenticationSuccessful() detects the stored element and calls the new StreamManager.processSasl2Resume() method. That method mirrors the existing startResume() logic but calls the new LocalSession.reattachForSasl2() instead of reattach(): the new variant takes over the connection and builds the <resumed/> element without sending it, so the caller can embed it inside the SASL2 <success/> stanza. Supporting refactors - StreamManager.onResume() is decomposed into buildResumedElement(), processClientAcknowledgementPublic(), and redeliverUnackedStanzas() so the SASL2 resume path can reuse the same logic without duplicating it. - StreamManager.enable() is decomposed into enableInternal() (returns the element) and the original enable() (sends it), with the new public enableAndBuildElement() delegating to enableInternal(). - LocalSession gains reattachForSasl2() alongside the existing reattach(). Tests - StreamManagerTest: three new tests for StreamManager.featureElement(). - Bind2StreamManagementHandlerTest: seven tests covering enable/resume attribute parsing, failure handling, and rejection of unexpected elements. - SASLAuthenticationTest: three new tests verifying that the <sm/> inline feature is present in SASL2 advertisements when SM is active, absent when SM is inactive, and absent from SASL1 advertisements entirely. Co-authored-by: Junie <junie@jetbrains.com>
When a session is resumed inline via SASL2 (XEP-0388 + XEP-0198), the
server must send pending unacknowledged stanzas only *after* the stream
features that follow the <success/> element, not before.
Previously, reattachForSasl2() called redeliverUnackedStanzas() directly,
which meant stanzas were sent before <success/> was even delivered to the
client, let alone the post-success stream features.
Fix:
- Add a boolean flag pendingSasl2Redelivery to StreamManager, with
setPendingSasl2Redelivery(boolean) and redeliverIfPendingSasl2(JID).
- reattachForSasl2() in LocalSession now sets the flag instead of
calling redeliverUnackedStanzas() directly.
- StanzaHandler calls redeliverIfPendingSasl2() immediately after
delivering stream features following a successful SASL2 authenticate
or response, ensuring the correct ordering:
1. <success/> (with embedded <resumed/>)
2. stream features
3. unacknowledged stanzas redelivered
Three new unit tests in StreamManagerTest verify the flag semantics.
Co-authored-by: Junie <junie@jetbrains.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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
2 participants
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.
DRaft FAST support, with HT-* and HT2-*