diff --git a/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/clarin/ClarinShibbolethLoginFilter.java b/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/clarin/ClarinShibbolethLoginFilter.java index aca9b64ab78b..1de93e74bad6 100644 --- a/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/clarin/ClarinShibbolethLoginFilter.java +++ b/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/clarin/ClarinShibbolethLoginFilter.java @@ -78,6 +78,16 @@ public class ClarinShibbolethLoginFilter extends StatelessLoginFilter { private static final Logger log = LogManager.getLogger(org.dspace.app.rest.security.ShibbolethLoginFilter.class); + /** + * Property which handles information if the IdP send required information. + */ + private boolean isMissingHeadersFromIdp = false; + + /** + * The netId of the user for which IdP send required information, but without Email. + */ + private String netId = ""; + private ConfigurationService configurationService = DSpaceServicesFactory.getInstance().getConfigurationService(); private ClarinVerificationTokenService clarinVerificationTokenService = ClarinServiceFactory.getInstance() .getClarinVerificationTokenService(); @@ -91,6 +101,10 @@ public ClarinShibbolethLoginFilter(String url, AuthenticationManager authenticat @Override public Authentication attemptAuthentication(HttpServletRequest req, HttpServletResponse res) throws AuthenticationException { + // Reset properties in every login request + this.setMissingHeadersFromIdp(false); + this.netId = ""; + // First, if Shibboleth is not enabled, throw an immediate ProviderNotFoundException // This tells Spring Security that authentication failed if (!ClarinShibAuthentication.isEnabled()) { @@ -147,16 +161,14 @@ public Authentication attemptAuthentication(HttpServletRequest req, try { if (StringUtils.isEmpty(netid) || StringUtils.isEmpty(idp)) { log.error("Cannot load the netid or idp from the request headers."); - this.redirectToMissingHeadersPage(res); - return null; + this.setMissingHeadersFromIdp(true); } // The Idp hasn't sent the email - the user will be redirected to the page where he must fill in that // missing email if (StringUtils.isBlank(email)) { log.error("Cannot load the shib email header from the request headers."); - this.redirectToWriteEmailPage(req, res); - return null; + this.setMissingUserEmail(req, res); } } catch (IOException e) { throw new RuntimeException("Cannot redirect the user to the Shibboleth authentication error page" + @@ -215,7 +227,31 @@ protected void unsuccessfulAuthentication(HttpServletRequest request, String authenticateHeaderValue = restAuthenticationService.getWwwAuthenticateHeaderValue(request, response); response.setHeader("WWW-Authenticate", authenticateHeaderValue); - response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Authentication failed!"); + + // Get redirect URL from request parameter + String redirectUrl = request.getParameter("redirectUrl"); + + // If redirectUrl unspecified, default to the configured UI + if (StringUtils.isEmpty(redirectUrl)) { + redirectUrl = configurationService.getProperty("dspace.ui.url"); + } + + + String loginUrl = "/login/"; + redirectUrl += loginUrl; + + String missingHeadersUrl = "missing-headers"; + String userWithoutEmailUrl = "auth-failed"; + + // Compose the redirect URL + if (this.isMissingHeadersFromIdp) { + redirectUrl += missingHeadersUrl; + } else if (StringUtils.isNotEmpty(this.netId)) { + // netId is set if the user doesn't have the email + redirectUrl += userWithoutEmailUrl + "?netid=" + this.netId; + } + + response.sendRedirect(redirectUrl); log.error("Authentication failed (status:{})", HttpServletResponse.SC_UNAUTHORIZED, failed); } @@ -261,9 +297,10 @@ private void redirectAfterSuccess(HttpServletRequest request, HttpServletRespons /** * The IdP hasn't sent the `Shib-Identity-Provider` or `SHIB-NETID` header. The user is redirected to the * static error page (The UI process error message). + * */ - protected void redirectToMissingHeadersPage(HttpServletResponse res) throws IOException { - res.sendError(HttpServletResponse.SC_UNAUTHORIZED, MISSING_HEADERS_FROM_IDP); + protected void setMissingHeadersFromIdp(boolean value) { + this.isMissingHeadersFromIdp = value; } /** @@ -272,7 +309,7 @@ protected void redirectToMissingHeadersPage(HttpServletResponse res) throws IOEx * The request headers passed by IdP are stored into the `verification_token` table the `shib_headers` column * for later usage. After successful signing in the `verification_token` record is removed from the DB. */ - protected void redirectToWriteEmailPage(HttpServletRequest req, + protected void setMissingUserEmail(HttpServletRequest req, HttpServletResponse res) throws IOException { Context context = ContextUtil.obtainContext(req); String authenticateHeaderValue = restAuthenticationService.getWwwAuthenticateHeaderValue(req, res); @@ -303,14 +340,7 @@ protected void redirectToWriteEmailPage(HttpServletRequest req, + e.getMessage()); } - // Add header values to the error message to retrieve them in the FE. That headers are needed for the - // next processing. - String separator = ","; - String[] headers = new String[] {USER_WITHOUT_EMAIL_EXCEPTION, netid}; - String errorMessage = StringUtils.join(headers, separator); - - res.setHeader("WWW-Authenticate", authenticateHeaderValue); - res.sendError(HttpServletResponse.SC_UNAUTHORIZED, errorMessage); + this.netId = netid; } private String getEpersonEmail(EPerson ePerson) { diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthenticationRestControllerIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthenticationRestControllerIT.java index 81685761e8e2..88e363f97adb 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthenticationRestControllerIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthenticationRestControllerIT.java @@ -336,37 +336,40 @@ public void testStatusNotAuthenticated() throws Exception { // .andExpect(status().isNoContent()); // } - @Test - public void testShibbolethEndpointCannotBeUsedWithShibDisabled() throws Exception { - // Enable only password login - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_ONLY); - - String uiURL = configurationService.getProperty("dspace.ui.url"); - - // Verify /api/authn/shibboleth endpoint does not work - // NOTE: this is the same call as in testStatusShibAuthenticatedWithCookie()) - String token = getClient().perform(get("/api/authn/shibboleth") - .header("Referer", "https://myshib.example.com") - .param("redirectUrl", uiURL) - .requestAttr("SHIB-MAIL", eperson.getEmail()) - .requestAttr("SHIB-SCOPED-AFFILIATION", "faculty;staff")) - .andExpect(status().isUnauthorized()) - .andReturn().getResponse().getHeader("Authorization"); - - getClient(token).perform(get("/api/authn/status")) - .andExpect(status().isOk()) - .andExpect(jsonPath("$.authenticated", is(false))) - .andExpect(jsonPath("$.authenticationMethod").doesNotExist()); - - getClient(token).perform( - get("/api/authz/authorizations/search/object") - .param("embed", "feature") - .param("feature", feature) - .param("uri", utils.linkToSingleResource(ePersonRest, "self").getHref())) - .andExpect(status().isOk()) - .andExpect(jsonPath("$.page.totalElements", is(0))) - .andExpect(jsonPath("$._embedded").doesNotExist()); - } + // Note: This test was commented because the Shibboleth Authentication was customized following the Clarin + // requirements. This test was copied and updated following the Clarin updates to the + // `ClarinAuthenticationRestControllerIT#testShibbolethEndpointCannotBeUsedWithShibDisabled` method. +// @Test +// public void testShibbolethEndpointCannotBeUsedWithShibDisabled() throws Exception { +// // Enable only password login +// configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_ONLY); +// +// String uiURL = configurationService.getProperty("dspace.ui.url"); +// +// // Verify /api/authn/shibboleth endpoint does not work +// // NOTE: this is the same call as in testStatusShibAuthenticatedWithCookie()) +// String token = getClient().perform(get("/api/authn/shibboleth") +// .header("Referer", "https://myshib.example.com") +// .param("redirectUrl", uiURL) +// .requestAttr("SHIB-MAIL", eperson.getEmail()) +// .requestAttr("SHIB-SCOPED-AFFILIATION", "faculty;staff")) +// .andExpect(status().isUnauthorized()) +// .andReturn().getResponse().getHeader("Authorization"); +// +// getClient(token).perform(get("/api/authn/status")) +// .andExpect(status().isOk()) +// .andExpect(jsonPath("$.authenticated", is(false))) +// .andExpect(jsonPath("$.authenticationMethod").doesNotExist()); +// +// getClient(token).perform( +// get("/api/authz/authorizations/search/object") +// .param("embed", "feature") +// .param("feature", feature) +// .param("uri", utils.linkToSingleResource(ePersonRest, "self").getHref())) +// .andExpect(status().isOk()) +// .andExpect(jsonPath("$.page.totalElements", is(0))) +// .andExpect(jsonPath("$._embedded").doesNotExist()); +// } // NOTE: This test is similar to testStatusShibAuthenticatedWithCookie(), but proves the same process works // for Password Authentication in theory (NOTE: at this time, there's no way to create an auth cookie via the diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ClarinShibbolethLoginFilterIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ClarinShibbolethLoginFilterIT.java index 91fd4162862b..b05ee224cf0d 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ClarinShibbolethLoginFilterIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ClarinShibbolethLoginFilterIT.java @@ -7,8 +7,7 @@ */ package org.dspace.app.rest.security; -import static org.dspace.app.rest.security.clarin.ClarinShibbolethLoginFilter.MISSING_HEADERS_FROM_IDP; -import static org.dspace.app.rest.security.clarin.ClarinShibbolethLoginFilter.USER_WITHOUT_EMAIL_EXCEPTION; +import static org.dspace.app.rest.security.ShibbolethLoginFilterIT.PASS_ONLY; import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; @@ -114,8 +113,8 @@ public void shouldReturnMissingHeadersFromIdpExceptionBecauseOfMissingIdp() thro // but the user will be redirected to the page where he will fill in the user email. getClient().perform(get("/api/authn/shibboleth") .header("SHIB-NETID", netId)) - .andExpect(status().isUnauthorized()) - .andExpect(status().reason(MISSING_HEADERS_FROM_IDP)); + .andExpect(status().isFound()) + .andExpect(redirectedUrl("http://localhost:4000/login/missing-headers")); } /** @@ -129,8 +128,8 @@ public void shouldReturnMissingHeadersFromIdpExceptionBecauseOfMissingNetId() th // but the user will be redirected to the page where he will fill in the user email. getClient().perform(get("/api/authn/shibboleth") .header("Shib-Identity-Provider", idp)) - .andExpect(status().isUnauthorized()) - .andExpect(status().reason(MISSING_HEADERS_FROM_IDP)); + .andExpect(status().isFound()) + .andExpect(redirectedUrl("http://localhost:4000/login/missing-headers")); } /** @@ -139,6 +138,7 @@ public void shouldReturnMissingHeadersFromIdpExceptionBecauseOfMissingNetId() th * The request headers passed by IdP are stored into the `verification_token` table the `shib_headers` column. * The user is redirected to the page when he must fill his email. */ + // HERE @Test public void shouldReturnUserWithoutEmailException() throws Exception { String netId = "123456"; @@ -149,8 +149,8 @@ public void shouldReturnUserWithoutEmailException() throws Exception { getClient().perform(get("/api/authn/shibboleth") .header("SHIB-NETID", netId) .header("Shib-Identity-Provider", idp)) - .andExpect(status().isUnauthorized()) - .andExpect(status().reason(USER_WITHOUT_EMAIL_EXCEPTION + "," + netId)); + .andExpect(status().isFound()) + .andExpect(redirectedUrl("http://localhost:4000/login/auth-failed?netid=" + netId)); } /** @@ -178,8 +178,8 @@ public void userFillInEmailAndShouldBeRegisteredByVerificationToken() throws Exc .header("SHIB-NETID", netId) .header("SHIB-GIVENNAME", firstname) .header("SHIB-SURNAME", lastname)) - .andExpect(status().isUnauthorized()) - .andExpect(status().reason(USER_WITHOUT_EMAIL_EXCEPTION + "," + netId)); + .andExpect(status().isFound()) + .andExpect(redirectedUrl("http://localhost:4000/login/auth-failed?netid=" + netId)); // Send the email with the verification token. String tokenAdmin = getAuthToken(admin.getEmail(), password); @@ -325,6 +325,80 @@ public void testRedirectToDefaultDspaceUrl() throws Exception { } + // This test is copied from the `AuthenticationRestControllerIT` and modified following the Clarin updates. + @Test + public void testShibbolethEndpointCannotBeUsedWithShibDisabled() throws Exception { + // Enable only password login + configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_ONLY); + + String uiURL = configurationService.getProperty("dspace.ui.url"); + + // Verify /api/authn/shibboleth endpoint does not work + // NOTE: this is the same call as in testStatusShibAuthenticatedWithCookie()) + String token = getClient().perform(get("/api/authn/shibboleth") + .header("Referer", "https://myshib.example.com") + .param("redirectUrl", uiURL) + .requestAttr("SHIB-MAIL", eperson.getEmail()) + .requestAttr("SHIB-SCOPED-AFFILIATION", "faculty;staff")) + .andExpect(status().isFound()) + .andReturn().getResponse().getHeader("Authorization"); + + getClient(token).perform(get("/api/authn/status")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.authenticated", is(false))) + .andExpect(jsonPath("$.authenticationMethod").doesNotExist()); + + getClient(token).perform( + get("/api/authz/authorizations/search/object") + .param("embed", "feature") + .param("feature", feature) + .param("uri", utils.linkToSingleResource(ePersonRest, "self").getHref())) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.page.totalElements", is(0))) + .andExpect(jsonPath("$._embedded").doesNotExist()); + } + + // This test is copied from the `ShibbolethLoginFilterIT` and modified following the Clarin updates. + @Test + public void testNoRedirectIfInvalidShibAttributes() throws Exception { + // In this request, we use a SHIB-MAIL attribute which does NOT match an EPerson. + getClient().perform(get("/api/authn/shibboleth") + .requestAttr("SHIB-MAIL", "not-an-eperson@example.com")) + .andExpect(status().isFound()); + } + + // This test is copied from the `ShibbolethLoginFilterIT` and modified following the Clarin updates. + @Test + public void testNoRedirectIfShibbolethDisabled() throws Exception { + // Enable Password authentication ONLY + configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_ONLY); + + // Test redirecting to a trusted URL (same as previous test). + // This time we should be unauthorized as Shibboleth is disabled. + getClient().perform(get("/api/authn/shibboleth") + .param("redirectUrl", "http://localhost:8080/server/api/authn/status") + .requestAttr("SHIB-MAIL", eperson.getEmail())) + .andExpect(status().isFound()); + } + + // This test is copied from the `ShibbolethLoginFilterIT` and modified following the Clarin updates. + @Test + public void testRedirectRequiresShibAttributes2() throws Exception { + String token = getAuthToken(eperson.getEmail(), password); + + // Verify this endpoint also doesn't work using a regular auth token (again if SHIB-* attributes missing) + getClient(token).perform(get("/api/authn/shibboleth")) + .andExpect(status().isFound()); + } + + // This test is copied from the `ShibbolethLoginFilterIT` and modified following the Clarin updates. + @Test + public void testRedirectRequiresShibAttributes() throws Exception { + // Verify this endpoint doesn't work if no SHIB-* attributes are set + getClient().perform(get("/api/authn/shibboleth")) + .andExpect(status().isFound()); + } + // This test is copied from the `ShibbolethLoginFilterIT` and modified following the Clarin updates. @Test public void testRedirectToAnotherGivenTrustedUrl() throws Exception { diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ShibbolethLoginFilterIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ShibbolethLoginFilterIT.java index 8ad78591983c..ac07c7fcf9ab 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ShibbolethLoginFilterIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/security/ShibbolethLoginFilterIT.java @@ -7,9 +7,6 @@ */ package org.dspace.app.rest.security; -import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; - import org.dspace.app.rest.authorization.AuthorizationFeature; import org.dspace.app.rest.authorization.AuthorizationFeatureService; import org.dspace.app.rest.authorization.impl.CanChangePasswordFeature; @@ -20,7 +17,7 @@ import org.dspace.app.rest.utils.Utils; import org.dspace.services.ConfigurationService; import org.junit.Before; -import org.junit.Test; +import org.junit.Ignore; import org.springframework.beans.factory.annotation.Autowired; /** @@ -28,6 +25,9 @@ * * @author Giuseppe Digilio (giuseppe dot digilio at 4science dot it) */ +// This class is ignored because all tests was copied and updated due to CLARIN requirements into +// `ClarinShibbolethLoginFilterIT` +@Ignore public class ShibbolethLoginFilterIT extends AbstractControllerIntegrationTest { @Autowired @@ -122,18 +122,21 @@ public void setup() throws Exception { // // } - @Test - public void testNoRedirectIfShibbolethDisabled() throws Exception { - // Enable Password authentication ONLY - configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_ONLY); - - // Test redirecting to a trusted URL (same as previous test). - // This time we should be unauthorized as Shibboleth is disabled. - getClient().perform(get("/api/authn/shibboleth") - .param("redirectUrl", "http://localhost:8080/server/api/authn/status") - .requestAttr("SHIB-MAIL", eperson.getEmail())) - .andExpect(status().isUnauthorized()); - } + // Note: This test was commented because the Shibboleth Authentication was customized following the Clarin + // requirements. This test was copied and updated following the Clarin updates to the + // `ClarinShibbolethLoginFilter#testNoRedirectIfShibbolethDisabled` method. +// @Test +// public void testNoRedirectIfShibbolethDisabled() throws Exception { +// // Enable Password authentication ONLY +// configurationService.setProperty("plugin.sequence.org.dspace.authenticate.AuthenticationMethod", PASS_ONLY); +// +// // Test redirecting to a trusted URL (same as previous test). +// // This time we should be unauthorized as Shibboleth is disabled. +// getClient().perform(get("/api/authn/shibboleth") +// .param("redirectUrl", "http://localhost:8080/server/api/authn/status") +// .requestAttr("SHIB-MAIL", eperson.getEmail())) +// .andExpect(status().isUnauthorized()); +// } // Note: This test was commented because the Shibboleth Authentication was customized following the Clarin // requirements. This test was copied and updated following the Clarin updates to the @@ -163,29 +166,38 @@ public void testNoRedirectIfShibbolethDisabled() throws Exception { // .andExpect(status().isBadRequest()); // } - @Test - public void testNoRedirectIfInvalidShibAttributes() throws Exception { - // In this request, we use a SHIB-MAIL attribute which does NOT match an EPerson. - getClient().perform(get("/api/authn/shibboleth") - .requestAttr("SHIB-MAIL", "not-an-eperson@example.com")) - .andExpect(status().isUnauthorized()); - } - - @Test - public void testRedirectRequiresShibAttributes() throws Exception { - // Verify this endpoint doesn't work if no SHIB-* attributes are set - getClient().perform(get("/api/authn/shibboleth")) - .andExpect(status().isUnauthorized()); - } + // Note: This test was commented because the Shibboleth Authentication was customized following the Clarin + // requirements. This test was copied and updated following the Clarin updates to the + // `ClarinShibbolethLoginFilter#testNoRedirectIfInvalidShibAttributes` method. +// @Test +// public void testNoRedirectIfInvalidShibAttributes() throws Exception { +// // In this request, we use a SHIB-MAIL attribute which does NOT match an EPerson. +// getClient().perform(get("/api/authn/shibboleth") +// .requestAttr("SHIB-MAIL", "not-an-eperson@example.com")) +// .andExpect(status().isUnauthorized()); +// } - @Test - public void testRedirectRequiresShibAttributes2() throws Exception { - String token = getAuthToken(eperson.getEmail(), password); + // Note: This test was commented because the Shibboleth Authentication was customized following the Clarin + // requirements. This test was copied and updated following the Clarin updates to the + // `ClarinShibbolethLoginFilter#testRedirectRequiresShibAttributes` method. +// @Test +// public void testRedirectRequiresShibAttributes() throws Exception { +// // Verify this endpoint doesn't work if no SHIB-* attributes are set +// getClient().perform(get("/api/authn/shibboleth")) +// .andExpect(status().isUnauthorized()); +// } - // Verify this endpoint also doesn't work using a regular auth token (again if SHIB-* attributes missing) - getClient(token).perform(get("/api/authn/shibboleth")) - .andExpect(status().isUnauthorized()); - } + // Note: This test was commented because the Shibboleth Authentication was customized following the Clarin + // requirements. This test was copied and updated following the Clarin updates to the + // `ClarinShibbolethLoginFilter#testRedirectRequiresShibAttributes2` method. +// @Test +// public void testRedirectRequiresShibAttributes2() throws Exception { +// String token = getAuthToken(eperson.getEmail(), password); +// +// // Verify this endpoint also doesn't work using a regular auth token (again if SHIB-* attributes missing) +// getClient(token).perform(get("/api/authn/shibboleth")) +// .andExpect(status().isUnauthorized()); +// } // Note: This test was commented because the Shibboleth Authentication was customized following the Clarin // requirements. This test was copied and updated following the Clarin updates to the