From a682f9ba98dbcc413fe819c3439e0ac361e39691 Mon Sep 17 00:00:00 2001 From: Roberto Tyley Date: Wed, 31 Jan 2024 14:47:20 +0000 Subject: [PATCH] Upgrade Editorial permissions library to latest version This upgrades the Media Atom Maker to use the latest version of the client for the Guardian's Editorial Permissions service - we need the latest version of the client to support the upgrade to Scala 2.13 in https://github.com/guardian/media-atom-maker/pull/1140 * Before: https://github.com/guardian/editorial-permissions-client/tree/v0.8 - supporting Scala 2.11 & 2.12 * After: https://github.com/guardian/permissions/tree/v2.15/client - supporting Scala 2.12 & 2.13 As you can see, the permissions client has moved repositories, to the main `permissions` repo - this happened in July 2018 with PR https://github.com/guardian/permissions/pull/103. This PR is also important because it removed use of `Future` from the permissions client API - as Michael Barton explained, permission lookups should be mostly instantaneous because they now come from an in-memory cache. The removal of `Future` means that this commit, upgrading permissions in Media Atom Maker, needs to remove several for-comprehensions/map-statements. The diff on these can look quite big, but they look smaller if whitespace changes are ignored. I have taken the opportunity to do small refactors to improve code clarity and remove repetition. # Permission to modify Privacy Status of a published Media Atom In particular, the code around modifying Privacy Status of a Media Atom _had_ to be changed because it involved removing `Future`, but I also included refactoring to make the code clearer. When reviewing this, you may want to look at the original PRs that introduced this logic: * https://github.com/guardian/media-atom-maker/pull/607 - introduced the concept of each of our YouTube channels having a different set of available PrivacyStatus (Private, Unlisted, Public) values. * https://github.com/guardian/media-atom-maker/pull/694 - you can always upload as Public unless the channel is in the youtube.channels.unlisted config, in which case you need permission. This means we can give general users the ability to upload as Public on the culture channel and grant specific people access to make a public video on the main channel. * https://github.com/guardian/media-atom-maker/pull/789 - public video should *stay* public when a metadata change is made by someone who does not have permission to *make* a video public on that channel. * https://github.com/guardian/media-atom-maker/pull/791 - code shouldn't fail if the atom has not been published yet! --- app/controllers/AtomController.scala | 19 ++--- app/controllers/VideoUIApp.scala | 62 ++++++++-------- app/controllers/Youtube.scala | 19 ++--- app/di.scala | 2 +- app/model/commands/PublishAtomCommand.scala | 74 ++++++++----------- build.sbt | 4 +- .../main/scala/com/gu/media/Permissions.scala | 68 +++++++---------- 7 files changed, 102 insertions(+), 146 deletions(-) diff --git a/app/controllers/AtomController.scala b/app/controllers/AtomController.scala index 8d35005d4..87d3f611b 100644 --- a/app/controllers/AtomController.scala +++ b/app/controllers/AtomController.scala @@ -1,6 +1,5 @@ package controllers -import com.gu.editorial.permissions.client.{Permission, PermissionGranted, PermissionsUser} import com.gu.media.Permissions._ import com.gu.media.{MediaAtomMakerPermissionsProvider, Permissions} import com.gu.pandahmac.HMACAuthActions @@ -10,6 +9,7 @@ import data.UnpackedDataStores import play.api.libs.json.{JsObject, JsString} import play.api.mvc._ import com.gu.media.util.ThriftUtil._ +import com.gu.permissions.PermissionDefinition import scala.concurrent.{ExecutionContext, Future} @@ -41,9 +41,7 @@ trait AtomController extends BaseController with UnpackedDataStores { object LookupPermissions extends ActionBuilder[UploadUserRequest, AnyContent] { override def invokeBlock[A](request: Request[A], block: UploadUserRequest[A] => Future[Result]): Future[Result] = { APIAuthAction.invokeBlock(request, { req: UserRequest[A] => - permissions.getAll(req.user).flatMap { p => - block(new UploadUserRequest(req.user, request, p)) - } + block(UploadUserRequest(req.user, request, permissions.getAll(req.user))) }) } @@ -52,18 +50,11 @@ trait AtomController extends BaseController with UnpackedDataStores { override protected def executionContext: ExecutionContext = controllerComponents.executionContext } - class PermissionedAction(permission: Permission) extends ActionBuilder[UserRequest, AnyContent] { + class PermissionedAction(permission: PermissionDefinition) extends ActionBuilder[UserRequest, AnyContent] { override def invokeBlock[A](request: Request[A], block: UserRequest[A] => Future[Result]): Future[Result] = { APIAuthAction.invokeBlock(request, { req: UserRequest[A] => - - permissions.get(permission)(PermissionsUser(req.user.email)).flatMap { - case PermissionGranted => - block(req) - - case _ => - Future.successful(Unauthorized(s"User ${req.user.email} is not authorised for permission ${permission.name}")) - - } + if (permissions.hasPermission(permission, req.user)) block(req) + else Future.successful(Unauthorized(s"User ${req.user.email} is not authorised for permission ${permission.name}")) }) } diff --git a/app/controllers/VideoUIApp.scala b/app/controllers/VideoUIApp.scala index 6edf45a1a..2fefae6f3 100644 --- a/app/controllers/VideoUIApp.scala +++ b/app/controllers/VideoUIApp.scala @@ -1,7 +1,6 @@ package controllers -import com.gu.editorial.permissions.client.{Permission, PermissionDenied, PermissionsUser} import com.gu.media.MediaAtomMakerPermissionsProvider import com.gu.media.logging.Logging import com.gu.media.youtube.YouTubeAccess @@ -17,13 +16,13 @@ import views.html.helper.CSRF import scala.concurrent.ExecutionContext class VideoUIApp(val authActions: HMACAuthActions, conf: Configuration, awsConfig: AWSConfig, - permissions: MediaAtomMakerPermissionsProvider, youtube: YouTubeAccess, + permissionsProvider: MediaAtomMakerPermissionsProvider, youtube: YouTubeAccess, val controllerComponents: ControllerComponents) extends BaseController with Logging with TrainingMode { import authActions.AuthAction implicit lazy val executionContext: ExecutionContext = defaultExecutionContext - def index(id: String = ""): Action[AnyContent] = AuthAction.async { implicit req => + def index(id: String = ""): Action[AnyContent] = AuthAction { implicit req => val isTrainingMode = isInTrainingMode(req) val jsFileName = "video-ui/build/main.js" @@ -43,36 +42,35 @@ class VideoUIApp(val authActions: HMACAuthActions, conf: Configuration, awsConfi val composerUrl = awsConfig.composerUrl - permissions.getAll(req.user).map { permissions => - val clientConfig = ClientConfig( - presence = presenceConfig(req.user), - youtubeEmbedUrl = "https://www.youtube.com/embed/", - youtubeThumbnailUrl = "https://img.youtube.com/vi/", - reauthUrl = "/reauth", - gridUrl = awsConfig.gridUrl, - capiProxyUrl = "/support/previewCapi", - liveCapiProxyUrl = "/support/liveCapi", - composerUrl = composerUrl, - ravenUrl = conf.get[String]("raven.url"), - stage = conf.get[String]("stage"), - viewerUrl = awsConfig.viewerUrl, - permissions, - minDurationForAds = youtube.minDurationForAds, - isTrainingMode = isTrainingMode, - workflowUrl = awsConfig.workflowUrl, - targetingUrl = awsConfig.targetingUrl - ) + val permissions = permissionsProvider.getAll(req.user) + val clientConfig = ClientConfig( + presence = presenceConfig(req.user), + youtubeEmbedUrl = "https://www.youtube.com/embed/", + youtubeThumbnailUrl = "https://img.youtube.com/vi/", + reauthUrl = "/reauth", + gridUrl = awsConfig.gridUrl, + capiProxyUrl = "/support/previewCapi", + liveCapiProxyUrl = "/support/liveCapi", + composerUrl = composerUrl, + ravenUrl = conf.get[String]("raven.url"), + stage = conf.get[String]("stage"), + viewerUrl = awsConfig.viewerUrl, + permissions, + minDurationForAds = youtube.minDurationForAds, + isTrainingMode = isTrainingMode, + workflowUrl = awsConfig.workflowUrl, + targetingUrl = awsConfig.targetingUrl + ) - Ok(views.html.VideoUIApp.app( - title = "Media Atom Maker", - jsLocation, - presenceJsLocation = clientConfig.presence.map(_.jsLocation), - pinboardJsLocation = if(permissions.pinboard) awsConfig.pinboardLoaderUrl else None, - Json.toJson(clientConfig).toString(), - isHotReloading, - CSRF.getToken.value - )) - } + Ok(views.html.VideoUIApp.app( + title = "Media Atom Maker", + jsLocation, + presenceJsLocation = clientConfig.presence.map(_.jsLocation), + pinboardJsLocation = if(permissions.pinboard) awsConfig.pinboardLoaderUrl else None, + Json.toJson(clientConfig).toString(), + isHotReloading, + CSRF.getToken.value + )) } def training(inTraining: Boolean): Action[AnyContent] = AuthAction { req => diff --git a/app/controllers/Youtube.scala b/app/controllers/Youtube.scala index 9697d68ad..dfd07a160 100644 --- a/app/controllers/Youtube.scala +++ b/app/controllers/Youtube.scala @@ -10,7 +10,7 @@ import com.gu.media.MediaAtomMakerPermissionsProvider import scala.concurrent.ExecutionContext.Implicits.global -class Youtube (val authActions: HMACAuthActions, youtube: YouTube, permissions: MediaAtomMakerPermissionsProvider, val controllerComponents: ControllerComponents) +class Youtube (val authActions: HMACAuthActions, youtube: YouTube, permissionsProvider: MediaAtomMakerPermissionsProvider, val controllerComponents: ControllerComponents) extends BaseController with TrainingMode { import authActions.AuthAction @@ -18,21 +18,16 @@ class Youtube (val authActions: HMACAuthActions, youtube: YouTube, permissions: Ok(Json.toJson(youtube.categories)) } - def listChannels() = AuthAction.async { req => - val isTrainingMode = isInTrainingMode(req) + def listChannels() = AuthAction { req => val user = req.user - permissions.getStatusPermissions(user).map(permissions => { - val hasMakePublicPermission = permissions.setVideosOnAllChannelsPublic + val hasMakePublicPermission = permissionsProvider.getStatusPermissions(user).setVideosOnAllChannelsPublic - val channels = if (isTrainingMode) { - youtube.channelsWithData(hasMakePublicPermission).filter(c => youtube.trainingChannels.contains(c.id)) - } else { - youtube.channelsWithData(hasMakePublicPermission).filter(c => youtube.allChannels.contains(c.id)) - } + val requiredChannels = if (isInTrainingMode(req)) youtube.trainingChannels else youtube.allChannels - Ok(Json.toJson(channels)) - }) + val channels = youtube.channelsWithData(hasMakePublicPermission).filter(c => requiredChannels.contains(c.id)) + + Ok(Json.toJson(channels)) } def commercialVideoInfo(id: String) = AuthAction { diff --git a/app/di.scala b/app/di.scala index 5d85618e0..be414b635 100644 --- a/app/di.scala +++ b/app/di.scala @@ -73,7 +73,7 @@ class MediaAtomMaker(context: Context) private val capi = new Capi(config) private val stores = new DataStores(aws, capi) - private val permissions = new MediaAtomMakerPermissionsProvider(aws.stage, aws.credentials.instance) + private val permissions = new MediaAtomMakerPermissionsProvider(aws.stage, aws.region.getName, aws.credentials.instance) private val reindexer = buildReindexer() diff --git a/app/model/commands/PublishAtomCommand.scala b/app/model/commands/PublishAtomCommand.scala index 3bc36db3c..fe10c6192 100644 --- a/app/model/commands/PublishAtomCommand.scala +++ b/app/model/commands/PublishAtomCommand.scala @@ -24,7 +24,7 @@ case class PublishAtomCommand( youtube: YouTube, user: PandaUser, capi: Capi, - permissions: MediaAtomMakerPermissionsProvider, + permissionsProvider: MediaAtomMakerPermissionsProvider, awsConfig: AWSConfig, thumbnailGenerator: ThumbnailGenerator) extends Command with Logging { @@ -64,34 +64,31 @@ case class PublishAtomCommand( } case (_, _, _) => { previewAtom.getActiveYouTubeAsset() match { - case Some(asset) => { + case Some(asset) => val publishedAtom = getPublishedAtom() val adSettings = AdSettings(youtube.minDurationForAds, youtube.minDurationForMidroll, previewAtom) - val privacyStatus: Future[PrivacyStatus] = getPrivacyStatus(previewAtom, publishedAtom) - - privacyStatus.flatMap(status => { - val updatedPreviewAtom = if (publishedAtom.isDefined) { - previewAtom.copy( - blockAds = adSettings.blockAds, - privacyStatus = Some(status) - ) - } else { - // on first publish, set YouTube title and description to that of the Atom - // this is because there's no guarantee that the YouTube furniture gets subbed before publication and can result in draft furniture being used - previewAtom.copy( - blockAds = adSettings.blockAds, - privacyStatus = Some(status), - youtubeTitle = previewAtom.title, - youtubeDescription = YoutubeDescription.clean(previewAtom.description) - ) - } - - updateYouTube(publishedAtom, updatedPreviewAtom, asset).map(atomWithYoutubeUpdates => { - publish(atomWithYoutubeUpdates, user) - }) - }) - } + val status = getResultingPrivacyStatus(previewAtom, publishedAtom) + + val updatedPreviewAtom = if (publishedAtom.isDefined) { + previewAtom.copy( + blockAds = adSettings.blockAds, + privacyStatus = Some(status) + ) + } else { + // on first publish, set YouTube title and description to that of the Atom + // this is because there's no guarantee that the YouTube furniture gets subbed before publication and can result in draft furniture being used + previewAtom.copy( + blockAds = adSettings.blockAds, + privacyStatus = Some(status), + youtubeTitle = previewAtom.title, + youtubeDescription = YoutubeDescription.clean(previewAtom.description) + ) + } + + updateYouTube(publishedAtom, updatedPreviewAtom, asset).map { atomWithYoutubeUpdates => + publish(atomWithYoutubeUpdates, user) + } case _ => Future.successful(publish(previewAtom, user)) } } @@ -107,24 +104,17 @@ case class PublishAtomCommand( } } - private def getPrivacyStatus(previewAtom: MediaAtom, maybePublishedAtom: Option[MediaAtom]): Future[PrivacyStatus] = { - (previewAtom.channelId, maybePublishedAtom) match { - case (Some(channel), Some(publishedAtom)) if youtube.channelsRequiringPermission.contains(channel) => { - permissions.getStatusPermissions(user).map(permissions => { - val canChangeVideoStatus = permissions.setVideosOnAllChannelsPublic - - if (canChangeVideoStatus) { - previewAtom.privacyStatus.getOrElse(PrivacyStatus.Unlisted) - } else { - // don't have permission to change the status, use the published atom status - publishedAtom.privacyStatus.getOrElse(PrivacyStatus.Unlisted) - } - }) - } - case _ => Future.successful(previewAtom.privacyStatus.getOrElse(PrivacyStatus.Unlisted)) - } + private def getResultingPrivacyStatus(previewAtom: MediaAtom, maybePublishedAtom: Option[MediaAtom]): PrivacyStatus = { + val atomToTakePrivacyStatusFrom = + if (userCanMakeVideoPublic(previewAtom, user)) previewAtom else maybePublishedAtom.getOrElse(previewAtom) + + atomToTakePrivacyStatusFrom.privacyStatus.getOrElse(PrivacyStatus.Unlisted) } + private def userCanMakeVideoPublic(atom: MediaAtom, user: PandaUser): Boolean = + !atom.channelId.exists(youtube.channelsRequiringPermission) || + permissionsProvider.getStatusPermissions(user).setVideosOnAllChannelsPublic + private def publish(atom: MediaAtom, user: PandaUser): MediaAtom = { log.info(s"Publishing atom $id") diff --git a/build.sbt b/build.sbt index 88d0fd681..08d534aa8 100644 --- a/build.sbt +++ b/build.sbt @@ -26,8 +26,6 @@ val awsLambdaEventsVersion = "1.3.0" val logbackClassicVersion = "1.2.3" val logstashLogbackEncoderVersion = "4.8" -val permissionsClientVersion = "0.8" - val guavaVersion = "31.1-jre" val googleOauthVersion = "1.33.3" val googleHttpJacksonVersion = "1.41.7" @@ -114,7 +112,7 @@ lazy val common = (project in file("common")) "org.scalacheck" %% "scalacheck" % scalaCheckVersion % "test", // to match ScalaTest version "com.amazonaws" % "aws-java-sdk-sns" % awsVersion, "com.amazonaws" % "aws-java-sdk-sqs" % awsVersion, - "com.gu" %% "editorial-permissions-client" % permissionsClientVersion, + "com.gu" %% "editorial-permissions-client" % "2.15", "com.amazonaws" % "aws-java-sdk-stepfunctions" % awsVersion, "com.amazonaws" % "aws-java-sdk-ses" % awsVersion, "com.gu" %% "content-api-client-aws" % capiAwsVersion, diff --git a/common/src/main/scala/com/gu/media/Permissions.scala b/common/src/main/scala/com/gu/media/Permissions.scala index 4eb576149..4c506c95f 100644 --- a/common/src/main/scala/com/gu/media/Permissions.scala +++ b/common/src/main/scala/com/gu/media/Permissions.scala @@ -1,65 +1,49 @@ package com.gu.media import com.amazonaws.auth.AWSCredentialsProvider -import com.gu.editorial.permissions.client._ +import com.gu.permissions._ import ai.x.play.json.Jsonx import ai.x.play.json.Encoders._ import play.api.libs.json.Format import com.gu.pandomainauth.model.{User => PandaUser} -import scala.concurrent.Future +import com.gu.permissions.PermissionDefinition + case class Permissions( - deleteAtom: Boolean, - addSelfHostedAsset: Boolean, - setVideosOnAllChannelsPublic: Boolean, - pinboard: Boolean + deleteAtom: Boolean = false, + addSelfHostedAsset: Boolean = false, + setVideosOnAllChannelsPublic: Boolean = false, + pinboard: Boolean = false ) object Permissions { implicit val format: Format[Permissions] = Jsonx.formatCaseClass[Permissions] val app = "atom-maker" - val deleteAtom = Permission("delete_atom", app, defaultVal = PermissionDenied) - val addSelfHostedAsset = Permission("add_self_hosted_asset", app, defaultVal = PermissionDenied) - val setVideosOnAllChannelsPublic = Permission("set_videos_on_all_channels_public", app, defaultVal = PermissionDenied) - val pinboard = Permission("pinboard", "pinboard", defaultVal = PermissionDenied) + val deleteAtom = PermissionDefinition("delete_atom", app) + val addSelfHostedAsset = PermissionDefinition("add_self_hosted_asset", app) + val setVideosOnAllChannelsPublic = PermissionDefinition("set_videos_on_all_channels_public", app) + val pinboard = PermissionDefinition("pinboard", "pinboard") } -class MediaAtomMakerPermissionsProvider(stage: String, credsProvider: AWSCredentialsProvider) extends PermissionsProvider { +class MediaAtomMakerPermissionsProvider(stage: String, region: String, credsProvider: AWSCredentialsProvider) { import Permissions._ - implicit def config = PermissionsConfig( - app = "media-atom-maker", - all = Seq(deleteAtom, addSelfHostedAsset, setVideosOnAllChannelsPublic, pinboard), - s3BucketPrefix = if(stage == "PROD") "PROD" else "CODE", - awsCredentials = credsProvider - ) + private val permissions: PermissionsProvider = PermissionsProvider(PermissionsConfig(stage, region, credsProvider)) - def getAll(user: PandaUser): Future[Permissions] = for { - deleteAtom <- hasPermission(deleteAtom, user) - selfHostedMediaAtom <- hasPermission(addSelfHostedAsset, user) - publicStatusPermissions <- hasPermission(setVideosOnAllChannelsPublic, user) - pinboard <- hasPermission(pinboard, user) - } yield Permissions(deleteAtom, selfHostedMediaAtom, publicStatusPermissions, pinboard) + def getAll(user: PandaUser): Permissions = Permissions( + deleteAtom = hasPermission(deleteAtom, user), + addSelfHostedAsset = hasPermission(addSelfHostedAsset, user), + setVideosOnAllChannelsPublic = hasPermission(setVideosOnAllChannelsPublic, user), + pinboard = hasPermission(pinboard, user) + ) - def getStatusPermissions(user: PandaUser): Future[Permissions] = for { - publicStatus <- hasPermission(setVideosOnAllChannelsPublic, user) - } yield { - Permissions(deleteAtom = false, addSelfHostedAsset = false, publicStatus, pinboard = false) - } + def getStatusPermissions(user: PandaUser): Permissions = + Permissions(setVideosOnAllChannelsPublic = hasPermission(setVideosOnAllChannelsPublic, user)) - private def hasPermission(permission: Permission, user: PandaUser): Future[Boolean] = { - user.email match { - // TODO be better - // HACK: HMAC authenticated users are a `PandaUser` without an email - case "" if user.firstName == "media-atom-scheduler-lambda" => { - Future.successful(true) - } - case _ => { - get(permission)(PermissionsUser(user.email)).map { - case PermissionGranted => true - case _ => false - } - } - } + def hasPermission(permission: PermissionDefinition, user: PandaUser): Boolean = user.email match { + // TODO be better + // HACK: HMAC authenticated users are a `PandaUser` without an email + case "" if user.firstName == "media-atom-scheduler-lambda" => true + case _ => permissions.hasPermission(permission, user.email) } }