From bf3a27df9a51c49a3e1c458f2801dd160bb14b28 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Wed, 8 Mar 2023 17:40:29 +0100 Subject: [PATCH] [core] Fix ErrorIQ not showing potential original IQ child Also delete StanzaIdTest since the test was fundamentally weak and flawed. It does not work anyway, since TestIQ has a fixed stanza ID. Fixes SMACK-931. --- .../smack/packet/AbstractIqBuilder.java | 2 +- .../smack/packet/EmptyResultIQ.java | 4 +- .../jivesoftware/smack/packet/ErrorIQ.java | 88 +++++++++++++++---- .../org/jivesoftware/smack/packet/IQ.java | 54 +++--------- .../org/jivesoftware/smack/packet/IqView.java | 24 ++++- .../smack/util/PacketParserUtils.java | 7 +- .../org/jivesoftware/smack/StanzaIdTest.java | 43 --------- .../org/jivesoftware/smack/packet/IqTest.java | 38 ++++++++ .../org/jivesoftware/smack/packet/TestIQ.java | 6 +- .../bytestreams/ibb/IBBPacketUtils.java | 9 +- .../socks5/Socks5ByteStreamManagerTest.java | 8 +- .../socks5/Socks5ClientForInitiatorTest.java | 7 +- 12 files changed, 164 insertions(+), 126 deletions(-) delete mode 100644 smack-core/src/test/java/org/jivesoftware/smack/StanzaIdTest.java create mode 100644 smack-core/src/test/java/org/jivesoftware/smack/packet/IqTest.java diff --git a/smack-core/src/main/java/org/jivesoftware/smack/packet/AbstractIqBuilder.java b/smack-core/src/main/java/org/jivesoftware/smack/packet/AbstractIqBuilder.java index 5eac124351..f2c080690b 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/packet/AbstractIqBuilder.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/packet/AbstractIqBuilder.java @@ -49,7 +49,7 @@ public static IqData createErrorResponse(IqView request) { } protected static IqData createResponse(IqView request, IQ.ResponseType responseType) { - if (!(request.getType() == IQ.Type.get || request.getType() == IQ.Type.set)) { + if (!request.isRequestIQ()) { throw new IllegalArgumentException("IQ request must be of type 'set' or 'get'. Original IQ: " + request); } diff --git a/smack-core/src/main/java/org/jivesoftware/smack/packet/EmptyResultIQ.java b/smack-core/src/main/java/org/jivesoftware/smack/packet/EmptyResultIQ.java index 248f0ebaa1..a4d977ac31 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/packet/EmptyResultIQ.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/packet/EmptyResultIQ.java @@ -1,6 +1,6 @@ /** * - * Copyright © 2014-2019 Florian Schmaus + * Copyright © 2014-2023 Florian Schmaus * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -24,7 +24,7 @@ public class EmptyResultIQ extends IQ { // TODO: Deprecate when stanza builder and parsing logic is ready. public EmptyResultIQ() { - super(null, null); + super((String) null, null); setType(IQ.Type.result); } diff --git a/smack-core/src/main/java/org/jivesoftware/smack/packet/ErrorIQ.java b/smack-core/src/main/java/org/jivesoftware/smack/packet/ErrorIQ.java index 23bf999b0d..5b8dd77336 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/packet/ErrorIQ.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/packet/ErrorIQ.java @@ -1,6 +1,6 @@ /** * - * Copyright © 2014 Florian Schmaus + * Copyright © 2014-2023 Florian Schmaus * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,24 +16,82 @@ */ package org.jivesoftware.smack.packet; -import org.jivesoftware.smack.util.Objects; +import java.util.Objects; -public class ErrorIQ extends SimpleIQ { +import javax.xml.namespace.QName; + +/** + * An XMPP error IQ. + *

+ * According to RFC 6120 § 8.3.1 "4. An error stanza MUST contain an <error/> child element.", so this class can + * only be constructed if a stanza error is provided. + */ +public final class ErrorIQ extends IQ { public static final String ELEMENT = StanzaError.ERROR; - /** - * Constructs a new error IQ. - *

- * According to RFC 6120 § 8.3.1 "4. An error stanza MUST contain an <error/> child element.", so the xmppError argument is mandatory. - *

- * @param stanzaError the stanzaError (required). - */ - public ErrorIQ(StanzaError stanzaError) { - super(ELEMENT, null); - Objects.requireNonNull(stanzaError, "stanzaError must not be null"); - setType(IQ.Type.error); - setError(stanzaError); + private final IQ request; + + private ErrorIQ(Builder builder, QName childElementQName) { + super(builder, childElementQName); + Objects.requireNonNull(builder.getError(), "Must provide an stanza error when building error IQs"); + this.request = builder.request; + } + + public static ErrorIQ createErrorResponse(final IQ request, final StanzaError error) { + Builder builder = new Builder(error, request); + builder.setError(error); + return builder.build(); + } + + @Override + protected IQChildElementXmlStringBuilder getIQChildElementBuilder(IQChildElementXmlStringBuilder xml) { + if (request == null) { + return null; + } + + return request.getIQChildElementBuilder(xml); + } + + public static Builder builder(StanzaError error) { + return new Builder(error, IqData.EMPTY.ofType(IQ.Type.error)); + } + + public static Builder builder(StanzaError error, IqData iqData) { + return new Builder(error, iqData); } + public static final class Builder extends IqBuilder { + + private IQ request; + + Builder(StanzaError error, IqData iqData) { + super(iqData); + if (iqData.getType() != IQ.Type.error) { + throw new IllegalArgumentException("Error IQs must be of type 'error'"); + } + Objects.requireNonNull(error, "Must provide an stanza error when building error IQs"); + setError(error); + } + + Builder(StanzaError error, IQ request) { + this(error, AbstractIqBuilder.createErrorResponse(request)); + this.request = request; + } + + @Override + public Builder getThis() { + return this; + } + + @Override + public ErrorIQ build() { + QName childElementQname = null; + if (request != null) { + childElementQname = request.getChildElementQName(); + } + return new ErrorIQ(this, childElementQname); + } + + } } diff --git a/smack-core/src/main/java/org/jivesoftware/smack/packet/IQ.java b/smack-core/src/main/java/org/jivesoftware/smack/packet/IQ.java index 6af350a66c..7a5fb376f1 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/packet/IQ.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/packet/IQ.java @@ -68,16 +68,22 @@ protected IQ(String childElementName, String childElementNamespace) { } protected IQ(AbstractIqBuilder iqBuilder, String childElementName, String childElementNamespace) { + this(iqBuilder, childElementName != null ? new QName(childElementNamespace, childElementName) : null); + } + + protected IQ(AbstractIqBuilder iqBuilder, QName childElementQName) { super(iqBuilder); type = iqBuilder.type; - this.childElementName = childElementName; - this.childElementNamespace = childElementNamespace; - if (childElementName == null) { - childElementQName = null; + if (childElementQName != null) { + this.childElementQName = childElementQName; + this.childElementName = childElementQName.getLocalPart(); + this.childElementNamespace = childElementQName.getNamespaceURI(); } else { - childElementQName = new QName(childElementNamespace, childElementName); + this.childElementQName = null; + this.childElementName = null; + this.childElementNamespace = null; } } @@ -100,32 +106,6 @@ public void setType(Type type) { this.type = Objects.requireNonNull(type, "type must not be null"); } - /** - * Return true if this IQ is a request IQ, i.e. an IQ of type {@link Type#get} or {@link Type#set}. - * - * @return true if IQ type is 'get' or 'set', false otherwise. - * @since 4.1 - */ - public boolean isRequestIQ() { - switch (type) { - case get: - case set: - return true; - default: - return false; - } - } - - /** - * Return true if this IQ is a request, i.e. an IQ of type {@link Type#result} or {@link Type#error}. - * - * @return true if IQ type is 'result' or 'error', false otherwise. - * @since 4.4 - */ - public boolean isResponseIQ() { - return !isRequestIQ(); - } - public final QName getChildElementQName() { return childElementQName; } @@ -194,7 +174,6 @@ private void appendInnerXml(XmlStringBuilder xml) { if (type == Type.error) { // Add the error sub-packet, if there is one. appendErrorIfExists(xml); - return; } if (childElementName == null) { return; @@ -305,16 +284,7 @@ public static IQ createResultIQ(final IQ request) { * @return a new {@link Type#error IQ.Type.error} IQ based on the originating IQ. */ public static ErrorIQ createErrorResponse(final IQ request, final StanzaError error) { - if (!request.isRequestIQ()) { - throw new IllegalArgumentException( - "IQ must be of type 'set' or 'get'. Original IQ: " + request.toXML()); - } - final ErrorIQ result = new ErrorIQ(error); - result.setStanzaId(request.getStanzaId()); - result.setFrom(request.getTo()); - result.setTo(request.getFrom()); - - return result; + return ErrorIQ.createErrorResponse(request, error); } /** diff --git a/smack-core/src/main/java/org/jivesoftware/smack/packet/IqView.java b/smack-core/src/main/java/org/jivesoftware/smack/packet/IqView.java index a888712eb3..2fbb6b709d 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/packet/IqView.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/packet/IqView.java @@ -1,6 +1,6 @@ /** * - * Copyright 2019 Florian Schmaus + * Copyright 2019-2023 Florian Schmaus * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,6 +16,8 @@ */ package org.jivesoftware.smack.packet; +import org.jivesoftware.smack.packet.IQ.Type; + public interface IqView extends StanzaView { /** @@ -25,4 +27,24 @@ public interface IqView extends StanzaView { */ IQ.Type getType(); + /** + * Return true if this IQ is a request IQ, i.e. an IQ of type {@link Type#get} or {@link Type#set}. + * + * @return true if IQ type is 'get' or 'set', false otherwise. + * @since 4.1 + */ + default boolean isRequestIQ() { + IQ.Type type = getType(); + return type == IQ.Type.get || type == IQ.Type.set; + } + + /** + * Return true if this IQ is a request, i.e. an IQ of type {@link Type#result} or {@link Type#error}. + * + * @return true if IQ type is 'result' or 'error', false otherwise. + * @since 4.4 + */ + default boolean isResponseIQ() { + return !isRequestIQ(); + } } diff --git a/smack-core/src/main/java/org/jivesoftware/smack/util/PacketParserUtils.java b/smack-core/src/main/java/org/jivesoftware/smack/util/PacketParserUtils.java index 82cace29d9..4a52261efc 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/util/PacketParserUtils.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/util/PacketParserUtils.java @@ -1,6 +1,6 @@ /** * - * Copyright 2003-2007 Jive Software, 2019 Florian Schmaus. + * Copyright 2003-2007 Jive Software, 2019-2023 Florian Schmaus. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -579,8 +579,9 @@ public static IQ parseIQ(XmlPullParser parser, XmlEnvironment outerXmlEnvironmen switch (iqData.getType()) { case error: // If an IQ packet wasn't created above, create an empty error IQ packet. - iqPacket = new ErrorIQ(error); - break; + iqPacket = ErrorIQ.builder(error, iqData).build(); + // The following return is simply to avoid setting iqData again below. + return iqPacket; case result: iqPacket = new EmptyResultIQ(); break; diff --git a/smack-core/src/test/java/org/jivesoftware/smack/StanzaIdTest.java b/smack-core/src/test/java/org/jivesoftware/smack/StanzaIdTest.java deleted file mode 100644 index 8a73f1a969..0000000000 --- a/smack-core/src/test/java/org/jivesoftware/smack/StanzaIdTest.java +++ /dev/null @@ -1,43 +0,0 @@ -/** - * - * Copyright © 2014 Florian Schmaus - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.jivesoftware.smack; - -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; - -import org.jivesoftware.smack.packet.IQ; -import org.jivesoftware.smack.packet.TestIQ; -import org.jivesoftware.smack.util.StringUtils; - -import org.junit.Test; - -public class StanzaIdTest { - - @Test - public void testIqId() { - IQ iq1 = new TestIQ(); - String iq1Id = iq1.getStanzaId(); - assertTrue(StringUtils.isNotEmpty(iq1Id)); - - IQ iq2 = new TestIQ(); - String iq2Id = iq2.getStanzaId(); - assertTrue(StringUtils.isNotEmpty(iq2Id)); - - assertFalse(iq1Id.equals(iq2Id)); - } - -} diff --git a/smack-core/src/test/java/org/jivesoftware/smack/packet/IqTest.java b/smack-core/src/test/java/org/jivesoftware/smack/packet/IqTest.java new file mode 100644 index 0000000000..b8f192bd74 --- /dev/null +++ b/smack-core/src/test/java/org/jivesoftware/smack/packet/IqTest.java @@ -0,0 +1,38 @@ +/** + * + * Copyright © 2023 Florian Schmaus + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.jivesoftware.smack.packet; + +import static org.jivesoftware.smack.test.util.XmlAssertUtil.assertXmlSimilar; + +import org.junit.jupiter.api.Test; + +public class IqTest { + + @Test + public void testIqErrorWithChildElement() { + IQ request = new TestIQ(); + StanzaError error = StanzaError.getBuilder().setCondition(StanzaError.Condition.bad_request).build(); + ErrorIQ errorIq = IQ.createErrorResponse(request, error); + + String expected = "" + + "" + + "" + + ""; + assertXmlSimilar(expected, errorIq.toXML()); + } + +} diff --git a/smack-core/src/test/java/org/jivesoftware/smack/packet/TestIQ.java b/smack-core/src/test/java/org/jivesoftware/smack/packet/TestIQ.java index c222035e17..ec80fb7ea5 100644 --- a/smack-core/src/test/java/org/jivesoftware/smack/packet/TestIQ.java +++ b/smack-core/src/test/java/org/jivesoftware/smack/packet/TestIQ.java @@ -1,6 +1,6 @@ /** * - * Copyright © 2014-2019 Florian Schmaus + * Copyright © 2014-2023 Florian Schmaus * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -21,11 +21,11 @@ public class TestIQ extends SimpleIQ { public TestIQ() { - this(SmackConfiguration.SMACK_URL_STRING, "test-iq"); + this("test-iq", SmackConfiguration.SMACK_URL_STRING); } public TestIQ(String element, String namespace) { - super(element, namespace); + super(StanzaBuilder.buildIqData("42"), element, namespace); } @Override diff --git a/smack-extensions/src/test/java/org/jivesoftware/smackx/bytestreams/ibb/IBBPacketUtils.java b/smack-extensions/src/test/java/org/jivesoftware/smackx/bytestreams/ibb/IBBPacketUtils.java index 9aa69919c0..f8ab45a4fc 100644 --- a/smack-extensions/src/test/java/org/jivesoftware/smackx/bytestreams/ibb/IBBPacketUtils.java +++ b/smack-extensions/src/test/java/org/jivesoftware/smackx/bytestreams/ibb/IBBPacketUtils.java @@ -40,11 +40,10 @@ public class IBBPacketUtils { */ public static IQ createErrorIQ(Jid from, Jid to, StanzaError.Condition condition) { StanzaError xmppError = StanzaError.getBuilder(condition).build(); - IQ errorIQ = new ErrorIQ(xmppError); - errorIQ.setType(IQ.Type.error); - errorIQ.setFrom(from); - errorIQ.setTo(to); - return errorIQ; + return ErrorIQ.builder(xmppError) + .from(from) + .to(to) + .build(); } /** diff --git a/smack-extensions/src/test/java/org/jivesoftware/smackx/bytestreams/socks5/Socks5ByteStreamManagerTest.java b/smack-extensions/src/test/java/org/jivesoftware/smackx/bytestreams/socks5/Socks5ByteStreamManagerTest.java index c2b7486867..cfa59197a5 100644 --- a/smack-extensions/src/test/java/org/jivesoftware/smackx/bytestreams/socks5/Socks5ByteStreamManagerTest.java +++ b/smack-extensions/src/test/java/org/jivesoftware/smackx/bytestreams/socks5/Socks5ByteStreamManagerTest.java @@ -38,12 +38,11 @@ import org.jivesoftware.smack.XMPPConnection; import org.jivesoftware.smack.XMPPException; import org.jivesoftware.smack.XMPPException.XMPPErrorException; -import org.jivesoftware.smack.packet.ErrorIQ; import org.jivesoftware.smack.packet.IQ; import org.jivesoftware.smack.packet.StanzaError; import org.jivesoftware.smack.test.util.NetworkUtil; import org.jivesoftware.smack.util.ExceptionUtil; - +import org.jivesoftware.smackx.bytestreams.ibb.IBBPacketUtils; import org.jivesoftware.smackx.bytestreams.socks5.packet.Bytestream; import org.jivesoftware.smackx.bytestreams.socks5.packet.Bytestream.StreamHost; import org.jivesoftware.smackx.disco.ServiceDiscoveryManager; @@ -415,10 +414,7 @@ public void shouldFailIfTargetDoesNotAcceptSocks5Bytestream() throws SmackExcept Verification.requestTypeGET); // build error packet to reject SOCKS5 Bytestream - StanzaError stanzaError = StanzaError.getBuilder(StanzaError.Condition.not_acceptable).build(); - IQ rejectPacket = new ErrorIQ(stanzaError); - rejectPacket.setFrom(targetJID); - rejectPacket.setTo(initiatorJID); + IQ rejectPacket = IBBPacketUtils.createErrorIQ(targetJID, initiatorJID, StanzaError.Condition.not_acceptable); // return error packet as response to the bytestream initiation protocol.addResponse(rejectPacket, Verification.correspondingSenderReceiver, diff --git a/smack-extensions/src/test/java/org/jivesoftware/smackx/bytestreams/socks5/Socks5ClientForInitiatorTest.java b/smack-extensions/src/test/java/org/jivesoftware/smackx/bytestreams/socks5/Socks5ClientForInitiatorTest.java index 91ec22b68a..3ed68d212b 100644 --- a/smack-extensions/src/test/java/org/jivesoftware/smackx/bytestreams/socks5/Socks5ClientForInitiatorTest.java +++ b/smack-extensions/src/test/java/org/jivesoftware/smackx/bytestreams/socks5/Socks5ClientForInitiatorTest.java @@ -30,11 +30,10 @@ import org.jivesoftware.smack.XMPPConnection; import org.jivesoftware.smack.XMPPException.XMPPErrorException; import org.jivesoftware.smack.packet.EmptyResultIQ; -import org.jivesoftware.smack.packet.ErrorIQ; import org.jivesoftware.smack.packet.IQ; import org.jivesoftware.smack.packet.StanzaError; import org.jivesoftware.smack.util.CloseableUtil; - +import org.jivesoftware.smackx.bytestreams.ibb.IBBPacketUtils; import org.jivesoftware.smackx.bytestreams.socks5.packet.Bytestream; import org.jivesoftware.smackx.bytestreams.socks5.packet.Bytestream.StreamHost; @@ -184,9 +183,7 @@ public void shouldFailIfActivateSocks5ProxyFails() throws Exception { XMPPConnection connection = ConnectionUtils.createMockedConnection(protocol, initiatorJID); // build error response as reply to the stream activation - IQ error = new ErrorIQ(StanzaError.getBuilder(StanzaError.Condition.internal_server_error).build()); - error.setFrom(proxyJID); - error.setTo(initiatorJID); + IQ error = IBBPacketUtils.createErrorIQ(proxyJID, initiatorJID, StanzaError.Condition.internal_server_error); protocol.addResponse(error, Verification.correspondingSenderReceiver, Verification.requestTypeSET);