From d549dda2d1cb2520c260f30027bfe248ec3c591c Mon Sep 17 00:00:00 2001 From: Jeroen van Erp Date: Mon, 25 Sep 2023 16:33:35 +0200 Subject: [PATCH] Use FILE_OPEN_IF if no CreateDisposition provided (Fixes #786) (#798) * Use FILE_OPEN_IF if no CreateDisposition provided (Fixes #786) Signed-off-by: Jeroen van Erp * Added license header --------- Signed-off-by: Jeroen van Erp --- README.adoc | 6 ++ .../mssmb2/messages/SMB2CreateRequest.java | 8 +- .../mssmb2/messages/SMB2CreateResponse.java | 8 ++ .../smbj/session/SessionSpec.groovy | 43 ---------- .../smbj/connection/ConnectionTest.java | 11 +-- .../hierynomus/smbj/session/SessionTest.java | 84 +++++++++++++++++++ .../com/hierynomus/smbj/testing/Utils.java | 27 ++++++ 7 files changed, 132 insertions(+), 55 deletions(-) delete mode 100644 src/test/groovy/com/hierynomus/smbj/session/SessionSpec.groovy create mode 100644 src/test/java/com/hierynomus/smbj/session/SessionTest.java create mode 100644 src/test/java/com/hierynomus/smbj/testing/Utils.java diff --git a/README.adoc b/README.adoc index 2c5ad2c0..28e6d8af 100644 --- a/README.adoc +++ b/README.adoc @@ -158,6 +158,12 @@ The implementation is based on the following specifications: == Changelog +=== 0.13.0 <-- UPCOMING +* Use FILE_OPEN_IF if no SMB2CreateDisposition provided (Fixes https://github.com/hierynomus/smbj/issues/786[#786]) +* Integration tests rewritten to JUnit Jupiter and TestContainers +* Use BufferedInputStream to read Socket (Merged https://github.com/hierynomus/smbj/issues/795[#795]) +* Clear serverList when last connection closed (Merged https://github.com/hierynomus/smbj/issues/719[#719]) + === 0.12.2 (2023-08-07) * Added missing applicationKey getter to SessionContext (Merged https://github.com/hierynomus/smbj/pulls/776[#776]) * Fix error with anonymous authentication (Fixes https://github.com/hierynomus/smbj/issues/779[#779]) diff --git a/src/main/java/com/hierynomus/mssmb2/messages/SMB2CreateRequest.java b/src/main/java/com/hierynomus/mssmb2/messages/SMB2CreateRequest.java index 264d4ab4..bc1a230e 100644 --- a/src/main/java/com/hierynomus/mssmb2/messages/SMB2CreateRequest.java +++ b/src/main/java/com/hierynomus/mssmb2/messages/SMB2CreateRequest.java @@ -53,7 +53,7 @@ public SMB2CreateRequest(SMB2Dialect smbDialect, this.accessMask = accessMask; this.fileAttributes = ensureNotNull(fileAttributes, FileAttributes.class); this.shareAccess = ensureNotNull(shareAccess, SMB2ShareAccess.class); - this.createDisposition = ensureNotNull(createDisposition, SMB2CreateDisposition.FILE_SUPERSEDE); + this.createDisposition = ensureNotNull(createDisposition, SMB2CreateDisposition.FILE_OPEN_IF); this.createOptions = ensureNotNull(createOptions, SMB2CreateOptions.class); this.path = path; } @@ -62,7 +62,7 @@ public SMB2CreateRequest(SMB2Dialect smbDialect, protected void writeTo(SMBBuffer buffer) { buffer.putUInt16(structureSize); // StructureSize (2 bytes) buffer.putByte((byte) 0); // SecurityFlags (1 byte) - Reserved - buffer.putByte((byte) 0); // RequestedOpLockLevel (1 byte) - None + buffer.putByte((byte) 0); // RequestedOpLockLevel (1 byte) - None buffer.putUInt32(impersonationLevel.getValue()); // ImpersonationLevel (4 bytes) - Identification buffer.putReserved(8); // SmbCreateFlags (8 bytes) buffer.putReserved(8); // Reserved (8 bytes) @@ -95,4 +95,8 @@ protected void writeTo(SMBBuffer buffer) { buffer.putRawBytes(nameBytes); } + + public SMB2CreateDisposition getCreateDisposition() { + return createDisposition; + } } diff --git a/src/main/java/com/hierynomus/mssmb2/messages/SMB2CreateResponse.java b/src/main/java/com/hierynomus/mssmb2/messages/SMB2CreateResponse.java index 274f20b0..f092f167 100644 --- a/src/main/java/com/hierynomus/mssmb2/messages/SMB2CreateResponse.java +++ b/src/main/java/com/hierynomus/mssmb2/messages/SMB2CreateResponse.java @@ -90,4 +90,12 @@ public Set getFileAttributes() { public SMB2FileId getFileId() { return fileId; } + + public void setFileAttributes(Set fileAttributes) { + this.fileAttributes = fileAttributes; + } + + public void setFileId(SMB2FileId fileId) { + this.fileId = fileId; + } } diff --git a/src/test/groovy/com/hierynomus/smbj/session/SessionSpec.groovy b/src/test/groovy/com/hierynomus/smbj/session/SessionSpec.groovy deleted file mode 100644 index d4fe1ad6..00000000 --- a/src/test/groovy/com/hierynomus/smbj/session/SessionSpec.groovy +++ /dev/null @@ -1,43 +0,0 @@ -/* - * Copyright (C)2016 - SMBJ Contributors - * - * 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 com.hierynomus.smbj.session - -import com.hierynomus.mssmb2.SMB2Dialect -import com.hierynomus.smbj.SmbConfig -import com.hierynomus.smbj.connection.Connection -import com.hierynomus.smbj.connection.NegotiatedProtocol -import com.hierynomus.smbj.event.SMBEventBus -import com.hierynomus.smbj.server.ServerList -import spock.lang.Specification - -class SessionSpec extends Specification { - - def "share name cannot contain '\\'"() { - given: - def config = SmbConfig.createDefaultConfig() - def connection = Stub(Connection, constructorArgs: [config, null, Mock(SMBEventBus), new ServerList()]) { - getNegotiatedProtocol() >> new NegotiatedProtocol(SMB2Dialect.SMB_2_0_2, 100, 100, 100, true) - } - def session = new Session(connection, config, null, null, null, null, null) - - when: - session.connectShare("foo\\bar") - - then: - def ex = thrown(IllegalArgumentException) - ex.message.contains("foo\\bar") - } -} diff --git a/src/test/java/com/hierynomus/smbj/connection/ConnectionTest.java b/src/test/java/com/hierynomus/smbj/connection/ConnectionTest.java index a6541701..c32aa570 100644 --- a/src/test/java/com/hierynomus/smbj/connection/ConnectionTest.java +++ b/src/test/java/com/hierynomus/smbj/connection/ConnectionTest.java @@ -18,6 +18,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static com.hierynomus.smbj.testing.Utils.*; import java.util.UUID; import java.util.concurrent.atomic.AtomicBoolean; @@ -33,20 +34,10 @@ import com.hierynomus.mssmb2.messages.SMB2NegotiateResponse; import com.hierynomus.smbj.SMBClient; import com.hierynomus.smbj.SmbConfig; -import com.hierynomus.smbj.testing.PacketProcessor; -import com.hierynomus.smbj.testing.StubAuthenticator; -import com.hierynomus.smbj.testing.StubTransportLayerFactory; -import com.hierynomus.smbj.testing.PacketProcessor.DefaultPacketProcessor; import com.hierynomus.smbj.testing.PacketProcessor.NoOpPacketProcessor;; public class ConnectionTest { - private static SmbConfig config(PacketProcessor processor) { - return SmbConfig.builder() - .withTransportLayerFactory(new StubTransportLayerFactory<>(new DefaultPacketProcessor().wrap(processor))) - .withAuthenticators(new StubAuthenticator.Factory()).build(); - } - @Test public void shouldUnregisterServerWhenConnectionClosed() throws Exception { SmbConfig config = config(new NoOpPacketProcessor()); diff --git a/src/test/java/com/hierynomus/smbj/session/SessionTest.java b/src/test/java/com/hierynomus/smbj/session/SessionTest.java new file mode 100644 index 00000000..eddbc151 --- /dev/null +++ b/src/test/java/com/hierynomus/smbj/session/SessionTest.java @@ -0,0 +1,84 @@ +/* + * Copyright (C)2016 - SMBJ Contributors + * + * 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 com.hierynomus.smbj.session; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.isA; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.EnumSet; +import java.util.concurrent.Future; + +import org.junit.jupiter.api.Test; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; + +import com.hierynomus.msfscc.FileAttributes; +import com.hierynomus.mssmb2.SMB2CreateDisposition; +import com.hierynomus.mssmb2.SMB2Dialect; +import com.hierynomus.mssmb2.SMB2ShareAccess; +import com.hierynomus.mssmb2.messages.SMB2CreateRequest; +import com.hierynomus.mssmb2.messages.SMB2CreateResponse; +import com.hierynomus.protocol.commons.concurrent.Promise; +import com.hierynomus.protocol.transport.TransportException; +import com.hierynomus.smbj.SmbConfig; +import com.hierynomus.smbj.common.SmbPath; +import com.hierynomus.smbj.connection.Connection; +import com.hierynomus.smbj.connection.NegotiatedProtocol; +import com.hierynomus.smbj.event.SMBEventBus; +import com.hierynomus.smbj.paths.PathResolver; +import com.hierynomus.smbj.share.DiskShare; +import com.hierynomus.smbj.share.File; +import com.hierynomus.smbj.share.TreeConnect; + +public class SessionTest { + @Test + public void shareNameCannotContainBackslashes() { + SmbConfig cfg = SmbConfig.createDefaultConfig(); + Connection c = mock(Connection.class); + Session s = new Session(c, cfg, null, mock(SMBEventBus.class), null, null, null); + Exception ex = assertThrows(IllegalArgumentException.class, () -> s.connectShare("foo\\bar")); + assertThat(ex.getMessage()).contains("foo\\bar"); + } + + @Test + public void shouldUseOpenIfIfNoCreateDispositionProvided() throws Exception { + SmbConfig cfg = SmbConfig.createDefaultConfig(); + Session s = mock(Session.class); + TreeConnect tc = mock(TreeConnect.class); + when(tc.getSession()).thenReturn(s); + when(tc.getConfig()).thenReturn(cfg); + when(tc.getNegotiatedProtocol()) + .thenReturn(new NegotiatedProtocol(SMB2Dialect.SMB_2_0_2, 1024, 1024, 1024, true)); + DiskShare share = new DiskShare(new SmbPath("localhost", "public"), tc, PathResolver.LOCAL); + when(s.send(isA(SMB2CreateRequest.class))).thenAnswer(new Answer>() { + @Override + public Future answer(InvocationOnMock invocation) throws Throwable { + SMB2CreateRequest req = (SMB2CreateRequest) invocation.getArguments()[0]; + assertThat(req.getCreateDisposition()).isEqualTo(SMB2CreateDisposition.FILE_OPEN_IF); + Promise p = new Promise<>("foo", TransportException.Wrapper); + SMB2CreateResponse resp = new SMB2CreateResponse(); + resp.setFileAttributes(EnumSet.of(FileAttributes.FILE_ATTRIBUTE_NORMAL)); + p.deliver(resp); + return p.future(); + } + }); + File f = share.openFile("foo", null, null, SMB2ShareAccess.ALL, null, null); + + } +} diff --git a/src/test/java/com/hierynomus/smbj/testing/Utils.java b/src/test/java/com/hierynomus/smbj/testing/Utils.java new file mode 100644 index 00000000..58fc96d1 --- /dev/null +++ b/src/test/java/com/hierynomus/smbj/testing/Utils.java @@ -0,0 +1,27 @@ +/* + * Copyright (C)2016 - SMBJ Contributors + * + * 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 com.hierynomus.smbj.testing; + +import com.hierynomus.smbj.SmbConfig; +import com.hierynomus.smbj.testing.PacketProcessor.DefaultPacketProcessor; + +public class Utils { + public static SmbConfig config(PacketProcessor processor) { + return SmbConfig.builder() + .withTransportLayerFactory(new StubTransportLayerFactory<>(new DefaultPacketProcessor().wrap(processor))) + .withAuthenticators(new StubAuthenticator.Factory()).build(); + } +}