6364 oauth2 abstract

pom.xml

@@ -28,9 +28,8 @@
         <commons.logging.version>1.2</commons.logging.version>
         <httpcomponents.client.version>4.5.5</httpcomponents.client.version>
         <junit.version>4.12</junit.version>
-        <junit.jupiter.version>5.3.1</junit.jupiter.version>
-        <junit.vintage.version>5.3.1</junit.vintage.version>
-        <junit.platform.version>1.3.1</junit.platform.version>
+        <junit.jupiter.version>5.5.2</junit.jupiter.version>
+        <junit.vintage.version>${junit.jupiter.version}</junit.vintage.version>
         <mockito.version>2.28.2</mockito.version>
         <flyway.version>5.2.4</flyway.version>
         <jhove.version>1.20.1</jhove.version>
@@ -114,7 +113,7 @@
         </dependency>
         <dependency>
             <groupId>org.junit.jupiter</groupId>
-            <artifactId>junit-jupiter-api</artifactId>
+            <artifactId>junit-jupiter</artifactId>
             <version>${junit.jupiter.version}</version>
             <scope>test</scope>
         </dependency>
@@ -124,24 +123,18 @@
             <version>${junit.version}</version>
             <scope>test</scope>
         </dependency>
-        <dependency>
-            <groupId>org.junit.jupiter</groupId>
-            <artifactId>junit-jupiter-engine</artifactId>
-            <version>${junit.jupiter.version}</version>
-            <scope>test</scope>
-        </dependency>
-        <dependency>
-            <groupId>org.junit.jupiter</groupId>
-            <artifactId>junit-jupiter-params</artifactId>
-            <version>${junit.jupiter.version}</version>
-            <scope>test</scope>
-        </dependency>
         <dependency>
             <groupId>org.junit.vintage</groupId>
             <artifactId>junit-vintage-engine</artifactId>
             <version>${junit.vintage.version}</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.hamcrest</groupId>
+            <artifactId>hamcrest-library</artifactId>
+            <version>2.2</version>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>org.glassfish</groupId>
             <artifactId>javax.json</artifactId>
@@ -753,7 +746,7 @@
             <plugin>
                 <!-- https://stackoverflow.com/questions/46177921/how-to-run-unit-tests-in-excludedgroups-in-maven -->
                 <artifactId>maven-surefire-plugin</artifactId>
-                <version>2.22.0</version>
+                <version>2.22.2</version>
                 <configuration>
                     <!-- testsToExclude come from the profile-->
                     <excludedGroups>${testsToExclude}</excludedGroups>

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;
@@ -100,7 +101,22 @@ public String toString() {
     protected abstract ParsedUserResponse parseUserResponse( String responseBody );
 
     /**
-     * Build an OAuth20Service based on client ID & secret. Add default scope and insert
+     * 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, 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
@@ -116,18 +132,19 @@ public OAuth20Service getService(String callbackUrl) {
      * 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
+     * @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, @NotNull OAuth20Service service)
+    public OAuth2UserRecord getUserRecord(String code, String redirectUrl)
         throws IOException, OAuth2Exception, InterruptedException, ExecutionException {
 
+        OAuth20Service service = getService(redirectUrl);
         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) ) ||

src/main/java/edu/harvard/iq/dataverse/authorization/providers/oauth2/OAuth2LoginBackingBean.java

@@ -1,16 +1,16 @@
 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;
 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;
@@ -19,16 +19,15 @@
 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;
 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
@@ -41,11 +40,14 @@
 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<String> redirectPage;
+    Optional<String> redirectPage = Optional.empty();
     private OAuth2Exception error;
+    /**
+     * TODO: Only used in exchangeCodeForToken(). Make local var in method.
+     */
     private OAuth2UserRecord oauthUser;
 
     @EJB
@@ -62,7 +64,11 @@ 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)
@@ -71,42 +77,32 @@ 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());
     }
 
     /**
      * View action for callback.xhtml, the browser redirect target for the OAuth2 provider.
      * @throws IOException
      */
     public void exchangeCodeForToken() throws IOException {
-        HttpServletRequest req = (HttpServletRequest) FacesContext.getCurrentInstance().getExternalContext().getRequest();
+        HttpServletRequest req = Faces.getRequest();
 
         try {
-            Optional<AbstractOAuth2AuthenticationProvider> oIdp = parseStateFromRequest(req);
+            Optional<AbstractOAuth2AuthenticationProvider> oIdp = parseStateFromRequest(req.getParameter("state"));
             Optional<String> code = parseCodeFromRequest(req);
 
             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(), systemConfig.getOAuth2CallbackUrl());
 
                 UserRecordIdentifier idtf = oauthUser.getUserRecordIdentifier();
                 AuthenticatedUser dvUser = authenticationSvc.lookupUser(idtf);
 
                 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).
@@ -116,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) {
@@ -132,6 +126,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<String> parseCodeFromRequest(@NotNull HttpServletRequest req) {
         String code = req.getParameter("code");
         if (code == null || code.trim().isEmpty()) {
@@ -152,11 +150,21 @@ private Optional<String> parseCodeFromRequest(@NotNull HttpServletRequest req) {
         }
         return Optional.of(code);
     }
-
-    private Optional<AbstractOAuth2AuthenticationProvider> 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<AbstractOAuth2AuthenticationProvider> parseStateFromRequest(@NotNull String state) {
+        if (state == null || state.trim().equals("")) {
             logger.log(Level.INFO, "No state present in request");
             return Optional.empty();
         }
@@ -177,10 +185,10 @@ private Optional<AbstractOAuth2AuthenticationProvider> parseStateFromRequest(@No
         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) {
-                    redirectPage = Optional.ofNullable(stateFields[3]);
+                    this.redirectPage = Optional.ofNullable(stateFields[3]);
                 }
                 return Optional.of(idp);
             } else {
@@ -192,48 +200,72 @@ private Optional<AbstractOAuth2AuthenticationProvider> parseStateFromRequest(@No
             return Optional.empty();
         }
     }
-
-    private String createState(AbstractOAuth2AuthenticationProvider idp, Optional<String> 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<String> redirectPage) {
         if (idp == null) {
             throw new IllegalArgumentException("idp cannot be null");
         }
         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("");
 
         String encrypted = StringUtil.encrypt(base, idp.clientSecret);
         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;
     }
 
     public OAuth2Exception getError() {
         return error;
     }
-
+
+    /**
+     * TODO: Unused. Remove.
+     */
     public boolean isInError() {
         return error != null;
     }
-
+
+    /**
+     * TODO: Unused. Remove.
+     */
     public List<AbstractOAuth2AuthenticationProvider> getProviders() {
         return authenticationSvc.getOAuth2Providers().stream()
                 .sorted(Comparator.comparing(AbstractOAuth2AuthenticationProvider::getTitle))
                 .collect(toList());
     }
-
+
+    /**
+     * TODO: Unused. Remove.
+     */
     public boolean isOAuth2ProvidersDefined() {
         return !authenticationSvc.getOAuth2Providers().isEmpty();
     }