Skip to content

Commit fb32102

Browse files
maheshpmaheshp
committed
Disallow Plugin Interact endpoint for non auth plugins #000
* Plugin interact enpoint does not need authentication, restricting access to this endpoint only for auth plugins and prohbiting request names which need authentication.
1 parent 3f1a42e commit fb32102

File tree

2 files changed

+69
-1
lines changed

2 files changed

+69
-1
lines changed

‎server/src/com/thoughtworks/go/server/plugin/controller/PluginController.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,13 @@
1616

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

19+
import com.google.common.collect.Sets;
20+
import com.thoughtworks.go.plugin.access.authentication.AuthenticationExtension;
1921
import com.thoughtworks.go.plugin.api.request.DefaultGoPluginApiRequest;
2022
import com.thoughtworks.go.plugin.api.response.DefaultGoApiResponse;
2123
import com.thoughtworks.go.plugin.api.response.GoPluginApiResponse;
2224
import com.thoughtworks.go.plugin.infra.PluginManager;
25+
import com.thoughtworks.go.server.web.ResponseCodeView;
2326
import com.thoughtworks.go.util.StringUtil;
2427
import org.springframework.beans.factory.annotation.Autowired;
2528
import org.springframework.stereotype.Controller;
@@ -34,12 +37,21 @@
3437
import java.util.Enumeration;
3538
import java.util.HashMap;
3639
import java.util.Map;
40+
import java.util.Set;
41+
42+
import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN;
3743

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

4248
private PluginManager pluginManager;
49+
private static final Set<String> BLACK_LISTED_REQUESTS = Sets.newHashSet("go.plugin-settings.get-configuration",
50+
"go.plugin-settings.get-view",
51+
"go.plugin-settings.validate-configuration",
52+
"go.authentication.plugin-configuration",
53+
"go.authentication.authenticate-user",
54+
"go.authentication.search-user");
4355

4456
@Autowired
4557
public PluginController(PluginManager pluginManager) {
@@ -51,6 +63,15 @@ public ModelAndView handlePluginInteractRequest(
5163
@PathVariable String pluginId,
5264
@PathVariable String requestName,
5365
HttpServletRequest request) {
66+
67+
if(!isAuthPlugin(pluginId)) {
68+
return ResponseCodeView.create(SC_FORBIDDEN, "Plugin interact endpoint is enabled only for Authentication Plugins");
69+
}
70+
71+
if(isRestrictedRequestName(requestName)) {
72+
return ResponseCodeView.create(SC_FORBIDDEN, String.format("Plugin interact for '%s' requestName is disallowed.", requestName));
73+
}
74+
5475
DefaultGoPluginApiRequest apiRequest = new DefaultGoPluginApiRequest(null, null, requestName);
5576
apiRequest.setRequestParams(getParameterMap(request));
5677
addRequestHeaders(request, apiRequest);
@@ -74,6 +95,14 @@ public ModelAndView handlePluginInteractRequest(
7495
throw new RuntimeException("render error page");
7596
}
7697

98+
private boolean isRestrictedRequestName(String requestName) {
99+
return BLACK_LISTED_REQUESTS.contains(requestName);
100+
}
101+
102+
private boolean isAuthPlugin(String pluginId) {
103+
return pluginManager.isPluginOfType(AuthenticationExtension.EXTENSION_NAME, pluginId);
104+
}
105+
77106
private Map<String, String> getParameterMap(HttpServletRequest request) {
78107
Map<String, String[]> springParameterMap = request.getParameterMap();
79108
Map<String, String> pluginParameterMap = new HashMap<>();

‎server/test/unit/com/thoughtworks/go/server/plugin/controller/PluginControllerTest.java‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import com.thoughtworks.go.plugin.api.request.GoPluginApiRequest;
2020
import com.thoughtworks.go.plugin.api.response.DefaultGoPluginApiResponse;
2121
import com.thoughtworks.go.plugin.infra.PluginManager;
22+
import com.thoughtworks.go.server.web.ResponseCodeView;
2223
import com.thoughtworks.go.util.ReflectionUtil;
2324
import org.junit.Before;
2425
import org.junit.Test;
@@ -29,7 +30,12 @@
2930
import javax.servlet.http.HttpServletRequest;
3031
import javax.servlet.http.HttpServletResponse;
3132
import java.io.PrintWriter;
32-
import java.util.*;
33+
import java.util.ArrayList;
34+
import java.util.Arrays;
35+
import java.util.Enumeration;
36+
import java.util.HashMap;
37+
import java.util.List;
38+
import java.util.Map;
3339

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

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

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

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

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

165+
@Test
166+
public void shouldAllowInteractionOnlyForAuthPlugins() {
167+
when(pluginManager.isPluginOfType("authentication", "github.pr")).thenReturn(false);
168+
169+
ModelAndView modelAndView = pluginController.handlePluginInteractRequest(PLUGIN_ID, REQUEST_NAME, servletRequest);
170+
ResponseCodeView view = (ResponseCodeView) modelAndView.getView();
171+
172+
assertThat(view.getStatusCode(), is(403));
173+
}
174+
175+
@Test
176+
public void shouldDisallowRequestsWhichNeedAuthentication() {
177+
when(pluginManager.isPluginOfType(any(String.class), any(String.class))).thenReturn(true);
178+
179+
List<String> restrictedRequests = Arrays.asList("go.plugin-settings.get-configuration",
180+
"go.plugin-settings.get-view",
181+
"go.plugin-settings.validate-configuration",
182+
"go.authentication.plugin-configuration",
183+
"go.authentication.authenticate-user",
184+
"go.authentication.search-user");
185+
186+
for (String requestName : restrictedRequests) {
187+
ModelAndView modelAndView = pluginController.handlePluginInteractRequest(PLUGIN_ID, requestName, servletRequest);
188+
ResponseCodeView view = (ResponseCodeView) modelAndView.getView();
189+
190+
assertThat(view.getStatusCode(), is(403));
191+
}
192+
}
193+
155194
private Enumeration<String> getMockEnumeration(List<String> elements) {
156195
Enumeration<String> enumeration = new Enumeration<String>() {
157196
private List<String> elements;

0 commit comments

Comments
 (0)