OA-37 - Fall back to client_id if id_token cannot be resolved

omod/src/main/java/org/openmrs/module/oauth2login/web/controller/CustomLogoutSuccessHandler.java

@@ -24,6 +24,8 @@
 import org.springframework.security.core.Authentication;
 import org.springframework.security.web.authentication.logout.LogoutSuccessHandler;
 import org.springframework.security.web.authentication.logout.SimpleUrlLogoutSuccessHandler;
+import org.springframework.util.MultiValueMap;
+import org.springframework.web.util.UriComponentsBuilder;
 
 public class CustomLogoutSuccessHandler extends SimpleUrlLogoutSuccessHandler implements LogoutSuccessHandler {
 
@@ -34,9 +36,23 @@ public void onLogoutSuccess(HttpServletRequest request, HttpServletResponse resp
 		String redirectPath = properties.getProperty("logoutUri");
 		//the redirect path can contain a [token] that should be replaced by the aut token
 		if (StringUtils.isNoneBlank(redirectPath) && redirectPath.contains("[token]")) {
-			String token = Context.getAuthenticatedUser().getUserProperty(USER_PROP_ID_TOKEN);
-			String encoded = URLEncoder.encode(token, "UTF-8");
-			redirectPath = StringUtils.replace(redirectPath, "[token]", encoded);
+			String token = null;
+			if (Context.getAuthenticatedUser() != null) {
+				token = Context.getAuthenticatedUser().getUserProperty(USER_PROP_ID_TOKEN);
+			}
+
+			if (StringUtils.isNotBlank(token)) {
+				String encoded = URLEncoder.encode(token, "UTF-8");
+				redirectPath = StringUtils.replace(redirectPath, "[token]", encoded);
+			} else {
+				//Oauth2 specification requires the id_token_hint or client_id, fallback to client_id
+				UriComponentsBuilder urlBuilder = UriComponentsBuilder.fromHttpUrl(redirectPath);
+				MultiValueMap<String, String> params = urlBuilder.build().getQueryParams();
+				if (!params.containsKey("client_id")) {
+					redirectPath = StringUtils.replace(redirectPath, "id_token_hint=[token]",
+					    "client_id=" + properties.getProperty("clientId"));
+				}
+			}
 		}
 		Context.logout();
 		redirectPath = StringUtils.defaultIfBlank(redirectPath, request.getContextPath() + "/oauth2login");

omod/src/main/java/org/openmrs/module/oauth2login/web/filter/OAuth2LoginRequestFilter.java

@@ -9,16 +9,22 @@
  */
 package org.openmrs.module.oauth2login.web.filter;
 
-import org.apache.commons.lang3.StringUtils;
-import org.openmrs.api.context.Context;
-
-import javax.servlet.*;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.List;
 
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import org.apache.commons.lang3.StringUtils;
+import org.openmrs.api.context.Context;
+
 /**
  * This servlet filter ensures that the only way to authenticate is through the appropriate URI
  * "/oauth2login". It also disables the possibility to logout from OpenMRS since the logout process
@@ -64,7 +70,7 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo
 		if (!requestURIs.contains(requestURI) && !servletPaths.contains(servletPath)) {
 
 			// Logout (forwarding)
-			if (isLogoutRequest(servletPath, httpRequest)) {
+			if (isLogoutRequest(servletPath, requestURI, httpRequest)) {
 				httpResponse.sendRedirect(httpRequest.getContextPath() + "/oauth2logout");
 				return;
 			}
@@ -80,12 +86,15 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo
 		chain.doFilter(httpRequest, httpResponse);
 	}
 
-	private boolean isLogoutRequest(String path, HttpServletRequest httpServletRequest) {
+	private boolean isLogoutRequest(String path, String uri, HttpServletRequest httpServletRequest) {
 		//"manual-logout": should be a constant from org.openmrs.module.appui.AppUiConstants
 		//in OpenMRS the path is ../../logout.action : should we use this in this test ?
 		//the attribute seems to be used in any case.
+		//TODO The module assumes that only legacyUI and RA logout endpoints are in use or won't change, they should 
+		//instead be configurable to adapt to changes and support other distributions that might use custom endpoints.
 		return path.equalsIgnoreCase("/logout")
-		//				|| path.equalsIgnoreCase("/oauth2logout")
+		        || uri.equalsIgnoreCase("/ms/logout")
+		        //				|| path.equalsIgnoreCase("/oauth2logout")
 		        || (httpServletRequest.getSession() != null && "true".equals(httpServletRequest.getSession().getAttribute(
 		            "manual-logout")));
 	}

omod/src/test/java/org/openmrs/module/oauth2login/web/controller/CustomLogoutSuccessHandlerTest.java

@@ -52,6 +52,44 @@ public void onLogoutSuccess_redirectToLogoutURL() throws IOException, ServletExc
 
 		verify(redirectStrategy, times(1)).sendRedirect(request, response,
 		    "http://localhost:8081/auth/realms/demo/protocol/openid-connect/logout?id_token_hint=" + idToken);
+	}
+
+	@Test
+	public void onLogoutSuccess_shouldNotSetIdTokenIfUserIsNotAuthenticated() throws IOException, ServletException {
+		PowerMockito.mockStatic(Context.class);
+		CustomLogoutSuccessHandler customLogoutSuccessHandler = new CustomLogoutSuccessHandler();
+		RedirectStrategy redirectStrategy = mock(RedirectStrategy.class);
+		customLogoutSuccessHandler.setRedirectStrategy(redirectStrategy);
+		MockHttpServletRequest request = new MockHttpServletRequest();
+		HttpServletResponse response = mock(HttpServletResponse.class);
+
+		customLogoutSuccessHandler.onLogoutSuccess(request, response, null);
 
+		PowerMockito.verifyStatic();
+		Context.logout();
+
+		verify(redirectStrategy).sendRedirect(request, response,
+		    "http://localhost:8081/auth/realms/demo/protocol/openid-connect/logout?client_id=openmrs");
 	}
+
+	@Test
+	public void onLogoutSuccess_shouldNotSetIdTokenIfNoneIsSetForTheUser() throws IOException, ServletException {
+		PowerMockito.mockStatic(Context.class);
+		Mockito.when(Context.getAuthenticatedUser()).thenReturn(new User());
+
+		CustomLogoutSuccessHandler customLogoutSuccessHandler = new CustomLogoutSuccessHandler();
+		RedirectStrategy redirectStrategy = mock(RedirectStrategy.class);
+		customLogoutSuccessHandler.setRedirectStrategy(redirectStrategy);
+		MockHttpServletRequest request = new MockHttpServletRequest();
+		HttpServletResponse response = mock(HttpServletResponse.class);
+
+		customLogoutSuccessHandler.onLogoutSuccess(request, response, null);
+
+		PowerMockito.verifyStatic();
+		Context.logout();
+
+		verify(redirectStrategy).sendRedirect(request, response,
+		    "http://localhost:8081/auth/realms/demo/protocol/openid-connect/logout?client_id=openmrs");
+	}
+
 }

omod/src/test/java/org/openmrs/module/oauth2login/web/filter/OAuth2LoginRequestFilterTest.java

@@ -166,4 +166,19 @@ public void secureUri_shouldProceedWhenAuthenticated() throws Exception {
 		verify(response, never()).sendRedirect(any(String.class));
 		verify(chain, times(1)).doFilter(request, response);
 	}
+
+	@Test
+	public void doFilter_shouldRedirectToLogoutUriForLegacyUiRequests() throws Exception {
+		MockHttpServletRequest request = new MockHttpServletRequest();
+		HttpServletResponse response = mock(HttpServletResponse.class);
+		FilterChain chain = mock(FilterChain.class);
+
+		request.setServletPath("/ms");
+		request.setRequestURI("/ms/logout");
+		filter.doFilter(request, response, chain);
+
+		verify(response).sendRedirect("/oauth2logout");
+		verifyZeroInteractions(chain);
+	}
+
 }