fix(account): repair OAuth social login end-to-end (#11)

build.sbt

@@ -18,7 +18,7 @@ ThisBuild / organization := "app.softnetwork"
 
 name := "account"
 
-ThisBuild / version := "0.8.5"
+ThisBuild / version := "0.8.6"
 
 ThisBuild / scalaVersion := scala212
 

common/src/main/resources/reference.conf

@@ -136,6 +136,13 @@ auth {
     path = "oauth"
     path = ${?AUTH_OAUTH_PATH}
 
+    callback {
+      # Frontend URL the social-login callback redirects to once the session is
+      # established; a status=success|error query param is appended.
+      redirect-url = "http://localhost/oauth/callback"
+      redirect-url = ${?AUTH_OAUTH_CALLBACK_REDIRECT_URL}
+    }
+
     authorization-code {
       expirationTime = 5
       expirationTime = ${?AUTH_AUTHORIZATION_CODE_EXPIRATION_TIME}
@@ -170,6 +177,9 @@ auth {
 
         client-secret = null
         client-secret = ${?AUTH_OAUTH_GITHUB_CLIENT_SECRET}
+
+        default-scope = "read:user,user:email"
+        default-scope = ${?AUTH_OAUTH_GITHUB_DEFAULT_SCOPE}
       }
 
       google {

common/src/main/scala/app/softnetwork/account/config/AccountSettings.scala

@@ -39,6 +39,11 @@ object AccountSettings extends StrictLogging {
 
   val OAuthPath: String = config.getString("auth.oauth.path")
 
+  /** Frontend URL the OAuth callback redirects the browser to once the session has been established
+    * (a `status=success|error` query param is appended).
+    */
+  val OAuthCallbackRedirectUrl: String = config.getString("auth.oauth.callback.redirect-url")
+
   val VerificationCodeSize: Int = config.getInt("auth.verification.code.size")
 
   val VerificationCodeExpirationTime: Int = config.getInt("auth.verification.code.expirationTime")

common/src/main/scala/app/softnetwork/account/spi/GitHubApiService.scala

@@ -2,6 +2,9 @@ package app.softnetwork.account.spi
 
 import com.github.scribejava.apis.GitHubApi
 import com.github.scribejava.core.builder.api.DefaultApi20
+import com.github.scribejava.core.model.OAuth2AccessToken
+
+import scala.util.Try
 
 class GitHubApiService extends OAuth2Service {
   override val networkName: String = "github"
@@ -14,4 +17,39 @@ class GitHubApiService extends OAuth2Service {
 
   override lazy val defaultScope: String =
     provider.defaultScope.getOrElse("user:email") // user includes email
+
+  /** GitHub returns `email: null` from /user when the primary email is private. The `user:email`
+    * scope lets us read /user/emails; resolve the primary verified address there and inject it so
+    * account login resolution (which keys on email) succeeds for users who keep their email
+    * private.
+    */
+  protected def emailsUrl: String = "https://api.github.com/user/emails"
+
+  override def userInfo(
+    code: String,
+    extraParameters: Map[String, String]
+  ): Try[Map[String, String]] = Try {
+    val accessToken = getAccessToken(code, extraParameters)
+    val user = fetch(accessToken, protectedResourceUrl)
+    if (user.get("email").exists(_.nonEmpty)) user
+    else primaryEmail(accessToken).fold(user)(email => user + ("email" -> email))
+  }
+
+  /** Returns the primary verified email from GitHub's /user/emails, falling back to any verified
+    * address, then the first one listed.
+    */
+  protected def primaryEmail(accessToken: OAuth2AccessToken): Option[String] = {
+    import scala.jdk.CollectionConverters._
+    val node = jsonMapper.readTree(get(accessToken, emailsUrl))
+    if (!node.isArray) None
+    else {
+      val entries = node.elements().asScala.toList
+      entries
+        .find(e => e.path("primary").asBoolean(false) && e.path("verified").asBoolean(false))
+        .orElse(entries.find(_.path("verified").asBoolean(false)))
+        .orElse(entries.headOption)
+        .map(_.path("email").asText(""))
+        .filter(_.nonEmpty)
+    }
+  }
 }

common/src/main/scala/app/softnetwork/account/spi/OAuth2Service.scala

@@ -7,7 +7,7 @@ import com.fasterxml.jackson.databind.{DeserializationFeature, ObjectMapper}
 import com.fasterxml.jackson.module.scala.DefaultScalaModule
 import com.github.scribejava.core.builder.ServiceBuilder
 import com.github.scribejava.core.builder.api.DefaultApi20
-import com.github.scribejava.core.model.{OAuth2AccessToken, OAuthRequest, Response}
+import com.github.scribejava.core.model.{OAuth2AccessToken, OAuthRequest, Response, Verb}
 import com.github.scribejava.core.oauth.{AccessTokenRequestParams, OAuth20Service}
 
 import scala.util.Try
@@ -63,16 +63,26 @@ trait OAuth2Service {
     .setSerializationInclusion(Include.NON_EMPTY)
   jsonMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
 
-  def userInfo(code: String, extraParameters: Map[String, String]): Try[Map[String, String]] = Try {
-    val accessToken = getAccessToken(code, extraParameters)
-    import com.github.scribejava.core.model.{OAuthRequest, Verb}
-    val request =
-      new OAuthRequest(Verb.GET, protectedResourceUrl)
+  /** Executes a signed GET against `url` and returns the raw response body. */
+  protected def get(accessToken: OAuth2AccessToken, url: String): String = {
+    val request = new OAuthRequest(Verb.GET, url)
     signRequest(accessToken, request)
+    execute(request).getBody
+  }
+
+  /** Fetches `url` as a JSON object and flattens it to a `Map[String, String]`. Providers (notably
+    * GitHub's /user) return many nullable fields; calling toString on a null value throws NPE, so
+    * null entries are dropped first.
+    */
+  protected def fetch(accessToken: OAuth2AccessToken, url: String): Map[String, String] =
     jsonMapper
-      .readValue(execute(request).getBody, classOf[Map[String, Any]])
+      .readValue(get(accessToken, url), classOf[Map[String, Any]])
+      .filter { case (_, value) => value != null }
       .mapValues(_.toString)
       .toMap
+
+  def userInfo(code: String, extraParameters: Map[String, String]): Try[Map[String, String]] = Try {
+    fetch(getAccessToken(code, extraParameters), protectedResourceUrl)
   }
 
   def extractData(data: Map[String, String]): OAuthData = OAuthData(networkName, data)

core/src/main/scala/app/softnetwork/account/service/BaseAccountService.scala

@@ -1,5 +1,6 @@
 package app.softnetwork.account.service
 
+import app.softnetwork.account.config.AccountSettings
 import app.softnetwork.account.handlers.AccountHandler
 import app.softnetwork.account.message.{AccountCommand, AccountCommandResult}
 import app.softnetwork.persistence.service.Service
@@ -10,6 +11,15 @@ import java.net.URLEncoder
 trait BaseAccountService extends Service[AccountCommand, AccountCommandResult] with AccountHandler {
   _: CommandTypeKey[AccountCommand] =>
 
+  /** Frontend URL the social-login callback redirects the browser to once it completes, with a
+    * `status` (and optional `reason`) query param appended.
+    */
+  protected def callbackRedirect(status: String, reason: Option[String] = None): String =
+    redirection(
+      AccountSettings.OAuthCallbackRedirectUrl,
+      Map("status" -> status) ++ reason.map("reason" -> _).toMap
+    )
+
   protected def redirection(uri: String, params: Map[String, String]): String = {
     var redirection = uri
     if (uri.contains("?")) {

core/src/main/scala/app/softnetwork/account/service/OAuthService.scala

@@ -214,11 +214,7 @@ trait OAuthService[SD <: SessionData with SessionDataDecorator[SD]]
                           setSession(sc, st, session) {
                             // create a new anti csrf token
                             setNewCsrfToken(checkHeader) {
-                              complete(
-                                HttpResponse(
-                                  StatusCodes.OK
-                                )
-                              )
+                              redirect(callbackRedirect("success"), StatusCodes.Found)
                             }
                           }
                         case _ =>
@@ -232,30 +228,35 @@ trait OAuthService[SD <: SessionData with SessionDataDecorator[SD]]
                               setSession(sc, st, session) {
                                 // create a new anti csrf token
                                 setNewCsrfToken(checkHeader) {
-                                  complete(
-                                    HttpResponse(
-                                      StatusCodes.OK
-                                    )
-                                  )
+                                  redirect(callbackRedirect("success"), StatusCodes.Found)
                                 }
                               }
                             case error: AccountErrorMessage =>
-                              complete(
-                                HttpResponse(StatusCodes.BadRequest, entity = error)
+                              log.error(s"sign up failed for ${service.networkName}: $error")
+                              redirect(
+                                callbackRedirect("error", Some("signup_failed")),
+                                StatusCodes.Found
+                              )
+                            case _ =>
+                              redirect(
+                                callbackRedirect("error", Some("signup_failed")),
+                                StatusCodes.Found
                               )
-                            case _ => complete(StatusCodes.BadRequest)
                           }
                       }
                     case _ =>
                       log.error(s"login not found within $data for ${service.networkName}")
-                      complete(HttpResponse(StatusCodes.InternalServerError))
+                      redirect(
+                        callbackRedirect("error", Some("login_not_found")),
+                        StatusCodes.Found
+                      )
                   }
                 case Failure(f) =>
                   log.error(f.getMessage, f)
-                  complete(HttpResponse(StatusCodes.InternalServerError))
+                  redirect(callbackRedirect("error", Some("user_info_failed")), StatusCodes.Found)
               }
             case _ =>
-              complete(HttpResponse(StatusCodes.BadRequest))
+              redirect(callbackRedirect("error", Some("missing_code")), StatusCodes.Found)
           }
         }
       }

core/src/main/scala/app/softnetwork/account/service/OAuthServiceEndpoints.scala

@@ -17,6 +17,7 @@ import sttp.capabilities
 import sttp.capabilities.akka.AkkaStreams
 import sttp.model.headers.AuthenticationScheme.{Basic, Bearer}
 import sttp.model.headers.WWWAuthenticateChallenge
+import sttp.model.{HeaderNames, StatusCode}
 import sttp.tapir.EndpointIO.Example
 import sttp.tapir.json.json4s.jsonBody
 import sttp.tapir.model.UsernamePassword
@@ -267,6 +268,10 @@ trait OAuthServiceEndpoints[SD <: SessionData with SessionDataDecorator[SD]]
           .description(s"OAuth2 ${service.networkName} callback endpoint")
           .securityIn(queryParams.description("authorization query parameters"))
           .errorOut(ApiErrors.oneOfApiErrors)
+          // On completion the session cookie is set (when present) and the browser
+          // is 302-redirected to the configured frontend URL.
+          .out(statusCode(StatusCode.Found))
+          .out(header[String](HeaderNames.Location))
           .serverSecurityLogicWithOutput(params =>
             params.get("code") match {
               case Some(code) =>
@@ -281,7 +286,7 @@ trait OAuthServiceEndpoints[SD <: SessionData with SessionDataDecorator[SD]]
                             session += ("provider", data.provider)
                             session ++= data.data.toSeq
                             Future.successful(
-                              Right((), Some(session))
+                              Right((callbackRedirect("success"), Some(session)))
                             )
                           case _ =>
                             run(generateUUID(), SignUpOAuth(data)) map {
@@ -290,20 +295,28 @@ trait OAuthServiceEndpoints[SD <: SessionData with SessionDataDecorator[SD]]
                                 var session =
                                   companion.newSession.withId(r.account.uuid).withAnonymous(false)
                                 session += ("provider", data.provider)
-                                Right((), Some(session))
-                              case other => Left(resultToApiError(other))
+                                Right((callbackRedirect("success"), Some(session)))
+                              case other =>
+                                log.error(s"sign up failed for ${service.networkName}: $other")
+                                Right((callbackRedirect("error", Some("signup_failed")), None))
                             }
                         }
                       case _ =>
                         log.error(s"login not found within $data for ${service.networkName}")
-                        Future.successful(Left(ApiErrors.InternalServerError()))
+                        Future.successful(
+                          Right((callbackRedirect("error", Some("login_not_found")), None))
+                        )
                     }
                   case Failure(f) =>
                     log.error(f.getMessage, f)
-                    Future.successful(Left(ApiErrors.InternalServerError()))
+                    Future.successful(
+                      Right((callbackRedirect("error", Some("user_info_failed")), None))
+                    )
                 }
               case _ =>
-                Future.successful(Left(ApiErrors.BadRequest()))
+                Future.successful(
+                  Right((callbackRedirect("error", Some("missing_code")), None))
+                )
             }
           )
       }

testkit/src/main/scala/app/softnetwork/account/scalatest/AccountRouteSpec.scala

@@ -272,7 +272,7 @@ trait AccountRouteSpec[
       Get(
         s"/$RootPath/${AccountSettings.OAuthPath}/dummy/callback?code=1234"
       ) ~> routes ~> check {
-        status shouldEqual StatusCodes.OK
+        status shouldEqual StatusCodes.Found
       }
     }
   }