Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error-page for POST,PUT, etc. #1498

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix error-page for POST,PUT, etc. #1498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a8eb8bb
Fix error-page for POST,PUT, etc.
thomasmey Jul 19, 2019
6fc1e4c
ErrorController: user /error as path for controller and /errorview for
thomasmey Aug 3, 2019
8f228e4
ErrorController: Move code from view into controller
thomasmey Aug 3, 2019
e2d1824
ErrorController: instanceof check includes null check.
thomasmey Aug 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions openid-connect-server-webapp/src/main/java/org/mitre/openid/connect/ErrorController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package org.mitre.openid.connect;

import javax.servlet.RequestDispatcher;
import javax.servlet.http.HttpServletRequest;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.http.HttpStatus;
import org.springframework.security.oauth2.common.exceptions.OAuth2Exception;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.RequestMapping;

@Controller
public class ErrorController {

private static final Logger logger = LoggerFactory.getLogger(ErrorController.class);

@RequestMapping("/error")
public String handle(HttpServletRequest req) {
Throwable errorException = (Throwable) req.getAttribute(RequestDispatcher.ERROR_EXCEPTION);
String message = (String) req.getAttribute(RequestDispatcher.ERROR_MESSAGE);
String requestUri = (String) req.getAttribute(RequestDispatcher.ERROR_REQUEST_URI);

logger.error("request {} failed with {}", requestUri, message);
logger.error("exception", errorException);

processError(req);
return "/error-view";
}

private void processError(HttpServletRequest request) {
if (request.getAttribute("error") instanceof OAuth2Exception) {
request.setAttribute("errorCode", ((OAuth2Exception)request.getAttribute("error")).getOAuth2ErrorCode());
request.setAttribute("message", ((OAuth2Exception)request.getAttribute("error")).getMessage());
} else if (request.getAttribute(RequestDispatcher.ERROR_EXCEPTION) != null) {
Throwable t = (Throwable)request.getAttribute(RequestDispatcher.ERROR_EXCEPTION);
request.setAttribute("errorCode", t.getClass().getSimpleName() + " (" + request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE) + ")");
request.setAttribute("message", t.getMessage());
} else if (request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE) != null) {
Integer code = (Integer)request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE);
HttpStatus status = HttpStatus.valueOf(code);
request.setAttribute("errorCode", status.toString() + " " + status.getReasonPhrase());
request.setAttribute("message", request.getAttribute(RequestDispatcher.ERROR_MESSAGE));
} else {
request.setAttribute("errorCode", "Server error");
request.setAttribute("message", "See the logs for details");
}
}
}
2 changes: 1 addition & 1 deletion openid-connect-server-webapp/src/main/webapp/WEB-INF/authz-config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,6 @@
<bean id="oauthRequestValidator" class="org.mitre.oauth2.token.ScopeServiceAwareOAuth2RequestValidator" />

<!-- Error page handler. -->
<mvc:view-controller path="/error" view-name="error" />
<mvc:view-controller path="/errorview" view-name="error-view" />

</beans>
26 changes: 2 additions & 24 deletions openid-connect-server-webapp/src/main/webapp/WEB-INF/views/error.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,7 @@
<%@ taglib prefix="o" tagdir="/WEB-INF/tags"%>
<%@ taglib prefix="spring" uri="http://www.springframework.org/tags"%>
<%@ taglib prefix="security" uri="http://www.springframework.org/security/tags"%>
<%@page import="org.springframework.security.oauth2.common.exceptions.OAuth2Exception"%>
<%

if (request.getAttribute("error") != null && request.getAttribute("error") instanceof OAuth2Exception) {
request.setAttribute("errorCode", ((OAuth2Exception)request.getAttribute("error")).getOAuth2ErrorCode());
request.setAttribute("message", ((OAuth2Exception)request.getAttribute("error")).getMessage());
} else if (request.getAttribute("javax.servlet.error.exception") != null) {
Throwable t = (Throwable)request.getAttribute("javax.servlet.error.exception");
request.setAttribute("errorCode", t.getClass().getSimpleName() + " (" + request.getAttribute("javax.servlet.error.status_code") + ")");
request.setAttribute("message", t.getMessage());
} else if (request.getAttribute("javax.servlet.error.status_code") != null) {
Integer code = (Integer)request.getAttribute("javax.servlet.error.status_code");
HttpStatus status = HttpStatus.valueOf(code);
request.setAttribute("errorCode", status.toString() + " " + status.getReasonPhrase());
request.setAttribute("message", request.getAttribute("javax.servlet.error.message"));
} else {
request.setAttribute("errorCode", "Server error");
request.setAttribute("message", "See the logs for details");
}

%>
<spring:message code="error.title" var="title"/>
<o:header title="${title}" />
<o:topbar pageName="Error" />
Expand All @@ -38,11 +18,9 @@ if (request.getAttribute("error") != null && request.getAttribute("error") insta
<p>
<spring:message code="error.message"/>
<blockquote class="text-error"><b><c:out value="${ message }" /></b></blockquote>
</p>

</p>
</div>

</div>
</div>
</div>
<o:footer />
<o:footer/>
6 changes: 3 additions & 3 deletions openid-connect-server-webapp/src/main/webapp/WEB-INF/web.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@
<trim-directive-whitespaces>true</trim-directive-whitespaces>
</jsp-property-group>
</jsp-config>

<error-page>
<location>/error</location>
<location>/errorController</location>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we still map this to /error?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm not sure this is possbile without breaking things. Some other jsp pages directly redirect/forwards to /error when changing the above "catch all" error location these wont get catch. Or do I misunderstand. Let me ask the otherway around:

  • under what path should the ErrorController be reachable?
  • under what path should the error.jsp be reachable?

another thing that should be done is to move the controller logic from error.jsp in the ErrorController.

</error-page>

</web-app>
6 changes: 3 additions & 3 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,8 +404,8 @@
</dependency>
<dependency>
<groupId>javax.servlet</groupId>
<artifactId>servlet-api</artifactId>
<version>2.5</version>
<artifactId>javax.servlet-api</artifactId>
<version>3.0.1</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any other changes that updating the servlet version would bring to the underlying project?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this gives the static defines like RequestDispatcher.ERROR_MESSAGE as web.xml uses 3.0 syntax already.

<scope>provided</scope>
</dependency>
<dependency>
Expand Down Expand Up @@ -644,7 +644,7 @@
</dependency>
<dependency>
<groupId>javax.servlet</groupId>
<artifactId>servlet-api</artifactId>
<artifactId>javax.servlet-api</artifactId>
</dependency>
<dependency>
<groupId>javax.servlet.jsp</groupId>
Expand Down