From ec7857cc5e353d0afdf40a80fb50e1c1b2ce2827 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 30 Oct 2019 10:47:09 +0100 Subject: [PATCH 01/19] Remove unused leftover line from AbstractOAuth2AuthenticationProvider.getUserRecord(). --- .../providers/oauth2/AbstractOAuth2AuthenticationProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java index 700bd17a176..5e8102c7606 100644 --- a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java +++ b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java @@ -127,7 +127,7 @@ public OAuth2UserRecord getUserRecord(String code, @NotNull OAuth20Service servi throws IOException, OAuth2Exception, InterruptedException, ExecutionException { OAuth2AccessToken accessToken = service.getAccessToken(code); - //final String userEndpoint = getUserEndpoint(accessToken); + // We need to check if scope is null first: GitHub is used without scope, so the responses scope is null. // Checking scopes via Stream to be independent from order. if ( ( accessToken.getScope() != null && ! getScope().stream().allMatch(accessToken.getScope()::contains) ) || From 2ef6951bebac1e368e9dbf2fbf75bb77379a1089 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 6 Nov 2019 12:57:53 +0100 Subject: [PATCH 02/19] Create unit test for OAuth2LoginBackingBean and make createState() testable. --- .../oauth2/OAuth2LoginBackingBean.java | 10 ++++++-- .../oauth2/OAuth2LoginBackingBeanTest.java | 23 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java index 5a43c255fcf..da62cb52f8d 100644 --- a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java +++ b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java @@ -192,8 +192,14 @@ private Optional parseStateFromRequest(@No return Optional.empty(); } } - - private String createState(AbstractOAuth2AuthenticationProvider idp, Optional redirectPage ) { + + /** + * Create a randomized unique state string to be used while crafting the autorization request + * @param idp + * @param redirectPage + * @return Random state string, composed from system time, random numbers and redirectPage parameter + */ + String createState(AbstractOAuth2AuthenticationProvider idp, Optional redirectPage) { if (idp == null) { throw new IllegalArgumentException("idp cannot be null"); } diff --git a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java new file mode 100644 index 00000000000..eb7a16c0769 --- /dev/null +++ b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java @@ -0,0 +1,23 @@ +package edu.harvard.iq.dataverse.authorization.providers.oauth2; + +import org.junit.jupiter.api.Test; + +import java.util.Optional; + +import static org.junit.jupiter.api.Assertions.*; + +class OAuth2LoginBackingBeanTest { + + OAuth2LoginBackingBean loginBackingBean = new OAuth2LoginBackingBean(); + + @Test + void createStateFailNullIdp() { + // given + AbstractOAuth2AuthenticationProvider idp = null; + Optional page = Optional.empty(); + // when & then + assertThrows(IllegalArgumentException.class, () -> { + loginBackingBean.createState(idp, page); + }); + } +} \ No newline at end of file From 3913e4252a249f0720d827e7140467dcd7392c9d Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 6 Nov 2019 16:10:02 +0100 Subject: [PATCH 03/19] Refactor OAuth2LoginBackingBean.linkFor(), moving link generation to abstracted provider. Added unit test, too. --- pom.xml | 6 +++ .../AbstractOAuth2AuthenticationProvider.java | 16 ++++++ .../oauth2/OAuth2LoginBackingBean.java | 12 +---- .../oauth2/OAuth2LoginBackingBeanTest.java | 49 ++++++++++++++++++- 4 files changed, 72 insertions(+), 11 deletions(-) diff --git a/pom.xml b/pom.xml index cc1954fd919..3caa6bafdab 100644 --- a/pom.xml +++ b/pom.xml @@ -142,6 +142,12 @@ ${junit.vintage.version} test + + org.hamcrest + hamcrest-library + 2.2 + test + org.glassfish javax.json diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java index 5e8102c7606..f143c68353f 100644 --- a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java +++ b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java @@ -6,6 +6,7 @@ import com.github.scribejava.core.model.OAuthRequest; import com.github.scribejava.core.model.Response; import com.github.scribejava.core.model.Verb; +import com.github.scribejava.core.oauth.AuthorizationUrlBuilder; import com.github.scribejava.core.oauth.OAuth20Service; import edu.harvard.iq.dataverse.authorization.AuthenticatedUserDisplayInfo; import edu.harvard.iq.dataverse.authorization.AuthenticationProvider; @@ -99,6 +100,21 @@ public String toString() { protected abstract ParsedUserResponse parseUserResponse( String responseBody ); + /** + * Build an Authorization URL for this identity provider + * @param state A randomized state, necessary to secure the authorization flow. @see OAuth2LoginBackingBean.createState() + * @param callbackUrl URL where the provider should send the browser after authn in code flow + */ + public String buildAuthzUrl(String state, String callbackUrl) { + OAuth20Service svc = this.getService(callbackUrl); + + AuthorizationUrlBuilder aub = svc.createAuthorizationUrlBuilder().state(state); + // Do not include scope if empty string (necessary for GitHub) + if (!this.getSpacedScope().isEmpty()) { aub.scope(this.getSpacedScope()); } + + return aub.build(); + } + /** * Build an OAuth20Service based on client ID & secret. Add default scope and insert * callback URL. Build uses the real API object for the target service like GitHub etc. diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java index da62cb52f8d..4c6f19cb9f2 100644 --- a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java +++ b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java @@ -62,7 +62,7 @@ public class OAuth2LoginBackingBean implements Serializable { @Inject OAuth2FirstLoginPage newAccountPage; - + /** * Generate the OAuth2 Provider URL to be used in the login page link for the provider. * @param idpId Unique ID for the provider (used to lookup in authn service bean) @@ -71,16 +71,8 @@ public class OAuth2LoginBackingBean implements Serializable { */ public String linkFor(String idpId, String redirectPage) { AbstractOAuth2AuthenticationProvider idp = authenticationSvc.getOAuth2Provider(idpId); - OAuth20Service svc = idp.getService(systemConfig.getOAuth2CallbackUrl()); String state = createState(idp, toOption(redirectPage)); - - AuthorizationUrlBuilder aub = svc.createAuthorizationUrlBuilder() - .state(state); - - // Do not include scope if empty string (necessary for GitHub) - if (!idp.getSpacedScope().isEmpty()) { aub.scope(idp.getSpacedScope()); } - - return aub.build(); + return idp.buildAuthzUrl(state, systemConfig.getOAuth2CallbackUrl()); } /** diff --git a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java index eb7a16c0769..15f8e8177a2 100644 --- a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java @@ -1,15 +1,62 @@ package edu.harvard.iq.dataverse.authorization.providers.oauth2; +import edu.harvard.iq.dataverse.authorization.AuthenticationServiceBean; +import edu.harvard.iq.dataverse.authorization.providers.oauth2.impl.GitHubOAuth2APTest; +import edu.harvard.iq.dataverse.util.SystemConfig; +import org.hamcrest.core.StringContains; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import static org.mockito.Mockito.when; import java.util.Optional; -import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.*; +@ExtendWith(MockitoExtension.class) class OAuth2LoginBackingBeanTest { OAuth2LoginBackingBean loginBackingBean = new OAuth2LoginBackingBean(); + @Mock + AuthenticationServiceBean authenticationServiceBean; + @Mock + SystemConfig systemConfig; + + @BeforeEach + void setUp() { + this.loginBackingBean.authenticationSvc = this.authenticationServiceBean; + this.loginBackingBean.systemConfig = this.systemConfig; + } + + /** + * TODO: this should be a parameterized test case testing all providers available + */ + @Test + void linkFor() { + // given + String idpId = "github"; + String redirectPage = "dataverse.xhtml"; // @see LoginPage.redirectPage + String callbackURL = "oauth2/callback.xhtml"; + AbstractOAuth2AuthenticationProvider idp = new GitHubOAuth2APTest(); + + // when + when(this.authenticationServiceBean.getOAuth2Provider(idpId)).thenReturn(idp); + when(this.systemConfig.getOAuth2CallbackUrl()).thenReturn(callbackURL); + + String link = loginBackingBean.linkFor(idpId, redirectPage); + + // then + assertThat(link, notNullValue()); + assertThat(link, not(isEmptyString())); + assertThat(link, StringContains.containsString(idp.getService(callbackURL).getAuthorizationUrl())); + } + @Test void createStateFailNullIdp() { // given From 7523aba129b1d8b6ebfdc208edfcdc124b833455 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Thu, 7 Nov 2019 15:28:32 +0100 Subject: [PATCH 04/19] Update JUnit5 libraries. --- pom.xml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pom.xml b/pom.xml index 3caa6bafdab..bcffdaa28c9 100644 --- a/pom.xml +++ b/pom.xml @@ -28,9 +28,9 @@ 1.2 4.5.5 4.12 - 5.3.1 - 5.3.1 - 1.3.1 + 5.5.2 + 5.5.2 + 1.5.2 2.28.2 5.2.4 1.20.1 From 69e021237838d27f75d5a702a475decda187395c Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Fri, 8 Nov 2019 12:59:27 +0100 Subject: [PATCH 05/19] Make OAuth2LoginBackingBean.parseStateFromRequest() testable. Add tests. --- .../oauth2/OAuth2LoginBackingBean.java | 28 +++++--- .../oauth2/OAuth2LoginBackingBeanTest.java | 66 +++++++++++++++++++ 2 files changed, 85 insertions(+), 9 deletions(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java index 4c6f19cb9f2..853d5085f1b 100644 --- a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java +++ b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java @@ -41,10 +41,10 @@ public class OAuth2LoginBackingBean implements Serializable { private static final Logger logger = Logger.getLogger(OAuth2LoginBackingBean.class.getName()); - private static final long STATE_TIMEOUT = 1000 * 60 * 15; // 15 minutes in msec + static final long STATE_TIMEOUT = 1000 * 60 * 15; // 15 minutes in msec private int responseCode; private String responseBody; - private Optional redirectPage; + Optional redirectPage = Optional.empty(); private OAuth2Exception error; private OAuth2UserRecord oauthUser; @@ -83,7 +83,7 @@ public void exchangeCodeForToken() throws IOException { HttpServletRequest req = (HttpServletRequest) FacesContext.getCurrentInstance().getExternalContext().getRequest(); try { - Optional oIdp = parseStateFromRequest(req); + Optional oIdp = parseStateFromRequest(req.getParameter("state")); Optional code = parseCodeFromRequest(req); if (oIdp.isPresent() && code.isPresent()) { @@ -144,11 +144,21 @@ private Optional parseCodeFromRequest(@NotNull HttpServletRequest req) { } return Optional.of(code); } - - private Optional parseStateFromRequest(@NotNull HttpServletRequest req) { - String state = req.getParameter("state"); - - if (state == null) { + + /** + * Parse and verify the state returned from the provider. + * + * As it contains the providers implementation "id" field when send by us, + * we can return the corresponding provider object. + * + * This function is not side effect free: it will (if present) set {@link #redirectPage} + * to the value received from the state. + * + * @param state The state string, created in {@link #createState(AbstractOAuth2AuthenticationProvider, Optional)}, send and returned by provider + * @return A corresponding provider object when state verification succeeded. + */ + Optional parseStateFromRequest(@NotNull String state) { + if (state == null || state.trim().equals("")) { logger.log(Level.INFO, "No state present in request"); return Optional.empty(); } @@ -172,7 +182,7 @@ private Optional parseStateFromRequest(@No long timeDifference = System.currentTimeMillis() - timeOrigin; if (timeDifference > 0 && timeDifference < STATE_TIMEOUT) { if ( stateFields.length > 3) { - redirectPage = Optional.ofNullable(stateFields[3]); + this.redirectPage = Optional.ofNullable(stateFields[3]); } return Optional.of(idp); } else { diff --git a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java index 15f8e8177a2..84d13b87787 100644 --- a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java @@ -2,17 +2,21 @@ import edu.harvard.iq.dataverse.authorization.AuthenticationServiceBean; import edu.harvard.iq.dataverse.authorization.providers.oauth2.impl.GitHubOAuth2APTest; +import edu.harvard.iq.dataverse.util.StringUtil; import edu.harvard.iq.dataverse.util.SystemConfig; import org.hamcrest.core.StringContains; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.*; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import static org.mockito.Mockito.when; import java.util.Optional; +import java.util.stream.Stream; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.hamcrest.MatcherAssert.assertThat; @@ -22,6 +26,7 @@ class OAuth2LoginBackingBeanTest { OAuth2LoginBackingBean loginBackingBean = new OAuth2LoginBackingBean(); + static AbstractOAuth2AuthenticationProvider testIdp = new GitHubOAuth2APTest(); @Mock AuthenticationServiceBean authenticationServiceBean; @@ -67,4 +72,65 @@ void createStateFailNullIdp() { loginBackingBean.createState(idp, page); }); } + + @ParameterizedTest(name = "\"{0}\" should be Optional.empty") + @NullSource + @EmptySource + @ValueSource(strings = {" ", "\t"}) + void parseStateFromRequestStateNullOrEmpty(String state) { + assertThat(loginBackingBean.parseStateFromRequest(state), is(Optional.empty())); + } + + @ParameterizedTest(name = "\"{0}\" should be Optional.empty") + @ValueSource(strings = {"test", "test~", "test~test"}) + void parseStateFromRequestStateInvalidString(String state) { + assertThat(loginBackingBean.parseStateFromRequest(state), is(Optional.empty())); + } + + static Stream tamperedStates() { + return Stream.of( + // encrypted nonsense + Arguments.of(testIdp.getId() + "~" + StringUtil.encrypt("test", testIdp.getClientSecret())), + // expired timestamp < 0 + Arguments.of(testIdp.getId() + "~" + StringUtil.encrypt(testIdp.getId()+"~-1", testIdp.getClientSecret())), + // expired timestamp (10 milliseconds to old...) + Arguments.of(testIdp.getId() + "~" + StringUtil.encrypt(testIdp.getId()+"~"+(System.currentTimeMillis()-OAuth2LoginBackingBean.STATE_TIMEOUT-10), testIdp.getClientSecret())) + ); + } + @ParameterizedTest + @MethodSource("tamperedStates") + void parseStateFromRequestStateTampered(String state) { + // when + when(this.authenticationServiceBean.getOAuth2Provider(testIdp.getId())).thenReturn(testIdp); + + // then + assertThat(loginBackingBean.parseStateFromRequest(state), is(Optional.empty())); + } + + /** + * Testing for side effect with proper parameters. + */ + static Stream provideStates() { + return Stream.of( + Arguments.of(Optional.of("dataverse.xhtml"), true), + Arguments.of(Optional.empty(), false) + ); + } + @ParameterizedTest + @MethodSource("provideStates") + void parseStateFromRequestStateValid(Optional redirectPage, boolean present) { + // given + String idpId = testIdp.getId(); + + // when + when(this.authenticationServiceBean.getOAuth2Provider(idpId)).thenReturn(testIdp); + String stateWithRedirect = loginBackingBean.createState(testIdp, redirectPage); + + // then + assertThat(loginBackingBean.parseStateFromRequest(stateWithRedirect), is(Optional.of(testIdp))); + assertThat(loginBackingBean.redirectPage.isPresent(), is(present)); + if (present) { + assertThat(loginBackingBean.redirectPage.get(), equalTo(redirectPage.get())); + } + } } \ No newline at end of file From 3169e6fe24a91de77c473594d92e4e362a973904 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Fri, 8 Nov 2019 14:16:08 +0100 Subject: [PATCH 06/19] Annotate OAuth2LoginBackingBean methods with TODOs for (later) refactoring. --- .../oauth2/OAuth2LoginBackingBean.java | 37 ++++++++++++++++--- 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java index 853d5085f1b..73401d8a69e 100644 --- a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java +++ b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java @@ -46,6 +46,9 @@ public class OAuth2LoginBackingBean implements Serializable { private String responseBody; Optional redirectPage = Optional.empty(); private OAuth2Exception error; + /** + * TODO: Only used in exchangeCodeForToken(). Make local var in method. + */ private OAuth2UserRecord oauthUser; @EJB @@ -124,6 +127,10 @@ public void exchangeCodeForToken() throws IOException { } } + /** + * TODO: Refactor this to be included in calling method. + * TODO: Use org.apache.commons.io.IOUtils.toString(req.getReader()) instead of overcomplicated code below. + */ private Optional parseCodeFromRequest(@NotNull HttpServletRequest req) { String code = req.getParameter("code"); if (code == null || code.trim().isEmpty()) { @@ -215,15 +222,24 @@ String createState(AbstractOAuth2AuthenticationProvider idp, Optional re final String state = idp.getId() + "~" + encrypted; return state; } - + + /** + * TODO: Unused. Remove. + */ public String getResponseBody() { return responseBody; } - + + /** + * TODO: Unused. Remove. + */ public int getResponseCode() { return responseCode; } - + + /** + * TODO: Unused. Remove. + */ public OAuth2UserRecord getUser() { return oauthUser; } @@ -231,17 +247,26 @@ public OAuth2UserRecord getUser() { public OAuth2Exception getError() { return error; } - + + /** + * TODO: Unused. Remove. + */ public boolean isInError() { return error != null; } - + + /** + * TODO: Unused. Remove. + */ public List getProviders() { return authenticationSvc.getOAuth2Providers().stream() .sorted(Comparator.comparing(AbstractOAuth2AuthenticationProvider::getTitle)) .collect(toList()); } - + + /** + * TODO: Unused. Remove. + */ public boolean isOAuth2ProvidersDefined() { return !authenticationSvc.getOAuth2Providers().isEmpty(); } From ce3c45572bee290f423d17f0f9afcbe90343b1e0 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Fri, 8 Nov 2019 14:39:09 +0100 Subject: [PATCH 07/19] Make OAuth2LoginBackingBean.exchangeCodeForToken() testable with mocked Faces context. Enhance readability. --- .../providers/oauth2/OAuth2LoginBackingBean.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java index 73401d8a69e..2667bf07874 100644 --- a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java +++ b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java @@ -29,6 +29,7 @@ import static edu.harvard.iq.dataverse.util.StringUtil.toOption; import edu.harvard.iq.dataverse.util.SystemConfig; +import org.omnifaces.util.Faces; /** * Backing bean of the oauth2 login process. Used from the login and the @@ -83,7 +84,7 @@ public String linkFor(String idpId, String redirectPage) { * @throws IOException */ public void exchangeCodeForToken() throws IOException { - HttpServletRequest req = (HttpServletRequest) FacesContext.getCurrentInstance().getExternalContext().getRequest(); + HttpServletRequest req = Faces.getRequest(); try { Optional oIdp = parseStateFromRequest(req.getParameter("state")); @@ -101,7 +102,7 @@ public void exchangeCodeForToken() throws IOException { if (dvUser == null) { // need to create the user newAccountPage.setNewUser(oauthUser); - FacesContext.getCurrentInstance().getExternalContext().redirect("/oauth2/firstLogin.xhtml"); + Faces.redirect("/oauth2/firstLogin.xhtml"); } else { // login the user and redirect to HOME of intended page (if any). @@ -111,10 +112,8 @@ public void exchangeCodeForToken() throws IOException { tokenData.setUser(dvUser); tokenData.setOauthProviderId(idp.getId()); oauth2Tokens.store(tokenData); - String destination = redirectPage.orElse("/"); - HttpServletResponse response = (HttpServletResponse) FacesContext.getCurrentInstance().getExternalContext().getResponse(); - String prettyUrl = response.encodeRedirectURL(destination); - FacesContext.getCurrentInstance().getExternalContext().redirect(prettyUrl); + + Faces.redirect(redirectPage.orElse("/")); } } } catch (OAuth2Exception ex) { From 15eed10295663aa87f55cfd06441dfbda59e39ee Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 13 Nov 2019 10:52:05 +0100 Subject: [PATCH 08/19] Refactor OAuth2LoginBackingBeanTest: easier use with mocked authSvc and test idp --- .../oauth2/OAuth2LoginBackingBeanTest.java | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java index 84d13b87787..89cf8c7b8f1 100644 --- a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java @@ -13,6 +13,7 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.when; import java.util.Optional; @@ -37,6 +38,7 @@ class OAuth2LoginBackingBeanTest { void setUp() { this.loginBackingBean.authenticationSvc = this.authenticationServiceBean; this.loginBackingBean.systemConfig = this.systemConfig; + lenient().when(this.authenticationServiceBean.getOAuth2Provider(testIdp.getId())).thenReturn(testIdp); } /** @@ -45,21 +47,18 @@ void setUp() { @Test void linkFor() { // given - String idpId = "github"; String redirectPage = "dataverse.xhtml"; // @see LoginPage.redirectPage String callbackURL = "oauth2/callback.xhtml"; - AbstractOAuth2AuthenticationProvider idp = new GitHubOAuth2APTest(); // when - when(this.authenticationServiceBean.getOAuth2Provider(idpId)).thenReturn(idp); when(this.systemConfig.getOAuth2CallbackUrl()).thenReturn(callbackURL); - String link = loginBackingBean.linkFor(idpId, redirectPage); + String link = loginBackingBean.linkFor(testIdp.getId(), redirectPage); // then assertThat(link, notNullValue()); assertThat(link, not(isEmptyString())); - assertThat(link, StringContains.containsString(idp.getService(callbackURL).getAuthorizationUrl())); + assertThat(link, StringContains.containsString(testIdp.getService(callbackURL).getAuthorizationUrl())); } @Test @@ -100,10 +99,6 @@ static Stream tamperedStates() { @ParameterizedTest @MethodSource("tamperedStates") void parseStateFromRequestStateTampered(String state) { - // when - when(this.authenticationServiceBean.getOAuth2Provider(testIdp.getId())).thenReturn(testIdp); - - // then assertThat(loginBackingBean.parseStateFromRequest(state), is(Optional.empty())); } @@ -120,13 +115,9 @@ static Stream provideStates() { @MethodSource("provideStates") void parseStateFromRequestStateValid(Optional redirectPage, boolean present) { // given - String idpId = testIdp.getId(); - - // when - when(this.authenticationServiceBean.getOAuth2Provider(idpId)).thenReturn(testIdp); String stateWithRedirect = loginBackingBean.createState(testIdp, redirectPage); - // then + // when & then assertThat(loginBackingBean.parseStateFromRequest(stateWithRedirect), is(Optional.of(testIdp))); assertThat(loginBackingBean.redirectPage.isPresent(), is(present)); if (present) { From 31ecf7f37d6eb2c897edc19d5b3da802ae4b5546 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 13 Nov 2019 12:04:45 +0100 Subject: [PATCH 09/19] Add first test for OAuth2LoginBackingBean.exchangeCodeForToken() --- .../oauth2/OAuth2LoginBackingBeanTest.java | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java index 89cf8c7b8f1..f95b4806834 100644 --- a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java @@ -4,20 +4,32 @@ import edu.harvard.iq.dataverse.authorization.providers.oauth2.impl.GitHubOAuth2APTest; import edu.harvard.iq.dataverse.util.StringUtil; import edu.harvard.iq.dataverse.util.SystemConfig; +import org.hamcrest.Matchers; import org.hamcrest.core.StringContains; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.*; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.omnifaces.util.Faces; +import javax.faces.context.ExternalContext; +import javax.faces.context.FacesContext; +import javax.faces.context.Flash; +import javax.servlet.http.HttpServletRequest; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.when; import java.util.Optional; import java.util.stream.Stream; +import java.io.BufferedReader; +import java.io.IOException; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.hamcrest.MatcherAssert.assertThat; @@ -61,6 +73,33 @@ void linkFor() { assertThat(link, StringContains.containsString(testIdp.getService(callbackURL).getAuthorizationUrl())); } + @Nested + @DisplayName("Tests for exchangeCodeForToken()") + class ecft { + @Mock FacesContext facesContextMock; + @Mock ExternalContext externalContextMock; + @Mock Flash flashMock; + @Mock HttpServletRequest requestMock; + @Mock BufferedReader reader; + + @BeforeEach + void setUp() throws IOException { + // mock FacesContext to make the method testable + Faces.setContext(facesContextMock); + when(facesContextMock.getExternalContext()).thenReturn(externalContextMock); + lenient().when(externalContextMock.getFlash()).thenReturn(flashMock); + when(externalContextMock.getRequest()).thenReturn(requestMock); + when(requestMock.getReader()).thenReturn(reader); + when(requestMock.getParameter("state")).thenReturn(loginBackingBean.createState(testIdp, Optional.of("/dataverse.xhtml"))); + } + + @Test + void noCode() { + assertDoesNotThrow(() -> loginBackingBean.exchangeCodeForToken()); + assertThat(loginBackingBean.getError(), Matchers.isA(OAuth2Exception.class)); + } + } + @Test void createStateFailNullIdp() { // given From 010feb992cf30667596c5ddca700ea1504eab861 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 13 Nov 2019 13:52:45 +0100 Subject: [PATCH 10/19] OAuth2LoginBackingBean: move service to IdP logic to abstract. --- .../AbstractOAuth2AuthenticationProvider.java | 18 +++++++++++++++--- .../oauth2/OAuth2LoginBackingBean.java | 8 +------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java index f143c68353f..5d8fc949f17 100644 --- a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java +++ b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java @@ -116,7 +116,7 @@ public String buildAuthzUrl(String state, String callbackUrl) { } /** - * Build an OAuth20Service based on client ID & secret. Add default scope and insert + * Build an OAuth20Service based on client ID & secret, also inserting the * callback URL. Build uses the real API object for the target service like GitHub etc. * @param callbackUrl URL where the OAuth2 Provider should send browsers to after authz. * @return A usable OAuth20Service object @@ -128,20 +128,32 @@ public OAuth20Service getService(String callbackUrl) { .build(getApiInstance()); } + /** + * Build an OAuth20Service based on client ID & secret. This will not contain a + * callback URL, which is not necessary after running through code flow. + * Build uses the real API object for the target service like GitHub etc. + * @return A usable OAuth20Service object + */ + public OAuth20Service getService() { + return new ServiceBuilder(getClientId()) + .apiSecret(getClientSecret()) + .build(getApiInstance()); + } + /** * Receive user data from OAuth2 provider after authn/z has been successfull. (Callback view uses this) * Request a token and access the resource, parse output and return user details. * @param code The authz code sent from the provider - * @param service The service object in use to communicate with the provider * @return A user record containing all user details accessible for us * @throws IOException Thrown when communication with the provider fails * @throws OAuth2Exception Thrown when we cannot access the user details for some reason * @throws InterruptedException Thrown when the requests thread is failing * @throws ExecutionException Thrown when the requests thread is failing */ - public OAuth2UserRecord getUserRecord(String code, @NotNull OAuth20Service service) + public OAuth2UserRecord getUserRecord(String code) throws IOException, OAuth2Exception, InterruptedException, ExecutionException { + OAuth20Service service = getService(); OAuth2AccessToken accessToken = service.getAccessToken(code); // We need to check if scope is null first: GitHub is used without scope, so the responses scope is null. diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java index 2667bf07874..7b7f278d8c9 100644 --- a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java +++ b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java @@ -1,7 +1,5 @@ package edu.harvard.iq.dataverse.authorization.providers.oauth2; -import com.github.scribejava.core.oauth.AuthorizationUrlBuilder; -import com.github.scribejava.core.oauth.OAuth20Service; import edu.harvard.iq.dataverse.DataverseSession; import edu.harvard.iq.dataverse.authorization.AuthenticationServiceBean; import edu.harvard.iq.dataverse.authorization.UserRecordIdentifier; @@ -19,12 +17,10 @@ import java.util.logging.Logger; import static java.util.stream.Collectors.toList; import javax.ejb.EJB; -import javax.faces.context.FacesContext; import javax.inject.Named; import javax.faces.view.ViewScoped; import javax.inject.Inject; import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; import javax.validation.constraints.NotNull; import static edu.harvard.iq.dataverse.util.StringUtil.toOption; @@ -92,9 +88,7 @@ public void exchangeCodeForToken() throws IOException { if (oIdp.isPresent() && code.isPresent()) { AbstractOAuth2AuthenticationProvider idp = oIdp.get(); - - OAuth20Service svc = idp.getService(systemConfig.getOAuth2CallbackUrl()); - oauthUser = idp.getUserRecord(code.get(), svc); + oauthUser = idp.getUserRecord(code.get()); UserRecordIdentifier idtf = oauthUser.getUserRecordIdentifier(); AuthenticatedUser dvUser = authenticationSvc.lookupUser(idtf); From 35dab571b5f8c06b31cf6b7665c4d8b277636634 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 13 Nov 2019 13:53:28 +0100 Subject: [PATCH 11/19] Add test for new user branch of OAuth2LoginBackingBean.exchangeCodeForToken() --- .../oauth2/OAuth2LoginBackingBeanTest.java | 47 +++++++++++++++---- .../oauth2/impl/GitHubOAuth2APTest.java | 7 +++ 2 files changed, 44 insertions(+), 10 deletions(-) diff --git a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java index f95b4806834..976840e6f9f 100644 --- a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java @@ -13,7 +13,9 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.*; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; +import org.mockito.Spy; import org.mockito.junit.jupiter.MockitoExtension; import org.omnifaces.util.Faces; @@ -23,8 +25,6 @@ import javax.servlet.http.HttpServletRequest; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; -import static org.mockito.Mockito.lenient; -import static org.mockito.Mockito.when; import java.util.Optional; import java.util.stream.Stream; @@ -34,17 +34,16 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; +import static org.mockito.Mockito.*; @ExtendWith(MockitoExtension.class) class OAuth2LoginBackingBeanTest { OAuth2LoginBackingBean loginBackingBean = new OAuth2LoginBackingBean(); - static AbstractOAuth2AuthenticationProvider testIdp = new GitHubOAuth2APTest(); + @Spy static AbstractOAuth2AuthenticationProvider testIdp = new GitHubOAuth2APTest(); - @Mock - AuthenticationServiceBean authenticationServiceBean; - @Mock - SystemConfig systemConfig; + @Mock AuthenticationServiceBean authenticationServiceBean; + @Mock SystemConfig systemConfig; @BeforeEach void setUp() { @@ -81,16 +80,19 @@ class ecft { @Mock Flash flashMock; @Mock HttpServletRequest requestMock; @Mock BufferedReader reader; + @Mock OAuth2FirstLoginPage newAccountPage; @BeforeEach void setUp() throws IOException { + loginBackingBean.newAccountPage = this.newAccountPage; + // mock FacesContext to make the method testable Faces.setContext(facesContextMock); when(facesContextMock.getExternalContext()).thenReturn(externalContextMock); - lenient().when(externalContextMock.getFlash()).thenReturn(flashMock); when(externalContextMock.getRequest()).thenReturn(requestMock); - when(requestMock.getReader()).thenReturn(reader); - when(requestMock.getParameter("state")).thenReturn(loginBackingBean.createState(testIdp, Optional.of("/dataverse.xhtml"))); + lenient().when(externalContextMock.getFlash()).thenReturn(flashMock); + lenient().when(requestMock.getReader()).thenReturn(reader); + doReturn(loginBackingBean.createState(testIdp, Optional.of("/dataverse.xhtml"))).when(requestMock).getParameter("state"); } @Test @@ -98,6 +100,31 @@ void noCode() { assertDoesNotThrow(() -> loginBackingBean.exchangeCodeForToken()); assertThat(loginBackingBean.getError(), Matchers.isA(OAuth2Exception.class)); } + + @Test + void newUser() throws Exception { + // GIVEN + String code = "randomstring"; + OAuth2UserRecord userRecord = ((GitHubOAuth2APTest) testIdp).getExampleUserRecord(); + String newUserRedirect = "/oauth2/firstLogin.xhtml"; + + // fake the code received from the provider + when(requestMock.getParameter("code")).thenReturn(code); + // let's deep-fake the result of getUserRecord() + doReturn(userRecord).when(testIdp).getUserRecord(code); + + // WHEN + // capture the redirect target from the faces context + ArgumentCaptor redirectUrlCaptor = ArgumentCaptor.forClass(String.class); + doNothing().when(externalContextMock).redirect(redirectUrlCaptor.capture()); + + // THEN + assertDoesNotThrow(() -> loginBackingBean.exchangeCodeForToken()); + // verify that the user object is passed on to the first login page + verify(newAccountPage, times(1)).setNewUser(userRecord); + // verify that the user is redirected to the first login page + assertThat(redirectUrlCaptor.getValue(), equalTo(newUserRedirect)); + } } @Test diff --git a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/impl/GitHubOAuth2APTest.java b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/impl/GitHubOAuth2APTest.java index 8b43b65f324..786c30fb2d7 100644 --- a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/impl/GitHubOAuth2APTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/impl/GitHubOAuth2APTest.java @@ -3,6 +3,8 @@ import edu.harvard.iq.dataverse.authorization.AuthenticatedUserDisplayInfo; import edu.harvard.iq.dataverse.authorization.providers.oauth2.AbstractOAuth2AuthenticationProvider; import static org.junit.Assert.assertEquals; + +import edu.harvard.iq.dataverse.authorization.providers.oauth2.OAuth2UserRecord; import org.junit.Test; public class GitHubOAuth2APTest extends GitHubOAuth2AP { @@ -57,5 +59,10 @@ public void testParseUserResponse() { assertEquals("21006", result.userIdInProvider); } + + public OAuth2UserRecord getExampleUserRecord() { + ParsedUserResponse res = parseUserResponse(GITHUB_RESPONSE); + return new OAuth2UserRecord(this.getId(), res.userIdInProvider, res.username, null, res.displayInfo, res.emails); + } } From 9073b8c78f3ba0cd842849ee6b690d0a60d58e75 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 13 Nov 2019 14:38:22 +0100 Subject: [PATCH 12/19] Add test for existing user branch in OAuth2LoginBackingBean.exchangeCodeForToken() --- .../oauth2/OAuth2LoginBackingBeanTest.java | 50 +++++++++++++++++-- 1 file changed, 46 insertions(+), 4 deletions(-) diff --git a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java index 976840e6f9f..4e066fc46fc 100644 --- a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java @@ -1,7 +1,11 @@ package edu.harvard.iq.dataverse.authorization.providers.oauth2; +import edu.harvard.iq.dataverse.DataverseSession; import edu.harvard.iq.dataverse.authorization.AuthenticationServiceBean; +import edu.harvard.iq.dataverse.authorization.UserRecordIdentifier; import edu.harvard.iq.dataverse.authorization.providers.oauth2.impl.GitHubOAuth2APTest; +import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser; +import edu.harvard.iq.dataverse.authorization.users.User; import edu.harvard.iq.dataverse.util.StringUtil; import edu.harvard.iq.dataverse.util.SystemConfig; import org.hamcrest.Matchers; @@ -81,10 +85,15 @@ class ecft { @Mock HttpServletRequest requestMock; @Mock BufferedReader reader; @Mock OAuth2FirstLoginPage newAccountPage; + @Mock DataverseSession session; + @Mock OAuth2TokenDataServiceBean oauth2Tokens; + Optional redirect = Optional.of("/hellotest"); @BeforeEach void setUp() throws IOException { loginBackingBean.newAccountPage = this.newAccountPage; + loginBackingBean.session = this.session; + loginBackingBean.oauth2Tokens = this.oauth2Tokens; // mock FacesContext to make the method testable Faces.setContext(facesContextMock); @@ -92,7 +101,7 @@ void setUp() throws IOException { when(externalContextMock.getRequest()).thenReturn(requestMock); lenient().when(externalContextMock.getFlash()).thenReturn(flashMock); lenient().when(requestMock.getReader()).thenReturn(reader); - doReturn(loginBackingBean.createState(testIdp, Optional.of("/dataverse.xhtml"))).when(requestMock).getParameter("state"); + doReturn(loginBackingBean.createState(testIdp, this.redirect)).when(requestMock).getParameter("state"); } @Test @@ -105,7 +114,7 @@ void noCode() { void newUser() throws Exception { // GIVEN String code = "randomstring"; - OAuth2UserRecord userRecord = ((GitHubOAuth2APTest) testIdp).getExampleUserRecord(); + OAuth2UserRecord userRecord = mock(OAuth2UserRecord.class); String newUserRedirect = "/oauth2/firstLogin.xhtml"; // fake the code received from the provider @@ -113,18 +122,51 @@ void newUser() throws Exception { // let's deep-fake the result of getUserRecord() doReturn(userRecord).when(testIdp).getUserRecord(code); - // WHEN + // WHEN (& then) // capture the redirect target from the faces context ArgumentCaptor redirectUrlCaptor = ArgumentCaptor.forClass(String.class); doNothing().when(externalContextMock).redirect(redirectUrlCaptor.capture()); - // THEN assertDoesNotThrow(() -> loginBackingBean.exchangeCodeForToken()); + + // THEN // verify that the user object is passed on to the first login page verify(newAccountPage, times(1)).setNewUser(userRecord); // verify that the user is redirected to the first login page assertThat(redirectUrlCaptor.getValue(), equalTo(newUserRedirect)); } + + @Test + void existingUser() throws Exception { + // GIVEN + String code = "randomstring"; + OAuth2UserRecord userRecord = mock(OAuth2UserRecord.class); + UserRecordIdentifier userIdentifier = mock(UserRecordIdentifier.class); + AuthenticatedUser user = mock(AuthenticatedUser.class); + OAuth2TokenData tokenData = mock(OAuth2TokenData.class); + + // fake the code received from the provider + when(requestMock.getParameter("code")).thenReturn(code); + // let's deep-fake the result of getUserRecord() + doReturn(userRecord).when(testIdp).getUserRecord(code); + doReturn(tokenData).when(userRecord).getTokenData(); + // also fake the result of the lookup in the auth service + doReturn(userIdentifier).when(userRecord).getUserRecordIdentifier(); + doReturn(user).when(authenticationServiceBean).lookupUser(userIdentifier); + + // WHEN (& then) + // capture the redirect target from the faces context + ArgumentCaptor redirectUrlCaptor = ArgumentCaptor.forClass(String.class); + doNothing().when(externalContextMock).redirect(redirectUrlCaptor.capture()); + + assertDoesNotThrow(() -> loginBackingBean.exchangeCodeForToken()); + + // THEN + // verify session handling: set the looked up user + verify(session, times(1)).setUser(user); + // verify that the user is redirected to the first login page + assertThat(redirectUrlCaptor.getValue(), equalTo(redirect.get())); + } } @Test From 7e81ebf39971712751b6e85506b7640562528c3f Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 13 Nov 2019 21:58:08 +0100 Subject: [PATCH 13/19] Add LocaleTest to ensure that tests run with the correct locale settings. IDEs might give us headaches and it ensures proper Maven configuration. --- .../edu/harvard/iq/dataverse/LocaleTest.java | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 src/test/java/edu/harvard/iq/dataverse/LocaleTest.java diff --git a/src/test/java/edu/harvard/iq/dataverse/LocaleTest.java b/src/test/java/edu/harvard/iq/dataverse/LocaleTest.java new file mode 100644 index 00000000000..1ca43cde138 --- /dev/null +++ b/src/test/java/edu/harvard/iq/dataverse/LocaleTest.java @@ -0,0 +1,30 @@ +package edu.harvard.iq.dataverse; + +import org.junit.jupiter.api.Test; + +import java.util.Locale; +import java.util.TimeZone; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; + +public class LocaleTest { + + /** + * Many of our tests require setting the locale and timezone appropriately + * as formating dates, numbers, etc are based on it. Ensure consistency. + */ + @Test + void ensureLocale() { + Locale l = Locale.getDefault(); + TimeZone tz = TimeZone.getDefault(); + + System.out.println("user.language="+l.getLanguage()); + System.out.println("user.region="+l.getCountry()); + System.out.println("user.timezone="+tz.getDefault().getID()); + + assertThat(l.getLanguage(), equalTo("en")); + assertThat(l.getCountry(), equalTo("US")); + assertThat(tz.getID(), equalTo("UTC")); + } +} From 3a8860537942ff79ab2fe9d60b17f8052e3efded Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 13 Nov 2019 23:20:34 +0100 Subject: [PATCH 14/19] Fix unit tests failing due to mocked FacesContext --- .../oauth2/OAuth2LoginBackingBeanTest.java | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java index 4e066fc46fc..d763b1b67fe 100644 --- a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java @@ -5,15 +5,11 @@ import edu.harvard.iq.dataverse.authorization.UserRecordIdentifier; import edu.harvard.iq.dataverse.authorization.providers.oauth2.impl.GitHubOAuth2APTest; import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser; -import edu.harvard.iq.dataverse.authorization.users.User; import edu.harvard.iq.dataverse.util.StringUtil; import edu.harvard.iq.dataverse.util.SystemConfig; import org.hamcrest.Matchers; import org.hamcrest.core.StringContains; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.DisplayName; -import org.junit.jupiter.api.Nested; -import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.*; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.*; @@ -49,6 +45,17 @@ class OAuth2LoginBackingBeanTest { @Mock AuthenticationServiceBean authenticationServiceBean; @Mock SystemConfig systemConfig; + /** + * Save the current JSF context to reset after all test cases done. + * Without doing this, many tests will fail with NPEs while fetching the bundle, + * when the locale is received from the context. + */ + static FacesContext save = Faces.getContext(); + @AfterAll + static void cleanupFaces() { + Faces.setContext(save); + } + @BeforeEach void setUp() { this.loginBackingBean.authenticationSvc = this.authenticationServiceBean; From 9f594b9531d3909fc0254770154a91c57d192c8d Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Wed, 13 Nov 2019 23:21:41 +0100 Subject: [PATCH 15/19] Refactor pom.xml to use new JUnit 5.4+ dependency package --- pom.xml | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/pom.xml b/pom.xml index bcffdaa28c9..97a38a2e59c 100644 --- a/pom.xml +++ b/pom.xml @@ -29,8 +29,7 @@ 4.5.5 4.12 5.5.2 - 5.5.2 - 1.5.2 + ${junit.jupiter.version} 2.28.2 5.2.4 1.20.1 @@ -114,7 +113,7 @@ org.junit.jupiter - junit-jupiter-api + junit-jupiter ${junit.jupiter.version} test @@ -124,18 +123,6 @@ ${junit.version} test - - org.junit.jupiter - junit-jupiter-engine - ${junit.jupiter.version} - test - - - org.junit.jupiter - junit-jupiter-params - ${junit.jupiter.version} - test - org.junit.vintage junit-vintage-engine @@ -759,7 +746,7 @@ maven-surefire-plugin - 2.22.0 + 2.22.2 ${testsToExclude} From 612cf8ca8c3d01c42492275b056bbbd42a2e05ec Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Fri, 22 Nov 2019 14:21:18 +0100 Subject: [PATCH 16/19] Create new ClockUtil utility class with annotations to inject clocks via CDI --- .../harvard/iq/dataverse/util/ClockUtil.java | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 src/main/java/edu/harvard/iq/dataverse/util/ClockUtil.java diff --git a/src/main/java/edu/harvard/iq/dataverse/util/ClockUtil.java b/src/main/java/edu/harvard/iq/dataverse/util/ClockUtil.java new file mode 100644 index 00000000000..9c1c89430d5 --- /dev/null +++ b/src/main/java/edu/harvard/iq/dataverse/util/ClockUtil.java @@ -0,0 +1,26 @@ +package edu.harvard.iq.dataverse.util; + +import javax.enterprise.inject.Produces; +import javax.inject.Qualifier; +import javax.inject.Singleton; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import java.time.Clock; + +@Singleton +public class ClockUtil { + + + @Qualifier + @Retention(RetentionPolicy.RUNTIME) + @Target({ElementType.FIELD, ElementType.METHOD, ElementType.TYPE, ElementType.PARAMETER}) + public @interface LocalTime { + } + + @Produces + @LocalTime + public static final Clock LOCAL_CLOCK = Clock.systemDefaultZone(); + +} From 5d03e67d99fcd508de9e5fdc0b1fc27b3edb73c4 Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Fri, 22 Nov 2019 14:22:21 +0100 Subject: [PATCH 17/19] Refactor OAuth2LoginBackingBean to use java.time.Clock instead of System.currentTimeMillis(). Using CDI injection of clock via ClockUtil. --- .../providers/oauth2/OAuth2LoginBackingBean.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java index 7b7f278d8c9..fa77a58a027 100644 --- a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java +++ b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java @@ -4,11 +4,13 @@ import edu.harvard.iq.dataverse.authorization.AuthenticationServiceBean; import edu.harvard.iq.dataverse.authorization.UserRecordIdentifier; import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser; +import edu.harvard.iq.dataverse.util.ClockUtil; import edu.harvard.iq.dataverse.util.StringUtil; import java.io.BufferedReader; import java.io.IOException; import java.io.Serializable; import java.security.SecureRandom; +import java.time.Clock; import java.util.Comparator; import java.util.List; import java.util.Optional; @@ -63,6 +65,10 @@ public class OAuth2LoginBackingBean implements Serializable { @Inject OAuth2FirstLoginPage newAccountPage; + @Inject + @ClockUtil.LocalTime + Clock clock; + /** * Generate the OAuth2 Provider URL to be used in the login page link for the provider. * @param idpId Unique ID for the provider (used to lookup in authn service bean) @@ -179,7 +185,7 @@ Optional parseStateFromRequest(@NotNull St String[] stateFields = raw.split("~", -1); if (idp.getId().equals(stateFields[0])) { long timeOrigin = Long.parseLong(stateFields[1]); - long timeDifference = System.currentTimeMillis() - timeOrigin; + long timeDifference = this.clock.millis() - timeOrigin; if (timeDifference > 0 && timeDifference < STATE_TIMEOUT) { if ( stateFields.length > 3) { this.redirectPage = Optional.ofNullable(stateFields[3]); @@ -207,7 +213,7 @@ String createState(AbstractOAuth2AuthenticationProvider idp, Optional re } SecureRandom rand = new SecureRandom(); - String base = idp.getId() + "~" + System.currentTimeMillis() + String base = idp.getId() + "~" + this.clock.millis() + "~" + rand.nextInt(1000) + redirectPage.map( page -> "~"+page).orElse(""); From db1e8e72c479f83809b136cf0cbfd161501257ef Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Fri, 22 Nov 2019 14:23:34 +0100 Subject: [PATCH 18/19] Refactore tests for OAuth2LoginBackingBeanTest to finally be non-flaky due to clock resolution of >1msec when using createState() and parseStateFromRequest(). --- .../oauth2/OAuth2LoginBackingBeanTest.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java index d763b1b67fe..1ada5bc01d8 100644 --- a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java @@ -26,6 +26,10 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import java.time.Clock; +import java.time.Duration; +import java.time.Instant; +import java.time.ZoneId; import java.util.Optional; import java.util.stream.Stream; import java.io.BufferedReader; @@ -45,6 +49,8 @@ class OAuth2LoginBackingBeanTest { @Mock AuthenticationServiceBean authenticationServiceBean; @Mock SystemConfig systemConfig; + Clock constantClock = Clock.fixed(Instant.now(), ZoneId.systemDefault()); + /** * Save the current JSF context to reset after all test cases done. * Without doing this, many tests will fail with NPEs while fetching the bundle, @@ -58,6 +64,10 @@ static void cleanupFaces() { @BeforeEach void setUp() { + + // create a fixed (not running) clock for testing. + // --> reset the clock for every test to the fixed time, to avoid sideeffects of a DeLorean time travel + this.loginBackingBean.clock = constantClock; this.loginBackingBean.authenticationSvc = this.authenticationServiceBean; this.loginBackingBean.systemConfig = this.systemConfig; lenient().when(this.authenticationServiceBean.getOAuth2Provider(testIdp.getId())).thenReturn(testIdp); @@ -109,6 +119,9 @@ void setUp() throws IOException { lenient().when(externalContextMock.getFlash()).thenReturn(flashMock); lenient().when(requestMock.getReader()).thenReturn(reader); doReturn(loginBackingBean.createState(testIdp, this.redirect)).when(requestMock).getParameter("state"); + // travel in time at least 10 milliseconds (remote calls & redirects are much likely longer) + // (if not doing this tests become flaky on fast machinas) + loginBackingBean.clock = Clock.offset(constantClock, Duration.ofMillis(10)); } @Test @@ -231,6 +244,9 @@ static Stream provideStates() { void parseStateFromRequestStateValid(Optional redirectPage, boolean present) { // given String stateWithRedirect = loginBackingBean.createState(testIdp, redirectPage); + // travel in time at least 10 milliseconds (remote calls & redirects are much likely longer) + // (if not doing this tests become flaky on fast machinas) + loginBackingBean.clock = Clock.offset(constantClock, Duration.ofMillis(10)); // when & then assertThat(loginBackingBean.parseStateFromRequest(stateWithRedirect), is(Optional.of(testIdp))); From 9a4b49fae048e727abb198c71bbd55127ef9934c Mon Sep 17 00:00:00 2001 From: Oliver Bertuch Date: Mon, 25 Nov 2019 09:53:05 +0100 Subject: [PATCH 19/19] Fix OAuth2 providers requiring redirect_uri parameter when requesting the access token during code flow. --- .../AbstractOAuth2AuthenticationProvider.java | 17 +++-------------- .../oauth2/OAuth2LoginBackingBean.java | 2 +- .../oauth2/OAuth2LoginBackingBeanTest.java | 4 ++-- 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java index 5d8fc949f17..9671621ac73 100644 --- a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java +++ b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/AbstractOAuth2AuthenticationProvider.java @@ -128,32 +128,21 @@ public OAuth20Service getService(String callbackUrl) { .build(getApiInstance()); } - /** - * Build an OAuth20Service based on client ID & secret. This will not contain a - * callback URL, which is not necessary after running through code flow. - * Build uses the real API object for the target service like GitHub etc. - * @return A usable OAuth20Service object - */ - public OAuth20Service getService() { - return new ServiceBuilder(getClientId()) - .apiSecret(getClientSecret()) - .build(getApiInstance()); - } - /** * Receive user data from OAuth2 provider after authn/z has been successfull. (Callback view uses this) * Request a token and access the resource, parse output and return user details. * @param code The authz code sent from the provider + * @param redirectUrl The redirect URL (some providers require this when fetching the access token, e. g. Google) * @return A user record containing all user details accessible for us * @throws IOException Thrown when communication with the provider fails * @throws OAuth2Exception Thrown when we cannot access the user details for some reason * @throws InterruptedException Thrown when the requests thread is failing * @throws ExecutionException Thrown when the requests thread is failing */ - public OAuth2UserRecord getUserRecord(String code) + public OAuth2UserRecord getUserRecord(String code, String redirectUrl) throws IOException, OAuth2Exception, InterruptedException, ExecutionException { - OAuth20Service service = getService(); + OAuth20Service service = getService(redirectUrl); OAuth2AccessToken accessToken = service.getAccessToken(code); // We need to check if scope is null first: GitHub is used without scope, so the responses scope is null. diff --git a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java index fa77a58a027..618b8e5ca1d 100644 --- a/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java +++ b/src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java @@ -94,7 +94,7 @@ public void exchangeCodeForToken() throws IOException { if (oIdp.isPresent() && code.isPresent()) { AbstractOAuth2AuthenticationProvider idp = oIdp.get(); - oauthUser = idp.getUserRecord(code.get()); + oauthUser = idp.getUserRecord(code.get(), systemConfig.getOAuth2CallbackUrl()); UserRecordIdentifier idtf = oauthUser.getUserRecordIdentifier(); AuthenticatedUser dvUser = authenticationSvc.lookupUser(idtf); diff --git a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java index 1ada5bc01d8..80249cc89e8 100644 --- a/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBeanTest.java @@ -140,7 +140,7 @@ void newUser() throws Exception { // fake the code received from the provider when(requestMock.getParameter("code")).thenReturn(code); // let's deep-fake the result of getUserRecord() - doReturn(userRecord).when(testIdp).getUserRecord(code); + doReturn(userRecord).when(testIdp).getUserRecord(code, null); // WHEN (& then) // capture the redirect target from the faces context @@ -168,7 +168,7 @@ void existingUser() throws Exception { // fake the code received from the provider when(requestMock.getParameter("code")).thenReturn(code); // let's deep-fake the result of getUserRecord() - doReturn(userRecord).when(testIdp).getUserRecord(code); + doReturn(userRecord).when(testIdp).getUserRecord(code, null); doReturn(tokenData).when(userRecord).getTokenData(); // also fake the result of the lookup in the auth service doReturn(userIdentifier).when(userRecord).getUserRecordIdentifier();