# HG changeset patch # User Elliott Baron # Date 1375380457 14400 # Node ID 1e59f46b6efa0f4642e225c512afff2479e4fd8f # Parent 9cb09d6aeddbe6f1d95f2887b13ba1fbd22d0fc8 Only send command channel request if authentication succeeds While fixing the JMX plugin, I noticed that its RequestResponseListener is receiving AUTH_FAILED responses twice. This is because even after the generate-token fails with a given action, the request queue still issues the command. This patch changes this behaviour so if generate-token fails, no request is sent to the agent. This fixes the issue of listeners being notified twice, and also has the benefit of not sending requests that are destined to fail. Reviewed-by: vanaltj, jerboaa Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2013-July/007725.html diff -r 9cb09d6aeddb -r 1e59f46b6efa client/command/src/main/java/com/redhat/thermostat/client/command/internal/RequestQueueImpl.java --- a/client/command/src/main/java/com/redhat/thermostat/client/command/internal/RequestQueueImpl.java Thu Aug 01 14:05:00 2013 -0400 +++ b/client/command/src/main/java/com/redhat/thermostat/client/command/internal/RequestQueueImpl.java Thu Aug 01 14:07:37 2013 -0400 @@ -80,20 +80,25 @@ @Override public void putRequest(Request request) { - authenticateRequest(request); - queue.add(request); + // Only enqueue request if we've successfully authenticated + if (authenticateRequest(request)) { + queue.add(request); + } } - private void authenticateRequest(Request request) { + private boolean authenticateRequest(Request request) { + boolean result = true; // Successful by default, unless storage is secure BundleContext bCtx = FrameworkUtil.getBundle(getClass()).getBundleContext(); ServiceReference storageRef = bCtx.getServiceReference(Storage.class.getName()); Storage storage = (Storage) bCtx.getService(storageRef); if (storage instanceof SecureStorage) { - authenticateRequest(request, (SecureStorage) storage); + result = authenticateRequest(request, (SecureStorage) storage); } + return result; } - private void authenticateRequest(Request request, SecureStorage storage) { + private boolean authenticateRequest(Request request, SecureStorage storage) { + boolean result = false; // Successful only if generateToken succeeds try { String actionName = request.getParameter(Request.ACTION); // actionName must not be null here. @@ -101,9 +106,12 @@ AuthToken token = storage.generateToken(actionName); request.setParameter(Request.CLIENT_TOKEN, Base64.encodeBase64String(token.getClientToken())); request.setParameter(Request.AUTH_TOKEN, Base64.encodeBase64String(token.getToken())); + result = true; } catch (StorageException ex) { + logger.log(Level.WARNING, "Authentication failed", ex); fireComplete(request, new Response(ResponseType.AUTH_FAILED)); } + return result; } synchronized void startProcessingRequests() { @@ -183,5 +191,12 @@ boolean performHostnameCheck = !SSLConfiguration.disableHostnameVerification(); future.addListener(new SSLHandshakeFinishedListener(request, performHostnameCheck, sslHandler, this)); } + + /* + * For testing purposes only. + */ + BlockingQueue getQueue() { + return queue; + } } diff -r 9cb09d6aeddb -r 1e59f46b6efa client/command/src/test/java/com/redhat/thermostat/client/command/internal/RequestQueueImplTest.java --- a/client/command/src/test/java/com/redhat/thermostat/client/command/internal/RequestQueueImplTest.java Thu Aug 01 14:05:00 2013 -0400 +++ b/client/command/src/test/java/com/redhat/thermostat/client/command/internal/RequestQueueImplTest.java Thu Aug 01 14:07:37 2013 -0400 @@ -36,14 +36,21 @@ package com.redhat.thermostat.client.command.internal; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; -import static org.mockito.Mockito.verify; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import java.util.Arrays; + +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.osgi.framework.Bundle; import org.osgi.framework.BundleContext; import org.osgi.framework.FrameworkUtil; @@ -53,39 +60,85 @@ import org.powermock.modules.junit4.PowerMockRunner; import com.redhat.thermostat.common.command.Request; +import com.redhat.thermostat.common.command.RequestResponseListener; +import com.redhat.thermostat.common.command.Response; +import com.redhat.thermostat.common.command.Response.ResponseType; import com.redhat.thermostat.storage.core.AuthToken; import com.redhat.thermostat.storage.core.SecureStorage; import com.redhat.thermostat.storage.core.Storage; +import com.redhat.thermostat.storage.core.StorageException; @RunWith(PowerMockRunner.class) @PrepareForTest({ FrameworkUtil.class }) public class RequestQueueImplTest { + private BundleContext mockContext; + private ServiceReference mockServiceRef; + + @Before + public void setup() { + PowerMockito.mockStatic(FrameworkUtil.class); + Bundle mockBundle = mock(Bundle.class); + mockContext = mock(BundleContext.class); + mockServiceRef = mock(ServiceReference.class); + when(mockContext.getServiceReference(Storage.class.getName())).thenReturn(mockServiceRef); + when(mockBundle.getBundleContext()).thenReturn(mockContext); + when(FrameworkUtil.getBundle(RequestQueueImpl.class)).thenReturn(mockBundle); + } /* * Other tests ensure that secure storage is returned from storage providers. - * This is an attemtp to make sure that authentication hooks are actually + * This is an attempt to make sure that authentication hooks are actually * called if storage is an instance of SecureStorage. * */ @Test public void putRequestAuthenticatesForSecureStorage() { - PowerMockito.mockStatic(FrameworkUtil.class); - Bundle mockBundle = mock(Bundle.class); - BundleContext mockContext = mock(BundleContext.class); - ServiceReference mockServiceRef = mock(ServiceReference.class); - when(mockContext.getServiceReference(Storage.class.getName())).thenReturn(mockServiceRef); SecureStorage mockStorage = mock(SecureStorage.class); + when(mockContext.getService(mockServiceRef)).thenReturn(mockStorage); + AuthToken mockToken = mock(AuthToken.class); when(mockStorage.generateToken(any(String.class))).thenReturn(mockToken); - when(mockContext.getService(mockServiceRef)).thenReturn(mockStorage); - when(mockBundle.getBundleContext()).thenReturn(mockContext); - when(FrameworkUtil.getBundle(RequestQueueImpl.class)).thenReturn(mockBundle); + ConfigurationRequestContext ctx = mock(ConfigurationRequestContext.class); RequestQueueImpl queue = new RequestQueueImpl(ctx); Request request = mock(Request.class); queue.putRequest(request); verify(request).setParameter(eq(Request.CLIENT_TOKEN), any(String.class)); verify(request).setParameter(eq(Request.AUTH_TOKEN), any(String.class)); + assertTrue(queue.getQueue().contains(request)); + } + + @Test + public void testNoEnqueueIfAuthFailed() { + SecureStorage mockStorage = mock(SecureStorage.class); + when(mockContext.getService(mockServiceRef)).thenReturn(mockStorage); + + when(mockStorage.generateToken(any(String.class))).thenThrow(new StorageException()); + + ConfigurationRequestContext ctx = mock(ConfigurationRequestContext.class); + RequestQueueImpl queue = new RequestQueueImpl(ctx); + Request request = mock(Request.class); + + RequestResponseListener listener = mock(RequestResponseListener.class); + when(request.getListeners()).thenReturn(Arrays.asList(listener)); + queue.putRequest(request); + + ArgumentCaptor responseCaptor = ArgumentCaptor.forClass(Response.class); + verify(listener).fireComplete(eq(request), responseCaptor.capture()); + assertEquals(ResponseType.AUTH_FAILED, responseCaptor.getValue().getType()); + assertFalse(queue.getQueue().contains(request)); + } + + @Test + public void testEnqueueNoSecureStorage() { + Storage mockStorage = mock(Storage.class); + when(mockContext.getService(mockServiceRef)).thenReturn(mockStorage); + + ConfigurationRequestContext ctx = mock(ConfigurationRequestContext.class); + RequestQueueImpl queue = new RequestQueueImpl(ctx); + Request request = mock(Request.class); + queue.putRequest(request); + assertTrue(queue.getQueue().contains(request)); } }