PakhomovAlexander commented on code in PR #2688: URL: https://github.com/apache/ignite-3/pull/2688#discussion_r1365451280
########## modules/client-handler/src/test/java/org/apache/ignite/client/handler/ClientInboundMessageHandlerTest.java: ########## @@ -0,0 +1,329 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.apache.ignite.client.handler; + +import static org.awaitility.Awaitility.await; +import static org.hamcrest.Matchers.is; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.verify; + +import io.netty.buffer.ByteBuf; +import io.netty.buffer.Unpooled; +import io.netty.buffer.UnpooledByteBufAllocator; +import io.netty.channel.Channel; +import io.netty.channel.ChannelFuture; +import io.netty.channel.ChannelHandlerContext; +import java.io.IOException; +import java.time.Duration; +import java.util.UUID; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import org.apache.ignite.client.handler.configuration.ClientConnectorConfiguration; +import org.apache.ignite.compute.IgniteCompute; +import org.apache.ignite.internal.catalog.CatalogService; +import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension; +import org.apache.ignite.internal.configuration.testframework.InjectConfiguration; +import org.apache.ignite.internal.hlc.HybridClock; +import org.apache.ignite.internal.security.authentication.AuthenticationManager; +import org.apache.ignite.internal.security.authentication.AuthenticationManagerImpl; +import org.apache.ignite.internal.security.authentication.basic.BasicAuthenticationProviderChange; +import org.apache.ignite.internal.security.configuration.SecurityConfiguration; +import org.apache.ignite.internal.sql.engine.QueryProcessor; +import org.apache.ignite.internal.table.IgniteTablesInternal; +import org.apache.ignite.internal.table.distributed.schema.SchemaSyncService; +import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest; +import org.apache.ignite.internal.tx.impl.IgniteTransactionsImpl; +import org.apache.ignite.network.ClusterNode; +import org.apache.ignite.network.ClusterNodeImpl; +import org.apache.ignite.network.ClusterService; +import org.apache.ignite.network.NetworkAddress; +import org.apache.ignite.network.TopologyService; +import org.apache.ignite.sql.IgniteSql; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.msgpack.core.MessagePack; + +@ExtendWith(MockitoExtension.class) +@ExtendWith(ConfigurationExtension.class) +class ClientInboundMessageHandlerTest extends BaseIgniteAbstractTest { + private static final Duration TIMEOUT_OF_DURING = Duration.ofSeconds(1); + + @InjectConfiguration + private ClientConnectorConfiguration configuration; + + @InjectConfiguration + private SecurityConfiguration securityConfiguration; + + @Mock + private IgniteTablesInternal igniteTables; + + @Mock + private IgniteTransactionsImpl igniteTransactions; + + @Mock + private QueryProcessor processor; + + @Mock + private IgniteCompute compute; + + @Mock + private TopologyService topologyService; + + @Mock + private ClusterService clusterService; + + @Mock + private IgniteSql sql; + + @Mock + private CompletableFuture<UUID> clusterId; + + @Mock + private ClientHandlerMetricSource metrics; + + @Mock + private HybridClock clock; + + @Mock + private SchemaSyncService schemaSyncService; + + @Mock + private CatalogService catalogService; + + @Mock + private ChannelHandlerContext ctx; + + @Mock + private Channel channel; + + @Mock + private ChannelFuture channelFuture; + + private ClientInboundMessageHandler handler; + + private final AtomicBoolean ctxClosed = new AtomicBoolean(false); + + @BeforeEach + void setUp() throws Exception { + doReturn(topologyService).when(clusterService).topologyService(); + + ClusterNode node = new ClusterNodeImpl("node1", "node1", new NetworkAddress("localhost", 10800)); + doReturn(node).when(topologyService).localMember(); + + doReturn(UUID.randomUUID()).when(clusterId).join(); + + doReturn(channelFuture).when(channel).closeFuture(); + + doReturn(new UnpooledByteBufAllocator(true)).when(ctx).alloc(); + doReturn(channel).when(ctx).channel(); + lenient().doAnswer(invocation -> { + ctxClosed.set(true); + return null; + }).when(ctx).close(); + + AuthenticationManager authenticationManager = new AuthenticationManagerImpl(); + + handler = new ClientInboundMessageHandler( + igniteTables, + igniteTransactions, + processor, + configuration.value(), + compute, + clusterService, + sql, + clusterId, + metrics, + authenticationManager, + clock, + schemaSyncService, + catalogService + ); + + authenticationManager.listen(handler); + securityConfiguration.listen(authenticationManager); + + securityConfiguration.change(change -> { + change.changeEnabled(true); + change.changeAuthentication(authChange -> { + authChange.changeProviders(providersChange -> { + providersChange.create("basic", basicChange -> { + basicChange.convert(BasicAuthenticationProviderChange.class) + .changeUsername("admin") + .changePassword("password"); + }).create("basic1", basicChange -> { + basicChange.convert(BasicAuthenticationProviderChange.class) + .changeUsername("admin1") + .changePassword("password"); + }); + }); + }); + }).join(); + + handler.channelRegistered(ctx); + } + + @Test + void disableAuthentication() throws IOException { + handshake(); + + securityConfiguration.change(change -> { + change.changeEnabled(false); + }).join(); + + await().during(TIMEOUT_OF_DURING).untilAtomic(ctxClosed, is(false)); + } + + @Test + void enableAuthentication() throws InterruptedException, IOException { + securityConfiguration.change(change -> { + change.changeEnabled(false); + }).join(); + + TimeUnit.SECONDS.sleep(TIMEOUT_OF_DURING.getSeconds()); + + handshake(); + + securityConfiguration.change(change -> { + change.changeEnabled(true); + }).join(); + + await().untilAtomic(ctxClosed, is(true)); + } + + @Test + void changeCurrentUser() throws IOException { + handshake(); + + securityConfiguration.change(change -> { + change.changeEnabled(true); + change.changeAuthentication(authChange -> { + authChange.changeProviders(providersChange -> { + providersChange.update("basic", basicChange -> { + basicChange.convert(BasicAuthenticationProviderChange.class) + .changeUsername("admin") + .changePassword("new-password"); + }); + }); + }); + }).join(); + + await().untilAtomic(ctxClosed, is(true)); + } + + @Test + void changeAnotherUser() throws IOException { + handshake(); + + securityConfiguration.change(change -> { + change.changeEnabled(true); + change.changeAuthentication(authChange -> { + authChange.changeProviders(providersChange -> { + providersChange.update("basic1", basicChange -> { + basicChange.convert(BasicAuthenticationProviderChange.class) + .changeUsername("admin1") + .changePassword("new-password"); + }); + }); + }); + }).join(); + + await().during(TIMEOUT_OF_DURING).untilAtomic(ctxClosed, is(false)); + } + + @Test + void deleteCurrentUser() throws IOException { Review Comment: ```suggestion void deleteCurrentProvider() throws IOException { ``` ########## modules/security/src/main/java/org/apache/ignite/internal/security/authentication/basic/BasicAuthenticator.java: ########## @@ -26,11 +26,20 @@ /** Implementation of basic authenticator. */ public class BasicAuthenticator implements Authenticator { + private final String authenticatorName; + private final String username; private final String password; - public BasicAuthenticator(String username, String password) { + /** + * Constructor. + * + * @param authenticatorName Authenticator name. + * @param username Username. Review Comment: password is missed ########## modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientContext.java: ########## @@ -79,6 +94,20 @@ public BitSet features() { return features; } + /** + * Gets the authentication request. + * + * @return Authentication request. + */ + public AuthenticationRequest<?, ?> authenticationRequest() { Review Comment: We must not store an authentication request in the client context. There is a password that the user passed into the system. The system must not store this value. I think we can just close the connection instead of re-authenticate it on the fly. ########## modules/security/src/test/java/org/apache/ignite/internal/security/authentication/AuthenticationManagerImplTest.java: ########## @@ -20,200 +20,191 @@ import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully; import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import java.util.ArrayList; import java.util.List; import java.util.concurrent.CompletableFuture; import java.util.function.Consumer; import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension; import org.apache.ignite.internal.configuration.testframework.InjectConfiguration; import org.apache.ignite.internal.security.authentication.basic.BasicAuthenticationProviderChange; +import org.apache.ignite.internal.security.authentication.event.AuthenticationDisabled; +import org.apache.ignite.internal.security.authentication.event.AuthenticationEnabled; +import org.apache.ignite.internal.security.authentication.event.AuthenticationEvent; +import org.apache.ignite.internal.security.authentication.event.AuthenticationListener; +import org.apache.ignite.internal.security.authentication.event.AuthenticationProviderRemoved; +import org.apache.ignite.internal.security.authentication.event.AuthenticationProviderUpdated; import org.apache.ignite.internal.security.configuration.SecurityChange; import org.apache.ignite.internal.security.configuration.SecurityConfiguration; import org.apache.ignite.internal.security.configuration.SecurityView; import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest; import org.apache.ignite.security.exception.InvalidCredentialsException; import org.apache.ignite.security.exception.UnsupportedAuthenticationTypeException; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; - @ExtendWith(ConfigurationExtension.class) class AuthenticationManagerImplTest extends BaseIgniteAbstractTest { + private static final String PROVIDER = "basic"; + + private static final String USERNAME = "admin"; + + private static final String PASSWORD = "password"; + + private static final UsernamePasswordRequest USERNAME_PASSWORD_REQUEST = new UsernamePasswordRequest(USERNAME, PASSWORD); + private final AuthenticationManagerImpl manager = new AuthenticationManagerImpl(); + private final List<AuthenticationEvent> events = new ArrayList<>(); + + private final AuthenticationListener listener = events::add; + @InjectConfiguration private SecurityConfiguration securityConfiguration; + @BeforeEach + void setUp() { + manager.listen(listener); + } + @Test public void enableAuth() { // when - SecurityView adminPasswordView = mutateConfiguration( - securityConfiguration, change -> { - change.changeAuthentication().changeProviders(providers -> providers.create("basic", provider -> { - provider.convert(BasicAuthenticationProviderChange.class) - .changeUsername("admin") - .changePassword("password"); - })); - change.changeEnabled(true); - }) - .value(); - - manager.onUpdate(new StubSecurityViewEvent(null, adminPasswordView)).join(); + enableAuthentication(); // then // successful authentication with valid credentials - UsernamePasswordRequest validCredentials = new UsernamePasswordRequest("admin", "password"); - assertEquals("admin", manager.authenticate(validCredentials).username()); + assertEquals(USERNAME, manager.authenticate(USERNAME_PASSWORD_REQUEST).username()); // and failed authentication with invalid credentials assertThrows(InvalidCredentialsException.class, - () -> manager.authenticate(new UsernamePasswordRequest("admin", "invalid-password"))); + () -> manager.authenticate(new UsernamePasswordRequest(USERNAME, "invalid-password"))); + + assertEquals(1, events.size()); Review Comment: Do we add an event in the same thread? Is it possible that this assert fails? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
