From 5b9d7288aa198c4e112fa0be9d71498935b983a7 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Tue, 8 Dec 2020 15:42:59 +0000 Subject: [PATCH 01/25] Prototype pluggable authentication in the grid --- .../mediaservice/lib/auth/ApiAccessor.scala | 4 +- .../ApiKeyAuthenticationProvider.scala | 70 +++++++++++ .../lib/auth/provider/Authentication.scala | 111 ++++++++++++++++++ .../provider/AuthenticationProvider.scala | 81 +++++++++++++ .../provider/AuthenticationProviders.scala | 3 + .../auth/provider/AuthenticationStatus.scala | 27 +++++ .../PandaAuthenticationProvider.scala | 101 ++++++++++++++++ 7 files changed, 395 insertions(+), 2 deletions(-) create mode 100644 common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala create mode 100644 common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/Authentication.scala create mode 100644 common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala create mode 100644 common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProviders.scala create mode 100644 common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala create mode 100644 common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/PandaAuthenticationProvider.scala diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/ApiAccessor.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/ApiAccessor.scala index 37d802f676..54e489af8c 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/ApiAccessor.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/ApiAccessor.scala @@ -2,7 +2,7 @@ package com.gu.mediaservice.lib.auth import com.gu.mediaservice.lib.argo.ArgoHelpers import com.gu.mediaservice.lib.config.Services -import play.api.mvc.{Request, Result} +import play.api.mvc.{RequestHeader, Result} sealed trait Tier case object Internal extends Tier @@ -28,7 +28,7 @@ object ApiAccessor extends ArgoHelpers { ApiAccessor(name, tier) } - def hasAccess(apiKey: ApiAccessor, request: Request[Any], services: Services): Boolean = apiKey.tier match { + def hasAccess(apiKey: ApiAccessor, request: RequestHeader, services: Services): Boolean = apiKey.tier match { case Internal => true case ReadOnly => request.method == "GET" case Syndication => request.method == "GET" && request.host == services.apiHost && request.path.startsWith("/images") diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala new file mode 100644 index 0000000000..19a3eaf63e --- /dev/null +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala @@ -0,0 +1,70 @@ +package com.gu.mediaservice.lib.auth.provider +import com.gu.mediaservice.lib.auth.provider.Authentication.ApiKeyAccessor +import com.gu.mediaservice.lib.auth.{ApiAccessor, KeyStore} +import com.typesafe.scalalogging.StrictLogging +import play.api.libs.ws.WSRequest +import play.api.mvc.RequestHeader + +object ApiKeyAuthenticationProvider { + val apiKeyHeaderName = "X-Gu-Media-Key" +} + +class ApiKeyAuthenticationProvider(resources: AuthenticationProviderResources) extends ApiAuthenticationProvider with StrictLogging { + var keyStorePlaceholder: Option[KeyStore] = _ + + // TODO: we should also shutdown the keystore but there isn't currently a hook + override def initialise(): Unit = { + val store = new KeyStore(resources.config.get[String]("authKeyStoreBucket"), resources.commonConfig)(resources.context) + store.scheduleUpdates(resources.actorSystem.scheduler) + keyStorePlaceholder = Some(store) + } + + def keyStore: KeyStore = keyStorePlaceholder.getOrElse(throw new IllegalStateException("Not initialised")) + + /** + * Establish the authentication status of the given request header. This can return an authenticated user or a number + * of reasons why a user is not authenticated. + * + * @param request The request header containing cookies and other request headers that can be used to establish the + * authentication status of a request. + * @return An authentication status expressing whether the + */ + override def authenticateRequest(request: RequestHeader): ApiAuthenticationStatus = { + request.headers.get(ApiKeyAuthenticationProvider.apiKeyHeaderName) match { + case Some(key) => + keyStore.lookupIdentity(key) match { + // api key provided + case Some(apiKey) => + // valid api key + val accessor = ApiKeyAccessor(apiKey) + logger.info(s"Using api key with name ${apiKey.identity} and tier ${apiKey.tier}", apiKey) + if (ApiAccessor.hasAccess(apiKey, request, resources.commonConfig.services)) { + // valid api key which has access + Authenticated(accessor) + } else { + // valid api key which doesn't have access + NotAuthorised("API key valid but not authorised") + } + // provided api key not known + case None => Invalid("API key not valid") + } + // no api key found + case None => NotAuthenticated + } + } + + /** + * A function that allows downstream API calls to be made using the credentials of the inflight request + * + * @param request The request header of the inflight call + * @return A function that adds appropriate data to a WSRequest + */ + override def onBehalfOf(request: RequestHeader): Either[String, WSRequest => WSRequest] = { + request.headers.get(ApiKeyAuthenticationProvider.apiKeyHeaderName) match { + case Some(key) => Right { + wsRequest: WSRequest => wsRequest.addHttpHeaders(ApiKeyAuthenticationProvider.apiKeyHeaderName -> key) + } + case None => Left(s"API key not found in request, no header ${ApiKeyAuthenticationProvider.apiKeyHeaderName}") + } + } +} diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/Authentication.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/Authentication.scala new file mode 100644 index 0000000000..f7ec787d05 --- /dev/null +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/Authentication.scala @@ -0,0 +1,111 @@ +package com.gu.mediaservice.lib.auth.provider + +import com.gu.mediaservice.lib.argo.ArgoHelpers +import com.gu.mediaservice.lib.argo.model.Link +import com.gu.mediaservice.lib.auth.provider.Authentication.{ApiKeyAccessor, OnBehalfOfPrincipal, PandaUser, Principal} +import com.gu.mediaservice.lib.auth.{ApiAccessor, Internal} +import com.gu.mediaservice.lib.config.CommonConfig +import com.gu.pandomainauth.model.{AuthenticatedUser, User} +import com.gu.pandomainauth.service.Google2FAGroupChecker +import play.api.libs.ws.WSRequest +import play.api.mvc.Security.AuthenticatedRequest +import play.api.mvc._ + +import scala.concurrent.{ExecutionContext, Future} + +class Authentication(config: CommonConfig, + providers: AuthenticationProviders, + override val parser: BodyParser[AnyContent], + override val executionContext: ExecutionContext) + extends ActionBuilder[Authentication.Request, AnyContent] with ArgoHelpers { + + // make the execution context implicit so it will be picked up appropriately + implicit val ec: ExecutionContext = executionContext + + val loginLinks = List( + Link("login", config.services.loginUriTemplate) + ) + + def unauthorised(errorMessage: String, throwable: Option[Throwable] = None): Future[Result] = { + logger.info(s"Authentication failure $errorMessage", throwable.orNull) + Future.successful(respondError(Unauthorized, "authentication-failure", "Authentication failure", loginLinks)) + } + + def forbidden(errorMessage: String): Future[Result] = { + logger.info(s"User not authorised: $errorMessage") + Future.successful(respondError(Forbidden, "principal-not-authorised", "Principal not authorised", loginLinks)) + } + + def authenticationStatus(requestHeader: RequestHeader, providers: AuthenticationProviders) = { + def sendForAuth(maybePrincipal: Option[Principal]): Future[Result] = { + providers.userProvider.sendForAuthentication.fold(unauthorised("No path to authenticate user"))(_(requestHeader, maybePrincipal)) + } + + def flushToken(resultWhenAbsent: Result): Result = { + providers.userProvider.flushToken.fold(resultWhenAbsent)(_(requestHeader)) + } + + providers.apiProvider.authenticateRequest(requestHeader) match { + case Authenticated(authedUser) => Right(authedUser) + case Invalid(message, throwable) => Left(unauthorised(message, throwable)) + case NotAuthorised(message) => Left(forbidden(s"Principal not authorised: $message")) + case NotAuthenticated => + providers.userProvider.authenticateRequest(requestHeader) match { + case NotAuthenticated => Left(sendForAuth(None)) + case Expired(principal) => Left(sendForAuth(Some(principal))) + case GracePeriod(authedUser) => Right(authedUser) + case Authenticated(authedUser) => Right(authedUser) + case Invalid(message, throwable) => Left(unauthorised(message, throwable).map(flushToken)) + case NotAuthorised(message) => Left(forbidden(s"Principal not authorised: $message")) + } + } + } + + override def invokeBlock[A](request: Request[A], block: Authentication.Request[A] => Future[Result]): Future[Result] = { + // Authenticate request. Try with API authenticator first and then with user authenticator + authenticationStatus(request, providers) match { + // we have a principal, so process the block + case Right(principal) => block(new AuthenticatedRequest(principal, request)) + // no principal so return a result which will either be an error or a form of redirect + case Left(result) => result + } + } + + def getOnBehalfOfPrincipal(principal: Principal, originalRequest: RequestHeader): OnBehalfOfPrincipal = { + val maybeEnrichFn: Either[String, WSRequest => WSRequest] = principal match { + case _:ApiKeyAccessor => providers.apiProvider.onBehalfOf(originalRequest) + case _:PandaUser => providers.userProvider.onBehalfOf(originalRequest) + } + val enrichFn: WSRequest => WSRequest = maybeEnrichFn.fold(error => throw new IllegalStateException(error), identity) + new OnBehalfOfPrincipal { + override def enrich: WSRequest => WSRequest = enrichFn + } + } +} + +object Authentication { + sealed trait Principal { + def accessor: ApiAccessor + } + case class PandaUser(user: User) extends Principal { + def accessor: ApiAccessor = ApiAccessor(identity = user.email, tier = Internal) + } + case class ApiKeyAccessor(accessor: ApiAccessor) extends Principal + + type Request[A] = AuthenticatedRequest[A, Principal] + + trait OnBehalfOfPrincipal { + def enrich: WSRequest => WSRequest + } + + val originalServiceHeaderName = "X-Gu-Original-Service" + + def getIdentity(principal: Principal): String = principal.accessor.identity + + def validateUser(authedUser: AuthenticatedUser, userValidationEmailDomain: String, multifactorChecker: Option[Google2FAGroupChecker]): Boolean = { + val isValidDomain = authedUser.user.email.endsWith("@" + userValidationEmailDomain) + val passesMultifactor = if(multifactorChecker.nonEmpty) { authedUser.multiFactor } else { true } + + isValidDomain && passesMultifactor + } +} diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala new file mode 100644 index 0000000000..affa02573d --- /dev/null +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala @@ -0,0 +1,81 @@ +package com.gu.mediaservice.lib.auth.provider + +import akka.actor.ActorSystem +import com.gu.mediaservice.lib.auth.provider.Authentication.Principal +import com.gu.mediaservice.lib.config.CommonConfig +import play.api.Configuration +import play.api.libs.ws.{WSClient, WSRequest} +import play.api.mvc.{ControllerComponents, RequestHeader, Result} + +import scala.concurrent.{ExecutionContext, Future} + +/** + * Case class containing useful resources for authentication providers to allow concurrent processing and external + * API calls to be conducted. + * @param config the tree of configuration for this provider + * @param commonConfig the Grid common config object + * @param context an execution context + * @param actorSystem an actor system + * @param wsClient a play WSClient for making API calls + */ +case class AuthenticationProviderResources(config: Configuration, + commonConfig: CommonConfig, + context: ExecutionContext, + actorSystem: ActorSystem, + wsClient: WSClient, + controllerComponents: ControllerComponents) + +sealed trait AuthenticationProvider { + def initialise(): Unit = {} + def shutdown(): Future[Unit] = Future.successful(()) + + /** + * A function that allows downstream API calls to be made using the credentials of the inflight request + * @param request The request header of the inflight call + * @return A function that adds appropriate authentication headers to a WSRequest or returns a runtime error + */ + def onBehalfOf(request: RequestHeader): Either[String, WSRequest => WSRequest] +} + +trait UserAuthenticationProvider extends AuthenticationProvider { + /** + * Establish the authentication status of the given request header. This can return an authenticated user or a number + * of reasons why a user is not authenticated. + * @param request The request header containing cookies and other request headers that can be used to establish the + * authentication status of a request. + * @return An authentication status expressing whether the + */ + def authenticateRequest(request: RequestHeader): AuthenticationStatus + + /** + * If this provider supports sending a user that is not authorised to a federated auth provider then it should + * provide a function here to redirect the user. The function signature takes the applications callback URL as + * well as the request and should return a result. + */ + def sendForAuthentication: Option[(RequestHeader, Option[Principal]) => Future[Result]] + + /** + * If this provider supports sending a user that is not authorised to a federated auth provider then it should + * provide an Play action here that deals with the return of a user from a federated provider. This should be + * used to set a cookie or similar to ensure that a subsequent call to authenticateRequest will succeed. If + * authentication failed then this should return an appropriate 4xx result. + */ + def processAuthentication: Option[RequestHeader => Future[Result]] + + /** + * If this provider is able to clear user tokens (i.e. by clearing cookies) then it should provide a function to + * do that here which will be used to log users out and also if the token is invalid. + */ + def flushToken: Option[RequestHeader => Result] +} + +trait ApiAuthenticationProvider extends AuthenticationProvider { + /** + * Establish the authentication status of the given request header. This can return an authenticated user or a number + * of reasons why a user is not authenticated. + * @param request The request header containing cookies and other request headers that can be used to establish the + * authentication status of a request. + * @return An authentication status expressing whether the + */ + def authenticateRequest(request: RequestHeader): ApiAuthenticationStatus +} diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProviders.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProviders.scala new file mode 100644 index 0000000000..72e79da8bb --- /dev/null +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProviders.scala @@ -0,0 +1,3 @@ +package com.gu.mediaservice.lib.auth.provider + +case class AuthenticationProviders(userProvider: UserAuthenticationProvider, apiProvider: ApiAuthenticationProvider) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala new file mode 100644 index 0000000000..5dcbe453d3 --- /dev/null +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala @@ -0,0 +1,27 @@ +package com.gu.mediaservice.lib.auth.provider + +import com.gu.mediaservice.lib.auth.provider.Authentication.Principal + +// statuses that directly extend this are for users only +/** Status of a client's authentication */ +sealed trait AuthenticationStatus + +/** User authentication is valid but expired */ +case class Expired(authedUser: Principal) extends AuthenticationStatus +/** User authentication is valid and expired but the expiry is within a grace period */ +case class GracePeriod(authedUser: Principal) extends AuthenticationStatus + +// statuses that extend this can be used by both users and machines +/** Status of an API client's authentication */ +sealed trait ApiAuthenticationStatus extends AuthenticationStatus + +/** User authentication is valid */ +case class Authenticated(authedUser: Principal) extends ApiAuthenticationStatus +/** User authentication is OK but the user is not authorised to use this system - might be a group or 2FA check failure */ +case class NotAuthorised(message: String) extends ApiAuthenticationStatus +/** User authentication token or key (cookie, header, query param) exists but isn't valid - + * the message and exception will be logged but not leaked to user */ +case class Invalid(message: String, throwable: Option[Throwable] = None) extends ApiAuthenticationStatus +/** User authentication token doesn't exist */ +case object NotAuthenticated extends ApiAuthenticationStatus + diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/PandaAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/PandaAuthenticationProvider.scala new file mode 100644 index 0000000000..0a68c2198b --- /dev/null +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/PandaAuthenticationProvider.scala @@ -0,0 +1,101 @@ +package com.gu.mediaservice.lib.auth.provider +import com.gu.mediaservice.lib.auth.provider.Authentication.{PandaUser, Principal} +import com.gu.mediaservice.lib.aws.S3Ops +import com.gu.pandomainauth.PanDomainAuthSettingsRefresher +import com.gu.pandomainauth.action.AuthActions +import com.gu.pandomainauth.model.{AuthenticatedUser, Authenticated => PandaAuthenticated, Expired => PandaExpired, GracePeriod => PandaGracePeriod, InvalidCookie => PandaInvalidCookie, NotAuthenticated => PandaNotAuthenticated, NotAuthorized => PandaNotAuthorised} +import play.api.libs.ws.{DefaultWSCookie, WSClient, WSRequest} +import play.api.mvc.{ControllerComponents, RequestHeader, Result} + +import scala.concurrent.Future + +class PandaAuthenticationProvider(resources: AuthenticationProviderResources) extends UserAuthenticationProvider with AuthActions { + + final override def authCallbackUrl: String = s"${resources.commonConfig.services.authBaseUri}/oauthCallback" + override lazy val panDomainSettings: PanDomainAuthSettingsRefresher = buildPandaSettings() + override def wsClient: WSClient = resources.wsClient + override def controllerComponents: ControllerComponents = resources.controllerComponents + + /** + * Establish the authentication status of the given request header. This can return an authenticated user or a number + * of reasons why a user is not authenticated. + * + * @param request The request header containing cookies and other request headers that can be used to establish the + * authentication status of a request. + * @return An authentication status expressing whether the + */ + override def authenticateRequest(request: RequestHeader): AuthenticationStatus = { + extractAuth(request) match { + case PandaNotAuthenticated => NotAuthenticated + case PandaInvalidCookie(e) => Invalid("error checking user's auth, clear cookie and re-auth", Some(e)) + case PandaExpired(authedUser) => Expired(PandaUser(authedUser.user)) + case PandaGracePeriod(authedUser) => GracePeriod(PandaUser(authedUser.user)) + case PandaNotAuthorised(authedUser) => NotAuthorised(s"${authedUser.user.email} not authorised to use application") + case PandaAuthenticated(authedUser) => Authenticated(PandaUser(authedUser.user)) + } + } + + /** + * If this provider supports sending a user that is not authorised to a federated auth provider then it should + * provide a function here to redirect the user. + */ + override def sendForAuthentication: Option[(RequestHeader, Option[Principal]) => Future[Result]] = Some( + { (requestHeader: RequestHeader, principal: Option[Principal]) => + val email = principal.collect{ + case PandaUser(user) => user.email + } + sendForAuth(requestHeader, email) + } + ) + + /** + * If this provider supports sending a user that is not authorised to a federated auth provider then it should + * provide an Play action here that deals with the return of a user from a federated provider. This should be + * used to set a cookie or similar to ensure that a subsequent call to authenticateRequest will succeed. If + * authentication failed then this should return an appropriate 4xx result. + */ + override def processAuthentication: Option[RequestHeader => Future[Result]] = Some( + processOAuthCallback()(_) + ) + + /** + * If this provider is able to clear user tokens (i.e. by clearing cookies) then it should provide a function to + * do that here which will be used to log users out and also if the token is invalid. + * + * @return + */ + override def flushToken: Option[RequestHeader => Result] = Some(processLogout(_)) + + /** + * A function that allows downstream API calls to be made using the credentials of the inflight request + * + * @param request The request header of the inflight call + * @return A function that adds appropriate data to a WSRequest + */ + override def onBehalfOf(request: RequestHeader): Either[String, WSRequest => WSRequest] = { + val cookieName = panDomainSettings.settings.cookieSettings.cookieName + request.cookies.get(cookieName) match { + case Some(cookie) => Right { wsRequest: WSRequest => + wsRequest.addCookies(DefaultWSCookie(cookieName, cookie.value)) + } + case None => Left(s"Pan domain cookie $cookieName is missing in original request.") + } + } + + private def buildPandaSettings() = { + new PanDomainAuthSettingsRefresher( + domain = resources.commonConfig.services.domainRoot, + system = resources.commonConfig.stringOpt("panda.system").getOrElse("media-service"), + bucketName = resources.commonConfig.stringOpt("panda.bucketName").getOrElse("pan-domain-auth-settings"), + settingsFileKey = resources.commonConfig.stringOpt("panda.settingsFileKey").getOrElse(s"${resources.commonConfig.services.domainRoot}.settings"), + s3Client = S3Ops.buildS3Client(resources.commonConfig, localstackAware=resources.commonConfig.useLocalAuth) + ) + } + + private val userValidationEmailDomain = resources.commonConfig.stringOpt("panda.userDomain").getOrElse("guardian.co.uk") + + final override def validateUser(authedUser: AuthenticatedUser): Boolean = { + Authentication.validateUser(authedUser, userValidationEmailDomain, multifactorChecker) + } + +} From b7665d3e2fe8f7a925dd5264ad776b86dd98c118 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Fri, 8 Jan 2021 16:37:35 +0000 Subject: [PATCH 02/25] Migrate code codebase to use auth providers --- .../ImageProjectionLambdaHandler.scala | 6 +- .../com/gu/mediaservice/GridClient.scala | 9 +- auth/app/auth/AuthComponents.scala | 2 +- auth/app/auth/AuthController.scala | 78 +++++------ .../lib/auth/Authentication.scala | 126 ++++++++---------- .../lib/auth/PermissionsHandler.scala | 4 +- .../ApiKeyAuthenticationProvider.scala | 16 ++- .../lib/auth/provider/Authentication.scala | 111 --------------- .../provider/AuthenticationProvider.scala | 4 +- .../auth/provider/AuthenticationStatus.scala | 2 +- .../auth}/PandaAuthenticationProvider.scala | 56 ++++++-- .../lib/play/GridComponents.scala | 9 +- .../app/controllers/CropperController.scala | 17 +-- media-api/app/controllers/MediaApi.scala | 16 +-- 14 files changed, 178 insertions(+), 278 deletions(-) delete mode 100644 common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/Authentication.scala rename common-lib/src/main/scala/com/gu/mediaservice/lib/{auth/provider => guardian/auth}/PandaAuthenticationProvider.scala (61%) diff --git a/admin-tools/lambda/src/main/scala/com/gu/mediaservice/ImageProjectionLambdaHandler.scala b/admin-tools/lambda/src/main/scala/com/gu/mediaservice/ImageProjectionLambdaHandler.scala index 076dd3bd15..9ec4515799 100644 --- a/admin-tools/lambda/src/main/scala/com/gu/mediaservice/ImageProjectionLambdaHandler.scala +++ b/admin-tools/lambda/src/main/scala/com/gu/mediaservice/ImageProjectionLambdaHandler.scala @@ -2,10 +2,10 @@ package com.gu.mediaservice import com.amazonaws.services.lambda.runtime.Context import com.amazonaws.services.lambda.runtime.events.{APIGatewayProxyRequestEvent, APIGatewayProxyResponseEvent} -import com.gu.mediaservice.lib.auth.Authentication +import com.gu.mediaservice.lib.auth.provider.ApiKeyAuthenticationProvider import com.gu.mediaservice.model.Image import com.typesafe.scalalogging.LazyLogging -import play.api.libs.json.{JsString, Json} +import play.api.libs.json.Json import scala.collection.JavaConverters._ import scala.concurrent.ExecutionContext.Implicits.global @@ -94,7 +94,7 @@ class ImageProjectionLambdaHandler extends LazyLogging { private def getAuthKeyFrom(headers: Map[String, String]) = { // clients like curl or API gateway may lowerCases custom header names, yay! headers.find { - case (k, _) => k.equalsIgnoreCase(Authentication.apiKeyHeaderName) + case (k, _) => k.equalsIgnoreCase(ApiKeyAuthenticationProvider.apiKeyHeaderName) }.map(_._2) } } diff --git a/admin-tools/lib/src/main/scala/com/gu/mediaservice/GridClient.scala b/admin-tools/lib/src/main/scala/com/gu/mediaservice/GridClient.scala index 6b6bbd1ca9..6abdbd8d2e 100644 --- a/admin-tools/lib/src/main/scala/com/gu/mediaservice/GridClient.scala +++ b/admin-tools/lib/src/main/scala/com/gu/mediaservice/GridClient.scala @@ -2,13 +2,12 @@ package com.gu.mediaservice import java.io.IOException import java.net.URL - -import com.gu.mediaservice.lib.auth.Authentication +import com.gu.mediaservice.lib.auth.provider.ApiKeyAuthenticationProvider import com.typesafe.scalalogging.LazyLogging import okhttp3._ import play.api.libs.json.{JsValue, Json} -import scala.util.{Try, Success, Failure} +import scala.util.{Failure, Success, Try} import scala.concurrent.{ExecutionContext, Future, Promise} object ClientResponse { @@ -52,7 +51,7 @@ class GridClient(maxIdleConnections: Int, debugHttpResponse: Boolean) extends La .build() def makeGetRequestSync(url: URL, apiKey: String): ResponseWrapper = { - val request = new Request.Builder().url(url).header(Authentication.apiKeyHeaderName, apiKey).build + val request = new Request.Builder().url(url).header(ApiKeyAuthenticationProvider.apiKeyHeaderName, apiKey).build val response = httpClient.newCall(request).execute processResponse(response, url) } @@ -128,7 +127,7 @@ class GridClient(maxIdleConnections: Int, debugHttpResponse: Boolean) extends La } private def makeRequestAsync(url: URL, apiKey: String): Future[Response] = { - val request = new Request.Builder().url(url).header(Authentication.apiKeyHeaderName, apiKey).build + val request = new Request.Builder().url(url).header(ApiKeyAuthenticationProvider.apiKeyHeaderName, apiKey).build val promise = Promise[Response]() httpClient.newCall(request).enqueue(new Callback { override def onFailure(call: Call, e: IOException): Unit = promise.failure(e) diff --git a/auth/app/auth/AuthComponents.scala b/auth/app/auth/AuthComponents.scala index 9814ab4ce1..e8fe7bd8aa 100644 --- a/auth/app/auth/AuthComponents.scala +++ b/auth/app/auth/AuthComponents.scala @@ -12,7 +12,7 @@ class AuthComponents(context: Context) extends GridComponents(context, new AuthC final override val buildInfo = utils.buildinfo.BuildInfo - val controller = new AuthController(auth, config, controllerComponents) + val controller = new AuthController(auth, providers, config, controllerComponents) val permissionsAwareManagement = new ManagementWithPermissions(controllerComponents, controller, buildInfo) override val router = new Routes(httpErrorHandler, controller, permissionsAwareManagement) diff --git a/auth/app/auth/AuthController.scala b/auth/app/auth/AuthController.scala index b4f32e5259..6bb8e5c05f 100644 --- a/auth/app/auth/AuthController.scala +++ b/auth/app/auth/AuthController.scala @@ -1,20 +1,18 @@ package auth import java.net.URI - import com.gu.mediaservice.lib.argo.ArgoHelpers import com.gu.mediaservice.lib.argo.model.Link -import com.gu.mediaservice.lib.auth.Authentication.PandaUser +import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, GridUser} +import com.gu.mediaservice.lib.auth.provider.AuthenticationProviders import com.gu.mediaservice.lib.auth.{Authentication, Permissions, PermissionsHandler} -import com.gu.mediaservice.lib.logging.GridLogging -import com.gu.pandomainauth.service.OAuthException import play.api.libs.json.Json import play.api.mvc.{BaseController, ControllerComponents} import scala.concurrent.{ExecutionContext, Future} import scala.util.Try -class AuthController(auth: Authentication, val config: AuthConfig, +class AuthController(auth: Authentication, providers: AuthenticationProviders, val config: AuthConfig, override val controllerComponents: ControllerComponents)(implicit ec: ExecutionContext) extends BaseController with ArgoHelpers @@ -31,30 +29,40 @@ class AuthController(auth: Authentication, val config: AuthConfig, respond(indexData, indexLinks) } - def index = auth.AuthAction { indexResponse } - - def session = auth.AuthAction { request => - val user = request.user - val firstName = user.firstName - val lastName = user.lastName + def index = auth { indexResponse } - val showPaid = hasPermission(PandaUser(request.user), Permissions.ShowPaid) + def session = auth { request => + val showPaid = hasPermission(request.user, Permissions.ShowPaid) + request.user match { + case GridUser(firstName, lastName, email, _) => - respond( - Json.obj("user" -> - Json.obj( - "name" -> s"$firstName $lastName", - "firstName" -> firstName, - "lastName" -> lastName, - "email" -> user.email, - "avatarUrl" -> user.avatarUrl, - "permissions" -> + respond( + Json.obj("user" -> Json.obj( - "showPaid" -> showPaid + "name" -> s"$firstName $lastName", + "firstName" -> firstName, + "lastName" -> lastName, + "email" -> email, + "permissions" -> + Json.obj( + "showPaid" -> showPaid + ) ) + ) + ) + case ApiKeyAccessor(accessor, _) => respond( + Json.obj("api-key" -> + Json.obj( + "name" -> accessor.identity, + "tier" -> accessor.tier.toString, + "permissions" -> + Json.obj( + "showPaid" -> showPaid + ) + ) ) ) - ) + } } @@ -65,12 +73,11 @@ class AuthController(auth: Authentication, val config: AuthConfig, Try(URI.create(inputUri)).filter(isOwnDomainAndSecure).isSuccess } - // Trigger the auth cycle // If a redirectUri is provided, redirect the browser there once auth'd, // else return a dummy page (e.g. for automatically re-auth'ing in the background) // FIXME: validate redirectUri before doing the auth - def doLogin(redirectUri: Option[String] = None) = auth.AuthAction { req => + def doLogin(redirectUri: Option[String] = None) = auth { req => redirectUri map { case uri if isValidDomain(uri) => Redirect(uri) case _ => Ok("logged in (not redirecting to external redirectUri)") @@ -78,23 +85,16 @@ class AuthController(auth: Authentication, val config: AuthConfig, } def oauthCallback = Action.async { implicit request => - // We use the `Try` here as the `GoogleAuthException` are thrown before we - // get to the asynchronicity of the `Future` it returns. - // We then have to flatten the Future[Future[T]]. Fiddly... - Future.fromTry(Try(auth.processOAuthCallback())).flatten.recover { - // This is when session session args are missing - case e: OAuthException => respondError(BadRequest, "google-auth-exception", e.getMessage, auth.loginLinks) - - // Class `missing anti forgery token` as a 4XX - // see https://github.com/guardian/pan-domain-authentication/blob/master/pan-domain-auth-play_2-6/src/main/scala/com/gu/pandomainauth/service/GoogleAuth.scala#L63 - case e: IllegalArgumentException if e.getMessage == "The anti forgery token did not match" => { - logger.error(e.getMessage) - respondError(BadRequest, "google-auth-exception", e.getMessage, auth.loginLinks) - } + providers.userProvider.processAuthentication match { + case Some(callback) => callback(request) + case None => Future.successful(InternalServerError("No callback for configured authentication provider")) } } def logout = Action { implicit request => - auth.processLogout + providers.userProvider.flushToken match { + case Some(callback) => callback(request) + case None => InternalServerError("Logout not supported by configured authentication provider") + } } } diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala index a14b8caa19..0ab2b22f1e 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala @@ -1,114 +1,100 @@ package com.gu.mediaservice.lib.auth -import akka.actor.ActorSystem -import com.amazonaws.services.s3.AmazonS3ClientBuilder import com.gu.mediaservice.lib.argo.ArgoHelpers import com.gu.mediaservice.lib.argo.model.Link -import com.gu.mediaservice.lib.auth.Authentication.{Request => _, _} -import com.gu.mediaservice.lib.aws.S3Ops +import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, GridUser, OnBehalfOfPrincipal, Principal} +import com.gu.mediaservice.lib.auth.provider.{Authenticated, AuthenticationProvider, AuthenticationProviders, Expired, GracePeriod, Invalid, NotAuthenticated, NotAuthorised} import com.gu.mediaservice.lib.config.CommonConfig -import com.gu.mediaservice.lib.logging.GridLogging -import com.gu.pandomainauth.PanDomainAuthSettingsRefresher -import com.gu.pandomainauth.action.{AuthActions, UserRequest} -import com.gu.pandomainauth.model.{AuthenticatedUser, User} +import com.gu.pandomainauth.model.AuthenticatedUser import com.gu.pandomainauth.service.Google2FAGroupChecker -import play.api.libs.ws.{DefaultWSCookie, WSClient, WSCookie} +import play.api.libs.typedmap.TypedMap +import play.api.libs.ws.WSRequest import play.api.mvc.Security.AuthenticatedRequest import play.api.mvc._ import scala.concurrent.{ExecutionContext, Future} -class Authentication(config: CommonConfig, actorSystem: ActorSystem, +class Authentication(config: CommonConfig, + providers: AuthenticationProviders, override val parser: BodyParser[AnyContent], - override val wsClient: WSClient, - override val controllerComponents: ControllerComponents, override val executionContext: ExecutionContext) + extends ActionBuilder[Authentication.Request, AnyContent] with ArgoHelpers { - extends ActionBuilder[Authentication.Request, AnyContent] with AuthActions with ArgoHelpers { - + // make the execution context implicit so it will be picked up appropriately implicit val ec: ExecutionContext = executionContext val loginLinks = List( Link("login", config.services.loginUriTemplate) ) - // API key errors - def invalidApiKeyResult = respondError(Unauthorized, "invalid-api-key", "Invalid API key provided", loginLinks) - - val keyStore = new KeyStore(config.authKeyStoreBucket, config) - - keyStore.scheduleUpdates(actorSystem.scheduler) + def unauthorised(errorMessage: String, throwable: Option[Throwable] = None): Future[Result] = { + logger.info(s"Authentication failure $errorMessage", throwable.orNull) + Future.successful(respondError(Unauthorized, "authentication-failure", "Authentication failure", loginLinks)) + } - private val userValidationEmailDomain = config.stringOpt("panda.userDomain").getOrElse("guardian.co.uk") + def forbidden(errorMessage: String): Future[Result] = { + logger.info(s"User not authorised: $errorMessage") + Future.successful(respondError(Forbidden, "principal-not-authorised", "Principal not authorised", loginLinks)) + } - override lazy val panDomainSettings = buildPandaSettings() + def authenticationStatus(requestHeader: RequestHeader, providers: AuthenticationProviders) = { + def sendForAuth(maybePrincipal: Option[Principal]): Future[Result] = { + providers.userProvider.sendForAuthentication.fold(unauthorised("No path to authenticate user"))(_(requestHeader, maybePrincipal)) + } - final override def authCallbackUrl: String = s"${config.services.authBaseUri}/oauthCallback" + def flushToken(resultWhenAbsent: Result): Result = { + providers.userProvider.flushToken.fold(resultWhenAbsent)(_(requestHeader)) + } - override def invokeBlock[A](request: Request[A], block: Authentication.Request[A] => Future[Result]): Future[Result] = { - // Try to auth by API key, and failing that, with Panda - request.headers.get(Authentication.apiKeyHeaderName) match { - case Some(key) => - keyStore.lookupIdentity(key) match { - case Some(apiKey) => - logger.info(s"Using api key with name ${apiKey.identity} and tier ${apiKey.tier}", apiKey) - if (ApiAccessor.hasAccess(apiKey, request, config.services)) - block(new AuthenticatedRequest(ApiKeyAccessor(apiKey), request)) - else - Future.successful(ApiAccessor.unauthorizedResult) - case None => Future.successful(invalidApiKeyResult) + providers.apiProvider.authenticateRequest(requestHeader) match { + case Authenticated(authedUser) => Right(authedUser) + case Invalid(message, throwable) => Left(unauthorised(message, throwable)) + case NotAuthorised(message) => Left(forbidden(s"Principal not authorised: $message")) + case NotAuthenticated => + providers.userProvider.authenticateRequest(requestHeader) match { + case NotAuthenticated => Left(sendForAuth(None)) + case Expired(principal) => Left(sendForAuth(Some(principal))) + case GracePeriod(authedUser) => Right(authedUser) + case Authenticated(authedUser) => Right(authedUser) + case Invalid(message, throwable) => Left(unauthorised(message, throwable).map(flushToken)) + case NotAuthorised(message) => Left(forbidden(s"Principal not authorised: $message")) } - case None => - APIAuthAction.invokeBlock(request, (userRequest: UserRequest[A]) => { - block(new AuthenticatedRequest(PandaUser(userRequest.user), request)) - }) } } - final override def validateUser(authedUser: AuthenticatedUser): Boolean = { - Authentication.validateUser(authedUser, userValidationEmailDomain, multifactorChecker) - } - - def getOnBehalfOfPrincipal(principal: Principal, originalRequest: Request[_]): OnBehalfOfPrincipal = principal match { - case service: ApiKeyAccessor => - OnBehalfOfApiKey(service) - - case user: PandaUser => - val cookieName = panDomainSettings.settings.cookieSettings.cookieName - - originalRequest.cookies.get(cookieName) match { - case Some(cookie) => OnBehalfOfUser(user, DefaultWSCookie(cookieName, cookie.value)) - case None => throw new IllegalStateException(s"Unable to generate cookie header on behalf of ${principal.accessor}. Missing original cookie $cookieName") - } + override def invokeBlock[A](request: Request[A], block: Authentication.Request[A] => Future[Result]): Future[Result] = { + // Authenticate request. Try with API authenticator first and then with user authenticator + authenticationStatus(request, providers) match { + // we have a principal, so process the block + case Right(principal) => block(new AuthenticatedRequest(principal, request)) + // no principal so return a result which will either be an error or a form of redirect + case Left(result) => result + } } - private def buildPandaSettings() = { - new PanDomainAuthSettingsRefresher( - domain = config.services.domainRoot, - system = config.stringOpt("panda.system").getOrElse("media-service"), - bucketName = config.stringOpt("panda.bucketName").getOrElse("pan-domain-auth-settings"), - settingsFileKey = config.stringOpt("panda.settingsFileKey").getOrElse(s"${config.services.domainRoot}.settings"), - s3Client = S3Ops.buildS3Client(config, localstackAware=config.useLocalAuth) - ) + def getOnBehalfOfPrincipal(principal: Principal): OnBehalfOfPrincipal = { + val provider: AuthenticationProvider = principal match { + case _:ApiKeyAccessor => providers.apiProvider + case _:GridUser => providers.userProvider + } + val maybeEnrichFn: Either[String, WSRequest => WSRequest] = provider.onBehalfOf(principal) + maybeEnrichFn.fold(error => throw new IllegalStateException(error), identity) } } object Authentication { sealed trait Principal { def accessor: ApiAccessor + def attributes: TypedMap } - case class PandaUser(user: User) extends Principal { - def accessor: ApiAccessor = ApiAccessor(identity = user.email, tier = Internal) + case class GridUser(firstName: String, lastName: String, email: String, attributes: TypedMap = TypedMap.empty) extends Principal { + def accessor: ApiAccessor = ApiAccessor(identity = email, tier = Internal) } - case class ApiKeyAccessor(accessor: ApiAccessor) extends Principal - + case class ApiKeyAccessor(accessor: ApiAccessor, attributes: TypedMap = TypedMap.empty) extends Principal type Request[A] = AuthenticatedRequest[A, Principal] - sealed trait OnBehalfOfPrincipal { def principal: Principal } - case class OnBehalfOfUser(override val principal: PandaUser, cookie: WSCookie) extends OnBehalfOfPrincipal - case class OnBehalfOfApiKey(override val principal: ApiKeyAccessor) extends OnBehalfOfPrincipal + type OnBehalfOfPrincipal = WSRequest => WSRequest - val apiKeyHeaderName = "X-Gu-Media-Key" val originalServiceHeaderName = "X-Gu-Original-Service" def getIdentity(principal: Principal): String = principal.accessor.identity diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/PermissionsHandler.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/PermissionsHandler.scala index e1d878b7bd..02e1c13380 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/PermissionsHandler.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/PermissionsHandler.scala @@ -1,6 +1,6 @@ package com.gu.mediaservice.lib.auth -import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, PandaUser, Principal} +import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, GridUser, Principal} import com.gu.mediaservice.lib.aws.S3Ops import com.gu.mediaservice.lib.config.CommonConfig import com.gu.permissions._ @@ -30,7 +30,7 @@ trait PermissionsHandler { def hasPermission(user: Principal, permission: PermissionDefinition): Boolean = { user match { - case PandaUser(u) => permissions.hasPermission(permission, u.email) + case GridUser(_, _, email, _) => permissions.hasPermission(permission, email) // think about only allowing certain services i.e. on `service.name`? case service: ApiKeyAccessor if service.accessor.tier == Internal => true case _ => false diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala index 19a3eaf63e..1eb21791a3 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala @@ -1,7 +1,8 @@ package com.gu.mediaservice.lib.auth.provider -import com.gu.mediaservice.lib.auth.provider.Authentication.ApiKeyAccessor +import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, Principal} import com.gu.mediaservice.lib.auth.{ApiAccessor, KeyStore} import com.typesafe.scalalogging.StrictLogging +import play.api.libs.typedmap.{TypedEntry, TypedKey, TypedMap} import play.api.libs.ws.WSRequest import play.api.mvc.RequestHeader @@ -10,6 +11,8 @@ object ApiKeyAuthenticationProvider { } class ApiKeyAuthenticationProvider(resources: AuthenticationProviderResources) extends ApiAuthenticationProvider with StrictLogging { + val ApiKeyHeader: TypedKey[(String, String)] = TypedKey[(String, String)]("ApiKeyHeader") + var keyStorePlaceholder: Option[KeyStore] = _ // TODO: we should also shutdown the keystore but there isn't currently a hook @@ -36,7 +39,8 @@ class ApiKeyAuthenticationProvider(resources: AuthenticationProviderResources) e // api key provided case Some(apiKey) => // valid api key - val accessor = ApiKeyAccessor(apiKey) + val header = TypedEntry(ApiKeyHeader, ApiKeyAuthenticationProvider.apiKeyHeaderName -> key) + val accessor = ApiKeyAccessor(apiKey, TypedMap(header)) logger.info(s"Using api key with name ${apiKey.identity} and tier ${apiKey.tier}", apiKey) if (ApiAccessor.hasAccess(apiKey, request, resources.commonConfig.services)) { // valid api key which has access @@ -59,10 +63,10 @@ class ApiKeyAuthenticationProvider(resources: AuthenticationProviderResources) e * @param request The request header of the inflight call * @return A function that adds appropriate data to a WSRequest */ - override def onBehalfOf(request: RequestHeader): Either[String, WSRequest => WSRequest] = { - request.headers.get(ApiKeyAuthenticationProvider.apiKeyHeaderName) match { - case Some(key) => Right { - wsRequest: WSRequest => wsRequest.addHttpHeaders(ApiKeyAuthenticationProvider.apiKeyHeaderName -> key) + override def onBehalfOf(principal: Principal): Either[String, WSRequest => WSRequest] = { + principal.attributes.get(ApiKeyHeader) match { + case Some(apiKeyHeaderTuple) => Right { + wsRequest: WSRequest => wsRequest.addHttpHeaders(apiKeyHeaderTuple) } case None => Left(s"API key not found in request, no header ${ApiKeyAuthenticationProvider.apiKeyHeaderName}") } diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/Authentication.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/Authentication.scala deleted file mode 100644 index f7ec787d05..0000000000 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/Authentication.scala +++ /dev/null @@ -1,111 +0,0 @@ -package com.gu.mediaservice.lib.auth.provider - -import com.gu.mediaservice.lib.argo.ArgoHelpers -import com.gu.mediaservice.lib.argo.model.Link -import com.gu.mediaservice.lib.auth.provider.Authentication.{ApiKeyAccessor, OnBehalfOfPrincipal, PandaUser, Principal} -import com.gu.mediaservice.lib.auth.{ApiAccessor, Internal} -import com.gu.mediaservice.lib.config.CommonConfig -import com.gu.pandomainauth.model.{AuthenticatedUser, User} -import com.gu.pandomainauth.service.Google2FAGroupChecker -import play.api.libs.ws.WSRequest -import play.api.mvc.Security.AuthenticatedRequest -import play.api.mvc._ - -import scala.concurrent.{ExecutionContext, Future} - -class Authentication(config: CommonConfig, - providers: AuthenticationProviders, - override val parser: BodyParser[AnyContent], - override val executionContext: ExecutionContext) - extends ActionBuilder[Authentication.Request, AnyContent] with ArgoHelpers { - - // make the execution context implicit so it will be picked up appropriately - implicit val ec: ExecutionContext = executionContext - - val loginLinks = List( - Link("login", config.services.loginUriTemplate) - ) - - def unauthorised(errorMessage: String, throwable: Option[Throwable] = None): Future[Result] = { - logger.info(s"Authentication failure $errorMessage", throwable.orNull) - Future.successful(respondError(Unauthorized, "authentication-failure", "Authentication failure", loginLinks)) - } - - def forbidden(errorMessage: String): Future[Result] = { - logger.info(s"User not authorised: $errorMessage") - Future.successful(respondError(Forbidden, "principal-not-authorised", "Principal not authorised", loginLinks)) - } - - def authenticationStatus(requestHeader: RequestHeader, providers: AuthenticationProviders) = { - def sendForAuth(maybePrincipal: Option[Principal]): Future[Result] = { - providers.userProvider.sendForAuthentication.fold(unauthorised("No path to authenticate user"))(_(requestHeader, maybePrincipal)) - } - - def flushToken(resultWhenAbsent: Result): Result = { - providers.userProvider.flushToken.fold(resultWhenAbsent)(_(requestHeader)) - } - - providers.apiProvider.authenticateRequest(requestHeader) match { - case Authenticated(authedUser) => Right(authedUser) - case Invalid(message, throwable) => Left(unauthorised(message, throwable)) - case NotAuthorised(message) => Left(forbidden(s"Principal not authorised: $message")) - case NotAuthenticated => - providers.userProvider.authenticateRequest(requestHeader) match { - case NotAuthenticated => Left(sendForAuth(None)) - case Expired(principal) => Left(sendForAuth(Some(principal))) - case GracePeriod(authedUser) => Right(authedUser) - case Authenticated(authedUser) => Right(authedUser) - case Invalid(message, throwable) => Left(unauthorised(message, throwable).map(flushToken)) - case NotAuthorised(message) => Left(forbidden(s"Principal not authorised: $message")) - } - } - } - - override def invokeBlock[A](request: Request[A], block: Authentication.Request[A] => Future[Result]): Future[Result] = { - // Authenticate request. Try with API authenticator first and then with user authenticator - authenticationStatus(request, providers) match { - // we have a principal, so process the block - case Right(principal) => block(new AuthenticatedRequest(principal, request)) - // no principal so return a result which will either be an error or a form of redirect - case Left(result) => result - } - } - - def getOnBehalfOfPrincipal(principal: Principal, originalRequest: RequestHeader): OnBehalfOfPrincipal = { - val maybeEnrichFn: Either[String, WSRequest => WSRequest] = principal match { - case _:ApiKeyAccessor => providers.apiProvider.onBehalfOf(originalRequest) - case _:PandaUser => providers.userProvider.onBehalfOf(originalRequest) - } - val enrichFn: WSRequest => WSRequest = maybeEnrichFn.fold(error => throw new IllegalStateException(error), identity) - new OnBehalfOfPrincipal { - override def enrich: WSRequest => WSRequest = enrichFn - } - } -} - -object Authentication { - sealed trait Principal { - def accessor: ApiAccessor - } - case class PandaUser(user: User) extends Principal { - def accessor: ApiAccessor = ApiAccessor(identity = user.email, tier = Internal) - } - case class ApiKeyAccessor(accessor: ApiAccessor) extends Principal - - type Request[A] = AuthenticatedRequest[A, Principal] - - trait OnBehalfOfPrincipal { - def enrich: WSRequest => WSRequest - } - - val originalServiceHeaderName = "X-Gu-Original-Service" - - def getIdentity(principal: Principal): String = principal.accessor.identity - - def validateUser(authedUser: AuthenticatedUser, userValidationEmailDomain: String, multifactorChecker: Option[Google2FAGroupChecker]): Boolean = { - val isValidDomain = authedUser.user.email.endsWith("@" + userValidationEmailDomain) - val passesMultifactor = if(multifactorChecker.nonEmpty) { authedUser.multiFactor } else { true } - - isValidDomain && passesMultifactor - } -} diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala index affa02573d..8ef05c3dcb 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala @@ -1,7 +1,7 @@ package com.gu.mediaservice.lib.auth.provider import akka.actor.ActorSystem -import com.gu.mediaservice.lib.auth.provider.Authentication.Principal +import com.gu.mediaservice.lib.auth.Authentication.Principal import com.gu.mediaservice.lib.config.CommonConfig import play.api.Configuration import play.api.libs.ws.{WSClient, WSRequest} @@ -34,7 +34,7 @@ sealed trait AuthenticationProvider { * @param request The request header of the inflight call * @return A function that adds appropriate authentication headers to a WSRequest or returns a runtime error */ - def onBehalfOf(request: RequestHeader): Either[String, WSRequest => WSRequest] + def onBehalfOf(request: Principal): Either[String, WSRequest => WSRequest] } trait UserAuthenticationProvider extends AuthenticationProvider { diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala index 5dcbe453d3..38f477dc87 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala @@ -1,6 +1,6 @@ package com.gu.mediaservice.lib.auth.provider -import com.gu.mediaservice.lib.auth.provider.Authentication.Principal +import com.gu.mediaservice.lib.auth.Authentication.Principal // statuses that directly extend this are for users only /** Status of a client's authentication */ diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/PandaAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala similarity index 61% rename from common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/PandaAuthenticationProvider.scala rename to common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala index 0a68c2198b..36814790be 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/PandaAuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala @@ -1,13 +1,19 @@ -package com.gu.mediaservice.lib.auth.provider -import com.gu.mediaservice.lib.auth.provider.Authentication.{PandaUser, Principal} +package com.gu.mediaservice.lib.guardian.auth + +import com.gu.mediaservice.lib.auth.Authentication +import com.gu.mediaservice.lib.auth.Authentication.{GridUser, Principal} +import com.gu.mediaservice.lib.auth.provider._ import com.gu.mediaservice.lib.aws.S3Ops import com.gu.pandomainauth.PanDomainAuthSettingsRefresher import com.gu.pandomainauth.action.AuthActions -import com.gu.pandomainauth.model.{AuthenticatedUser, Authenticated => PandaAuthenticated, Expired => PandaExpired, GracePeriod => PandaGracePeriod, InvalidCookie => PandaInvalidCookie, NotAuthenticated => PandaNotAuthenticated, NotAuthorized => PandaNotAuthorised} +import com.gu.pandomainauth.model.{AuthenticatedUser, User, Authenticated => PandaAuthenticated, Expired => PandaExpired, GracePeriod => PandaGracePeriod, InvalidCookie => PandaInvalidCookie, NotAuthenticated => PandaNotAuthenticated, NotAuthorized => PandaNotAuthorised} +import com.gu.pandomainauth.service.OAuthException +import play.api.libs.typedmap.{TypedEntry, TypedKey, TypedMap} import play.api.libs.ws.{DefaultWSCookie, WSClient, WSRequest} -import play.api.mvc.{ControllerComponents, RequestHeader, Result} +import play.api.mvc.{ControllerComponents, Cookie, RequestHeader, Result} import scala.concurrent.Future +import scala.util.Try class PandaAuthenticationProvider(resources: AuthenticationProviderResources) extends UserAuthenticationProvider with AuthActions { @@ -28,10 +34,10 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources) ex extractAuth(request) match { case PandaNotAuthenticated => NotAuthenticated case PandaInvalidCookie(e) => Invalid("error checking user's auth, clear cookie and re-auth", Some(e)) - case PandaExpired(authedUser) => Expired(PandaUser(authedUser.user)) - case PandaGracePeriod(authedUser) => GracePeriod(PandaUser(authedUser.user)) + case PandaExpired(authedUser) => Expired(gridUserFrom(authedUser.user, request)) + case PandaGracePeriod(authedUser) => GracePeriod(gridUserFrom(authedUser.user, request)) case PandaNotAuthorised(authedUser) => NotAuthorised(s"${authedUser.user.email} not authorised to use application") - case PandaAuthenticated(authedUser) => Authenticated(PandaUser(authedUser.user)) + case PandaAuthenticated(authedUser) => Authenticated(gridUserFrom(authedUser.user, request)) } } @@ -42,7 +48,7 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources) ex override def sendForAuthentication: Option[(RequestHeader, Option[Principal]) => Future[Result]] = Some( { (requestHeader: RequestHeader, principal: Option[Principal]) => val email = principal.collect{ - case PandaUser(user) => user.email + case GridUser(_, _, email, _) => email } sendForAuth(requestHeader, email) } @@ -55,6 +61,21 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources) ex * authentication failed then this should return an appropriate 4xx result. */ override def processAuthentication: Option[RequestHeader => Future[Result]] = Some( + // TODO: note from the previous implementation + // We use the `Try` here as the `GoogleAuthException` are thrown before we + // get to the asynchronicity of the `Future` it returns. + // We then have to flatten the Future[Future[T]]. Fiddly... +// Future.fromTry(Try(auth.processOAuthCallback())).flatten.recover { +// // This is when session session args are missing +// case e: OAuthException => respondError(BadRequest, "google-auth-exception", e.getMessage, auth.loginLinks) +// +// // Class `missing anti forgery token` as a 4XX +// // see https://github.com/guardian/pan-domain-authentication/blob/master/pan-domain-auth-play_2-6/src/main/scala/com/gu/pandomainauth/service/GoogleAuth.scala#L63 +// case e: IllegalArgumentException if e.getMessage == "The anti forgery token did not match" => { +// logger.error(e.getMessage) +// respondError(BadRequest, "google-auth-exception", e.getMessage, auth.loginLinks) +// } +// } processOAuthCallback()(_) ) @@ -66,22 +87,35 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources) ex */ override def flushToken: Option[RequestHeader => Result] = Some(processLogout(_)) + val PandaCookieKey: TypedKey[Cookie] = TypedKey[Cookie]("PandaCookie") + /** * A function that allows downstream API calls to be made using the credentials of the inflight request * * @param request The request header of the inflight call * @return A function that adds appropriate data to a WSRequest */ - override def onBehalfOf(request: RequestHeader): Either[String, WSRequest => WSRequest] = { + override def onBehalfOf(request: Principal): Either[String, WSRequest => WSRequest] = { val cookieName = panDomainSettings.settings.cookieSettings.cookieName - request.cookies.get(cookieName) match { + request.attributes.get(PandaCookieKey) match { case Some(cookie) => Right { wsRequest: WSRequest => wsRequest.addCookies(DefaultWSCookie(cookieName, cookie.value)) } - case None => Left(s"Pan domain cookie $cookieName is missing in original request.") + case None => Left(s"Pan domain cookie $cookieName is missing in principal.") } } + private def gridUserFrom(pandaUser: User, request: RequestHeader): GridUser = { + val maybePandaCookie: Option[TypedEntry[Cookie]] = request.cookies.get(panDomainSettings.settings.cookieSettings.cookieName).map(TypedEntry[Cookie](PandaCookieKey, _)) + val attributes = TypedMap.empty + (maybePandaCookie.toSeq:_*) + GridUser( + firstName = pandaUser.firstName, + lastName = pandaUser.lastName, + email = pandaUser.email, + attributes = attributes + ) + } + private def buildPandaSettings() = { new PanDomainAuthSettingsRefresher( domain = resources.commonConfig.services.domainRoot, diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala index f64897f1dc..ba51dc2e66 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala @@ -1,12 +1,12 @@ package com.gu.mediaservice.lib.play -import akka.actor.ActorSystem import com.gu.mediaservice.lib.auth.Authentication +import com.gu.mediaservice.lib.auth.provider.AuthenticationProviders import com.gu.mediaservice.lib.config.{CommonConfig, GridConfigResources} -import com.gu.mediaservice.lib.logging.{GridLogging, LogConfig} +import com.gu.mediaservice.lib.logging.LogConfig import com.gu.mediaservice.lib.management.{BuildInfo, Management} import play.api.ApplicationLoader.Context -import play.api.{BuiltInComponentsFromContext, Configuration} +import play.api.BuiltInComponentsFromContext import play.api.libs.ws.ahc.AhcWSComponents import play.api.mvc.EssentialFilter import play.filters.HttpFiltersComponents @@ -43,5 +43,6 @@ abstract class GridComponents[Config <: CommonConfig](context: Context, val load ) lazy val management = new Management(controllerComponents, buildInfo) - val auth = new Authentication(config, actorSystem, defaultBodyParser, wsClient, controllerComponents, executionContext) + val providers: AuthenticationProviders = ??? + val auth = new Authentication(config, providers, controllerComponents.parsers.default, executionContext) } diff --git a/cropper/app/controllers/CropperController.scala b/cropper/app/controllers/CropperController.scala index 20ec7b0973..6303d78664 100644 --- a/cropper/app/controllers/CropperController.scala +++ b/cropper/app/controllers/CropperController.scala @@ -1,13 +1,11 @@ package controllers -import java.net.URI - import _root_.play.api.Logger import _root_.play.api.libs.json._ import _root_.play.api.mvc.{BaseController, ControllerComponents} import com.gu.mediaservice.lib.argo.ArgoHelpers import com.gu.mediaservice.lib.argo.model.Link -import com.gu.mediaservice.lib.auth.Authentication.{OnBehalfOfApiKey, OnBehalfOfUser, Principal} +import com.gu.mediaservice.lib.auth.Authentication.Principal import com.gu.mediaservice.lib.auth._ import com.gu.mediaservice.lib.aws.UpdateMessage import com.gu.mediaservice.lib.imaging.ExportResult @@ -16,8 +14,9 @@ import com.gu.mediaservice.model._ import lib._ import model._ import org.joda.time.DateTime -import play.api.libs.ws.{WSClient, WSCookie} +import play.api.libs.ws.WSClient +import java.net.URI import scala.concurrent.{ExecutionContext, Future} import scala.util.control.NonFatal @@ -48,7 +47,7 @@ class CropperController(auth: Authentication, crops: Crops, store: CropStore, no def export = auth.async(parse.json) { httpRequest => httpRequest.body.validate[ExportRequest] map { exportRequest => val user = httpRequest.user - val onBehalfOfPrincipal = auth.getOnBehalfOfPrincipal(user, httpRequest) + val onBehalfOfPrincipal = auth.getOnBehalfOfPrincipal(user) executeRequest(exportRequest, user, onBehalfOfPrincipal).map { case (imageId, export) => val cropJson = Json.toJson(export).as[JsObject] @@ -151,13 +150,7 @@ class CropperController(auth: Authentication, crops: Crops, store: CropStore, no .withQueryStringParameters("include" -> "fileMetadata") .withHttpHeaders(Authentication.originalServiceHeaderName -> "cropper") - val request = onBehalfOfPrincipal match { - case OnBehalfOfApiKey(service) => - baseRequest.addHttpHeaders(Authentication.apiKeyHeaderName -> service.accessor.identity) - - case OnBehalfOfUser(_, cookie) => - baseRequest.addCookies(cookie) - } + val request = onBehalfOfPrincipal(baseRequest) val responseFuture = request.get.map { r => HttpClientResponse(r.status, r.statusText, Json.parse(r.body)) diff --git a/media-api/app/controllers/MediaApi.scala b/media-api/app/controllers/MediaApi.scala index 626a56099b..c0e4de4b6c 100644 --- a/media-api/app/controllers/MediaApi.scala +++ b/media-api/app/controllers/MediaApi.scala @@ -93,8 +93,8 @@ class MediaApi( permission: PermissionDefinition ) = { request.user match { - case user: PandaUser => - if (user.user.email.toLowerCase == image.uploadedBy.toLowerCase) { + case user: GridUser => + if (user.email.toLowerCase == image.uploadedBy.toLowerCase) { true } else { hasPermission(user, permission) @@ -217,7 +217,7 @@ class MediaApi( val entity = HttpEntity.Streamed(file, image.source.size, image.source.mimeType.map(_.name)) if(config.recordDownloadAsUsage) { - postToUsages(config.usageUri + "/usages/download", auth.getOnBehalfOfPrincipal(request.user, request), id, Authentication.getIdentity(request.user)) + postToUsages(config.usageUri + "/usages/download", auth.getOnBehalfOfPrincipal(request.user), id, Authentication.getIdentity(request.user)) } Future.successful( @@ -244,7 +244,7 @@ class MediaApi( })) if(config.recordDownloadAsUsage) { - postToUsages(config.usageUri + "/usages/download", auth.getOnBehalfOfPrincipal(request.user, request), id, Authentication.getIdentity(request.user)) + postToUsages(config.usageUri + "/usages/download", auth.getOnBehalfOfPrincipal(request.user), id, Authentication.getIdentity(request.user)) } Future.successful( @@ -261,13 +261,7 @@ class MediaApi( HttpHeaders.ORIGIN -> config.rootUri, HttpHeaders.CONTENT_TYPE -> ContentType.APPLICATION_JSON.getMimeType) - val request = onBehalfOfPrincipal match { - case OnBehalfOfApiKey(service) => - print(service.accessor.identity) - baseRequest.addHttpHeaders(Authentication.apiKeyHeaderName -> service.accessor.identity) - case OnBehalfOfUser(_, cookie) => - baseRequest.addCookies(cookie) - } + val request = onBehalfOfPrincipal(baseRequest) val usagesMetadata = Map("mediaId" -> mediaId, "dateAdded" -> printDateTime(DateTime.now()), From d09031be96d60243c20171cf4ba21ab8bd7e5e29 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Fri, 8 Jan 2021 16:51:51 +0000 Subject: [PATCH 03/25] Complete the processAuth callback --- .../lib/auth/Authentication.scala | 1 + .../auth/PandaAuthenticationProvider.scala | 41 +++++++++++-------- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala index 0ab2b22f1e..755a7f57f6 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala @@ -23,6 +23,7 @@ class Authentication(config: CommonConfig, // make the execution context implicit so it will be picked up appropriately implicit val ec: ExecutionContext = executionContext + // TODO: SAH / Evaluate MB's suggestion that this moves to the provider val loginLinks = List( Link("login", config.services.loginUriTemplate) ) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala index 36814790be..7bf413a60b 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala @@ -1,5 +1,8 @@ package com.gu.mediaservice.lib.guardian.auth +import com.gu.mediaservice.lib.argo.ArgoHelpers +import com.gu.mediaservice.lib.argo.model.Link +import com.gu.mediaservice.lib.auth.ApiAccessor.respondError import com.gu.mediaservice.lib.auth.Authentication import com.gu.mediaservice.lib.auth.Authentication.{GridUser, Principal} import com.gu.mediaservice.lib.auth.provider._ @@ -8,20 +11,26 @@ import com.gu.pandomainauth.PanDomainAuthSettingsRefresher import com.gu.pandomainauth.action.AuthActions import com.gu.pandomainauth.model.{AuthenticatedUser, User, Authenticated => PandaAuthenticated, Expired => PandaExpired, GracePeriod => PandaGracePeriod, InvalidCookie => PandaInvalidCookie, NotAuthenticated => PandaNotAuthenticated, NotAuthorized => PandaNotAuthorised} import com.gu.pandomainauth.service.OAuthException +import com.typesafe.scalalogging.StrictLogging import play.api.libs.typedmap.{TypedEntry, TypedKey, TypedMap} import play.api.libs.ws.{DefaultWSCookie, WSClient, WSRequest} -import play.api.mvc.{ControllerComponents, Cookie, RequestHeader, Result} +import play.api.mvc.{ControllerComponents, Cookie, RequestHeader, Result, Results} import scala.concurrent.Future import scala.util.Try -class PandaAuthenticationProvider(resources: AuthenticationProviderResources) extends UserAuthenticationProvider with AuthActions { +class PandaAuthenticationProvider(resources: AuthenticationProviderResources) + extends UserAuthenticationProvider with AuthActions with StrictLogging with ArgoHelpers { final override def authCallbackUrl: String = s"${resources.commonConfig.services.authBaseUri}/oauthCallback" override lazy val panDomainSettings: PanDomainAuthSettingsRefresher = buildPandaSettings() override def wsClient: WSClient = resources.wsClient override def controllerComponents: ControllerComponents = resources.controllerComponents + val loginLinks = List( + Link("login", resources.commonConfig.services.loginUriTemplate) + ) + /** * Establish the authentication status of the given request header. This can return an authenticated user or a number * of reasons why a user is not authenticated. @@ -60,24 +69,22 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources) ex * used to set a cookie or similar to ensure that a subsequent call to authenticateRequest will succeed. If * authentication failed then this should return an appropriate 4xx result. */ - override def processAuthentication: Option[RequestHeader => Future[Result]] = Some( - // TODO: note from the previous implementation + override def processAuthentication: Option[RequestHeader => Future[Result]] = Some({ requestHeader: RequestHeader => // We use the `Try` here as the `GoogleAuthException` are thrown before we // get to the asynchronicity of the `Future` it returns. // We then have to flatten the Future[Future[T]]. Fiddly... -// Future.fromTry(Try(auth.processOAuthCallback())).flatten.recover { -// // This is when session session args are missing -// case e: OAuthException => respondError(BadRequest, "google-auth-exception", e.getMessage, auth.loginLinks) -// -// // Class `missing anti forgery token` as a 4XX -// // see https://github.com/guardian/pan-domain-authentication/blob/master/pan-domain-auth-play_2-6/src/main/scala/com/gu/pandomainauth/service/GoogleAuth.scala#L63 -// case e: IllegalArgumentException if e.getMessage == "The anti forgery token did not match" => { -// logger.error(e.getMessage) -// respondError(BadRequest, "google-auth-exception", e.getMessage, auth.loginLinks) -// } -// } - processOAuthCallback()(_) - ) + Future.fromTry(Try(processOAuthCallback()(requestHeader))).flatten.recover { + // This is when session session args are missing + case e: OAuthException => respondError(BadRequest, "google-auth-exception", e.getMessage, loginLinks) + + // Class `missing anti forgery token` as a 4XX + // see https://github.com/guardian/pan-domain-authentication/blob/master/pan-domain-auth-play_2-6/src/main/scala/com/gu/pandomainauth/service/GoogleAuth.scala#L63 + case e: IllegalArgumentException if e.getMessage == "The anti forgery token did not match" => { + logger.error("Anti-forgery exception encountered", e) + respondError(BadRequest, "google-auth-exception", e.getMessage, loginLinks) + } + }(controllerComponents.executionContext) + }) /** * If this provider is able to clear user tokens (i.e. by clearing cookies) then it should provide a function to From 230cad953546f27facfd5db537cdb9ac58ab323f Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Mon, 11 Jan 2021 14:34:30 +0000 Subject: [PATCH 04/25] Make the image processing SPI loader generic --- .../src/main/resources/application.conf | 16 ++ .../ApiKeyAuthenticationProvider.scala | 8 +- .../provider/AuthenticationProvider.scala | 10 +- .../provider/AuthenticationProviders.scala | 2 +- .../config/AuthenticationProviderLoader.scala | 6 + .../lib/config/ImageProcessorLoader.scala | 117 +--------- .../lib/config/ProviderLoader.scala | 183 +++++++++++++++ .../lib/play/GridComponents.scala | 16 +- .../lib/config/ProviderLoaderTest.scala | 219 ++++++++++++++++++ .../scala/lib/ImageProcessorLoaderTest.scala | 203 ---------------- 10 files changed, 448 insertions(+), 332 deletions(-) create mode 100644 common-lib/src/main/scala/com/gu/mediaservice/lib/config/AuthenticationProviderLoader.scala create mode 100644 common-lib/src/main/scala/com/gu/mediaservice/lib/config/ProviderLoader.scala create mode 100644 common-lib/src/test/scala/com/gu/mediaservice/lib/config/ProviderLoaderTest.scala delete mode 100644 image-loader/test/scala/lib/ImageProcessorLoaderTest.scala diff --git a/common-lib/src/main/resources/application.conf b/common-lib/src/main/resources/application.conf index 8bedc81814..6ddb1a5087 100644 --- a/common-lib/src/main/resources/application.conf +++ b/common-lib/src/main/resources/application.conf @@ -23,3 +23,19 @@ image.processors = [ "com.gu.mediaservice.lib.cleanup.GuardianMetadataCleaners", "com.gu.mediaservice.lib.cleanup.SupplierProcessors$" ] + +authentication.providers { + api { + className = "com.gu.mediaservice.lib.auth.provider.ApiKeyAuthenticationProvider" + config { + authKeyStoreBucket = ${authKeyBucketStore} + } + } + # TODO: short term we put panda here for backwards compatibility but the default provider should be something better + user { + className = "com.gu.mediaservice.lib.guardian.auth.PandaAuthenticationProvider" + config { + # all of the things relating to pan domain auth + } + } +} diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala index 1eb21791a3..7c5d84ada1 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala @@ -2,22 +2,26 @@ package com.gu.mediaservice.lib.auth.provider import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, Principal} import com.gu.mediaservice.lib.auth.{ApiAccessor, KeyStore} import com.typesafe.scalalogging.StrictLogging +import play.api.Configuration import play.api.libs.typedmap.{TypedEntry, TypedKey, TypedMap} import play.api.libs.ws.WSRequest import play.api.mvc.RequestHeader +import scala.concurrent.ExecutionContext + object ApiKeyAuthenticationProvider { val apiKeyHeaderName = "X-Gu-Media-Key" } -class ApiKeyAuthenticationProvider(resources: AuthenticationProviderResources) extends ApiAuthenticationProvider with StrictLogging { +class ApiKeyAuthenticationProvider(configuration: Configuration, resources: AuthenticationProviderResources) extends MachineAuthenticationProvider with StrictLogging { val ApiKeyHeader: TypedKey[(String, String)] = TypedKey[(String, String)]("ApiKeyHeader") + implicit val executionContext: ExecutionContext = resources.controllerComponents.executionContext var keyStorePlaceholder: Option[KeyStore] = _ // TODO: we should also shutdown the keystore but there isn't currently a hook override def initialise(): Unit = { - val store = new KeyStore(resources.config.get[String]("authKeyStoreBucket"), resources.commonConfig)(resources.context) + val store = new KeyStore(configuration.get[String]("authKeyStoreBucket"), resources.commonConfig) store.scheduleUpdates(resources.actorSystem.scheduler) keyStorePlaceholder = Some(store) } diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala index 8ef05c3dcb..e4afce89b8 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala @@ -3,24 +3,20 @@ package com.gu.mediaservice.lib.auth.provider import akka.actor.ActorSystem import com.gu.mediaservice.lib.auth.Authentication.Principal import com.gu.mediaservice.lib.config.CommonConfig -import play.api.Configuration import play.api.libs.ws.{WSClient, WSRequest} import play.api.mvc.{ControllerComponents, RequestHeader, Result} -import scala.concurrent.{ExecutionContext, Future} +import scala.concurrent.Future /** * Case class containing useful resources for authentication providers to allow concurrent processing and external * API calls to be conducted. - * @param config the tree of configuration for this provider * @param commonConfig the Grid common config object * @param context an execution context * @param actorSystem an actor system * @param wsClient a play WSClient for making API calls */ -case class AuthenticationProviderResources(config: Configuration, - commonConfig: CommonConfig, - context: ExecutionContext, +case class AuthenticationProviderResources(commonConfig: CommonConfig, actorSystem: ActorSystem, wsClient: WSClient, controllerComponents: ControllerComponents) @@ -69,7 +65,7 @@ trait UserAuthenticationProvider extends AuthenticationProvider { def flushToken: Option[RequestHeader => Result] } -trait ApiAuthenticationProvider extends AuthenticationProvider { +trait MachineAuthenticationProvider extends AuthenticationProvider { /** * Establish the authentication status of the given request header. This can return an authenticated user or a number * of reasons why a user is not authenticated. diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProviders.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProviders.scala index 72e79da8bb..1d219d8383 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProviders.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProviders.scala @@ -1,3 +1,3 @@ package com.gu.mediaservice.lib.auth.provider -case class AuthenticationProviders(userProvider: UserAuthenticationProvider, apiProvider: ApiAuthenticationProvider) +case class AuthenticationProviders(userProvider: UserAuthenticationProvider, apiProvider: MachineAuthenticationProvider) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/config/AuthenticationProviderLoader.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/config/AuthenticationProviderLoader.scala new file mode 100644 index 0000000000..5a264a6edf --- /dev/null +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/config/AuthenticationProviderLoader.scala @@ -0,0 +1,6 @@ +package com.gu.mediaservice.lib.config + +import com.gu.mediaservice.lib.auth.provider.{MachineAuthenticationProvider, AuthenticationProviderResources, UserAuthenticationProvider} + +object ApiAuthenticationProviderLoader extends ProviderLoader[MachineAuthenticationProvider, AuthenticationProviderResources]("api authentication provider") +object UserAuthenticationProviderLoader extends ProviderLoader[UserAuthenticationProvider, AuthenticationProviderResources]("user authentication provider") diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/config/ImageProcessorLoader.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/config/ImageProcessorLoader.scala index bd373ea1f1..d5792bfa0f 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/config/ImageProcessorLoader.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/config/ImageProcessorLoader.scala @@ -1,120 +1,5 @@ package com.gu.mediaservice.lib.config -import akka.actor.ActorSystem import com.gu.mediaservice.lib.cleanup.{ImageProcessor, ImageProcessorResources} -import com.typesafe.config.ConfigException.BadValue -import com.typesafe.config.{Config, ConfigObject, ConfigOrigin, ConfigValue, ConfigValueType} -import com.typesafe.scalalogging.StrictLogging -import play.api.{ConfigLoader, Configuration} -import scala.collection.JavaConverters._ -import scala.language.existentials -import scala.util.Try -import scala.util.control.NonFatal - -object ImageProcessorLoader extends StrictLogging { - case class ImageProcessorConfigDetails(className: String, - config: Option[Configuration], - commonConfiguration: CommonConfig, - actorSystem: ActorSystem, - origin: ConfigOrigin, - path: String) - - def imageProcessorsConfigLoader(commonConfiguration: CommonConfig, actorSystem: ActorSystem): ConfigLoader[Seq[ImageProcessor]] = (config: Config, path: String) => { - config - .getList(path) - .iterator() - .asScala.map { configValue => - parseConfigValue(configValue, path, commonConfiguration, actorSystem) - }.map(loadImageProcessor).toList - } - - def imageProcessorConfigLoader(commonConfiguration: CommonConfig, actorSystem: ActorSystem): ConfigLoader[ImageProcessor] = (config: Config, path: String) => { - val configDetails = parseConfigValue(config.getValue(path), path, commonConfiguration, actorSystem) - loadImageProcessor(configDetails) - } - - private def parseConfigValue(configValue: ConfigValue, path: String, commonConfiguration: CommonConfig, actorSystem: ActorSystem): ImageProcessorConfigDetails = { - configValue match { - case plainClass if plainClass.valueType == ConfigValueType.STRING => - ImageProcessorConfigDetails(plainClass.unwrapped.asInstanceOf[String], None, commonConfiguration, actorSystem, plainClass.origin, path) - case withConfig:ConfigObject if validConfigObject(withConfig) => - val config = withConfig.toConfig - val className = config.getString("className") - val processorConfig = config.getConfig("config") - ImageProcessorConfigDetails(className, Some(Configuration(processorConfig)), commonConfiguration, actorSystem, withConfig.origin, path) - case _ => - throw new BadValue(configValue.origin, path, s"An image processor can either a class name (string) or object with className (string) and config (object) fields. This ${configValue.valueType} is not valid.") - } - } - - private def validConfigObject(configObject: ConfigObject): Boolean = { - val config = configObject.toConfig - config.hasPath("className") && config.hasPath("config") - } - - private def loadImageProcessor(details: ImageProcessorConfigDetails): ImageProcessor = { - val config = new ImageProcessorResources { - override def processorConfiguration: Configuration = details.config.getOrElse(Configuration.empty) - override def commonConfiguration: CommonConfig = details.commonConfiguration - override def actorSystem: ActorSystem = details.actorSystem - } - ImageProcessorLoader - .loadImageProcessor(details.className, config) - .getOrElse { error => - val configError = s"Unable to instantiate image processor from config: $error" - logger.error(configError) - throw new BadValue(details.origin, details.path, configError) - } - } - - def loadImageProcessor(className: String, config: ImageProcessorResources): Either[String, ImageProcessor] = { - for { - imageProcessorClass <- loadClass(className) - imageProcessorInstance <- instantiate(imageProcessorClass, config) - } yield imageProcessorInstance - } - - private def loadClass(className: String): Either[String, Class[_]] = catchNonFatal(Class.forName(className)) { - case _: ClassNotFoundException => s"Unable to find image processor class $className" - case other => - logger.error(s"Error whilst loading $className", other) - s"Unknown error whilst loading $className, check logs" - } - - private def instantiate(imageProcessorClass: Class[_], resources: ImageProcessorResources): Either[String, ImageProcessor] = { - // Fail fast if processor config is provided but the specified image processor doesn't take any resources - def assertNoConfiguration[T](ok: Right[String, T]): Either[String, T] = { - if (resources.processorConfiguration.keys.nonEmpty) { - Left(s"Attempt to initialise image processor ${imageProcessorClass.getCanonicalName} failed as configuration is provided but the constructor does not take configuration as an argument.") - } else { - ok - } - } - - val maybeCompanionObject = Try(imageProcessorClass.getField("MODULE$").get(imageProcessorClass)).toOption - val maybeNoArgCtor = Try(imageProcessorClass.getDeclaredConstructor()).toOption - val maybeConfigCtor = Try(imageProcessorClass.getDeclaredConstructor(classOf[ImageProcessorResources])).toOption - for { - instance <- (maybeCompanionObject, maybeNoArgCtor, maybeConfigCtor) match { - case (Some(companionObject), _, _) => assertNoConfiguration(Right(companionObject)) - case (_, _, Some(configCtor)) => Right(configCtor.newInstance(resources)) - case (_, Some(noArgCtor), None) => assertNoConfiguration(Right(noArgCtor.newInstance())) - case (None, None, None) => Left(s"Unable to find a suitable constructor for ${imageProcessorClass.getCanonicalName}. Must either have a no arg constructor or a constructor taking one argument of type ImageProcessorConfig.") - } - castInstance <- try { - Right(instance.asInstanceOf[ImageProcessor]) - } catch { - case _: ClassCastException => Left(s"Failed to cast ${imageProcessorClass.getCanonicalName} to an ImageProcessor") - } - } yield castInstance - } - - private def catchNonFatal[T](block: => T)(error: Throwable => String): Either[String, T] = { - try { - Right(block) - } catch { - case NonFatal(e) => Left(error(e)) - } - } -} +object ImageProcessorLoader extends ProviderLoader[ImageProcessor, ImageProcessorResources]("image processor") diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/config/ProviderLoader.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/config/ProviderLoader.scala new file mode 100644 index 0000000000..803b7baa2f --- /dev/null +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/config/ProviderLoader.scala @@ -0,0 +1,183 @@ +package com.gu.mediaservice.lib.config + +import com.typesafe.config.ConfigException.BadValue +import com.typesafe.config._ +import com.typesafe.scalalogging.StrictLogging +import play.api.{ConfigLoader, Configuration} + +import java.lang.reflect.Constructor +import scala.collection.JavaConverters.asScalaIteratorConverter +import scala.reflect.ClassTag +import scala.util.Try +import scala.util.control.NonFatal + +case class ProviderResources[Resources](configuration: Configuration, resources: Resources) + +class ProviderLoader[ProviderType, ResourcesType](providerDescription: String)(implicit providerTag: ClassTag[ProviderType], resourcesTag: ClassTag[ResourcesType]) extends StrictLogging { + + private case class ConfigDetails(className: String, + config: Option[Configuration], + resources: ResourcesType, + origin: ConfigOrigin, + path: String) + + def seqConfigLoader(resources: ResourcesType): ConfigLoader[Seq[ProviderType]] = (config: Config, path: String) => { + config + .getList(path) + .iterator() + .asScala.map { configValue => + parseConfigValue(configValue, path, resources) + }.map(loadProvider).toList + } + + def singletonConfigLoader(resources: ResourcesType): ConfigLoader[ProviderType] = (config: Config, path: String) => { + val configDetails = parseConfigValue(config.getValue(path), path, resources) + loadProvider(configDetails) + } + + private def parseConfigValue(configValue: ConfigValue, path: String, resources: ResourcesType): ConfigDetails = { + configValue match { + case plainClass if plainClass.valueType == ConfigValueType.STRING => + ConfigDetails(plainClass.unwrapped.asInstanceOf[String], None, resources, plainClass.origin, path) + case withConfig:ConfigObject if validConfigObject(withConfig) => + val config = withConfig.toConfig + val className = config.getString("className") + val processorConfig = config.getConfig("config") + ConfigDetails(className, Some(Configuration(processorConfig)), resources, withConfig.origin, path) + case _ => + throw new BadValue(configValue.origin, path, s"A ${providerDescription} can either be a class name (string) or object with className (string) and config (object) fields. This ${configValue.valueType} is not valid.") + } + } + + private def validConfigObject(configObject: ConfigObject): Boolean = { + val config = configObject.toConfig + config.hasPath("className") && config.hasPath("config") + } + + private def loadProvider(details: ConfigDetails): ProviderType = { + val config = ProviderResources(details.config.getOrElse(Configuration.empty), details.resources) + loadProvider(details.className, config) match { + case Right(provider) => provider + case Left(error) => + val configError = s"Unable to instantiate ${providerDescription} from config: $error" + logger.error(configError) + throw new BadValue(details.origin, details.path, configError) + } + } + + def loadProvider(className: String, config: ProviderResources[ResourcesType]): Either[String, ProviderType] = { + for { + imageProcessorClass <- loadClass(className) + imageProcessorInstance <- instantiate(imageProcessorClass, config) + } yield imageProcessorInstance + } + + private def loadClass(className: String): Either[String, Class[_]] = catchNonFatal(Class.forName(className)) { + case _: ClassNotFoundException => s"Unable to find ${providerDescription} class $className" + case other => + logger.error(s"Error whilst loading $className", other) + s"Unknown error whilst loading $className, check logs" + } + + trait ProviderClassType + case class ProviderCompanionObject(companionObject: AnyRef) extends ProviderClassType + case class ProviderConstructor(ctor: Constructor[_]) extends ProviderClassType + + private def instantiate(clazz: Class[_], resources: ProviderResources[ResourcesType]): Either[String, ProviderType] = { + for { + providerClassType <- discoverProviderClassType(clazz, resources.configuration.keys.nonEmpty) + instance <- getProviderInstance(providerClassType, resources) + castInstance <- castProvider(instance) + } yield castInstance + } + + private def discoverProviderClassType(clazz: Class[_], configProvided: Boolean): Either[String, ProviderClassType] = { + Try(clazz.getField("MODULE$").get(clazz)).toOption match { + case Some(companionObject) if configProvided => Left(s"Configuration provided but ${clazz.getCanonicalName} is a companion object and doesn't take configuration.") + case Some(companionObject) => Right(ProviderCompanionObject(companionObject)) + case None => + for { + ctor <- findConstructor(clazz, configProvided) + } yield ProviderConstructor(ctor) + } + } + + private def findConstructor(clazz: Class[_], configurationProvided: Boolean): Either[String, Constructor[_]] = { + /* if config is provided but the constructor doesn't take config then it violates our contract */ + def configViolation(ctor: Constructor[_]): Boolean = { + val paramTypes = ctor.getParameterTypes.toList + val hasConfigParam = paramTypes.contains(classOf[Configuration]) + configurationProvided && !hasConfigParam + } + + // get all constructors + val allConstructors = clazz.getConstructors.toList + // get a list of constructors that we know how to use (this should be size one) + val validConstructors: List[Constructor[_]] = allConstructors.filter(validProviderConstructor) + + validConstructors match { + case configViolationConstructor :: Nil if configViolation(configViolationConstructor) => + Left(s"Configuration provided but constructor of ${clazz.getCanonicalName} with args ${constructorParamsString(configViolationConstructor)} doesn't take it.") + case singleValidConstructor :: Nil => + Right(singleValidConstructor) + case otherCombinations => + Left(s"""A provider must have one and only one valid constructors taking arguments of type + |${resourcesTag.runtimeClass.getCanonicalName} or ${classOf[Configuration].getCanonicalName}. + |${clazz.getCanonicalName} has ${otherCombinations.length} constructors: + |${otherCombinations.map(constructorParamsString)}""".stripMargin) + } + } + + private def validProviderConstructor: Constructor[_] => Boolean = { constructor => + val paramTypes = constructor.getParameterTypes.toList + // if the same type appears twice then we don't know what to do + val noDuplicates = paramTypes.length == paramTypes.toSet.size + // only pick constructors that take types of resources or config + val onlyKnownTypes = paramTypes.forall { paramType => + paramType == resourcesTag.runtimeClass || paramType == classOf[Configuration] + } + noDuplicates && onlyKnownTypes + } + + private def getProviderInstance(providerType: ProviderClassType, resources: ProviderResources[ResourcesType]): Either[String, ProviderType] = { + for { + instance <- providerType match { + case ProviderCompanionObject(companionObject) => Right(companionObject) + case ProviderConstructor(ctor) => Right(ctor.newInstance(paramsFor(ctor, resources):_*)) + } + castInstance <- castProvider(instance) + } yield castInstance + } + + private def paramsFor(ctor: Constructor[_], resources: ProviderResources[ResourcesType]): Array[Object] = { + val array = new Array[Object](ctor.getParameterCount) + + ctor.getParameters.zipWithIndex.foreach { case (param, index) => + if (param.getType == classOf[Configuration]) { + array(index) = resources.configuration.asInstanceOf[Object] + } else if (param.getType == resourcesTag.runtimeClass) { + array(index) = resources.resources.asInstanceOf[Object] + } + } + + array + } + + private def castProvider(instance: Any): Either[String, ProviderType] = { + if (providerTag.runtimeClass.isAssignableFrom(instance.getClass)) { + Right(instance.asInstanceOf[ProviderType]) + } else { + Left(s"Failed to cast ${instance.getClass.getCanonicalName} to a ${providerTag.runtimeClass.getCanonicalName}") + } + } + + private def constructorParamsString(ctor: Constructor[_]): String = ctor.getParameterTypes.map(_.getCanonicalName).mkString("(", ", ", ")") + + private def catchNonFatal[T](block: => T)(error: Throwable => String): Either[String, T] = { + try { + Right(block) + } catch { + case NonFatal(e) => Left(error(e)) + } + } +} diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala index ba51dc2e66..2a33472fca 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala @@ -1,8 +1,8 @@ package com.gu.mediaservice.lib.play import com.gu.mediaservice.lib.auth.Authentication -import com.gu.mediaservice.lib.auth.provider.AuthenticationProviders -import com.gu.mediaservice.lib.config.{CommonConfig, GridConfigResources} +import com.gu.mediaservice.lib.auth.provider.{MachineAuthenticationProvider, AuthenticationProviderResources, AuthenticationProviders, UserAuthenticationProvider} +import com.gu.mediaservice.lib.config.{ApiAuthenticationProviderLoader, CommonConfig, GridConfigResources, UserAuthenticationProviderLoader} import com.gu.mediaservice.lib.logging.LogConfig import com.gu.mediaservice.lib.management.{BuildInfo, Management} import play.api.ApplicationLoader.Context @@ -43,6 +43,16 @@ abstract class GridComponents[Config <: CommonConfig](context: Context, val load ) lazy val management = new Management(controllerComponents, buildInfo) - val providers: AuthenticationProviders = ??? + private val authProviderResources = AuthenticationProviderResources( + commonConfig = config, + actorSystem = actorSystem, + wsClient = wsClient, + controllerComponents = controllerComponents + ) + + val providers: AuthenticationProviders = AuthenticationProviders( + userProvider = config.configuration.get[UserAuthenticationProvider]("authentication.providers.user")(UserAuthenticationProviderLoader.singletonConfigLoader(authProviderResources)), + apiProvider = config.configuration.get[MachineAuthenticationProvider]("authentication.providers.api")(ApiAuthenticationProviderLoader.singletonConfigLoader(authProviderResources)) + ) val auth = new Authentication(config, providers, controllerComponents.parsers.default, executionContext) } diff --git a/common-lib/src/test/scala/com/gu/mediaservice/lib/config/ProviderLoaderTest.scala b/common-lib/src/test/scala/com/gu/mediaservice/lib/config/ProviderLoaderTest.scala new file mode 100644 index 0000000000..53796b6f35 --- /dev/null +++ b/common-lib/src/test/scala/com/gu/mediaservice/lib/config/ProviderLoaderTest.scala @@ -0,0 +1,219 @@ +package com.gu.mediaservice.lib.config + +import com.typesafe.config.ConfigException.BadValue +import com.typesafe.config.ConfigFactory +import org.scalatest.Inside.inside +import org.scalatest.{EitherValues, FreeSpec, Matchers} +import play.api.Configuration + +trait TestProvider { + def info: String +} + +case class TestProviderResources(aResource: String) + +object ObjectTestProvider extends TestProvider { + override def info: String = "object-test-provider" +} + +class NoArgTestProvider extends TestProvider { + override def info: String = "no-arg-test-provider" +} + +case class ResourceTestProvider(resources: TestProviderResources) extends TestProvider { + override def info: String = s"resource-test-provider ${resources.aResource}" +} + +case class ConfigTestProvider(config: Configuration) extends TestProvider { + override def info: String = s"config-test-provider ${config.hashCode}" +} + +case class ConfigResourceTestProvider(config: Configuration, resources: TestProviderResources) extends TestProvider { + override def info: String = s"config-resource-test-provider ${config.hashCode} ${resources.aResource}" +} + +case class ResourceConfigTestProvider(resources: TestProviderResources, config: Configuration) extends TestProvider { + override def info: String = s"resource-config-test-provider ${resources.aResource} ${config.hashCode}" +} + +class NotATestProvider { + def monkey: String = "not-test-provider" +} + +class TestProviderWithStringConstructor(configString: String) extends TestProvider { + def info: String = "not-test-provider" +} + +object TestProviderLoader extends ProviderLoader[TestProvider, TestProviderResources]("test provider") + + +class ProviderLoaderTest extends FreeSpec with Matchers with EitherValues { + + "The class reflector" - { + val resources = TestProviderResources("sausages") + val emptyConfig = Configuration.empty + val providerResources = ProviderResources(emptyConfig, resources) + + "should successfully load a no arg TestProvider instance" in { + val instance = TestProviderLoader.loadProvider(classOf[NoArgTestProvider].getCanonicalName, providerResources) + instance.right.value.info shouldBe "no-arg-test-provider" + } + + "should successfully load a companion object TestProvider" in { + val instance = TestProviderLoader.loadProvider(ObjectTestProvider.getClass.getCanonicalName, providerResources) + instance.right.value.info shouldBe s"object-test-provider" + } + + "should successfully load a config arg TestProvider instance" in { + val instance = TestProviderLoader.loadProvider(classOf[ConfigTestProvider].getCanonicalName, providerResources) + instance.right.value.info shouldBe s"config-test-provider ${emptyConfig.hashCode}" + } + + "should successfully load a resource arg TestProvider instance" in { + val instance = TestProviderLoader.loadProvider(classOf[ResourceTestProvider].getCanonicalName, providerResources) + instance.right.value.info shouldBe s"resource-test-provider sausages" + } + + "should successfully load a config, resource arg TestProvider instance" in { + val instance = TestProviderLoader.loadProvider(classOf[ConfigResourceTestProvider].getCanonicalName, providerResources) + instance.right.value.info shouldBe s"config-resource-test-provider ${emptyConfig.hashCode} sausages" + } + + "should successfully load a resource, config arg TestProvider instance" in { + val instance = TestProviderLoader.loadProvider(classOf[ResourceConfigTestProvider].getCanonicalName, providerResources) + instance.right.value.info shouldBe s"resource-config-test-provider sausages ${emptyConfig.hashCode}" + } + + "should fail to load something that isn't an TestProvider" in { + val instance = TestProviderLoader.loadProvider(classOf[NotATestProvider].getCanonicalName, providerResources) + instance.left.value shouldBe "Failed to cast com.gu.mediaservice.lib.config.NotATestProvider to a com.gu.mediaservice.lib.config.TestProvider" + } + + "should fail to load something that doesn't have a suitable constructor" in { + val instance = TestProviderLoader.loadProvider(classOf[TestProviderWithStringConstructor].getCanonicalName, providerResources) + instance.left.value shouldBe """A provider must have one and only one valid constructors taking arguments of type + |com.gu.mediaservice.lib.config.TestProviderResources or play.api.Configuration. + |com.gu.mediaservice.lib.config.TestProviderWithStringConstructor has 0 constructors: + |List()""".stripMargin + } + + "should fail to load something that doesn't exist" in { + val instance = TestProviderLoader.loadProvider("com.gu.mediaservice.lib.config.TestProviderThatDoesntExist", providerResources) + instance.left.value shouldBe "Unable to find test provider class com.gu.mediaservice.lib.config.TestProviderThatDoesntExist" + } + + val nonEmptyConfig = Configuration.from(Map("key" -> "value")) + val nonEmptyConfigProviderResources = ProviderResources(nonEmptyConfig, resources) + + "should fail to load a no arg processor that doesn't take configuration with non-empty configuration" in { + val instance = TestProviderLoader.loadProvider(classOf[NoArgTestProvider].getCanonicalName, nonEmptyConfigProviderResources) + instance.left.value shouldBe "Configuration provided but constructor of com.gu.mediaservice.lib.config.NoArgTestProvider with args () doesn't take it." + } + + "should fail to load an object processor that doesn't take configuration with non-empty configuration" in { + val instance = TestProviderLoader.loadProvider(ObjectTestProvider.getClass.getCanonicalName, nonEmptyConfigProviderResources) + instance.left.value shouldBe "Configuration provided but com.gu.mediaservice.lib.config.ObjectTestProvider$ is a companion object and doesn't take configuration." + } + } + + "The config loader" - { + val resources = TestProviderResources("sausages") + + implicit val testProviderConfigLoader = TestProviderLoader.singletonConfigLoader(resources) + implicit val testProvidersConfigLoader = TestProviderLoader.seqConfigLoader(resources) + + "should load an image processor from a classname" in { + val conf:Configuration = Configuration.from(Map( + "some.path" -> List( + "com.gu.mediaservice.lib.config.NoArgTestProvider" + ) + )) + + val processors = conf.get[Seq[TestProvider]]("some.path") + processors.head shouldBe a[NoArgTestProvider] + } + + "should load an image processor which has configuration" in { + val conf:Configuration = Configuration.from(Map( + "some.path" -> List( + Map( + "className" -> "com.gu.mediaservice.lib.config.ConfigTestProvider", + "config" -> Map("parameter" -> "value") + ) + ) + )) + val processors = conf.get[Seq[TestProvider]]("some.path") + val processor = processors.head + inside(processor) { + case ConfigTestProvider(config) => config.get[String]("parameter") shouldBe "value" + } + } + + "should load multiple image processors of mixed config types" in { + val conf:Configuration = Configuration.from(Map( + "some.path" -> List( + "com.gu.mediaservice.lib.config.NoArgTestProvider", + Map( + "className" -> "com.gu.mediaservice.lib.config.ConfigTestProvider", + "config" -> Map("parameter" -> "value") + ) + ) + )) + val processors = conf.get[Seq[TestProvider]]("some.path") + processors.length shouldBe 2 + processors.toList should matchPattern { + case (_:NoArgTestProvider) :: ConfigTestProvider(_) :: Nil => + } + } + + "should load multiple image processors of mixed config types from HOCON" in { + val conf:Configuration = Configuration(ConfigFactory.parseString( + """ + |some.path: [ + | com.gu.mediaservice.lib.config.NoArgTestProvider, + | { + | className: com.gu.mediaservice.lib.config.ConfigTestProvider + | config: { + | parameter: value + | } + | } + |] + |""".stripMargin)) + val processors = conf.get[Seq[TestProvider]]("some.path") + processors.length shouldBe 2 + processors.toList should matchPattern { + case (_:NoArgTestProvider) :: ConfigTestProvider(_) :: Nil => + } + } + + "should fail to load multiple image processors if they don't meet the spec" in { + val conf:Configuration = Configuration.from(Map( + "some.path" -> List( + "com.gu.mediaservice.lib.config.NoArgTestProvider", + Map( + "noClassName" -> "com.gu.mediaservice.lib.config.ConfigTestProvider", + "config" -> Map("parameter" -> "value") + ) + ) + )) + val thrown = the[BadValue] thrownBy { + conf.get[Seq[TestProvider]]("some.path") + } + thrown.getMessage should include ("A test provider can either be a class name (string) or object with className (string) and config (object) fields. This OBJECT is not valid.") + } + + "should fail to load an image processors if the config isn't a string" in { + val conf:Configuration = Configuration.from(Map( + "some.path" -> List( + List("fred") + ) + )) + val thrown = the[BadValue] thrownBy { + conf.get[Seq[TestProvider]]("some.path") + } + thrown.getMessage should include ("A test provider can either be a class name (string) or object with className (string) and config (object) fields. This LIST is not valid") + } + + + } +} diff --git a/image-loader/test/scala/lib/ImageProcessorLoaderTest.scala b/image-loader/test/scala/lib/ImageProcessorLoaderTest.scala deleted file mode 100644 index 25b3e55b62..0000000000 --- a/image-loader/test/scala/lib/ImageProcessorLoaderTest.scala +++ /dev/null @@ -1,203 +0,0 @@ -package scala.lib - -import akka.actor.ActorSystem -import com.gu.mediaservice.lib.cleanup.{ImageProcessor, ImageProcessorResources} -import com.gu.mediaservice.lib.config.{CommonConfig, ImageProcessorLoader} -import com.gu.mediaservice.model.Image -import com.typesafe.config.ConfigException.BadValue -import com.typesafe.config.ConfigFactory -import org.joda.time.DateTime -import org.scalatest.Inside.inside -import org.scalatest.{EitherValues, FreeSpec, Matchers} -import play.api.Configuration - -object ObjectImageProcessor extends ImageProcessor { - override def apply(image: Image): Image = image.copy(id = "object-image-processed") -} - -class NoArgImageProcessor extends ImageProcessor { - override def apply(image: Image): Image = image.copy(id = "no-arg-image-processed") -} - -case class ConfigImageProcessor(resources: ImageProcessorResources) extends ImageProcessor { - override def apply(image: Image): Image = image.copy(id = s"config-image-processed ${resources.hashCode}") -} - -class NotAnImageProcessor { - def apply(image: Image): Image = image.copy(id = "not-image-processed") -} - -class ImageProcessorWithStringConstructor(configString: String) extends ImageProcessor { - def apply(image: Image): Image = image.copy(id = "not-image-processed") -} - -class ImageProcessorLoaderTest extends FreeSpec with Matchers with EitherValues { - val akkaActorSystem: ActorSystem = ActorSystem("test") - val testImage: Image = Image("image", DateTime.now(), "Test", None, Map.empty, null, null, null, null, null, null, null, null, null, null) - val testConfig: Configuration = Configuration.empty - val testNonEmptyConfig: Configuration = Configuration.from(Map("someConfig" -> "my value")) - - val commonConfiguration: Configuration = Configuration.from(Map( - "grid.stage" -> "DEV", - "grid.appName" -> "image-loader", - "auth.keystore.bucket" -> "not-used-in-test", - "thrall.kinesis.stream.name" -> "not-used-in-test", - "thrall.kinesis.lowPriorityStream.name" -> "not-used-in-test", - "domain.root" -> "not.used.in.test.com" - )) - val commonConfig: CommonConfig = new CommonConfig(commonConfiguration) { - - } - def resources(processorConfig: Configuration) = new ImageProcessorResources { - override def processorConfiguration: Configuration = processorConfig - override def commonConfiguration: CommonConfig = commonConfig - override def actorSystem: ActorSystem = akkaActorSystem - } - - "The class reflector" - { - "should successfully load a no arg ImageProcessor instance" in { - val instance = ImageProcessorLoader.loadImageProcessor(classOf[NoArgImageProcessor].getCanonicalName, resources(testConfig)) - instance.right.value.apply(testImage).id shouldBe "no-arg-image-processed" - } - - "should successfully load a config arg ImageProcessor instance" in { - val resources1 = resources(testConfig) - val instance = ImageProcessorLoader.loadImageProcessor(classOf[ConfigImageProcessor].getCanonicalName, resources1) - instance.right.value.apply(testImage).id shouldBe s"config-image-processed ${resources1.hashCode}" - } - - "should successfully load a companion object ImageProcessor" in { - val instance = ImageProcessorLoader.loadImageProcessor(ObjectImageProcessor.getClass.getCanonicalName, resources(testConfig)) - instance.right.value.apply(testImage).id shouldBe s"object-image-processed" - } - - "should fail to load something that isn't an ImageProcessor" in { - val instance = ImageProcessorLoader.loadImageProcessor(classOf[NotAnImageProcessor].getCanonicalName, resources(testConfig)) - instance.left.value shouldBe "Failed to cast scala.lib.NotAnImageProcessor to an ImageProcessor" - } - - "should fail to load something that doesn't have a suitable constructor" in { - val instance = ImageProcessorLoader.loadImageProcessor(classOf[ImageProcessorWithStringConstructor].getCanonicalName, resources(testConfig)) - instance.left.value shouldBe "Unable to find a suitable constructor for scala.lib.ImageProcessorWithStringConstructor. Must either have a no arg constructor or a constructor taking one argument of type ImageProcessorConfig." - } - - "should fail to load something that doesn't exist" in { - val instance = ImageProcessorLoader.loadImageProcessor("scala.lib.ImageProcessorThatDoesntExist", resources(testConfig)) - instance.left.value shouldBe "Unable to find image processor class scala.lib.ImageProcessorThatDoesntExist" - } - - "should fail to load a no arg processor that doesn't take configuration with non-empty configuration" in { - val instance = ImageProcessorLoader.loadImageProcessor(classOf[NoArgImageProcessor].getCanonicalName, resources(testNonEmptyConfig)) - instance.left.value shouldBe "Attempt to initialise image processor scala.lib.NoArgImageProcessor failed as configuration is provided but the constructor does not take configuration as an argument." - } - - "should fail to load an object processor that doesn't take configuration with non-empty configuration" in { - val instance = ImageProcessorLoader.loadImageProcessor(ObjectImageProcessor.getClass.getCanonicalName, resources(testNonEmptyConfig)) - instance.left.value shouldBe "Attempt to initialise image processor scala.lib.ObjectImageProcessor$ failed as configuration is provided but the constructor does not take configuration as an argument." - } - - } - - import ImageProcessorLoader._ - - "The config loader" - { - - - implicit val imageProcessorsConfigLoader = ImageProcessorLoader.imageProcessorsConfigLoader(commonConfig, akkaActorSystem) - implicit val imageProcessorConfigLoader = ImageProcessorLoader.imageProcessorConfigLoader(commonConfig, akkaActorSystem) - - "should load an image processor from a classname" in { - val conf:Configuration = Configuration.from(Map( - "some.path" -> List( - "scala.lib.NoArgImageProcessor" - ) - )) - - val processors = conf.get[Seq[ImageProcessor]]("some.path") - processors.head shouldBe a[NoArgImageProcessor] - } - - "should load an image processor which has configuration" in { - val conf:Configuration = Configuration.from(Map( - "some.path" -> List( - Map( - "className" -> "scala.lib.ConfigImageProcessor", - "config" -> Map("parameter" -> "value") - ) - ) - )) - val processors = conf.get[Seq[ImageProcessor]]("some.path") - val processor = processors.head - inside(processor) { - case ConfigImageProcessor(config) => config.processorConfiguration.get[String]("parameter") shouldBe "value" - } - } - - "should load multiple image processors of mixed config types" in { - val conf:Configuration = Configuration.from(Map( - "some.path" -> List( - "scala.lib.NoArgImageProcessor", - Map( - "className" -> "scala.lib.ConfigImageProcessor", - "config" -> Map("parameter" -> "value") - ) - ) - )) - val processors = conf.get[Seq[ImageProcessor]]("some.path") - processors.length shouldBe 2 - processors.toList should matchPattern { - case (_:NoArgImageProcessor) :: ConfigImageProcessor(_) :: Nil => - } - } - - "should load multiple image processors of mixed config types from HOCON" in { - val conf:Configuration = Configuration(ConfigFactory.parseString( - """ - |some.path: [ - | scala.lib.NoArgImageProcessor, - | { - | className: scala.lib.ConfigImageProcessor - | config: { - | parameter: value - | } - | } - |] - |""".stripMargin)) - val processors = conf.get[Seq[ImageProcessor]]("some.path") - processors.length shouldBe 2 - processors.toList should matchPattern { - case (_:NoArgImageProcessor) :: ConfigImageProcessor(_) :: Nil => - } - } - - "should fail to load multiple image processors if they don't meet the spec" in { - val conf:Configuration = Configuration.from(Map( - "some.path" -> List( - "scala.lib.NoArgImageProcessor", - Map( - "noClassName" -> "scala.lib.ConfigImageProcessor", - "config" -> Map("parameter" -> "value") - ) - ) - )) - val thrown = the[BadValue] thrownBy { - conf.get[Seq[ImageProcessor]]("some.path") - } - thrown.getMessage should include ("An image processor can either a class name (string) or object with className (string) and config (object) fields. This OBJECT is not valid.") - } - - "should fail to load an image processors if the config isn't a string" in { - val conf:Configuration = Configuration.from(Map( - "some.path" -> List( - List("fred") - ) - )) - val thrown = the[BadValue] thrownBy { - conf.get[Seq[ImageProcessor]]("some.path") - } - thrown.getMessage should include ("An image processor can either a class name (string) or object with className (string) and config (object) fields. This LIST is not valid") - } - - - } -} From 4679fd7846bb32547b24cbfe99389d609916517f Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Mon, 11 Jan 2021 15:03:24 +0000 Subject: [PATCH 05/25] Fixup the ImageLoaderConfig to use generic provider loader --- .../mediaservice/lib/cleanup/ImageProcessorResources.scala | 6 +----- image-loader/app/lib/ImageLoaderConfig.scala | 4 ++-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/cleanup/ImageProcessorResources.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/cleanup/ImageProcessorResources.scala index 1dc2d7aafe..1de5e60543 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/cleanup/ImageProcessorResources.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/cleanup/ImageProcessorResources.scala @@ -7,8 +7,4 @@ import play.api.Configuration /** * Resources that can be injected into a dynamically loaded ImageProcessor */ -trait ImageProcessorResources { - def processorConfiguration: Configuration - def commonConfiguration: CommonConfig - def actorSystem: ActorSystem -} +case class ImageProcessorResources(commonConfiguration: CommonConfig, actorSystem: ActorSystem) diff --git a/image-loader/app/lib/ImageLoaderConfig.scala b/image-loader/app/lib/ImageLoaderConfig.scala index e7fd5858e5..2fe7dd2a87 100644 --- a/image-loader/app/lib/ImageLoaderConfig.scala +++ b/image-loader/app/lib/ImageLoaderConfig.scala @@ -1,7 +1,7 @@ package lib import java.io.File -import com.gu.mediaservice.lib.cleanup.{ComposedImageProcessor, ImageProcessor} +import com.gu.mediaservice.lib.cleanup.{ComposedImageProcessor, ImageProcessor, ImageProcessorResources} import com.gu.mediaservice.lib.config.{CommonConfig, GridConfigResources, ImageProcessorLoader} import com.gu.mediaservice.model._ import com.typesafe.scalalogging.StrictLogging @@ -51,7 +51,7 @@ class ImageLoaderConfig(resources: GridConfigResources) extends CommonConfig(res * If a configuration is needed by is not provided by the config, the module configuration will be used instead. */ val imageProcessor: ComposedImageProcessor = { - val configLoader = ImageProcessorLoader.imageProcessorsConfigLoader(this, resources.actorSystem) + val configLoader = ImageProcessorLoader.seqConfigLoader(ImageProcessorResources(this, resources.actorSystem)) val processors = configuration .get[Seq[ImageProcessor]]("image.processors")(configLoader) ImageProcessor.compose("ImageConfigLoader-imageProcessor", processors:_*) From 3433af8b4441f5f17cc52f3fbd9a30bbada26a04 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Tue, 12 Jan 2021 12:28:33 +0000 Subject: [PATCH 06/25] Update the core stack if it exists --- dev/script/setup.sh | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/dev/script/setup.sh b/dev/script/setup.sh index 86185f4014..712d8d9782 100755 --- a/dev/script/setup.sh +++ b/dev/script/setup.sh @@ -208,12 +208,28 @@ getStackResource() { createCoreStack() { echo "creating local core cloudformation stack" + set -x + stackNames=$( + aws cloudformation list-stacks \ + --endpoint-url http://localhost:4566 | \ + jq -r '.StackSummaries[].StackName' + ) + if echo ${stackNames} | grep -q '^grid-dev-core$'; then + aws cloudformation update-stack \ + --stack-name "$CORE_STACK_NAME" \ + --template-body "file://$CORE_STACK_FILE" \ + --endpoint-url $LOCALSTACK_ENDPOINT > /dev/null + echo " updated stack $CORE_STACK_NAME using $CORE_STACK_FILENAME" + else + aws cloudformation create-stack \ + --stack-name "$CORE_STACK_NAME" \ + --template-body "file://$CORE_STACK_FILE" \ + --endpoint-url $LOCALSTACK_ENDPOINT > /dev/null + echo " created stack $CORE_STACK_NAME using $CORE_STACK_FILENAME" + fi + set +x - aws cloudformation create-stack \ - --stack-name "$CORE_STACK_NAME" \ - --template-body "file://$CORE_STACK_FILE" \ - --endpoint-url $LOCALSTACK_ENDPOINT > /dev/null - echo " created stack $CORE_STACK_NAME using $CORE_STACK_FILENAME" + # TODO - this should wait until the stack operation has completed } createLocalAuthStack() { @@ -279,7 +295,7 @@ main() { setupPermissionConfiguration setupPanDomainConfiguration fi - + setupPhotographersConfiguration setupUsageRightsConfiguration setupDevNginx From 0fafaef5b18e80d6b5a7cbce44a798a211d2349e Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Tue, 12 Jan 2021 12:29:59 +0000 Subject: [PATCH 07/25] Changes to get the pan domain auth provider working locally --- auth/app/auth/AuthController.scala | 35 ++++++--- .../src/main/resources/application.conf | 7 +- .../lib/auth/Authentication.scala | 15 ++-- .../provider/AuthenticationProvider.scala | 17 ++-- .../auth/provider/AuthenticationStatus.scala | 6 +- .../lib/config/CommonConfig.scala | 2 - .../auth/PandaAuthenticationProvider.scala | 78 +++++++++++-------- dev/script/generate-config/service-config.js | 2 +- 8 files changed, 100 insertions(+), 62 deletions(-) diff --git a/auth/app/auth/AuthController.scala b/auth/app/auth/AuthController.scala index 6bb8e5c05f..57c4df1b48 100644 --- a/auth/app/auth/AuthController.scala +++ b/auth/app/auth/AuthController.scala @@ -7,7 +7,7 @@ import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, GridUser} import com.gu.mediaservice.lib.auth.provider.AuthenticationProviders import com.gu.mediaservice.lib.auth.{Authentication, Permissions, PermissionsHandler} import play.api.libs.json.Json -import play.api.mvc.{BaseController, ControllerComponents} +import play.api.mvc.{BaseController, ControllerComponents, Result} import scala.concurrent.{ExecutionContext, Future} import scala.util.Try @@ -70,31 +70,44 @@ class AuthController(auth: Authentication, providers: AuthenticationProviders, v uri.getHost.endsWith(config.domainRoot) && uri.getScheme == "https" } def isValidDomain(inputUri: String): Boolean = { - Try(URI.create(inputUri)).filter(isOwnDomainAndSecure).isSuccess + val success = Try(URI.create(inputUri)).filter(isOwnDomainAndSecure).isSuccess + if (!success) logger.warn(s"Provided login redirect URI is invalid: $inputUri") + success } + // Play session key used to store the URI to redirect to during login + val REDIRECT_SESSION_KEY = "gridRedirectUri" + // Trigger the auth cycle // If a redirectUri is provided, redirect the browser there once auth'd, // else return a dummy page (e.g. for automatically re-auth'ing in the background) - // FIXME: validate redirectUri before doing the auth - def doLogin(redirectUri: Option[String] = None) = auth { req => - redirectUri map { - case uri if isValidDomain(uri) => Redirect(uri) - case _ => Ok("logged in (not redirecting to external redirectUri)") - } getOrElse Ok("logged in") + def doLogin(redirectUri: Option[String] = None) = Action.async { implicit req => + val checkedRedirectUri = redirectUri collect { + case uri if isValidDomain(uri) => uri + } + providers.userProvider.sendForAuthentication match { + case Some(authCallback) => + authCallback(req).map(_.addingToSession(checkedRedirectUri.map(REDIRECT_SESSION_KEY -> _).toSeq:_*)) + case None => + Future.successful(InternalServerError("Login not supported by configured authentication provider")) + } } def oauthCallback = Action.async { implicit request => providers.userProvider.processAuthentication match { - case Some(callback) => callback(request) - case None => Future.successful(InternalServerError("No callback for configured authentication provider")) + case Some(callback) => + val maybeRedirectUri = request.session.get(REDIRECT_SESSION_KEY) + callback(request, maybeRedirectUri).map(_.removingFromSession(REDIRECT_SESSION_KEY)) + case None => + Future.successful(InternalServerError("No callback for configured authentication provider")) } } def logout = Action { implicit request => - providers.userProvider.flushToken match { + val result: Result = providers.userProvider.flushToken match { case Some(callback) => callback(request) case None => InternalServerError("Logout not supported by configured authentication provider") } + result.withNewSession } } diff --git a/common-lib/src/main/resources/application.conf b/common-lib/src/main/resources/application.conf index 6ddb1a5087..169abd3fc9 100644 --- a/common-lib/src/main/resources/application.conf +++ b/common-lib/src/main/resources/application.conf @@ -28,14 +28,17 @@ authentication.providers { api { className = "com.gu.mediaservice.lib.auth.provider.ApiKeyAuthenticationProvider" config { - authKeyStoreBucket = ${authKeyBucketStore} + # authKeyStoreBucket = } } # TODO: short term we put panda here for backwards compatibility but the default provider should be something better user { className = "com.gu.mediaservice.lib.guardian.auth.PandaAuthenticationProvider" config { - # all of the things relating to pan domain auth + # all of the things relating to pan domain auth (these are currently sensibly defaulted in code) + # panda.system = media-service + # panda.bucketName = + # panda.settingsFileKey = } } } diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala index 755a7f57f6..bac4237175 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala @@ -38,23 +38,25 @@ class Authentication(config: CommonConfig, Future.successful(respondError(Forbidden, "principal-not-authorised", "Principal not authorised", loginLinks)) } - def authenticationStatus(requestHeader: RequestHeader, providers: AuthenticationProviders) = { - def sendForAuth(maybePrincipal: Option[Principal]): Future[Result] = { - providers.userProvider.sendForAuthentication.fold(unauthorised("No path to authenticate user"))(_(requestHeader, maybePrincipal)) - } + def expired(user: GridUser): Future[Result] = { + logger.info(s"User token expired for ${user.email}, return 419") + Future.successful(respondError(new Status(419), errorKey = "authentication-expired", errorMessage = "User authentication token has expired", loginLinks)) + } + def authenticationStatus(requestHeader: RequestHeader, providers: AuthenticationProviders) = { def flushToken(resultWhenAbsent: Result): Result = { providers.userProvider.flushToken.fold(resultWhenAbsent)(_(requestHeader)) } + // Authenticate request. Try with API authenticator first and then with user authenticator providers.apiProvider.authenticateRequest(requestHeader) match { case Authenticated(authedUser) => Right(authedUser) case Invalid(message, throwable) => Left(unauthorised(message, throwable)) case NotAuthorised(message) => Left(forbidden(s"Principal not authorised: $message")) case NotAuthenticated => providers.userProvider.authenticateRequest(requestHeader) match { - case NotAuthenticated => Left(sendForAuth(None)) - case Expired(principal) => Left(sendForAuth(Some(principal))) + case NotAuthenticated => Left(unauthorised("Not authenticated")) + case Expired(principal) => Left(expired(principal)) case GracePeriod(authedUser) => Right(authedUser) case Authenticated(authedUser) => Right(authedUser) case Invalid(message, throwable) => Left(unauthorised(message, throwable).map(flushToken)) @@ -64,7 +66,6 @@ class Authentication(config: CommonConfig, } override def invokeBlock[A](request: Request[A], block: Authentication.Request[A] => Future[Result]): Future[Result] = { - // Authenticate request. Try with API authenticator first and then with user authenticator authenticationStatus(request, providers) match { // we have a principal, so process the block case Right(principal) => block(new AuthenticatedRequest(principal, request)) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala index e4afce89b8..eb014f8769 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala @@ -2,6 +2,7 @@ package com.gu.mediaservice.lib.auth.provider import akka.actor.ActorSystem import com.gu.mediaservice.lib.auth.Authentication.Principal +import com.gu.mediaservice.lib.auth.provider.AuthenticationProvider.RedirectUri import com.gu.mediaservice.lib.config.CommonConfig import play.api.libs.ws.{WSClient, WSRequest} import play.api.mvc.{ControllerComponents, RequestHeader, Result} @@ -33,6 +34,10 @@ sealed trait AuthenticationProvider { def onBehalfOf(request: Principal): Either[String, WSRequest => WSRequest] } +object AuthenticationProvider { + type RedirectUri = String +} + trait UserAuthenticationProvider extends AuthenticationProvider { /** * Establish the authentication status of the given request header. This can return an authenticated user or a number @@ -45,18 +50,20 @@ trait UserAuthenticationProvider extends AuthenticationProvider { /** * If this provider supports sending a user that is not authorised to a federated auth provider then it should - * provide a function here to redirect the user. The function signature takes the applications callback URL as - * well as the request and should return a result. + * provide a function here to redirect the user. The function signature takes the the request and returns a result + * which is likely a redirect to an external authentication system. */ - def sendForAuthentication: Option[(RequestHeader, Option[Principal]) => Future[Result]] + def sendForAuthentication: Option[RequestHeader => Future[Result]] /** * If this provider supports sending a user that is not authorised to a federated auth provider then it should - * provide an Play action here that deals with the return of a user from a federated provider. This should be + * provide a function here that deals with the return of a user from a federated provider. This should be * used to set a cookie or similar to ensure that a subsequent call to authenticateRequest will succeed. If * authentication failed then this should return an appropriate 4xx result. + * The function should take the Play request header and the redirect URI that the user should be + * sent to on successful completion of the authentication. */ - def processAuthentication: Option[RequestHeader => Future[Result]] + def processAuthentication: Option[(RequestHeader, Option[RedirectUri]) => Future[Result]] /** * If this provider is able to clear user tokens (i.e. by clearing cookies) then it should provide a function to diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala index 38f477dc87..5b0427baae 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala @@ -1,15 +1,15 @@ package com.gu.mediaservice.lib.auth.provider -import com.gu.mediaservice.lib.auth.Authentication.Principal +import com.gu.mediaservice.lib.auth.Authentication.{GridUser, Principal} // statuses that directly extend this are for users only /** Status of a client's authentication */ sealed trait AuthenticationStatus /** User authentication is valid but expired */ -case class Expired(authedUser: Principal) extends AuthenticationStatus +case class Expired(authedUser: GridUser) extends AuthenticationStatus /** User authentication is valid and expired but the expiry is within a grace period */ -case class GracePeriod(authedUser: Principal) extends AuthenticationStatus +case class GracePeriod(authedUser: GridUser) extends AuthenticationStatus // statuses that extend this can be used by both users and machines /** Status of an API client's authentication */ diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/config/CommonConfig.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/config/CommonConfig.scala index ee585a0342..8b5a876e84 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/config/CommonConfig.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/config/CommonConfig.scala @@ -30,8 +30,6 @@ abstract class CommonConfig(val configuration: Configuration) extends AwsClientB override val awsLocalEndpoint: Option[String] = if(isDev) stringOpt("aws.local.endpoint") else None - val authKeyStoreBucket = string("auth.keystore.bucket") - val useLocalAuth: Boolean = isDev && boolean("auth.useLocal") val permissionsBucket: String = stringDefault("permissions.bucket", "permissions-cache") diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala index 7bf413a60b..3544666e04 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala @@ -2,9 +2,9 @@ package com.gu.mediaservice.lib.guardian.auth import com.gu.mediaservice.lib.argo.ArgoHelpers import com.gu.mediaservice.lib.argo.model.Link -import com.gu.mediaservice.lib.auth.ApiAccessor.respondError import com.gu.mediaservice.lib.auth.Authentication import com.gu.mediaservice.lib.auth.Authentication.{GridUser, Principal} +import com.gu.mediaservice.lib.auth.provider.AuthenticationProvider.RedirectUri import com.gu.mediaservice.lib.auth.provider._ import com.gu.mediaservice.lib.aws.S3Ops import com.gu.pandomainauth.PanDomainAuthSettingsRefresher @@ -12,15 +12,19 @@ import com.gu.pandomainauth.action.AuthActions import com.gu.pandomainauth.model.{AuthenticatedUser, User, Authenticated => PandaAuthenticated, Expired => PandaExpired, GracePeriod => PandaGracePeriod, InvalidCookie => PandaInvalidCookie, NotAuthenticated => PandaNotAuthenticated, NotAuthorized => PandaNotAuthorised} import com.gu.pandomainauth.service.OAuthException import com.typesafe.scalalogging.StrictLogging +import play.api.Configuration +import play.api.http.HeaderNames import play.api.libs.typedmap.{TypedEntry, TypedKey, TypedMap} import play.api.libs.ws.{DefaultWSCookie, WSClient, WSRequest} -import play.api.mvc.{ControllerComponents, Cookie, RequestHeader, Result, Results} +import play.api.mvc.{ControllerComponents, Cookie, RequestHeader, Result} -import scala.concurrent.Future +import scala.concurrent.{ExecutionContext, Future} import scala.util.Try -class PandaAuthenticationProvider(resources: AuthenticationProviderResources) - extends UserAuthenticationProvider with AuthActions with StrictLogging with ArgoHelpers { +class PandaAuthenticationProvider(resources: AuthenticationProviderResources, providerConfiguration: Configuration) + extends UserAuthenticationProvider with AuthActions with StrictLogging with ArgoHelpers with HeaderNames { + + implicit val ec: ExecutionContext = controllerComponents.executionContext final override def authCallbackUrl: String = s"${resources.commonConfig.services.authBaseUri}/oauthCallback" override lazy val panDomainSettings: PanDomainAuthSettingsRefresher = buildPandaSettings() @@ -40,7 +44,8 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources) * @return An authentication status expressing whether the */ override def authenticateRequest(request: RequestHeader): AuthenticationStatus = { - extractAuth(request) match { + val pandaStatus = extractAuth(request) + val providerStatus = pandaStatus match { case PandaNotAuthenticated => NotAuthenticated case PandaInvalidCookie(e) => Invalid("error checking user's auth, clear cookie and re-auth", Some(e)) case PandaExpired(authedUser) => Expired(gridUserFrom(authedUser.user, request)) @@ -48,20 +53,24 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources) case PandaNotAuthorised(authedUser) => NotAuthorised(s"${authedUser.user.email} not authorised to use application") case PandaAuthenticated(authedUser) => Authenticated(gridUserFrom(authedUser.user, request)) } + logger.info(s"Authenticating request ${request.uri}. Panda $pandaStatus Provider $providerStatus") + providerStatus } /** * If this provider supports sending a user that is not authorised to a federated auth provider then it should * provide a function here to redirect the user. */ - override def sendForAuthentication: Option[(RequestHeader, Option[Principal]) => Future[Result]] = Some( - { (requestHeader: RequestHeader, principal: Option[Principal]) => - val email = principal.collect{ - case GridUser(_, _, email, _) => email - } - sendForAuth(requestHeader, email) + override def sendForAuthentication: Option[RequestHeader => Future[Result]] = Some({ requestHeader: RequestHeader => + val maybePrincipal = authenticateRequest(requestHeader) match { + case Expired(principal) => Some(principal) + case GracePeriod(principal) => Some(principal) + case Authenticated(principal: GridUser) => Some(principal) + case _ => None } - ) + val email = maybePrincipal.map(_.email) + sendForAuth(requestHeader, email) + }) /** * If this provider supports sending a user that is not authorised to a federated auth provider then it should @@ -69,22 +78,29 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources) * used to set a cookie or similar to ensure that a subsequent call to authenticateRequest will succeed. If * authentication failed then this should return an appropriate 4xx result. */ - override def processAuthentication: Option[RequestHeader => Future[Result]] = Some({ requestHeader: RequestHeader => - // We use the `Try` here as the `GoogleAuthException` are thrown before we - // get to the asynchronicity of the `Future` it returns. - // We then have to flatten the Future[Future[T]]. Fiddly... - Future.fromTry(Try(processOAuthCallback()(requestHeader))).flatten.recover { - // This is when session session args are missing - case e: OAuthException => respondError(BadRequest, "google-auth-exception", e.getMessage, loginLinks) - - // Class `missing anti forgery token` as a 4XX - // see https://github.com/guardian/pan-domain-authentication/blob/master/pan-domain-auth-play_2-6/src/main/scala/com/gu/pandomainauth/service/GoogleAuth.scala#L63 - case e: IllegalArgumentException if e.getMessage == "The anti forgery token did not match" => { - logger.error("Anti-forgery exception encountered", e) - respondError(BadRequest, "google-auth-exception", e.getMessage, loginLinks) + override def processAuthentication: Option[(RequestHeader, Option[RedirectUri]) => Future[Result]] = + Some({ (requestHeader: RequestHeader, maybeUri: Option[RedirectUri]) => + // We use the `Try` here as the `GoogleAuthException` are thrown before we + // get to the asynchronicity of the `Future` it returns. + // We then have to flatten the Future[Future[T]]. Fiddly... + Future.fromTry(Try(processOAuthCallback()(requestHeader))).flatten.recover { + // This is when session session args are missing + case e: OAuthException => respondError(BadRequest, "google-auth-exception", e.getMessage, loginLinks) + + // Class `missing anti forgery token` as a 4XX + // see https://github.com/guardian/pan-domain-authentication/blob/master/pan-domain-auth-play_2-6/src/main/scala/com/gu/pandomainauth/service/GoogleAuth.scala#L63 + case e: IllegalArgumentException if e.getMessage == "The anti forgery token did not match" => { + logger.error("Anti-forgery exception encountered", e) + respondError(BadRequest, "google-auth-exception", e.getMessage, loginLinks) + } + }.map { + // not very elegant, but this will override the redirect from panda with any alternative destination + case overrideRedirect if overrideRedirect.header.headers.contains(LOCATION) && maybeUri.nonEmpty => + val uri = maybeUri.get + Redirect(uri).copy(newCookies = overrideRedirect.newCookies, newSession = overrideRedirect.newSession) + case other => other } - }(controllerComponents.executionContext) - }) + }) /** * If this provider is able to clear user tokens (i.e. by clearing cookies) then it should provide a function to @@ -126,9 +142,9 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources) private def buildPandaSettings() = { new PanDomainAuthSettingsRefresher( domain = resources.commonConfig.services.domainRoot, - system = resources.commonConfig.stringOpt("panda.system").getOrElse("media-service"), - bucketName = resources.commonConfig.stringOpt("panda.bucketName").getOrElse("pan-domain-auth-settings"), - settingsFileKey = resources.commonConfig.stringOpt("panda.settingsFileKey").getOrElse(s"${resources.commonConfig.services.domainRoot}.settings"), + system = providerConfiguration.getOptional[String]("panda.system").getOrElse("media-service"), + bucketName = providerConfiguration.getOptional[String]("panda.bucketName").getOrElse("pan-domain-auth-settings"), + settingsFileKey = providerConfiguration.getOptional[String]("panda.settingsFileKey").getOrElse(s"${resources.commonConfig.services.domainRoot}.settings"), s3Client = S3Ops.buildS3Client(resources.commonConfig, localstackAware=resources.commonConfig.useLocalAuth) ) } diff --git a/dev/script/generate-config/service-config.js b/dev/script/generate-config/service-config.js index 033709d181..9bbba7518b 100644 --- a/dev/script/generate-config/service-config.js +++ b/dev/script/generate-config/service-config.js @@ -11,7 +11,7 @@ function getCorsAllowedOriginString(config) { function getCommonConfig(config) { return `domain.root="${config.DOMAIN}" - |auth.keystore.bucket="${config.coreStackProps.KeyBucket}" + |authentication.providers.api.config.authKeyStoreBucket="${config.coreStackProps.KeyBucket}" |thrall.kinesis.stream.name="${config.coreStackProps.ThrallMessageStream}" |thrall.kinesis.lowPriorityStream.name="${config.coreStackProps.ThrallLowPriorityMessageStream}" |`; From 8e0203382155f8996772c77df97fae09b4e1234c Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Tue, 12 Jan 2021 15:29:00 +0000 Subject: [PATCH 08/25] Initialise and shutdown authentication providers --- .../scala/com/gu/mediaservice/lib/play/GridComponents.scala | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala index 2a33472fca..e6044af614 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala @@ -54,5 +54,10 @@ abstract class GridComponents[Config <: CommonConfig](context: Context, val load userProvider = config.configuration.get[UserAuthenticationProvider]("authentication.providers.user")(UserAuthenticationProviderLoader.singletonConfigLoader(authProviderResources)), apiProvider = config.configuration.get[MachineAuthenticationProvider]("authentication.providers.api")(ApiAuthenticationProviderLoader.singletonConfigLoader(authProviderResources)) ) + providers.userProvider.initialise() + applicationLifecycle.addStopHook(() => providers.userProvider.shutdown()) + providers.apiProvider.initialise() + applicationLifecycle.addStopHook(() => providers.apiProvider.shutdown()) + val auth = new Authentication(config, providers, controllerComponents.parsers.default, executionContext) } From 7a27a54f58261cc3bdcb22690aaefa0b1dcf22e6 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Tue, 12 Jan 2021 16:24:01 +0000 Subject: [PATCH 09/25] Move validateUser function into PandaAuthenticationProvider --- .../gu/mediaservice/lib/auth/Authentication.scala | 11 +---------- .../guardian/auth/PandaAuthenticationProvider.scala | 12 +++++++++--- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala index bac4237175..7cc98bfb26 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala @@ -3,10 +3,8 @@ package com.gu.mediaservice.lib.auth import com.gu.mediaservice.lib.argo.ArgoHelpers import com.gu.mediaservice.lib.argo.model.Link import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, GridUser, OnBehalfOfPrincipal, Principal} -import com.gu.mediaservice.lib.auth.provider.{Authenticated, AuthenticationProvider, AuthenticationProviders, Expired, GracePeriod, Invalid, NotAuthenticated, NotAuthorised} +import com.gu.mediaservice.lib.auth.provider._ import com.gu.mediaservice.lib.config.CommonConfig -import com.gu.pandomainauth.model.AuthenticatedUser -import com.gu.pandomainauth.service.Google2FAGroupChecker import play.api.libs.typedmap.TypedMap import play.api.libs.ws.WSRequest import play.api.mvc.Security.AuthenticatedRequest @@ -100,11 +98,4 @@ object Authentication { val originalServiceHeaderName = "X-Gu-Original-Service" def getIdentity(principal: Principal): String = principal.accessor.identity - - def validateUser(authedUser: AuthenticatedUser, userValidationEmailDomain: String, multifactorChecker: Option[Google2FAGroupChecker]): Boolean = { - val isValidDomain = authedUser.user.email.endsWith("@" + userValidationEmailDomain) - val passesMultifactor = if(multifactorChecker.nonEmpty) { authedUser.multiFactor } else { true } - - isValidDomain && passesMultifactor - } } diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala index 3544666e04..fa74f776f6 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala @@ -2,7 +2,6 @@ package com.gu.mediaservice.lib.guardian.auth import com.gu.mediaservice.lib.argo.ArgoHelpers import com.gu.mediaservice.lib.argo.model.Link -import com.gu.mediaservice.lib.auth.Authentication import com.gu.mediaservice.lib.auth.Authentication.{GridUser, Principal} import com.gu.mediaservice.lib.auth.provider.AuthenticationProvider.RedirectUri import com.gu.mediaservice.lib.auth.provider._ @@ -10,7 +9,7 @@ import com.gu.mediaservice.lib.aws.S3Ops import com.gu.pandomainauth.PanDomainAuthSettingsRefresher import com.gu.pandomainauth.action.AuthActions import com.gu.pandomainauth.model.{AuthenticatedUser, User, Authenticated => PandaAuthenticated, Expired => PandaExpired, GracePeriod => PandaGracePeriod, InvalidCookie => PandaInvalidCookie, NotAuthenticated => PandaNotAuthenticated, NotAuthorized => PandaNotAuthorised} -import com.gu.pandomainauth.service.OAuthException +import com.gu.pandomainauth.service.{Google2FAGroupChecker, OAuthException} import com.typesafe.scalalogging.StrictLogging import play.api.Configuration import play.api.http.HeaderNames @@ -152,7 +151,14 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources, pr private val userValidationEmailDomain = resources.commonConfig.stringOpt("panda.userDomain").getOrElse("guardian.co.uk") final override def validateUser(authedUser: AuthenticatedUser): Boolean = { - Authentication.validateUser(authedUser, userValidationEmailDomain, multifactorChecker) + validateUser(authedUser, userValidationEmailDomain, multifactorChecker) + } + + def validateUser(authedUser: AuthenticatedUser, userValidationEmailDomain: String, multifactorChecker: Option[Google2FAGroupChecker]): Boolean = { + val isValidDomain = authedUser.user.email.endsWith("@" + userValidationEmailDomain) + val passesMultifactor = if(multifactorChecker.nonEmpty) { authedUser.multiFactor } else { true } + + isValidDomain && passesMultifactor } } From 9f9bee9ef98a58edeecd1326e147e28783da6433 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Wed, 13 Jan 2021 12:53:16 +0000 Subject: [PATCH 10/25] Fix validateUser test --- .../lib/guardian/auth/PandaAuthenticationProvider.scala | 5 +++-- .../com/gu/mediaservice/lib/auth/AuthenticationTest.scala | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala index fa74f776f6..decd30473b 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala @@ -151,14 +151,15 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources, pr private val userValidationEmailDomain = resources.commonConfig.stringOpt("panda.userDomain").getOrElse("guardian.co.uk") final override def validateUser(authedUser: AuthenticatedUser): Boolean = { - validateUser(authedUser, userValidationEmailDomain, multifactorChecker) + PandaAuthenticationProvider.validateUser(authedUser, userValidationEmailDomain, multifactorChecker) } +} +object PandaAuthenticationProvider { def validateUser(authedUser: AuthenticatedUser, userValidationEmailDomain: String, multifactorChecker: Option[Google2FAGroupChecker]): Boolean = { val isValidDomain = authedUser.user.email.endsWith("@" + userValidationEmailDomain) val passesMultifactor = if(multifactorChecker.nonEmpty) { authedUser.multiFactor } else { true } isValidDomain && passesMultifactor } - } diff --git a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala index b6644969c1..197aa69836 100644 --- a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala +++ b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala @@ -6,7 +6,7 @@ import com.gu.pandomainauth.model.{AuthenticatedUser, User} import org.scalatest.{FunSuite, MustMatchers} class AuthenticationTest extends FunSuite with MustMatchers { - import Authentication.validateUser + import com.gu.mediaservice.lib.guardian.auth.PandaAuthenticationProvider.validateUser val user = AuthenticatedUser(User("Barry", "Chuckle", "barry.chuckle@guardian.co.uk", None), "media-service", Set("media-service"), Instant.now().plusSeconds(100).toEpochMilli, multiFactor = true) From b0922b90b162ab648678f27a9dc2b66d818526e6 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Wed, 13 Jan 2021 12:53:45 +0000 Subject: [PATCH 11/25] Catch exceptions thrown in provider constructors --- .../mediaservice/lib/config/ProviderLoader.scala | 15 +++++++++++++-- .../lib/config/ProviderLoaderTest.scala | 10 ++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/config/ProviderLoader.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/config/ProviderLoader.scala index 803b7baa2f..2820adf65b 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/config/ProviderLoader.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/config/ProviderLoader.scala @@ -5,7 +5,7 @@ import com.typesafe.config._ import com.typesafe.scalalogging.StrictLogging import play.api.{ConfigLoader, Configuration} -import java.lang.reflect.Constructor +import java.lang.reflect.{Constructor, InvocationTargetException} import scala.collection.JavaConverters.asScalaIteratorConverter import scala.reflect.ClassTag import scala.util.Try @@ -55,6 +55,7 @@ class ProviderLoader[ProviderType, ResourcesType](providerDescription: String)(i } private def loadProvider(details: ConfigDetails): ProviderType = { + logger.info(s"Dynamically loading provider from ${details.className} as specified by config path ${details.path}") val config = ProviderResources(details.config.getOrElse(Configuration.empty), details.resources) loadProvider(details.className, config) match { case Right(provider) => provider @@ -143,7 +144,17 @@ class ProviderLoader[ProviderType, ResourcesType](providerDescription: String)(i for { instance <- providerType match { case ProviderCompanionObject(companionObject) => Right(companionObject) - case ProviderConstructor(ctor) => Right(ctor.newInstance(paramsFor(ctor, resources):_*)) + case ProviderConstructor(ctor) => catchNonFatal(ctor.newInstance(paramsFor(ctor, resources):_*)){ + case ite: InvocationTargetException => + val cause = Option(ite.getCause) + val error = s"${cause.map(_.getClass.getName).getOrElse("Unknown exception")} thrown when executing constructor ${ctor.getClass.getCanonicalName}${constructorParamsString(ctor)}. Search logs for stack trace." + logger.error(error, cause.getOrElse(ite)) + error + case NonFatal(other) => + val error = s"${other.getClass.getName} thrown whilst creating a new instance using constructor ${ctor.getClass.getCanonicalName}${constructorParamsString(ctor)}. Search logs for stack trace." + logger.error(error, other) + error + } } castInstance <- castProvider(instance) } yield castInstance diff --git a/common-lib/src/test/scala/com/gu/mediaservice/lib/config/ProviderLoaderTest.scala b/common-lib/src/test/scala/com/gu/mediaservice/lib/config/ProviderLoaderTest.scala index 53796b6f35..226eb50435 100644 --- a/common-lib/src/test/scala/com/gu/mediaservice/lib/config/ProviderLoaderTest.scala +++ b/common-lib/src/test/scala/com/gu/mediaservice/lib/config/ProviderLoaderTest.scala @@ -36,6 +36,11 @@ case class ResourceConfigTestProvider(resources: TestProviderResources, config: override def info: String = s"resource-config-test-provider ${resources.aResource} ${config.hashCode}" } +case class BadTestProvider(resources: TestProviderResources, config: Configuration) extends TestProvider { + throw new IllegalArgumentException("Oh dear, something went wrong") + override def info: String = s"resource-config-test-provider ${resources.aResource} ${config.hashCode}" +} + class NotATestProvider { def monkey: String = "not-test-provider" } @@ -114,6 +119,11 @@ class ProviderLoaderTest extends FreeSpec with Matchers with EitherValues { val instance = TestProviderLoader.loadProvider(ObjectTestProvider.getClass.getCanonicalName, nonEmptyConfigProviderResources) instance.left.value shouldBe "Configuration provided but com.gu.mediaservice.lib.config.ObjectTestProvider$ is a companion object and doesn't take configuration." } + + "should fail to load a provider if the constructor throws an exception" in { + val instance = TestProviderLoader.loadProvider(classOf[BadTestProvider].getCanonicalName, providerResources) + instance.left.value shouldBe "java.lang.IllegalArgumentException thrown when executing constructor java.lang.reflect.Constructor(com.gu.mediaservice.lib.config.TestProviderResources, play.api.Configuration). Search logs for stack trace." + } } "The config loader" - { From 5c9387de8dcce45c84e04456d2e2dc6a01e66b31 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Wed, 13 Jan 2021 15:16:28 +0000 Subject: [PATCH 12/25] Rename test file --- .../mediaservice/lib/config/ProviderLoaderTest.scala | 8 ++++---- .../auth/PandaAuthenticationProviderTest.scala} | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) rename common-lib/src/test/scala/com/gu/mediaservice/lib/{auth/AuthenticationTest.scala => guardian/auth/PandaAuthenticationProviderTest.scala} (80%) diff --git a/common-lib/src/test/scala/com/gu/mediaservice/lib/config/ProviderLoaderTest.scala b/common-lib/src/test/scala/com/gu/mediaservice/lib/config/ProviderLoaderTest.scala index 226eb50435..2726848ee4 100644 --- a/common-lib/src/test/scala/com/gu/mediaservice/lib/config/ProviderLoaderTest.scala +++ b/common-lib/src/test/scala/com/gu/mediaservice/lib/config/ProviderLoaderTest.scala @@ -4,7 +4,7 @@ import com.typesafe.config.ConfigException.BadValue import com.typesafe.config.ConfigFactory import org.scalatest.Inside.inside import org.scalatest.{EitherValues, FreeSpec, Matchers} -import play.api.Configuration +import play.api.{ConfigLoader, Configuration} trait TestProvider { def info: String @@ -46,7 +46,7 @@ class NotATestProvider { } class TestProviderWithStringConstructor(configString: String) extends TestProvider { - def info: String = "not-test-provider" + def info: String = s"not-test-provider $configString" } object TestProviderLoader extends ProviderLoader[TestProvider, TestProviderResources]("test provider") @@ -129,8 +129,8 @@ class ProviderLoaderTest extends FreeSpec with Matchers with EitherValues { "The config loader" - { val resources = TestProviderResources("sausages") - implicit val testProviderConfigLoader = TestProviderLoader.singletonConfigLoader(resources) - implicit val testProvidersConfigLoader = TestProviderLoader.seqConfigLoader(resources) + implicit val testProviderConfigLoader: ConfigLoader[TestProvider] = TestProviderLoader.singletonConfigLoader(resources) + implicit val testProvidersConfigLoader: ConfigLoader[Seq[TestProvider]] = TestProviderLoader.seqConfigLoader(resources) "should load an image processor from a classname" in { val conf:Configuration = Configuration.from(Map( diff --git a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala b/common-lib/src/test/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProviderTest.scala similarity index 80% rename from common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala rename to common-lib/src/test/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProviderTest.scala index 197aa69836..1bb9f11303 100644 --- a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala +++ b/common-lib/src/test/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProviderTest.scala @@ -1,14 +1,14 @@ -package com.gu.mediaservice.lib.auth - -import java.time.Instant +package com.gu.mediaservice.lib.guardian.auth import com.gu.pandomainauth.model.{AuthenticatedUser, User} import org.scalatest.{FunSuite, MustMatchers} -class AuthenticationTest extends FunSuite with MustMatchers { +import java.time.Instant + +class PandaAuthenticationProviderTest extends FunSuite with MustMatchers { import com.gu.mediaservice.lib.guardian.auth.PandaAuthenticationProvider.validateUser - val user = AuthenticatedUser(User("Barry", "Chuckle", "barry.chuckle@guardian.co.uk", None), + val user: AuthenticatedUser = AuthenticatedUser(User("Barry", "Chuckle", "barry.chuckle@guardian.co.uk", None), "media-service", Set("media-service"), Instant.now().plusSeconds(100).toEpochMilli, multiFactor = true) test("user fails email domain validation") { From bfe508d3b5254249d42d8394abbd8be0dbe7a313 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Thu, 14 Jan 2021 13:02:25 +0000 Subject: [PATCH 13/25] Change signature of flushToken to take initial result --- auth/app/auth/AuthController.scala | 2 +- .../scala/com/gu/mediaservice/lib/auth/Authentication.scala | 2 +- .../lib/auth/provider/AuthenticationProvider.scala | 3 ++- .../lib/guardian/auth/PandaAuthenticationProvider.scala | 2 +- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/auth/app/auth/AuthController.scala b/auth/app/auth/AuthController.scala index 57c4df1b48..da2311efde 100644 --- a/auth/app/auth/AuthController.scala +++ b/auth/app/auth/AuthController.scala @@ -105,7 +105,7 @@ class AuthController(auth: Authentication, providers: AuthenticationProviders, v def logout = Action { implicit request => val result: Result = providers.userProvider.flushToken match { - case Some(callback) => callback(request) + case Some(callback) => callback(request, Ok("Logged out")) case None => InternalServerError("Logout not supported by configured authentication provider") } result.withNewSession diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala index 7cc98bfb26..c006c47df7 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala @@ -43,7 +43,7 @@ class Authentication(config: CommonConfig, def authenticationStatus(requestHeader: RequestHeader, providers: AuthenticationProviders) = { def flushToken(resultWhenAbsent: Result): Result = { - providers.userProvider.flushToken.fold(resultWhenAbsent)(_(requestHeader)) + providers.userProvider.flushToken.fold(resultWhenAbsent)(_(requestHeader, resultWhenAbsent)) } // Authenticate request. Try with API authenticator first and then with user authenticator diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala index eb014f8769..741ab93b94 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala @@ -68,8 +68,9 @@ trait UserAuthenticationProvider extends AuthenticationProvider { /** * If this provider is able to clear user tokens (i.e. by clearing cookies) then it should provide a function to * do that here which will be used to log users out and also if the token is invalid. + * This function takes the request header and a result to modify and returns the modified result. */ - def flushToken: Option[RequestHeader => Result] + def flushToken: Option[(RequestHeader, Result) => Result] } trait MachineAuthenticationProvider extends AuthenticationProvider { diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala index decd30473b..b4960f40f9 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala @@ -107,7 +107,7 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources, pr * * @return */ - override def flushToken: Option[RequestHeader => Result] = Some(processLogout(_)) + override def flushToken: Option[(RequestHeader, Result) => Result] = Some((rh, _) => processLogout(rh)) val PandaCookieKey: TypedKey[Cookie] = TypedKey[Cookie]("PandaCookie") From be32c5b154fa2be850007e11d7470f88d9312a8d Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Thu, 14 Jan 2021 13:03:13 +0000 Subject: [PATCH 14/25] Access providers via member --- .../scala/com/gu/mediaservice/lib/auth/Authentication.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala index c006c47df7..3f11284967 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala @@ -41,7 +41,7 @@ class Authentication(config: CommonConfig, Future.successful(respondError(new Status(419), errorKey = "authentication-expired", errorMessage = "User authentication token has expired", loginLinks)) } - def authenticationStatus(requestHeader: RequestHeader, providers: AuthenticationProviders) = { + def authenticationStatus(requestHeader: RequestHeader) = { def flushToken(resultWhenAbsent: Result): Result = { providers.userProvider.flushToken.fold(resultWhenAbsent)(_(requestHeader, resultWhenAbsent)) } @@ -64,7 +64,7 @@ class Authentication(config: CommonConfig, } override def invokeBlock[A](request: Request[A], block: Authentication.Request[A] => Future[Result]): Future[Result] = { - authenticationStatus(request, providers) match { + authenticationStatus(request) match { // we have a principal, so process the block case Right(principal) => block(new AuthenticatedRequest(principal, request)) // no principal so return a result which will either be an error or a form of redirect From 81e3a8bc81ac9e15d248e6239e22541ef3fe8b95 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Thu, 14 Jan 2021 13:04:13 +0000 Subject: [PATCH 15/25] Migrate to scalatest's play integration --- build.sbt | 4 ++-- .../scala/com/gu/mediaservice/model/FileMetadataTest.scala | 4 ++-- cropper/test/lib/CropsTest.scala | 2 +- image-loader/test/scala/model/ImageUploadTest.scala | 2 +- image-loader/test/scala/model/ProjectorTest.scala | 2 +- media-api/test/lib/elasticsearch/ElasticSearchTest.scala | 2 +- media-api/test/scala/lib/ImageExtrasTest.scala | 3 +-- media-api/test/usagerights/CostCalculatorTest.scala | 2 +- thrall/test/lib/ThrallStreamProcessorTest.scala | 2 +- thrall/test/lib/kinesis/MessageProcessorTest.scala | 2 +- 10 files changed, 12 insertions(+), 13 deletions(-) diff --git a/build.sbt b/build.sbt index 605674e0e4..6585cb7592 100644 --- a/build.sbt +++ b/build.sbt @@ -17,8 +17,8 @@ val commonSettings = Seq( testOptions in Test ++= Seq(Tests.Argument(TestFrameworks.ScalaTest, "-o"), Tests.Argument(TestFrameworks.ScalaTest, "-u", "logs/test-reports")), libraryDependencies ++= Seq( - "org.scalatest" %% "scalatest" % "3.0.5", - "org.mockito" % "mockito-core" % "2.18.0" + "org.scalatestplus.play" %% "scalatestplus-play" % "3.1.3" % Test, + "org.mockito" % "mockito-core" % "2.18.0" % Test ) ) diff --git a/common-lib/src/test/scala/com/gu/mediaservice/model/FileMetadataTest.scala b/common-lib/src/test/scala/com/gu/mediaservice/model/FileMetadataTest.scala index 2c4b5444d7..e7f8390a0a 100644 --- a/common-lib/src/test/scala/com/gu/mediaservice/model/FileMetadataTest.scala +++ b/common-lib/src/test/scala/com/gu/mediaservice/model/FileMetadataTest.scala @@ -1,10 +1,10 @@ package com.gu.mediaservice.model -import org.scalatest.prop.{Checkers, PropertyChecks} import org.scalatest.{FreeSpec, Matchers} +import org.scalatestplus.scalacheck.{Checkers, ScalaCheckPropertyChecks} import play.api.libs.json.Json -class FileMetadataTest extends FreeSpec with Matchers with Checkers with PropertyChecks { +class FileMetadataTest extends FreeSpec with Matchers with Checkers with ScalaCheckPropertyChecks { "Dehydrate a non-empty object" - { "Leave all short values alone" in { diff --git a/cropper/test/lib/CropsTest.scala b/cropper/test/lib/CropsTest.scala index 88b47dc7a7..66fc8304b6 100644 --- a/cropper/test/lib/CropsTest.scala +++ b/cropper/test/lib/CropsTest.scala @@ -2,7 +2,7 @@ package lib import com.gu.mediaservice.lib.imaging.ImageOperations import com.gu.mediaservice.model._ -import org.scalatest.mockito.MockitoSugar +import org.scalatestplus.mockito.MockitoSugar import org.scalatest.{FunSpec, Matchers} diff --git a/image-loader/test/scala/model/ImageUploadTest.scala b/image-loader/test/scala/model/ImageUploadTest.scala index 4e6ec20e16..f3db3cf136 100644 --- a/image-loader/test/scala/model/ImageUploadTest.scala +++ b/image-loader/test/scala/model/ImageUploadTest.scala @@ -14,7 +14,7 @@ import com.gu.mediaservice.model.{FileMetadata, Jpeg, MimeType, Png, Tiff, Uploa import lib.imaging.MimeTypeDetection import model.upload.{OptimiseWithPngQuant, UploadRequest} import org.joda.time.DateTime -import org.scalatest.mockito.MockitoSugar +import org.scalatestplus.mockito.MockitoSugar import org.scalatest.{Assertion, AsyncFunSuite, Matchers} import test.lib.ResourceHelpers diff --git a/image-loader/test/scala/model/ProjectorTest.scala b/image-loader/test/scala/model/ProjectorTest.scala index ba860dc3e9..c5cfb864e6 100644 --- a/image-loader/test/scala/model/ProjectorTest.scala +++ b/image-loader/test/scala/model/ProjectorTest.scala @@ -13,7 +13,7 @@ import com.gu.mediaservice.model.leases.LeasesByMedia import lib.DigestedFile import org.joda.time.{DateTime, DateTimeZone} import org.scalatest.concurrent.ScalaFutures -import org.scalatest.mockito.MockitoSugar +import org.scalatestplus.mockito.MockitoSugar import org.scalatest.time.{Millis, Span} import org.scalatest.{FunSuite, Matchers} import play.api.libs.json.{JsArray, JsString} diff --git a/media-api/test/lib/elasticsearch/ElasticSearchTest.scala b/media-api/test/lib/elasticsearch/ElasticSearchTest.scala index 3c18e3d709..b9f742a00b 100644 --- a/media-api/test/lib/elasticsearch/ElasticSearchTest.scala +++ b/media-api/test/lib/elasticsearch/ElasticSearchTest.scala @@ -16,7 +16,7 @@ import lib.querysyntax._ import lib.{MediaApiConfig, MediaApiMetrics} import org.joda.time.DateTime import org.scalatest.concurrent.Eventually -import org.scalatest.mockito.MockitoSugar +import org.scalatestplus.mockito.MockitoSugar import play.api.{Configuration, Mode} import play.api.libs.json.{JsString, Json} import play.api.mvc.AnyContent diff --git a/media-api/test/scala/lib/ImageExtrasTest.scala b/media-api/test/scala/lib/ImageExtrasTest.scala index f30601cd24..778293fbe9 100644 --- a/media-api/test/scala/lib/ImageExtrasTest.scala +++ b/media-api/test/scala/lib/ImageExtrasTest.scala @@ -1,12 +1,11 @@ package lib import java.net.URI - import com.gu.mediaservice.model._ import lib.usagerights.CostCalculator import org.joda.time.DateTime -import org.scalatest.mockito.MockitoSugar import org.scalatest.{FunSpec, Matchers} +import org.scalatestplus.mockito.MockitoSugar class ImageExtrasTest extends FunSpec with Matchers with MockitoSugar { diff --git a/media-api/test/usagerights/CostCalculatorTest.scala b/media-api/test/usagerights/CostCalculatorTest.scala index 6a90767bdd..22c77b3c9d 100644 --- a/media-api/test/usagerights/CostCalculatorTest.scala +++ b/media-api/test/usagerights/CostCalculatorTest.scala @@ -2,8 +2,8 @@ package lib.usagerights import com.gu.mediaservice.model._ import lib.UsageQuota -import org.scalatest.mockito.MockitoSugar import org.scalatest.{FunSpec, Matchers} +import org.scalatestplus.mockito.MockitoSugar class CostCalculatorTest extends FunSpec with Matchers with MockitoSugar { diff --git a/thrall/test/lib/ThrallStreamProcessorTest.scala b/thrall/test/lib/ThrallStreamProcessorTest.scala index 678c97ed83..960f9adc07 100644 --- a/thrall/test/lib/ThrallStreamProcessorTest.scala +++ b/thrall/test/lib/ThrallStreamProcessorTest.scala @@ -9,7 +9,7 @@ import akka.stream.scaladsl.{Sink, Source} import akka.util.ByteString import com.contxt.kinesis.KinesisRecord import lib.kinesis.ThrallEventConsumer -import org.scalatest.mockito.MockitoSugar +import org.scalatestplus.mockito.MockitoSugar import org.scalatest.{BeforeAndAfterAll, FunSpec, Matchers} import scala.concurrent.duration._ diff --git a/thrall/test/lib/kinesis/MessageProcessorTest.scala b/thrall/test/lib/kinesis/MessageProcessorTest.scala index 4652a36e2a..53ec4b317f 100644 --- a/thrall/test/lib/kinesis/MessageProcessorTest.scala +++ b/thrall/test/lib/kinesis/MessageProcessorTest.scala @@ -10,7 +10,7 @@ import com.gu.mediaservice.model.{Collection, Crop, Edits, Image, ImageMetadata, import lib.{MetadataEditorNotifications, ThrallStore} import lib.elasticsearch.{ElasticSearchTestBase, ElasticSearchUpdateResponse, SyndicationRightsOps} import org.joda.time.DateTime -import org.scalatest.mockito.MockitoSugar +import org.scalatestplus.mockito.MockitoSugar import play.api.libs.json.{JsArray, Json} import scala.concurrent.{Await, Future} From 32f8fe5736a1b651213b27c79f092b31c6bb2f37 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Thu, 14 Jan 2021 13:04:35 +0000 Subject: [PATCH 16/25] Add set of tests for authenticationStatus --- .../lib/auth/AuthenticationTest.scala | 162 ++++++++++++++++++ 1 file changed, 162 insertions(+) create mode 100644 common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala diff --git a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala new file mode 100644 index 0000000000..ef42c7ecab --- /dev/null +++ b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala @@ -0,0 +1,162 @@ +package com.gu.mediaservice.lib.auth + +import akka.actor.ActorSystem +import akka.stream.ActorMaterializer +import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, GridUser} +import com.gu.mediaservice.lib.auth.provider.AuthenticationProvider.RedirectUri +import com.gu.mediaservice.lib.auth.provider._ +import com.gu.mediaservice.lib.config.CommonConfig +import org.scalatest.{AsyncFreeSpec, BeforeAndAfterAll, EitherValues, Matchers} +import play.api.Configuration +import play.api.http.Status +import play.api.libs.json.{Format, Json} +import play.api.libs.ws.WSRequest +import play.api.mvc.{Cookie, DiscardingCookie, PlayBodyParsers, RequestHeader, Result} +import play.api.test.Helpers.defaultAwaitTimeout +import play.api.test.{FakeRequest, Helpers} + +import scala.concurrent.ExecutionContext.global +import scala.concurrent.Future +import scala.util.Try + +//noinspection NotImplementedCode,SpellCheckingInspection +class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues with BeforeAndAfterAll { + + implicit val actorSystem: ActorSystem = ActorSystem() + implicit val materializer: ActorMaterializer = ActorMaterializer() + + private val COOKIE_NAME = "TestGridAuth" + private val HEADER_NAME = "X-TestMachine-Auth" + private case class AuthToken(firstName: String, lastName: String, email: String, expired: Boolean, veryExpired: Boolean) { + def user: GridUser = GridUser(firstName, lastName, email) + } + private implicit val cookieFormats: Format[AuthToken] = Json.format[AuthToken] + private def makeCookie(firstName: String = "Test", lastName: String = "User", email: String = "test@user", expired: Boolean = false, veryExpired: Boolean = false): Cookie = { + val data = AuthToken(firstName, lastName, email, expired, veryExpired) + val value = Json.stringify(Json.toJson(data)) + Cookie(COOKIE_NAME, value) + } + private def parseCookie(cookie: Cookie): Option[AuthToken] = { + Try(Json.parse(cookie.value)).toOption.flatMap(_.asOpt[AuthToken]) + } + + "authenticationStatus" - { + val config = new CommonConfig(Configuration.from(Map( + "grid.stage" -> "TEST", + "grid.appName" -> "test", + "thrall.kinesis.stream.name" -> "not-used", + "thrall.kinesis.lowPriorityStream.name" -> "not-used", + "domain.root" -> "notused.example.com" + ))) {} + + val testProviders = AuthenticationProviders( + new UserAuthenticationProvider { + override def authenticateRequest(request: RequestHeader): AuthenticationStatus = { + request.cookies.get(COOKIE_NAME) match { + case None => NotAuthenticated + case Some(cookie) => + parseCookie(cookie) match { + case None => Invalid("Token not valid") + case Some(token@AuthToken(_, _, _, true, false)) => GracePeriod(token.user) + case Some(token@AuthToken(_, _, _, _, true)) => Expired(token.user) + case Some(token) if token.email == "test@user" => Authenticated(token.user) + case Some(token) => NotAuthorised(s"${token.email} not authorised") + } + } + } + + override def sendForAuthentication: Option[RequestHeader => Future[Result]] = ??? + override def processAuthentication: Option[(RequestHeader, Option[RedirectUri]) => Future[Result]] = ??? + override def flushToken: Option[(RequestHeader, Result) => Result] = Some({(_: RequestHeader, result: Result) => + result.discardingCookies(DiscardingCookie(COOKIE_NAME)) + }) + override def onBehalfOf(request: Authentication.Principal): Either[String, WSRequest => WSRequest] = ??? + }, + new MachineAuthenticationProvider { + override def authenticateRequest(request: RequestHeader): ApiAuthenticationStatus = { + request.headers.get(HEADER_NAME) match { + case None => NotAuthenticated + case Some(key) if key.startsWith("key-") && key.endsWith("-blocked") => NotAuthorised(s"$key is blocked") + case Some(key) if key.startsWith("key-") => Authenticated(ApiKeyAccessor(ApiAccessor(key, Internal))) + case Some(_) => Invalid("Key doesn't start with 'key-'") + } + } + override def onBehalfOf(request: Authentication.Principal): Either[String, WSRequest => WSRequest] = ??? + } + ) + + val auth = new Authentication( + config = config, + providers = testProviders, + parser = PlayBodyParsers().default, + executionContext = global + ) + "should return unauthorised if request is empty (no headers)" in { + val authStatus = auth.authenticationStatus(FakeRequest()) + authStatus.left.value.map { result => + result.header.status shouldBe Status.UNAUTHORIZED + Helpers.contentAsJson(Future.successful(result)).\("errorKey").as[String] shouldBe "authentication-failure" + } + } + "should return invalid if the cookie is present but invalid" in { + val request = FakeRequest().withCookies(Cookie(COOKIE_NAME, "garbage")) + val authStatus = auth.authenticationStatus(request) + authStatus.left.value.map { result => + result.header.status shouldBe Status.UNAUTHORIZED + Helpers.contentAsJson(Future.successful(result)).\("errorKey").as[String] shouldBe "authentication-failure" + } + } + "should return user when valid" in { + val request = FakeRequest().withCookies(makeCookie()) + val authStatus = auth.authenticationStatus(request) + authStatus.right.value shouldBe GridUser("Test", "User", "test@user") + } + "should return user when expired but in grace period" in { + val request = FakeRequest().withCookies(makeCookie(expired = true)) + val authStatus = auth.authenticationStatus(request) + authStatus.right.value shouldBe GridUser("Test", "User", "test@user") + } + "should return 419 when expired" in { + val request = FakeRequest().withCookies(makeCookie(veryExpired = true)) + val authStatus = auth.authenticationStatus(request) + authStatus.left.value.map { result => + result.header.status shouldBe 419 + Helpers.contentAsJson(Future.successful(result)).\("errorKey").as[String] shouldBe "authentication-expired" + } + } + "should return forbidden when user is not authorised by provider" in { + val request = FakeRequest().withCookies(makeCookie(email = "l33t@hacker")) + val authStatus = auth.authenticationStatus(request) + authStatus.left.value.map { result => + result.header.status shouldBe Status.FORBIDDEN + Helpers.contentAsJson(Future.successful(result)).\("errorKey").as[String] shouldBe "principal-not-authorised" + } + } + "should authenticate with an API key" in { + val request = FakeRequest().withHeaders(HEADER_NAME -> "key-client") + val authStatus = auth.authenticationStatus(request) + authStatus.right.value shouldBe ApiKeyAccessor(ApiAccessor("key-client", Internal)) + } + "should return unauthorised when the API key is garbage" in { + val request = FakeRequest().withHeaders(HEADER_NAME -> "garbage") + val authStatus = auth.authenticationStatus(request) + authStatus.left.value.map { result => + result.header.status shouldBe Status.UNAUTHORIZED + Helpers.contentAsJson(Future.successful(result)).\("errorKey").as[String] shouldBe "authentication-failure" + } + } + "should return forbidden if valid key is blocked" in { + val request = FakeRequest().withHeaders(HEADER_NAME -> "key-is-blocked") + val authStatus = auth.authenticationStatus(request) + authStatus.left.value.map { result => + result.header.status shouldBe Status.FORBIDDEN + Helpers.contentAsJson(Future.successful(result)).\("errorKey").as[String] shouldBe "principal-not-authorised" + } + } + "should prioritise machine authentication over user authentication" in { + val request = FakeRequest().withCookies(makeCookie()).withHeaders(HEADER_NAME -> "key-client") + val authStatus = auth.authenticationStatus(request) + authStatus.right.value shouldBe ApiKeyAccessor(ApiAccessor("key-client", Internal)) + } + } +} From eb4af3a616224d099de51bff95a939d783a4d48c Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Thu, 14 Jan 2021 13:50:35 +0000 Subject: [PATCH 17/25] Add set of tests for on behalf of functions --- .../lib/auth/AuthenticationTest.scala | 94 ++++++++++++++++--- 1 file changed, 83 insertions(+), 11 deletions(-) diff --git a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala index ef42c7ecab..332322df0d 100644 --- a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala +++ b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala @@ -2,19 +2,23 @@ package com.gu.mediaservice.lib.auth import akka.actor.ActorSystem import akka.stream.ActorMaterializer -import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, GridUser} +import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, GridUser, OnBehalfOfPrincipal} import com.gu.mediaservice.lib.auth.provider.AuthenticationProvider.RedirectUri import com.gu.mediaservice.lib.auth.provider._ -import com.gu.mediaservice.lib.config.CommonConfig +import com.gu.mediaservice.lib.config.{CommonConfig, TestProvider} import org.scalatest.{AsyncFreeSpec, BeforeAndAfterAll, EitherValues, Matchers} +import org.scalatestplus.play.PlaySpec import play.api.Configuration import play.api.http.Status import play.api.libs.json.{Format, Json} -import play.api.libs.ws.WSRequest +import play.api.libs.typedmap.{TypedKey, TypedMap} +import play.api.libs.ws.{DefaultWSCookie, WSRequest} import play.api.mvc.{Cookie, DiscardingCookie, PlayBodyParsers, RequestHeader, Result} import play.api.test.Helpers.defaultAwaitTimeout -import play.api.test.{FakeRequest, Helpers} +import play.api.test.{FakeRequest, Helpers, WsTestClient} +import play.libs.ws.WSCookie +import java.lang.IllegalStateException import scala.concurrent.ExecutionContext.global import scala.concurrent.Future import scala.util.Try @@ -40,7 +44,7 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w Try(Json.parse(cookie.value)).toOption.flatMap(_.asOpt[AuthToken]) } - "authenticationStatus" - { + def makeAuthenticationInstance(testProviders: AuthenticationProviders): Authentication = { val config = new CommonConfig(Configuration.from(Map( "grid.stage" -> "TEST", "grid.appName" -> "test", @@ -48,7 +52,15 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w "thrall.kinesis.lowPriorityStream.name" -> "not-used", "domain.root" -> "notused.example.com" ))) {} + new Authentication( + config = config, + providers = testProviders, + parser = PlayBodyParsers().default, + executionContext = global + ) + } + "authenticationStatus" - { val testProviders = AuthenticationProviders( new UserAuthenticationProvider { override def authenticateRequest(request: RequestHeader): AuthenticationStatus = { @@ -85,12 +97,8 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w } ) - val auth = new Authentication( - config = config, - providers = testProviders, - parser = PlayBodyParsers().default, - executionContext = global - ) + val auth = makeAuthenticationInstance(testProviders) + "should return unauthorised if request is empty (no headers)" in { val authStatus = auth.authenticationStatus(FakeRequest()) authStatus.left.value.map { result => @@ -159,4 +167,68 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w authStatus.right.value shouldBe ApiKeyAccessor(ApiAccessor("key-client", Internal)) } } + + "getOnBehalfOfPrincipal" - { + val CookieKey: TypedKey[Cookie] = TypedKey("cookie-key") + val HeaderKey: TypedKey[(String, String)] = TypedKey("header-key") + val testProviders = AuthenticationProviders( + new UserAuthenticationProvider { + override def authenticateRequest(request: RequestHeader): AuthenticationStatus = ??? + override def sendForAuthentication: Option[RequestHeader => Future[Result]] = ??? + override def processAuthentication: Option[(RequestHeader, Option[RedirectUri]) => Future[Result]] = ??? + override def flushToken: Option[(RequestHeader, Result) => Result] = ??? + override def onBehalfOf(request: Authentication.Principal): Either[String, WSRequest => WSRequest] = request match { + case GridUser(_,_,_,attributes) if attributes.contains(CookieKey) => Right(req => req.addCookies(DefaultWSCookie(COOKIE_NAME, attributes.get(CookieKey).get.value))) + case GridUser(_, _, email, _) => Left(s"Unable to build onBehalfOf function for $email") + } + }, + new MachineAuthenticationProvider { + override def authenticateRequest(request: RequestHeader): ApiAuthenticationStatus = ??? + override def onBehalfOf(request: Authentication.Principal): Either[String, WSRequest => WSRequest] = request match { + case ApiKeyAccessor(_, attributes) if attributes.contains(HeaderKey) => Right(req => req.addHttpHeaders(attributes.get(HeaderKey).get)) + case ApiKeyAccessor(ApiAccessor(identity, _), _) => Left(s"Unable to build onBehalfOf function for $identity") + } + } + ) + val auth: Authentication = makeAuthenticationInstance(testProviders) + + "return function for user principal" in { + val testUser = GridUser("Test", "User", "test@user", TypedMap(CookieKey -> Cookie(COOKIE_NAME, "this is my cookie value"))) + val onBehalfOfFn: OnBehalfOfPrincipal = auth.getOnBehalfOfPrincipal(testUser) + WsTestClient.withClient{ client => + val req = client.url("https://example.com") + val modifiedReq = onBehalfOfFn(req) + val maybeCookie = modifiedReq.cookies.find(_.name == COOKIE_NAME) + maybeCookie.nonEmpty shouldBe true + maybeCookie.get.name shouldBe COOKIE_NAME + maybeCookie.get.value shouldBe "this is my cookie value" + } + } + + "fail to get function for user principal if the user doesn't have the cookie" in { + val testUser = GridUser("Test", "User", "test@user") + the [IllegalStateException] thrownBy { + val onBehalfOfFn: OnBehalfOfPrincipal = auth.getOnBehalfOfPrincipal(testUser) + } should have message "Unable to build onBehalfOf function for test@user" + } + + "return function for machine principal" in { + val apiAccessor = ApiKeyAccessor(ApiAccessor("my-client-id", Internal), TypedMap(HeaderKey -> (HEADER_NAME -> "my-client-id-key"))) + val onBehalfOfFn: OnBehalfOfPrincipal = auth.getOnBehalfOfPrincipal(apiAccessor) + WsTestClient.withClient{ client => + val req = client.url("https://example.com") + val modifiedReq = onBehalfOfFn(req) + val maybeHeader = modifiedReq.headers.get(HEADER_NAME) + maybeHeader.nonEmpty shouldBe true + maybeHeader.get.head shouldBe "my-client-id-key" + } + } + + "fail to get function for user principal if the api accessor doesn't have the header" in { + val apiAccessor = ApiKeyAccessor(ApiAccessor("my-client-id", Internal)) + the [IllegalStateException] thrownBy { + val onBehalfOfFn: OnBehalfOfPrincipal = auth.getOnBehalfOfPrincipal(apiAccessor) + } should have message "Unable to build onBehalfOf function for my-client-id" + } + } } From c9580f7b51f0c65b59915ea215e8fd3c048cb46f Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Thu, 14 Jan 2021 13:54:58 +0000 Subject: [PATCH 18/25] Rename process authentication callback as a result of review --- auth/app/auth/AuthController.scala | 2 +- .../lib/auth/provider/AuthenticationProvider.scala | 2 +- .../lib/guardian/auth/PandaAuthenticationProvider.scala | 2 +- .../com/gu/mediaservice/lib/auth/AuthenticationTest.scala | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/auth/app/auth/AuthController.scala b/auth/app/auth/AuthController.scala index da2311efde..a9cc4ec623 100644 --- a/auth/app/auth/AuthController.scala +++ b/auth/app/auth/AuthController.scala @@ -94,7 +94,7 @@ class AuthController(auth: Authentication, providers: AuthenticationProviders, v } def oauthCallback = Action.async { implicit request => - providers.userProvider.processAuthentication match { + providers.userProvider.sendForAuthenticationCallback match { case Some(callback) => val maybeRedirectUri = request.session.get(REDIRECT_SESSION_KEY) callback(request, maybeRedirectUri).map(_.removingFromSession(REDIRECT_SESSION_KEY)) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala index 741ab93b94..596a019c27 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala @@ -63,7 +63,7 @@ trait UserAuthenticationProvider extends AuthenticationProvider { * The function should take the Play request header and the redirect URI that the user should be * sent to on successful completion of the authentication. */ - def processAuthentication: Option[(RequestHeader, Option[RedirectUri]) => Future[Result]] + def sendForAuthenticationCallback: Option[(RequestHeader, Option[RedirectUri]) => Future[Result]] /** * If this provider is able to clear user tokens (i.e. by clearing cookies) then it should provide a function to diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala index b4960f40f9..4372ec6c03 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala @@ -77,7 +77,7 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources, pr * used to set a cookie or similar to ensure that a subsequent call to authenticateRequest will succeed. If * authentication failed then this should return an appropriate 4xx result. */ - override def processAuthentication: Option[(RequestHeader, Option[RedirectUri]) => Future[Result]] = + override def sendForAuthenticationCallback: Option[(RequestHeader, Option[RedirectUri]) => Future[Result]] = Some({ (requestHeader: RequestHeader, maybeUri: Option[RedirectUri]) => // We use the `Try` here as the `GoogleAuthException` are thrown before we // get to the asynchronicity of the `Future` it returns. diff --git a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala index 332322df0d..fccfaf6a33 100644 --- a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala +++ b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala @@ -78,7 +78,7 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w } override def sendForAuthentication: Option[RequestHeader => Future[Result]] = ??? - override def processAuthentication: Option[(RequestHeader, Option[RedirectUri]) => Future[Result]] = ??? + override def sendForAuthenticationCallback: Option[(RequestHeader, Option[RedirectUri]) => Future[Result]] = ??? override def flushToken: Option[(RequestHeader, Result) => Result] = Some({(_: RequestHeader, result: Result) => result.discardingCookies(DiscardingCookie(COOKIE_NAME)) }) @@ -175,7 +175,7 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w new UserAuthenticationProvider { override def authenticateRequest(request: RequestHeader): AuthenticationStatus = ??? override def sendForAuthentication: Option[RequestHeader => Future[Result]] = ??? - override def processAuthentication: Option[(RequestHeader, Option[RedirectUri]) => Future[Result]] = ??? + override def sendForAuthenticationCallback: Option[(RequestHeader, Option[RedirectUri]) => Future[Result]] = ??? override def flushToken: Option[(RequestHeader, Result) => Result] = ??? override def onBehalfOf(request: Authentication.Principal): Either[String, WSRequest => WSRequest] = request match { case GridUser(_,_,_,attributes) if attributes.contains(CookieKey) => Right(req => req.addCookies(DefaultWSCookie(COOKIE_NAME, attributes.get(CookieKey).get.value))) From 5af22a05d9d2aad29c9ec056fec9324a8777c490 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Thu, 14 Jan 2021 14:20:19 +0000 Subject: [PATCH 19/25] Correct/improve docs --- .../lib/auth/provider/AuthenticationProvider.scala | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala index 596a019c27..c07dbe276b 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala @@ -13,9 +13,9 @@ import scala.concurrent.Future * Case class containing useful resources for authentication providers to allow concurrent processing and external * API calls to be conducted. * @param commonConfig the Grid common config object - * @param context an execution context * @param actorSystem an actor system * @param wsClient a play WSClient for making API calls + * @param controllerComponents play components, including the execution context for example */ case class AuthenticationProviderResources(commonConfig: CommonConfig, actorSystem: ActorSystem, @@ -27,9 +27,12 @@ sealed trait AuthenticationProvider { def shutdown(): Future[Unit] = Future.successful(()) /** - * A function that allows downstream API calls to be made using the credentials of the inflight request - * @param request The request header of the inflight call - * @return A function that adds appropriate authentication headers to a WSRequest or returns a runtime error + * A function that allows downstream API calls to be made using the credentials of the current principal. + * It is recommended that any data required for this downstream request enrichment is put into the principal's + * attribute map when the principal is created in the authenticateRequest call. + * @param request The principal for the current request + * @return Either a function that adds appropriate authentication headers to a WSRequest or an error string explaining + * why it wasn't possible to create a function. */ def onBehalfOf(request: Principal): Either[String, WSRequest => WSRequest] } From 1f09667a4757925abd35fa156a6c0aa2eeee894f Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Thu, 14 Jan 2021 14:33:57 +0000 Subject: [PATCH 20/25] Allow the login link to be disabled or overridden if appropriate --- .../gu/mediaservice/lib/auth/Authentication.scala | 9 +++++---- .../auth/provider/AuthenticationProvider.scala | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala index 3f11284967..7d48e760bd 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala @@ -21,10 +21,11 @@ class Authentication(config: CommonConfig, // make the execution context implicit so it will be picked up appropriately implicit val ec: ExecutionContext = executionContext - // TODO: SAH / Evaluate MB's suggestion that this moves to the provider - val loginLinks = List( - Link("login", config.services.loginUriTemplate) - ) + val loginLinks: List[Link] = providers.userProvider.loginLink match { + case DisableLoginLink => Nil + case BuiltInAuthService => List(Link("login", config.services.loginUriTemplate)) + case ExternalLoginLink(link) => List(Link("login", link)) + } def unauthorised(errorMessage: String, throwable: Option[Throwable] = None): Future[Result] = { logger.info(s"Authentication failure $errorMessage", throwable.orNull) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala index c07dbe276b..ea6cdab753 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala @@ -22,6 +22,11 @@ case class AuthenticationProviderResources(commonConfig: CommonConfig, wsClient: WSClient, controllerComponents: ControllerComponents) +sealed trait LoginLink +case object BuiltInAuthService extends LoginLink +case object DisableLoginLink extends LoginLink +case class ExternalLoginLink(link: String) extends LoginLink + sealed trait AuthenticationProvider { def initialise(): Unit = {} def shutdown(): Future[Unit] = Future.successful(()) @@ -74,6 +79,16 @@ trait UserAuthenticationProvider extends AuthenticationProvider { * This function takes the request header and a result to modify and returns the modified result. */ def flushToken: Option[(RequestHeader, Result) => Result] + + /** + * The login link is provided to the client to tell them where to go if they are + * not authenticated. By default the Grid provides a link to the authentication + * microservice but this behaviour can be modified. If it is not possible to login + * or authentication is handled by a proxy you can set this to DisableLoginLink. + * Alternatively if you are using an alternative external service to do authentication + * then this can be explicitly set to an alternative URL using ExternalLoginLink. + */ + def loginLink: LoginLink = BuiltInAuthService } trait MachineAuthenticationProvider extends AuthenticationProvider { From f3bef97fc07e470cb9d99271e6ffb4259f75e551 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Thu, 14 Jan 2021 14:49:07 +0000 Subject: [PATCH 21/25] Make it possible to cancel scheduled updates in stores --- .../com/gu/mediaservice/lib/BaseStore.scala | 11 ++++++--- .../ApiKeyAuthenticationProvider.scala | 23 ++++++++----------- media-api/app/MediaApiComponents.scala | 3 +++ media-api/app/lib/UsageQuota.scala | 7 +++++- 4 files changed, 27 insertions(+), 17 deletions(-) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/BaseStore.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/BaseStore.scala index 180521235b..28e307ee69 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/BaseStore.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/BaseStore.scala @@ -1,8 +1,7 @@ package com.gu.mediaservice.lib import java.io.InputStream - -import akka.actor.Scheduler +import akka.actor.{Cancellable, Scheduler} import com.gu.Box import com.gu.mediaservice.lib.aws.S3 import com.gu.mediaservice.lib.config.CommonConfig @@ -42,8 +41,14 @@ abstract class BaseStore[TStoreKey, TStoreVal](bucket: String, config: CommonCon } } + private var cancellable: Option[Cancellable] = None + def scheduleUpdates(scheduler: Scheduler) { - scheduler.schedule(0.seconds, 10.minutes)(update()) + cancellable = Some(scheduler.schedule(0.seconds, 10.minutes)(update())) + } + + def stopUpdates(): Unit = { + cancellable.foreach(_.cancel()) } def update(): Unit diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala index 7c5d84ada1..bf39d24a80 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala @@ -3,11 +3,11 @@ import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, Principal} import com.gu.mediaservice.lib.auth.{ApiAccessor, KeyStore} import com.typesafe.scalalogging.StrictLogging import play.api.Configuration -import play.api.libs.typedmap.{TypedEntry, TypedKey, TypedMap} +import play.api.libs.typedmap.{TypedKey, TypedMap} import play.api.libs.ws.WSRequest import play.api.mvc.RequestHeader -import scala.concurrent.ExecutionContext +import scala.concurrent.{ExecutionContext, Future} object ApiKeyAuthenticationProvider { val apiKeyHeaderName = "X-Gu-Media-Key" @@ -19,13 +19,16 @@ class ApiKeyAuthenticationProvider(configuration: Configuration, resources: Auth implicit val executionContext: ExecutionContext = resources.controllerComponents.executionContext var keyStorePlaceholder: Option[KeyStore] = _ - // TODO: we should also shutdown the keystore but there isn't currently a hook override def initialise(): Unit = { val store = new KeyStore(configuration.get[String]("authKeyStoreBucket"), resources.commonConfig) store.scheduleUpdates(resources.actorSystem.scheduler) keyStorePlaceholder = Some(store) } + override def shutdown(): Future[Unit] = Future { + keyStorePlaceholder.foreach(_.stopUpdates()) + } + def keyStore: KeyStore = keyStorePlaceholder.getOrElse(throw new IllegalStateException("Not initialised")) /** @@ -43,15 +46,15 @@ class ApiKeyAuthenticationProvider(configuration: Configuration, resources: Auth // api key provided case Some(apiKey) => // valid api key - val header = TypedEntry(ApiKeyHeader, ApiKeyAuthenticationProvider.apiKeyHeaderName -> key) - val accessor = ApiKeyAccessor(apiKey, TypedMap(header)) - logger.info(s"Using api key with name ${apiKey.identity} and tier ${apiKey.tier}", apiKey) if (ApiAccessor.hasAccess(apiKey, request, resources.commonConfig.services)) { // valid api key which has access + // store the header that was used in the attributes map of the principal for use in onBehalfOf calls + val accessor = ApiKeyAccessor(apiKey, TypedMap(ApiKeyHeader -> (ApiKeyAuthenticationProvider.apiKeyHeaderName -> key))) + logger.info(s"Using api key with name ${apiKey.identity} and tier ${apiKey.tier}", apiKey) Authenticated(accessor) } else { // valid api key which doesn't have access - NotAuthorised("API key valid but not authorised") + NotAuthorised(s"API key ${apiKey.identity} valid but not authorised for this request") } // provided api key not known case None => Invalid("API key not valid") @@ -61,12 +64,6 @@ class ApiKeyAuthenticationProvider(configuration: Configuration, resources: Auth } } - /** - * A function that allows downstream API calls to be made using the credentials of the inflight request - * - * @param request The request header of the inflight call - * @return A function that adds appropriate data to a WSRequest - */ override def onBehalfOf(principal: Principal): Either[String, WSRequest => WSRequest] = { principal.attributes.get(ApiKeyHeader) match { case Some(apiKeyHeaderTuple) => Right { diff --git a/media-api/app/MediaApiComponents.scala b/media-api/app/MediaApiComponents.scala index 06fad383cc..b6df3a850d 100644 --- a/media-api/app/MediaApiComponents.scala +++ b/media-api/app/MediaApiComponents.scala @@ -9,6 +9,8 @@ import lib.elasticsearch.ElasticSearch import play.api.ApplicationLoader.Context import router.Routes +import scala.concurrent.Future + class MediaApiComponents(context: Context) extends GridComponents(context, new MediaApiConfig(_)) { final override val buildInfo = utils.buildinfo.BuildInfo @@ -30,6 +32,7 @@ class MediaApiComponents(context: Context) extends GridComponents(context, new M val usageQuota = new UsageQuota(config, actorSystem.scheduler) usageQuota.quotaStore.update() usageQuota.scheduleUpdates() + applicationLifecycle.addStopHook(() => Future{usageQuota.stopUpdates()}) val elasticSearch = new ElasticSearch(config, mediaApiMetrics, es6Config, () => usageQuota.usageStore.overQuotaAgencies) elasticSearch.ensureAliasAssigned() diff --git a/media-api/app/lib/UsageQuota.scala b/media-api/app/lib/UsageQuota.scala index 1a67ce91f5..7b88105852 100644 --- a/media-api/app/lib/UsageQuota.scala +++ b/media-api/app/lib/UsageQuota.scala @@ -30,7 +30,12 @@ class UsageQuota(config: MediaApiConfig, scheduler: Scheduler) { usageStore.scheduleUpdates(scheduler) } - def isOverQuota(rights: UsageRights, waitMillis: Int = 100) = Try { + def stopUpdates(): Unit = { + quotaStore.stopUpdates() + usageStore.stopUpdates() + } + + def isOverQuota(rights: UsageRights, waitMillis: Int = 100): Boolean = Try { Await.result( usageStore.getUsageStatusForUsageRights(rights), waitMillis.millis) From e33c0c28fc14a97a2be89e6c85975eebdda93c51 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Thu, 14 Jan 2021 14:49:32 +0000 Subject: [PATCH 22/25] Fix typo in provider trait --- .../lib/auth/provider/AuthenticationProvider.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala index ea6cdab753..7d599483ed 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationProvider.scala @@ -35,11 +35,11 @@ sealed trait AuthenticationProvider { * A function that allows downstream API calls to be made using the credentials of the current principal. * It is recommended that any data required for this downstream request enrichment is put into the principal's * attribute map when the principal is created in the authenticateRequest call. - * @param request The principal for the current request + * @param principal The principal for the current request * @return Either a function that adds appropriate authentication headers to a WSRequest or an error string explaining * why it wasn't possible to create a function. */ - def onBehalfOf(request: Principal): Either[String, WSRequest => WSRequest] + def onBehalfOf(principal: Principal): Either[String, WSRequest => WSRequest] } object AuthenticationProvider { From d20d608412875b0964eaff8433a4479a88b1f8be Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Fri, 15 Jan 2021 15:22:29 +0000 Subject: [PATCH 23/25] Change names of Principal implementations for consistency --- auth/app/auth/AuthController.scala | 6 ++-- .../src/main/resources/application.conf | 2 +- .../lib/auth/Authentication.scala | 15 ++++++---- .../lib/auth/PermissionsHandler.scala | 6 ++-- .../ApiKeyAuthenticationProvider.scala | 4 +-- .../auth/provider/AuthenticationStatus.scala | 6 ++-- .../auth/PandaAuthenticationProvider.scala | 8 ++--- .../lib/play/GridComponents.scala | 2 +- .../lib/auth/AuthenticationTest.scala | 30 +++++++++---------- dev/script/generate-config/service-config.js | 2 +- media-api/app/controllers/MediaApi.scala | 4 +-- 11 files changed, 44 insertions(+), 41 deletions(-) diff --git a/auth/app/auth/AuthController.scala b/auth/app/auth/AuthController.scala index a9cc4ec623..dc10bde039 100644 --- a/auth/app/auth/AuthController.scala +++ b/auth/app/auth/AuthController.scala @@ -3,7 +3,7 @@ package auth import java.net.URI import com.gu.mediaservice.lib.argo.ArgoHelpers import com.gu.mediaservice.lib.argo.model.Link -import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, GridUser} +import com.gu.mediaservice.lib.auth.Authentication.{MachinePrincipal, UserPrincipal} import com.gu.mediaservice.lib.auth.provider.AuthenticationProviders import com.gu.mediaservice.lib.auth.{Authentication, Permissions, PermissionsHandler} import play.api.libs.json.Json @@ -34,7 +34,7 @@ class AuthController(auth: Authentication, providers: AuthenticationProviders, v def session = auth { request => val showPaid = hasPermission(request.user, Permissions.ShowPaid) request.user match { - case GridUser(firstName, lastName, email, _) => + case UserPrincipal(firstName, lastName, email, _) => respond( Json.obj("user" -> @@ -50,7 +50,7 @@ class AuthController(auth: Authentication, providers: AuthenticationProviders, v ) ) ) - case ApiKeyAccessor(accessor, _) => respond( + case MachinePrincipal(accessor, _) => respond( Json.obj("api-key" -> Json.obj( "name" -> accessor.identity, diff --git a/common-lib/src/main/resources/application.conf b/common-lib/src/main/resources/application.conf index 169abd3fc9..c4bf2d7e08 100644 --- a/common-lib/src/main/resources/application.conf +++ b/common-lib/src/main/resources/application.conf @@ -25,7 +25,7 @@ image.processors = [ ] authentication.providers { - api { + machine { className = "com.gu.mediaservice.lib.auth.provider.ApiKeyAuthenticationProvider" config { # authKeyStoreBucket = diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala index 7d48e760bd..08ba775bcd 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala @@ -2,7 +2,7 @@ package com.gu.mediaservice.lib.auth import com.gu.mediaservice.lib.argo.ArgoHelpers import com.gu.mediaservice.lib.argo.model.Link -import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, GridUser, OnBehalfOfPrincipal, Principal} +import com.gu.mediaservice.lib.auth.Authentication.{MachinePrincipal, UserPrincipal, OnBehalfOfPrincipal, Principal} import com.gu.mediaservice.lib.auth.provider._ import com.gu.mediaservice.lib.config.CommonConfig import play.api.libs.typedmap.TypedMap @@ -37,7 +37,7 @@ class Authentication(config: CommonConfig, Future.successful(respondError(Forbidden, "principal-not-authorised", "Principal not authorised", loginLinks)) } - def expired(user: GridUser): Future[Result] = { + def expired(user: UserPrincipal): Future[Result] = { logger.info(s"User token expired for ${user.email}, return 419") Future.successful(respondError(new Status(419), errorKey = "authentication-expired", errorMessage = "User authentication token has expired", loginLinks)) } @@ -75,8 +75,8 @@ class Authentication(config: CommonConfig, def getOnBehalfOfPrincipal(principal: Principal): OnBehalfOfPrincipal = { val provider: AuthenticationProvider = principal match { - case _:ApiKeyAccessor => providers.apiProvider - case _:GridUser => providers.userProvider + case _:MachinePrincipal => providers.apiProvider + case _:UserPrincipal => providers.userProvider } val maybeEnrichFn: Either[String, WSRequest => WSRequest] = provider.onBehalfOf(principal) maybeEnrichFn.fold(error => throw new IllegalStateException(error), identity) @@ -88,10 +88,13 @@ object Authentication { def accessor: ApiAccessor def attributes: TypedMap } - case class GridUser(firstName: String, lastName: String, email: String, attributes: TypedMap = TypedMap.empty) extends Principal { + /** A human user with a name */ + case class UserPrincipal(firstName: String, lastName: String, email: String, attributes: TypedMap = TypedMap.empty) extends Principal { def accessor: ApiAccessor = ApiAccessor(identity = email, tier = Internal) } - case class ApiKeyAccessor(accessor: ApiAccessor, attributes: TypedMap = TypedMap.empty) extends Principal + /** A machine user doing work automatically for its human programmers */ + case class MachinePrincipal(accessor: ApiAccessor, attributes: TypedMap = TypedMap.empty) extends Principal + type Request[A] = AuthenticatedRequest[A, Principal] type OnBehalfOfPrincipal = WSRequest => WSRequest diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/PermissionsHandler.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/PermissionsHandler.scala index 02e1c13380..e81de454dc 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/PermissionsHandler.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/PermissionsHandler.scala @@ -1,6 +1,6 @@ package com.gu.mediaservice.lib.auth -import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, GridUser, Principal} +import com.gu.mediaservice.lib.auth.Authentication.{MachinePrincipal, UserPrincipal, Principal} import com.gu.mediaservice.lib.aws.S3Ops import com.gu.mediaservice.lib.config.CommonConfig import com.gu.permissions._ @@ -30,9 +30,9 @@ trait PermissionsHandler { def hasPermission(user: Principal, permission: PermissionDefinition): Boolean = { user match { - case GridUser(_, _, email, _) => permissions.hasPermission(permission, email) + case UserPrincipal(_, _, email, _) => permissions.hasPermission(permission, email) // think about only allowing certain services i.e. on `service.name`? - case service: ApiKeyAccessor if service.accessor.tier == Internal => true + case service: MachinePrincipal if service.accessor.tier == Internal => true case _ => false } } diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala index bf39d24a80..94b6537015 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala @@ -1,5 +1,5 @@ package com.gu.mediaservice.lib.auth.provider -import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, Principal} +import com.gu.mediaservice.lib.auth.Authentication.{MachinePrincipal, Principal} import com.gu.mediaservice.lib.auth.{ApiAccessor, KeyStore} import com.typesafe.scalalogging.StrictLogging import play.api.Configuration @@ -49,7 +49,7 @@ class ApiKeyAuthenticationProvider(configuration: Configuration, resources: Auth if (ApiAccessor.hasAccess(apiKey, request, resources.commonConfig.services)) { // valid api key which has access // store the header that was used in the attributes map of the principal for use in onBehalfOf calls - val accessor = ApiKeyAccessor(apiKey, TypedMap(ApiKeyHeader -> (ApiKeyAuthenticationProvider.apiKeyHeaderName -> key))) + val accessor = MachinePrincipal(apiKey, TypedMap(ApiKeyHeader -> (ApiKeyAuthenticationProvider.apiKeyHeaderName -> key))) logger.info(s"Using api key with name ${apiKey.identity} and tier ${apiKey.tier}", apiKey) Authenticated(accessor) } else { diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala index 5b0427baae..bf2761f5b7 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala @@ -1,15 +1,15 @@ package com.gu.mediaservice.lib.auth.provider -import com.gu.mediaservice.lib.auth.Authentication.{GridUser, Principal} +import com.gu.mediaservice.lib.auth.Authentication.{UserPrincipal, Principal} // statuses that directly extend this are for users only /** Status of a client's authentication */ sealed trait AuthenticationStatus /** User authentication is valid but expired */ -case class Expired(authedUser: GridUser) extends AuthenticationStatus +case class Expired(authedUser: UserPrincipal) extends AuthenticationStatus /** User authentication is valid and expired but the expiry is within a grace period */ -case class GracePeriod(authedUser: GridUser) extends AuthenticationStatus +case class GracePeriod(authedUser: UserPrincipal) extends AuthenticationStatus // statuses that extend this can be used by both users and machines /** Status of an API client's authentication */ diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala index 4372ec6c03..6ad7168ed8 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala @@ -2,7 +2,7 @@ package com.gu.mediaservice.lib.guardian.auth import com.gu.mediaservice.lib.argo.ArgoHelpers import com.gu.mediaservice.lib.argo.model.Link -import com.gu.mediaservice.lib.auth.Authentication.{GridUser, Principal} +import com.gu.mediaservice.lib.auth.Authentication.{UserPrincipal, Principal} import com.gu.mediaservice.lib.auth.provider.AuthenticationProvider.RedirectUri import com.gu.mediaservice.lib.auth.provider._ import com.gu.mediaservice.lib.aws.S3Ops @@ -64,7 +64,7 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources, pr val maybePrincipal = authenticateRequest(requestHeader) match { case Expired(principal) => Some(principal) case GracePeriod(principal) => Some(principal) - case Authenticated(principal: GridUser) => Some(principal) + case Authenticated(principal: UserPrincipal) => Some(principal) case _ => None } val email = maybePrincipal.map(_.email) @@ -127,10 +127,10 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources, pr } } - private def gridUserFrom(pandaUser: User, request: RequestHeader): GridUser = { + private def gridUserFrom(pandaUser: User, request: RequestHeader): UserPrincipal = { val maybePandaCookie: Option[TypedEntry[Cookie]] = request.cookies.get(panDomainSettings.settings.cookieSettings.cookieName).map(TypedEntry[Cookie](PandaCookieKey, _)) val attributes = TypedMap.empty + (maybePandaCookie.toSeq:_*) - GridUser( + UserPrincipal( firstName = pandaUser.firstName, lastName = pandaUser.lastName, email = pandaUser.email, diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala index e6044af614..6f8c775487 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/play/GridComponents.scala @@ -52,7 +52,7 @@ abstract class GridComponents[Config <: CommonConfig](context: Context, val load val providers: AuthenticationProviders = AuthenticationProviders( userProvider = config.configuration.get[UserAuthenticationProvider]("authentication.providers.user")(UserAuthenticationProviderLoader.singletonConfigLoader(authProviderResources)), - apiProvider = config.configuration.get[MachineAuthenticationProvider]("authentication.providers.api")(ApiAuthenticationProviderLoader.singletonConfigLoader(authProviderResources)) + apiProvider = config.configuration.get[MachineAuthenticationProvider]("authentication.providers.machine")(ApiAuthenticationProviderLoader.singletonConfigLoader(authProviderResources)) ) providers.userProvider.initialise() applicationLifecycle.addStopHook(() => providers.userProvider.shutdown()) diff --git a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala index fccfaf6a33..0a22f651be 100644 --- a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala +++ b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala @@ -2,7 +2,7 @@ package com.gu.mediaservice.lib.auth import akka.actor.ActorSystem import akka.stream.ActorMaterializer -import com.gu.mediaservice.lib.auth.Authentication.{ApiKeyAccessor, GridUser, OnBehalfOfPrincipal} +import com.gu.mediaservice.lib.auth.Authentication.{MachinePrincipal, UserPrincipal, OnBehalfOfPrincipal} import com.gu.mediaservice.lib.auth.provider.AuthenticationProvider.RedirectUri import com.gu.mediaservice.lib.auth.provider._ import com.gu.mediaservice.lib.config.{CommonConfig, TestProvider} @@ -32,7 +32,7 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w private val COOKIE_NAME = "TestGridAuth" private val HEADER_NAME = "X-TestMachine-Auth" private case class AuthToken(firstName: String, lastName: String, email: String, expired: Boolean, veryExpired: Boolean) { - def user: GridUser = GridUser(firstName, lastName, email) + def user: UserPrincipal = UserPrincipal(firstName, lastName, email) } private implicit val cookieFormats: Format[AuthToken] = Json.format[AuthToken] private def makeCookie(firstName: String = "Test", lastName: String = "User", email: String = "test@user", expired: Boolean = false, veryExpired: Boolean = false): Cookie = { @@ -89,7 +89,7 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w request.headers.get(HEADER_NAME) match { case None => NotAuthenticated case Some(key) if key.startsWith("key-") && key.endsWith("-blocked") => NotAuthorised(s"$key is blocked") - case Some(key) if key.startsWith("key-") => Authenticated(ApiKeyAccessor(ApiAccessor(key, Internal))) + case Some(key) if key.startsWith("key-") => Authenticated(MachinePrincipal(ApiAccessor(key, Internal))) case Some(_) => Invalid("Key doesn't start with 'key-'") } } @@ -117,12 +117,12 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w "should return user when valid" in { val request = FakeRequest().withCookies(makeCookie()) val authStatus = auth.authenticationStatus(request) - authStatus.right.value shouldBe GridUser("Test", "User", "test@user") + authStatus.right.value shouldBe UserPrincipal("Test", "User", "test@user") } "should return user when expired but in grace period" in { val request = FakeRequest().withCookies(makeCookie(expired = true)) val authStatus = auth.authenticationStatus(request) - authStatus.right.value shouldBe GridUser("Test", "User", "test@user") + authStatus.right.value shouldBe UserPrincipal("Test", "User", "test@user") } "should return 419 when expired" in { val request = FakeRequest().withCookies(makeCookie(veryExpired = true)) @@ -143,7 +143,7 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w "should authenticate with an API key" in { val request = FakeRequest().withHeaders(HEADER_NAME -> "key-client") val authStatus = auth.authenticationStatus(request) - authStatus.right.value shouldBe ApiKeyAccessor(ApiAccessor("key-client", Internal)) + authStatus.right.value shouldBe MachinePrincipal(ApiAccessor("key-client", Internal)) } "should return unauthorised when the API key is garbage" in { val request = FakeRequest().withHeaders(HEADER_NAME -> "garbage") @@ -164,7 +164,7 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w "should prioritise machine authentication over user authentication" in { val request = FakeRequest().withCookies(makeCookie()).withHeaders(HEADER_NAME -> "key-client") val authStatus = auth.authenticationStatus(request) - authStatus.right.value shouldBe ApiKeyAccessor(ApiAccessor("key-client", Internal)) + authStatus.right.value shouldBe MachinePrincipal(ApiAccessor("key-client", Internal)) } } @@ -178,22 +178,22 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w override def sendForAuthenticationCallback: Option[(RequestHeader, Option[RedirectUri]) => Future[Result]] = ??? override def flushToken: Option[(RequestHeader, Result) => Result] = ??? override def onBehalfOf(request: Authentication.Principal): Either[String, WSRequest => WSRequest] = request match { - case GridUser(_,_,_,attributes) if attributes.contains(CookieKey) => Right(req => req.addCookies(DefaultWSCookie(COOKIE_NAME, attributes.get(CookieKey).get.value))) - case GridUser(_, _, email, _) => Left(s"Unable to build onBehalfOf function for $email") + case UserPrincipal(_,_,_,attributes) if attributes.contains(CookieKey) => Right(req => req.addCookies(DefaultWSCookie(COOKIE_NAME, attributes.get(CookieKey).get.value))) + case UserPrincipal(_, _, email, _) => Left(s"Unable to build onBehalfOf function for $email") } }, new MachineAuthenticationProvider { override def authenticateRequest(request: RequestHeader): ApiAuthenticationStatus = ??? override def onBehalfOf(request: Authentication.Principal): Either[String, WSRequest => WSRequest] = request match { - case ApiKeyAccessor(_, attributes) if attributes.contains(HeaderKey) => Right(req => req.addHttpHeaders(attributes.get(HeaderKey).get)) - case ApiKeyAccessor(ApiAccessor(identity, _), _) => Left(s"Unable to build onBehalfOf function for $identity") + case MachinePrincipal(_, attributes) if attributes.contains(HeaderKey) => Right(req => req.addHttpHeaders(attributes.get(HeaderKey).get)) + case MachinePrincipal(ApiAccessor(identity, _), _) => Left(s"Unable to build onBehalfOf function for $identity") } } ) val auth: Authentication = makeAuthenticationInstance(testProviders) "return function for user principal" in { - val testUser = GridUser("Test", "User", "test@user", TypedMap(CookieKey -> Cookie(COOKIE_NAME, "this is my cookie value"))) + val testUser = UserPrincipal("Test", "User", "test@user", TypedMap(CookieKey -> Cookie(COOKIE_NAME, "this is my cookie value"))) val onBehalfOfFn: OnBehalfOfPrincipal = auth.getOnBehalfOfPrincipal(testUser) WsTestClient.withClient{ client => val req = client.url("https://example.com") @@ -206,14 +206,14 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w } "fail to get function for user principal if the user doesn't have the cookie" in { - val testUser = GridUser("Test", "User", "test@user") + val testUser = UserPrincipal("Test", "User", "test@user") the [IllegalStateException] thrownBy { val onBehalfOfFn: OnBehalfOfPrincipal = auth.getOnBehalfOfPrincipal(testUser) } should have message "Unable to build onBehalfOf function for test@user" } "return function for machine principal" in { - val apiAccessor = ApiKeyAccessor(ApiAccessor("my-client-id", Internal), TypedMap(HeaderKey -> (HEADER_NAME -> "my-client-id-key"))) + val apiAccessor = MachinePrincipal(ApiAccessor("my-client-id", Internal), TypedMap(HeaderKey -> (HEADER_NAME -> "my-client-id-key"))) val onBehalfOfFn: OnBehalfOfPrincipal = auth.getOnBehalfOfPrincipal(apiAccessor) WsTestClient.withClient{ client => val req = client.url("https://example.com") @@ -225,7 +225,7 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w } "fail to get function for user principal if the api accessor doesn't have the header" in { - val apiAccessor = ApiKeyAccessor(ApiAccessor("my-client-id", Internal)) + val apiAccessor = MachinePrincipal(ApiAccessor("my-client-id", Internal)) the [IllegalStateException] thrownBy { val onBehalfOfFn: OnBehalfOfPrincipal = auth.getOnBehalfOfPrincipal(apiAccessor) } should have message "Unable to build onBehalfOf function for my-client-id" diff --git a/dev/script/generate-config/service-config.js b/dev/script/generate-config/service-config.js index 9bbba7518b..d20da2265d 100644 --- a/dev/script/generate-config/service-config.js +++ b/dev/script/generate-config/service-config.js @@ -11,7 +11,7 @@ function getCorsAllowedOriginString(config) { function getCommonConfig(config) { return `domain.root="${config.DOMAIN}" - |authentication.providers.api.config.authKeyStoreBucket="${config.coreStackProps.KeyBucket}" + |authentication.providers.machine.config.authKeyStoreBucket="${config.coreStackProps.KeyBucket}" |thrall.kinesis.stream.name="${config.coreStackProps.ThrallMessageStream}" |thrall.kinesis.lowPriorityStream.name="${config.coreStackProps.ThrallLowPriorityMessageStream}" |`; diff --git a/media-api/app/controllers/MediaApi.scala b/media-api/app/controllers/MediaApi.scala index c0e4de4b6c..8fb85f6a85 100644 --- a/media-api/app/controllers/MediaApi.scala +++ b/media-api/app/controllers/MediaApi.scala @@ -93,13 +93,13 @@ class MediaApi( permission: PermissionDefinition ) = { request.user match { - case user: GridUser => + case user: UserPrincipal => if (user.email.toLowerCase == image.uploadedBy.toLowerCase) { true } else { hasPermission(user, permission) } - case service: ApiKeyAccessor if service.accessor.tier == Internal => true + case service: MachinePrincipal if service.accessor.tier == Internal => true case _ => false } } From 2841623c69a4023b3dd13813dafca3be03da29bb Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Mon, 18 Jan 2021 14:14:32 +0000 Subject: [PATCH 24/25] Remove GracePeriod auth status from API The idea of a grace period is specific to Pan Domain Auth so doesn't need to leak into the API surface of the general case. In any case it wasn't actually used. --- .../scala/com/gu/mediaservice/lib/auth/Authentication.scala | 1 - .../mediaservice/lib/auth/provider/AuthenticationStatus.scala | 2 -- .../lib/guardian/auth/PandaAuthenticationProvider.scala | 3 +-- .../com/gu/mediaservice/lib/auth/AuthenticationTest.scala | 1 - 4 files changed, 1 insertion(+), 6 deletions(-) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala index 08ba775bcd..57adcc2b7c 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/Authentication.scala @@ -56,7 +56,6 @@ class Authentication(config: CommonConfig, providers.userProvider.authenticateRequest(requestHeader) match { case NotAuthenticated => Left(unauthorised("Not authenticated")) case Expired(principal) => Left(expired(principal)) - case GracePeriod(authedUser) => Right(authedUser) case Authenticated(authedUser) => Right(authedUser) case Invalid(message, throwable) => Left(unauthorised(message, throwable).map(flushToken)) case NotAuthorised(message) => Left(forbidden(s"Principal not authorised: $message")) diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala index bf2761f5b7..581eebba17 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/AuthenticationStatus.scala @@ -8,8 +8,6 @@ sealed trait AuthenticationStatus /** User authentication is valid but expired */ case class Expired(authedUser: UserPrincipal) extends AuthenticationStatus -/** User authentication is valid and expired but the expiry is within a grace period */ -case class GracePeriod(authedUser: UserPrincipal) extends AuthenticationStatus // statuses that extend this can be used by both users and machines /** Status of an API client's authentication */ diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala index 6ad7168ed8..891eefd02f 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/guardian/auth/PandaAuthenticationProvider.scala @@ -48,7 +48,7 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources, pr case PandaNotAuthenticated => NotAuthenticated case PandaInvalidCookie(e) => Invalid("error checking user's auth, clear cookie and re-auth", Some(e)) case PandaExpired(authedUser) => Expired(gridUserFrom(authedUser.user, request)) - case PandaGracePeriod(authedUser) => GracePeriod(gridUserFrom(authedUser.user, request)) + case PandaGracePeriod(authedUser) => Authenticated(gridUserFrom(authedUser.user, request)) case PandaNotAuthorised(authedUser) => NotAuthorised(s"${authedUser.user.email} not authorised to use application") case PandaAuthenticated(authedUser) => Authenticated(gridUserFrom(authedUser.user, request)) } @@ -63,7 +63,6 @@ class PandaAuthenticationProvider(resources: AuthenticationProviderResources, pr override def sendForAuthentication: Option[RequestHeader => Future[Result]] = Some({ requestHeader: RequestHeader => val maybePrincipal = authenticateRequest(requestHeader) match { case Expired(principal) => Some(principal) - case GracePeriod(principal) => Some(principal) case Authenticated(principal: UserPrincipal) => Some(principal) case _ => None } diff --git a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala index 0a22f651be..860b848d70 100644 --- a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala +++ b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala @@ -69,7 +69,6 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w case Some(cookie) => parseCookie(cookie) match { case None => Invalid("Token not valid") - case Some(token@AuthToken(_, _, _, true, false)) => GracePeriod(token.user) case Some(token@AuthToken(_, _, _, _, true)) => Expired(token.user) case Some(token) if token.email == "test@user" => Authenticated(token.user) case Some(token) => NotAuthorised(s"${token.email} not authorised") From 98db717f807f8f46005a335093d95da743116fe4 Mon Sep 17 00:00:00 2001 From: Simon Hildrew Date: Mon, 18 Jan 2021 15:14:54 +0000 Subject: [PATCH 25/25] Add a test suite for default api key auth provider --- .../mediaservice/lib/auth/ApiAccessor.scala | 2 - .../ApiKeyAuthenticationProvider.scala | 7 +- .../src/test/resources/application.conf | 5 ++ .../ApiKeyAuthenticationProviderTest.scala | 75 +++++++++++++++++++ .../lib/auth/AuthenticationTest.scala | 12 +-- 5 files changed, 86 insertions(+), 15 deletions(-) create mode 100644 common-lib/src/test/resources/application.conf create mode 100644 common-lib/src/test/scala/com/gu/mediaservice/lib/auth/ApiKeyAuthenticationProviderTest.scala diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/ApiAccessor.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/ApiAccessor.scala index 54e489af8c..3e1924451d 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/ApiAccessor.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/ApiAccessor.scala @@ -19,8 +19,6 @@ object Tier { case class ApiAccessor(identity: String, tier: Tier) object ApiAccessor extends ArgoHelpers { - def unauthorizedResult: Result = respondError(Forbidden, "forbidden", "Unauthorized - the API key is not allowed to perform this operation", List.empty) - def apply(content: String): ApiAccessor = { val rows = content.split("\n") val name = rows.headOption.getOrElse("") diff --git a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala index 94b6537015..1c71aca703 100644 --- a/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala +++ b/common-lib/src/main/scala/com/gu/mediaservice/lib/auth/provider/ApiKeyAuthenticationProvider.scala @@ -10,12 +10,11 @@ import play.api.mvc.RequestHeader import scala.concurrent.{ExecutionContext, Future} object ApiKeyAuthenticationProvider { + val ApiKeyHeader: TypedKey[(String, String)] = TypedKey[(String, String)]("ApiKeyHeader") val apiKeyHeaderName = "X-Gu-Media-Key" } class ApiKeyAuthenticationProvider(configuration: Configuration, resources: AuthenticationProviderResources) extends MachineAuthenticationProvider with StrictLogging { - val ApiKeyHeader: TypedKey[(String, String)] = TypedKey[(String, String)]("ApiKeyHeader") - implicit val executionContext: ExecutionContext = resources.controllerComponents.executionContext var keyStorePlaceholder: Option[KeyStore] = _ @@ -49,7 +48,7 @@ class ApiKeyAuthenticationProvider(configuration: Configuration, resources: Auth if (ApiAccessor.hasAccess(apiKey, request, resources.commonConfig.services)) { // valid api key which has access // store the header that was used in the attributes map of the principal for use in onBehalfOf calls - val accessor = MachinePrincipal(apiKey, TypedMap(ApiKeyHeader -> (ApiKeyAuthenticationProvider.apiKeyHeaderName -> key))) + val accessor = MachinePrincipal(apiKey, TypedMap(ApiKeyAuthenticationProvider.ApiKeyHeader -> (ApiKeyAuthenticationProvider.apiKeyHeaderName -> key))) logger.info(s"Using api key with name ${apiKey.identity} and tier ${apiKey.tier}", apiKey) Authenticated(accessor) } else { @@ -65,7 +64,7 @@ class ApiKeyAuthenticationProvider(configuration: Configuration, resources: Auth } override def onBehalfOf(principal: Principal): Either[String, WSRequest => WSRequest] = { - principal.attributes.get(ApiKeyHeader) match { + principal.attributes.get(ApiKeyAuthenticationProvider.ApiKeyHeader) match { case Some(apiKeyHeaderTuple) => Right { wsRequest: WSRequest => wsRequest.addHttpHeaders(apiKeyHeaderTuple) } diff --git a/common-lib/src/test/resources/application.conf b/common-lib/src/test/resources/application.conf new file mode 100644 index 0000000000..930c6a00d5 --- /dev/null +++ b/common-lib/src/test/resources/application.conf @@ -0,0 +1,5 @@ +grid.stage: "TEST" +grid.appName: "test" +thrall.kinesis.stream.name: "not-used" +thrall.kinesis.lowPriorityStream.name: "not-used" +domain.root: "notused.example.com" diff --git a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/ApiKeyAuthenticationProviderTest.scala b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/ApiKeyAuthenticationProviderTest.scala new file mode 100644 index 0000000000..587c2efdbb --- /dev/null +++ b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/ApiKeyAuthenticationProviderTest.scala @@ -0,0 +1,75 @@ +package com.gu.mediaservice.lib.auth + +import akka.actor.ActorSystem +import com.gu.mediaservice.lib.auth.Authentication.MachinePrincipal +import com.gu.mediaservice.lib.auth.provider.{ApiKeyAuthenticationProvider, Authenticated, AuthenticationProviderResources, Invalid, NotAuthenticated, NotAuthorised} +import com.gu.mediaservice.lib.config.CommonConfig +import org.scalatest.Inside.inside +import org.scalatest.{AsyncFreeSpec, BeforeAndAfterAll, EitherValues, Matchers} +import play.api.mvc.DefaultControllerComponents +import play.api.test.{FakeRequest, WsTestClient} +import play.api.{Configuration, Environment} + +import scala.concurrent.ExecutionContext.global +import scala.concurrent.Future + +//noinspection NotImplementedCode,SpellCheckingInspection +class ApiKeyAuthenticationProviderTest extends AsyncFreeSpec with Matchers with EitherValues with BeforeAndAfterAll { + + private val actorSystem: ActorSystem = ActorSystem() + private val wsClient = new WsTestClient.InternalWSClient("https", 443) + private val config = new CommonConfig(Configuration.load(Environment.simple())) {} + private val providerConfig = Configuration.empty + private val controllerComponents: DefaultControllerComponents = DefaultControllerComponents(null, null, null, null, null, global) + private val resources = AuthenticationProviderResources(config, actorSystem, wsClient, controllerComponents) + private val provider = new ApiKeyAuthenticationProvider(providerConfig, resources) { + override def initialise(): Unit = { /* do nothing */ } + + override def shutdown(): Future[Unit] = { /* do nothing */ + Future.successful(()) + } + + override def keyStore: KeyStore = new KeyStore("not-used", resources.commonConfig) { + override def lookupIdentity(key: String): Option[ApiAccessor] = { + key match { + case "key-chuckle" => Some(ApiAccessor("brothers", Internal)) + case "key-limited" => Some(ApiAccessor("locked-down", ReadOnly)) + case _ => None + } + } + } + } + + "requestAuthentication" - { + "should return Authenticated if the key is valid" in { + val testHeader = ApiKeyAuthenticationProvider.apiKeyHeaderName -> "key-chuckle" + val status = provider.authenticateRequest(FakeRequest().withHeaders(testHeader)) + inside(status) { + case Authenticated(MachinePrincipal(apiAccessor, attributes)) => + apiAccessor shouldBe ApiAccessor("brothers", Internal) + attributes.contains(ApiKeyAuthenticationProvider.ApiKeyHeader) shouldBe true + attributes.get(ApiKeyAuthenticationProvider.ApiKeyHeader) shouldBe Some(testHeader) + } + } + "should return NotAuthenticated if the header is missing" in { + val status = provider.authenticateRequest(FakeRequest()) + status shouldBe NotAuthenticated + } + "should return Invalid if the key is invalid" in { + val testHeader = ApiKeyAuthenticationProvider.apiKeyHeaderName -> "key-banana" + val status = provider.authenticateRequest(FakeRequest().withHeaders(testHeader)) + inside(status) { + case Invalid(message, _) => + message shouldBe "API key not valid" + } + } + "should return NotAuthorised if the key doesn't have enough permissions" in { + val testHeader = ApiKeyAuthenticationProvider.apiKeyHeaderName -> "key-limited" + val status = provider.authenticateRequest(FakeRequest().withHeaders(testHeader).withMethod("POST")) + inside(status) { + case NotAuthorised(message) => + message shouldBe "API key locked-down valid but not authorised for this request" + } + } + } +} diff --git a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala index 860b848d70..ddc0c1a305 100644 --- a/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala +++ b/common-lib/src/test/scala/com/gu/mediaservice/lib/auth/AuthenticationTest.scala @@ -2,13 +2,13 @@ package com.gu.mediaservice.lib.auth import akka.actor.ActorSystem import akka.stream.ActorMaterializer -import com.gu.mediaservice.lib.auth.Authentication.{MachinePrincipal, UserPrincipal, OnBehalfOfPrincipal} +import com.gu.mediaservice.lib.auth.Authentication.{MachinePrincipal, OnBehalfOfPrincipal, UserPrincipal} import com.gu.mediaservice.lib.auth.provider.AuthenticationProvider.RedirectUri import com.gu.mediaservice.lib.auth.provider._ import com.gu.mediaservice.lib.config.{CommonConfig, TestProvider} import org.scalatest.{AsyncFreeSpec, BeforeAndAfterAll, EitherValues, Matchers} import org.scalatestplus.play.PlaySpec -import play.api.Configuration +import play.api.{Configuration, Environment} import play.api.http.Status import play.api.libs.json.{Format, Json} import play.api.libs.typedmap.{TypedKey, TypedMap} @@ -45,13 +45,7 @@ class AuthenticationTest extends AsyncFreeSpec with Matchers with EitherValues w } def makeAuthenticationInstance(testProviders: AuthenticationProviders): Authentication = { - val config = new CommonConfig(Configuration.from(Map( - "grid.stage" -> "TEST", - "grid.appName" -> "test", - "thrall.kinesis.stream.name" -> "not-used", - "thrall.kinesis.lowPriorityStream.name" -> "not-used", - "domain.root" -> "notused.example.com" - ))) {} + val config = new CommonConfig(Configuration.load(Environment.simple())) {} new Authentication( config = config, providers = testProviders,