Skip to content

Commit

Permalink
Disallow Plugin Interact endpoint for non auth plugins #000
Browse files Browse the repository at this point in the history 
* Plugin interact enpoint does not need authentication, restricting
  access to this endpoint only for auth plugins and prohbiting request
  names which need authentication.
  • Loading branch information
@maheshp
maheshp committed Aug 11, 2016
1 parent 3f1a42e commit fb32102
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 1 deletion.
29 changes: 29 additions & 0 deletions server/src/com/thoughtworks/go/server/plugin/controller/PluginController.java
View file
Expand Up @@ -16,10 +16,13 @@

package com.thoughtworks.go.server.plugin.controller;

import com.google.common.collect.Sets;
import com.thoughtworks.go.plugin.access.authentication.AuthenticationExtension;
import com.thoughtworks.go.plugin.api.request.DefaultGoPluginApiRequest;
import com.thoughtworks.go.plugin.api.response.DefaultGoApiResponse;
import com.thoughtworks.go.plugin.api.response.GoPluginApiResponse;
import com.thoughtworks.go.plugin.infra.PluginManager;
import com.thoughtworks.go.server.web.ResponseCodeView;
import com.thoughtworks.go.util.StringUtil;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Controller;
Expand All @@ -34,12 +37,21 @@
import java.util.Enumeration;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;

import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN;

@Controller
public class PluginController {
public static final String CONTENT_TYPE_HTML = "text/html; charset=UTF-8";

private PluginManager pluginManager;
private static final Set<String> BLACK_LISTED_REQUESTS = Sets.newHashSet("go.plugin-settings.get-configuration",
"go.plugin-settings.get-view",
"go.plugin-settings.validate-configuration",
"go.authentication.plugin-configuration",
"go.authentication.authenticate-user",
"go.authentication.search-user");

@Autowired
public PluginController(PluginManager pluginManager) {
Expand All @@ -51,6 +63,15 @@ public ModelAndView handlePluginInteractRequest(
@PathVariable String pluginId,
@PathVariable String requestName,
HttpServletRequest request) {

if(!isAuthPlugin(pluginId)) {
return ResponseCodeView.create(SC_FORBIDDEN, "Plugin interact endpoint is enabled only for Authentication Plugins");
}

if(isRestrictedRequestName(requestName)) {
return ResponseCodeView.create(SC_FORBIDDEN, String.format("Plugin interact for '%s' requestName is disallowed.", requestName));
}

DefaultGoPluginApiRequest apiRequest = new DefaultGoPluginApiRequest(null, null, requestName);
apiRequest.setRequestParams(getParameterMap(request));
addRequestHeaders(request, apiRequest);
Expand All @@ -74,6 +95,14 @@ public ModelAndView handlePluginInteractRequest(
throw new RuntimeException("render error page");
}

private boolean isRestrictedRequestName(String requestName) {
return BLACK_LISTED_REQUESTS.contains(requestName);
}

private boolean isAuthPlugin(String pluginId) {
return pluginManager.isPluginOfType(AuthenticationExtension.EXTENSION_NAME, pluginId);
}

private Map<String, String> getParameterMap(HttpServletRequest request) {
Map<String, String[]> springParameterMap = request.getParameterMap();
Map<String, String> pluginParameterMap = new HashMap<>();
Expand Down
41 changes: 40 additions & 1 deletion server/test/unit/com/thoughtworks/go/server/plugin/controller/PluginControllerTest.java
View file
Expand Up @@ -19,6 +19,7 @@
import com.thoughtworks.go.plugin.api.request.GoPluginApiRequest;
import com.thoughtworks.go.plugin.api.response.DefaultGoPluginApiResponse;
import com.thoughtworks.go.plugin.infra.PluginManager;
import com.thoughtworks.go.server.web.ResponseCodeView;
import com.thoughtworks.go.util.ReflectionUtil;
import org.junit.Before;
import org.junit.Test;
Expand All @@ -29,7 +30,12 @@
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.PrintWriter;
import java.util.*;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import static org.hamcrest.Matchers.nullValue;
import static org.hamcrest.core.Is.is;
Expand Down Expand Up @@ -71,6 +77,7 @@ public void setUp() throws Exception {
@Test
public void shouldForwardWebRequestToPlugin() throws Exception {
when(pluginManager.submitTo(eq(PLUGIN_ID), requestArgumentCaptor.capture())).thenReturn(new DefaultGoPluginApiResponse(200));
when(pluginManager.isPluginOfType(any(String.class), any(String.class))).thenReturn(true);

Map<String, String[]> springParameterMap = new HashMap<String, String[]>();
springParameterMap.put("k1", new String[]{"v1"});
Expand Down Expand Up @@ -101,6 +108,7 @@ public void shouldForwardWebRequestToPlugin() throws Exception {

@Test
public void shouldRenderPluginResponseWithDefaultContentTypeOn200() throws Exception {
when(pluginManager.isPluginOfType(any(String.class), any(String.class))).thenReturn(true);
DefaultGoPluginApiResponse apiResponse = new DefaultGoPluginApiResponse(200);
String responseBody = "response-body";
apiResponse.setResponseBody(responseBody);
Expand All @@ -119,6 +127,7 @@ public void shouldRenderPluginResponseWithDefaultContentTypeOn200() throws Excep

@Test
public void shouldRenderPluginResponseWithSpecifiedContentTypeOn200() throws Exception {
when(pluginManager.isPluginOfType(any(String.class), any(String.class))).thenReturn(true);
DefaultGoPluginApiResponse apiResponse = new DefaultGoPluginApiResponse(200);
String contentType = "image/png";
apiResponse.responseHeaders().put("Content-Type", contentType);
Expand All @@ -138,6 +147,7 @@ public void shouldRenderPluginResponseWithSpecifiedContentTypeOn200() throws Exc

@Test
public void shouldRedirectToSpecifiedLocationOn302() throws Exception {
when(pluginManager.isPluginOfType(any(String.class), any(String.class))).thenReturn(true);
DefaultGoPluginApiResponse apiResponse = new DefaultGoPluginApiResponse(302);
String redirectLocation = "/go/plugin/interact/plugin.id/request.name";
apiResponse.responseHeaders().put("Location", redirectLocation);
Expand All @@ -152,6 +162,35 @@ public void shouldRedirectToSpecifiedLocationOn302() throws Exception {
assertThat(modelAndView.getViewName(), is("redirect:" + redirectLocation));
}

@Test
public void shouldAllowInteractionOnlyForAuthPlugins() {
when(pluginManager.isPluginOfType("authentication", "github.pr")).thenReturn(false);

ModelAndView modelAndView = pluginController.handlePluginInteractRequest(PLUGIN_ID, REQUEST_NAME, servletRequest);
ResponseCodeView view = (ResponseCodeView) modelAndView.getView();

assertThat(view.getStatusCode(), is(403));
}

@Test
public void shouldDisallowRequestsWhichNeedAuthentication() {
when(pluginManager.isPluginOfType(any(String.class), any(String.class))).thenReturn(true);

List<String> restrictedRequests = Arrays.asList("go.plugin-settings.get-configuration",
"go.plugin-settings.get-view",
"go.plugin-settings.validate-configuration",
"go.authentication.plugin-configuration",
"go.authentication.authenticate-user",
"go.authentication.search-user");

for (String requestName : restrictedRequests) {
ModelAndView modelAndView = pluginController.handlePluginInteractRequest(PLUGIN_ID, requestName, servletRequest);
ResponseCodeView view = (ResponseCodeView) modelAndView.getView();

assertThat(view.getStatusCode(), is(403));
}
}

private Enumeration<String> getMockEnumeration(List<String> elements) {
Enumeration<String> enumeration = new Enumeration<String>() {
private List<String> elements;
Expand Down

0 comments on commit fb32102

Please sign in to comment.