Mercurial > hg > release > thermostat-0.15
changeset 1199:1e59f46b6efa
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
author | Elliott Baron <ebaron@redhat.com> |
---|---|
date | Thu, 01 Aug 2013 14:07:37 -0400 |
parents | 9cb09d6aeddb |
children | ed17d02927c5 |
files | client/command/src/main/java/com/redhat/thermostat/client/command/internal/RequestQueueImpl.java client/command/src/test/java/com/redhat/thermostat/client/command/internal/RequestQueueImplTest.java |
diffstat | 2 files changed, 85 insertions(+), 17 deletions(-) [+] |
line wrap: on
line diff
--- 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<Request> getQueue() { + return queue; + } }
--- 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<Response> 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)); } }