diff --git a/modules/portal/app/controllers/FrontendController.scala b/modules/portal/app/controllers/FrontendController.scala index ce302118f..681281a9f 100644 --- a/modules/portal/app/controllers/FrontendController.scala +++ b/modules/portal/app/controllers/FrontendController.scala @@ -52,7 +52,7 @@ class FrontendController @Inject()( case Some(_) => Redirect("/index") case None => logger.info(s"No ${VinylDNS.ID_TOKEN} in session; Initializing oidc login") - Redirect(oidcAuthenticator.getCodeCall.toString) + Redirect(oidcAuthenticator.getCodeCall.toString, 302) } } else { request.session.get("username") match { @@ -70,9 +70,20 @@ class FrontendController @Inject()( } } - def logout(): Action[AnyContent] = Action { implicit request => + private def getLoggedInUser(request: RequestHeader) = + if (oidcEnabled) { + request.session + .get(VinylDNS.ID_TOKEN) + .flatMap { + oidcAuthenticator.getValidUsernameFromToken + } + } else { + request.session.get("username") + }.getOrElse("No user in session") + + def logout(): Action[AnyContent] = Action { implicit request => + logger.info(s"Initializing logout for user [${getLoggedInUser(request)}]") if (oidcEnabled) { - logger.info(s"Initializing oidc logout") Redirect(oidcAuthenticator.oidcLogoutUrl).withNewSession } else { Redirect("/login").withNewSession @@ -80,8 +91,7 @@ class FrontendController @Inject()( } def noAccess(): Action[AnyContent] = Action { implicit request => - logger.info( - s"User account for '${request.session.get("username").getOrElse("username not found")}' is locked.") + logger.info(s"User account for '${getLoggedInUser(request)}' is locked.") Unauthorized(views.html.noAccess()) } diff --git a/modules/portal/app/controllers/OidcAuthenticator.scala b/modules/portal/app/controllers/OidcAuthenticator.scala index 5534f1af2..5fd01b73d 100644 --- a/modules/portal/app/controllers/OidcAuthenticator.scala +++ b/modules/portal/app/controllers/OidcAuthenticator.scala @@ -17,7 +17,7 @@ package controllers import java.net.{URI, URL} -import java.util.Date +import java.util.{Date, UUID} import akka.http.scaladsl.model.Uri import akka.http.scaladsl.model.Uri.Query @@ -42,7 +42,7 @@ import play.api.libs.ws.WSClient import play.api.mvc.RequestHeader import scala.concurrent.ExecutionContext -import scala.util.Try +import scala.util.{Failure, Success, Try} import scala.collection.JavaConverters._ object OidcAuthenticator { @@ -84,9 +84,8 @@ class OidcAuthenticator @Inject()(wsClient: WSClient, configuration: Configurati lazy val clientSecret = new Secret(oidcInfo.secret) lazy val clientAuth = new ClientSecretBasic(clientID, clientSecret) lazy val tokenEndpoint = new URI(oidcInfo.tokenEndpoint) - lazy val redirectUriString: String = oidcInfo.redirectUri + "/callback" + lazy val redirectUriString: String = oidcInfo.redirectUri + "/callback/" lazy val oidcLogoutUrl: String = oidcInfo.logoutEndpoint - lazy val redirectUri = new URI(redirectUriString) lazy val sc = new SimpleSecurityContext() @@ -105,23 +104,26 @@ class OidcAuthenticator @Inject()(wsClient: WSClient, configuration: Configurati def getCodeCall: Uri = { val nonce = new Nonce() + val loginId = UUID.randomUUID().toString + val redirectUri = s"${oidcInfo.redirectUri}/callback/$loginId" val query = Query( "client_id" -> oidcInfo.clientId, "response_type" -> "code", - "redirect_uri" -> redirectUriString, + "redirect_uri" -> redirectUri, "scope" -> oidcInfo.scope, "nonce" -> nonce.toString ) + logger.info(s"Generated LoginId $loginId") Uri(s"${oidcInfo.authorizationEndpoint}").withQuery(query) } def getCodeFromAuthResponse(request: RequestHeader): Either[ErrorResponse, AuthorizationCode] = Try(AuthenticationResponseParser.parse(new URI(request.uri))).toEither .leftMap { err => - logger.error(s"Unexpected parse error in getCodeFromAuthResponse: ${err.getMessage}") - ErrorResponse(500, err.getMessage) + val errorMessage = s"Unexpected parse error in getCodeFromAuthResponse: ${err.getMessage}" + ErrorResponse(500, errorMessage) } .flatMap { case s: AuthenticationSuccessResponse => @@ -176,13 +178,23 @@ class OidcAuthenticator @Inject()(wsClient: WSClient, configuration: Configurati } def getValidUsernameFromToken(jwtClaimsSetString: String): Option[String] = { - val claimsSet = Try(JWTClaimsSet.parse(jwtClaimsSetString)).toOption + val claimsSet = Try(JWTClaimsSet.parse(jwtClaimsSetString)) match { + case Success(s) => Some(s) + case Failure(e) => + logger.error(s"oidc session token parse error: ${e.getMessage}") + None + } val isValid = claimsSet.exists(isValidIdToken) + val username = claimsSet.flatMap(getStringFieldOption(_, oidcInfo.jwtUsernameField)) if (isValid) { // only return username if the token is valid - claimsSet.flatMap(getStringFieldOption(_, oidcInfo.jwtUsernameField)) + if (username.isEmpty) { + logger.error("valid id token is missing username") + } + username } else { + logger.info(s"oidc session token for user [$username] is invalid") None } } @@ -225,12 +237,15 @@ class OidcAuthenticator @Inject()(wsClient: WSClient, configuration: Configurati } yield claims } - def oidcCallback(code: AuthorizationCode)( + def oidcCallback(code: AuthorizationCode, loginId: String)( implicit executionContext: ExecutionContext): EitherT[IO, ErrorResponse, JWTClaimsSet] = EitherT { + val redirectUriString = s"${oidcInfo.redirectUri}/callback/$loginId" + val redirectUri = new URI(redirectUriString) val codeGrant = new AuthorizationCodeGrant(code, redirectUri) val request = new TokenRequest(tokenEndpoint, clientAuth, codeGrant) + logger.info(s"Sending token_id request for loginId [$loginId]") IO(request.toHTTPRequest.send()).map(handleCallbackResponse) } diff --git a/modules/portal/app/controllers/VinylDNS.scala b/modules/portal/app/controllers/VinylDNS.scala index ccbc34aee..a203752e3 100644 --- a/modules/portal/app/controllers/VinylDNS.scala +++ b/modules/portal/app/controllers/VinylDNS.scala @@ -128,10 +128,11 @@ class VinylDNS @Inject()( implicit val userInfoReads: Reads[VinylDNS.UserInfo] = Json.reads[VinylDNS.UserInfo] implicit val userInfoWrites: Writes[VinylDNS.UserInfo] = Json.writes[VinylDNS.UserInfo] - def oidcCallback(): Action[AnyContent] = Action.async { implicit request => + def oidcCallback(loginId: String): Action[AnyContent] = Action.async { implicit request => + Logger.info(s"Received callback for LoginId [$loginId]") val details = for { code <- EitherT.fromEither[IO](oidcAuthenticator.getCodeFromAuthResponse(request)) - validToken <- oidcAuthenticator.oidcCallback(code) + validToken <- oidcAuthenticator.oidcCallback(code, loginId) userDetails <- EitherT.fromEither[IO](oidcAuthenticator.getUserFromClaims(validToken)) userCreate <- EitherT.right[ErrorResponse](processLoginWithDetails(userDetails)) } yield (userCreate, validToken) @@ -139,10 +140,11 @@ class VinylDNS @Inject()( details.value .map { case Right((user, token)) => - Logger.info(s"--LOGIN-- user [${user.userName}] logged in with id ${user.id}") + Logger.info( + s"LoginId [$loginId] complete: --LOGIN-- user [${user.userName}] logged in with id ${user.id}") Redirect("/index").withSession(ID_TOKEN -> token.toString) case Left(err) => - Logger.error(s"Oidc callback error response: $err") + Logger.error(s"LoginId [$loginId] complete with error: $err") Status(err.code)(err.message) } .unsafeToFuture() diff --git a/modules/portal/conf/routes b/modules/portal/conf/routes index 7ad3dc69b..4a0e240ec 100644 --- a/modules/portal/conf/routes +++ b/modules/portal/conf/routes @@ -54,7 +54,7 @@ GET /api/batchchanges/:id @controllers.VinylDNS.getBatchC GET /api/batchchanges @controllers.VinylDNS.listBatchChanges POST /api/batchchanges @controllers.VinylDNS.newBatchChange -GET /callback @controllers.VinylDNS.oidcCallback +GET /callback/:id @controllers.VinylDNS.oidcCallback(id: String) # Map static resources from the /public folder to the /assets URL path GET /public/*file controllers.Assets.versioned(path="/public", file: Asset) diff --git a/modules/portal/test/controllers/FrontendControllerSpec.scala b/modules/portal/test/controllers/FrontendControllerSpec.scala index 0fc7d491e..f75b51f2b 100644 --- a/modules/portal/test/controllers/FrontendControllerSpec.scala +++ b/modules/portal/test/controllers/FrontendControllerSpec.scala @@ -248,7 +248,7 @@ class FrontendControllerSpec extends Specification with Mockito with TestApplica val result = oidcUnderTest.loginPage()(FakeRequest(GET, "/login").withCSRFToken) - status(result) must equalTo(SEE_OTHER) + status(result) must equalTo(302) headers(result) must contain("Location" -> "http://test.com") } "redirect to the index page when a user is logged in" in new WithApplication(app) {