Skip to content

Commit b5c298e

Browse files
committed
Remove legacy CSRF protection for approve page
Instead, we rely on the Spring Security CSRF protection, like we already do for the login page. Additionally, we remove the authentication check in`isApproved`, because this is already done by Spring Security (and if not, we have bigger problems to worry about).
1 parent 8b362f2 commit b5c298e

File tree

5 files changed

+5
-34
lines changed

5 files changed

+5
-34
lines changed

openid-connect-server-webapp/src/main/webapp/WEB-INF/views/approve.jsp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -260,8 +260,8 @@
260260
</h3>
261261
<spring:message code="approve.label.authorize" var="authorize_label"/>
262262
<spring:message code="approve.label.deny" var="deny_label"/>
263-
<input id="user_oauth_approval" name="user_oauth_approval" value="true" type="hidden" />
264-
<input name="csrf" value="${ csrf }" type="hidden" />
263+
<input id="user_oauth_approval" name="user_oauth_approval" value="true" type="hidden" />
264+
<input type="hidden" name="${_csrf.parameterName}" value="${_csrf.token}" />
265265
<input name="authorize" value="${authorize_label}" type="submit"
266266
onclick="$('#user_oauth_approval').attr('value',true)" class="btn btn-success btn-large" />
267267
&nbsp;

openid-connect-server/src/main/java/org/mitre/oauth2/web/OAuthConfirmationController.java

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@
5757
import com.google.common.collect.Sets;
5858
import com.google.gson.JsonObject;
5959

60-
import static org.mitre.openid.connect.request.ConnectRequestParameters.CSRF;
6160
import static org.mitre.openid.connect.request.ConnectRequestParameters.PROMPT;
6261
import static org.mitre.openid.connect.request.ConnectRequestParameters.PROMPT_SEPARATOR;
6362

@@ -221,9 +220,6 @@ public String confimAccess(Map<String, Object> model, @ModelAttribute("authoriza
221220
model.put("gras", false);
222221
}
223222

224-
// inject a random value for CSRF purposes
225-
model.put("csrf", authRequest.getExtensions().get(CSRF));
226-
227223
return "approve";
228224
}
229225

openid-connect-server/src/main/java/org/mitre/openid/connect/request/ConnectOAuth2RequestFactory.java

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,6 @@
5555

5656
import static org.mitre.openid.connect.request.ConnectRequestParameters.CLAIMS;
5757
import static org.mitre.openid.connect.request.ConnectRequestParameters.CLIENT_ID;
58-
import static org.mitre.openid.connect.request.ConnectRequestParameters.CSRF;
5958
import static org.mitre.openid.connect.request.ConnectRequestParameters.DISPLAY;
6059
import static org.mitre.openid.connect.request.ConnectRequestParameters.LOGIN_HINT;
6160
import static org.mitre.openid.connect.request.ConnectRequestParameters.MAX_AGE;
@@ -157,13 +156,6 @@ public AuthorizationRequest createAuthorizationRequest(Map<String, String> input
157156
}
158157
}
159158

160-
161-
// add CSRF protection to the request on first parse
162-
String csrf = UUID.randomUUID().toString();
163-
request.getExtensions().put(CSRF, csrf);
164-
165-
166-
167159
return request;
168160
}
169161

openid-connect-server/src/main/java/org/mitre/openid/connect/request/ConnectRequestParameters.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ public interface ConnectRequestParameters {
3838
public String PROMPT_SEPARATOR = " ";
3939

4040
// extensions
41-
public String CSRF = "csrf";
4241
public String APPROVED_SITE = "approved_site";
4342

4443
// responses

openid-connect-server/src/main/java/org/mitre/openid/connect/token/TofuUserApprovalHandler.java

Lines changed: 3 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,6 @@
4848
import com.google.common.collect.Sets;
4949

5050
import static org.mitre.openid.connect.request.ConnectRequestParameters.APPROVED_SITE;
51-
import static org.mitre.openid.connect.request.ConnectRequestParameters.CSRF;
5251
import static org.mitre.openid.connect.request.ConnectRequestParameters.PROMPT;
5352
import static org.mitre.openid.connect.request.ConnectRequestParameters.PROMPT_SEPARATOR;
5453

@@ -102,21 +101,8 @@ public boolean isApproved(AuthorizationRequest authorizationRequest, Authenticat
102101
return true;
103102
} else {
104103
// if not, check to see if the user has approved it
105-
if (Boolean.parseBoolean(authorizationRequest.getApprovalParameters().get("user_oauth_approval"))) {// TODO: make parameter name configurable?
106-
107-
// check the value of the CSRF parameter
108-
109-
if (authorizationRequest.getExtensions().get(CSRF) != null) {
110-
if (authorizationRequest.getExtensions().get(CSRF).equals(authorizationRequest.getApprovalParameters().get(CSRF))) {
111-
112-
// make sure the user is actually authenticated
113-
return userAuthentication.isAuthenticated();
114-
}
115-
}
116-
}
117-
118-
// if the above doesn't pass, it's not yet approved
119-
return false;
104+
// TODO: make parameter name configurable?
105+
return Boolean.parseBoolean(authorizationRequest.getApprovalParameters().get("user_oauth_approval"));
120106
}
121107

122108
}
@@ -195,9 +181,7 @@ public AuthorizationRequest updateAfterApproval(AuthorizationRequest authorizati
195181
ClientDetails client = clientDetailsService.loadClientByClientId(clientId);
196182

197183
// This must be re-parsed here because SECOAUTH forces us to call things in a strange order
198-
if (Boolean.parseBoolean(authorizationRequest.getApprovalParameters().get("user_oauth_approval"))
199-
&& authorizationRequest.getExtensions().get(CSRF) != null
200-
&& authorizationRequest.getExtensions().get(CSRF).equals(authorizationRequest.getApprovalParameters().get(CSRF))) {
184+
if (Boolean.parseBoolean(authorizationRequest.getApprovalParameters().get("user_oauth_approval"))) {
201185

202186
authorizationRequest.setApproved(true);
203187

0 commit comments

Comments
 (0)