From 60ee196cf4c4170f73332b2ce3167723ee07da09 Mon Sep 17 00:00:00 2001 From: Tom Forbes Date: Fri, 20 Dec 2024 12:59:59 +0000 Subject: [PATCH 1/7] Add allowedGroups check --- .../scala/com/gu/googleauth/actions.scala | 38 +++++++++++---- .../com/gu/googleauth/GoogleAuthTest.scala | 46 +++++++++++++++++-- 2 files changed, 71 insertions(+), 13 deletions(-) diff --git a/play-v29/src/main/scala/com/gu/googleauth/actions.scala b/play-v29/src/main/scala/com/gu/googleauth/actions.scala index 8989a5a..7ef73c8 100644 --- a/play-v29/src/main/scala/com/gu/googleauth/actions.scala +++ b/play-v29/src/main/scala/com/gu/googleauth/actions.scala @@ -89,6 +89,8 @@ class AuthAction[A](val authConfig: GoogleAuthConfig, loginTarget: Call, bodyPar } trait LoginSupport extends Logging { + import Actions.GroupCheckConfig + implicit def wsClient: WSClient /** @@ -154,15 +156,15 @@ trait LoginSupport extends Logging { * Looks up user's Google Groups and ensures they belong to any that are required. Redirects to * `failureRedirectTarget` if the user is not a member of any required group. */ - def enforceGoogleGroups(userIdentity: UserIdentity, requiredGoogleGroups: Set[String], googleGroupChecker: GoogleGroupChecker, errorMessage: String = "Login failure. You do not belong to the required Google groups") + def enforceGoogleGroups(userIdentity: UserIdentity, groupChecker: GoogleGroupChecker, groupCheckConfig: GroupCheckConfig, errorMessage: String = "Login failure. You do not belong to the required Google groups") (implicit request: RequestHeader, ec: ExecutionContext): EitherT[Future, Result, Unit] = { - googleGroupChecker.retrieveGroupsFor(userIdentity.email).attemptT + groupChecker.retrieveGroupsFor(userIdentity.email).attemptT .leftMap { t => logger.warn("Login failure, Could not look up user's Google groups", t) redirectWithError(failureRedirectTarget, "Login failure. Unable to look up Google Group membership") } .subflatMap { userGroups => - if (Actions.checkGoogleGroups(userGroups, requiredGoogleGroups)) { + if (Actions.checkGoogleGroups(userGroups, groupCheckConfig)) { Right(()) } else { logger.info("Login failure, user not in required Google groups") @@ -185,13 +187,12 @@ trait LoginSupport extends Logging { /** * Handle the OAuth2 callback, which logs the user in and redirects them appropriately. * - * Also ensures the user belongs to the (provided) required Google Groups. + * Also ensures the user belongs to *all* the (provided) required Google Groups, and *at least one of* the allowedGoogleGroups */ - def processOauth2Callback(requiredGoogleGroups: Set[String], groupChecker: GoogleGroupChecker) - (implicit request: RequestHeader, ec: ExecutionContext): Future[Result] = { + def processOauth2Callback(groupChecker: GoogleGroupChecker, groupCheckConfig: GroupCheckConfig)(implicit request: RequestHeader, ec: ExecutionContext): Future[Result] = { (for { identity <- checkIdentity() - _ <- enforceGoogleGroups(identity, requiredGoogleGroups, groupChecker) + _ <- enforceGoogleGroups(identity, groupChecker, groupCheckConfig) } yield { setupSessionWhenSuccessful(identity) }).merge @@ -216,9 +217,30 @@ trait LoginSupport extends Logging { } object Actions { - private[googleauth] def checkGoogleGroups(userGroups: Set[String], requiredGroups: Set[String]): Boolean = { + /** + * @param requiredGroups If defined, user must be a member of *all* groups in requiredGroups + * @param allowedGroups If defined, user must be a member of *at least one of* the groups in allowedGroups + */ + case class GroupCheckConfig( + requiredGroups: Option[Set[String]] = None, + allowedGroups: Option[Set[String]] = None + ) + + private[googleauth] def checkGoogleGroups(userGroups: Set[String], groupCheckConfig: GroupCheckConfig): Boolean = { + val requiredGroupCheck = groupCheckConfig.requiredGroups.map(required => Actions.checkRequiredGoogleGroups(userGroups, required)).getOrElse(true) + val allowedGroupCheck = groupCheckConfig.allowedGroups.map(allowed => Actions.checkAllowedGoogleGroups(userGroups, allowed)).getOrElse(true) + requiredGroupCheck && allowedGroupCheck + } + + // User must be a member of *all* groups in requiredGroups + private def checkRequiredGoogleGroups(userGroups: Set[String], requiredGroups: Set[String]): Boolean = { userGroups.intersect(requiredGroups) == requiredGroups } + + // User must be a member of *at least one* of the groups in allowedGroups + private def checkAllowedGoogleGroups(userGroups: Set[String], allowedGroups: Set[String]): Boolean = { + allowedGroups.intersect(userGroups).nonEmpty + } } trait Filters extends UserIdentifier with Logging { diff --git a/play-v29/src/test/scala/com/gu/googleauth/GoogleAuthTest.scala b/play-v29/src/test/scala/com/gu/googleauth/GoogleAuthTest.scala index 73aa798..69c7685 100644 --- a/play-v29/src/test/scala/com/gu/googleauth/GoogleAuthTest.scala +++ b/play-v29/src/test/scala/com/gu/googleauth/GoogleAuthTest.scala @@ -2,6 +2,7 @@ package com.gu.googleauth import com.gu.play.secretrotation.DualSecretTransition.TransitioningSecret import com.gu.play.secretrotation.SnapshotProvider +import com.gu.googleauth.Actions.GroupCheckConfig import org.apache.commons.codec.binary.Base64 import org.scalatest.freespec.AsyncFreeSpec import org.scalatest.matchers.should.Matchers @@ -15,30 +16,65 @@ import scala.concurrent.ExecutionContext.global class GoogleAuthTest extends AsyncFreeSpec with Matchers { - "enforceUserGroups" - { + "requiredGroups check" - { val requiredGroups = Set("required-group-1", "required-group-2") "returns false if the user has no groups" in { val userGroups = Set.empty[String] - val result = Actions.checkGoogleGroups(userGroups, requiredGroups) + val result = Actions.checkGoogleGroups(userGroups, GroupCheckConfig(requiredGroups = Some(requiredGroups))) result shouldEqual false } "returns false if the user is missing a group" in { val userGroups = Set(requiredGroups.head) - val result = Actions.checkGoogleGroups(userGroups, requiredGroups) + val result = Actions.checkGoogleGroups(userGroups, GroupCheckConfig(requiredGroups = Some(requiredGroups))) result shouldEqual false } "returns false if the user has other groups but is missing a required group" in { val userGroups = Set(requiredGroups.head, "example-group", "another-group") - val result = Actions.checkGoogleGroups(userGroups, requiredGroups) + val result = Actions.checkGoogleGroups(userGroups, GroupCheckConfig(requiredGroups = Some(requiredGroups))) result shouldEqual false } "returns true if the required groups are present" in { val userGroups = requiredGroups + "example-group" - val result = Actions.checkGoogleGroups(userGroups, requiredGroups) + val result = Actions.checkGoogleGroups(userGroups, GroupCheckConfig(requiredGroups = Some(requiredGroups))) + result shouldEqual true + } + } + + "allowedGroups check" - { + val requiredGroups = Set("required-group-1", "required-group-2") + val allowedGroups = Set("allowed-group-1", "allowed-group-2") + + "returns false if the user has no groups" in { + val userGroups = Set.empty[String] + val result = Actions.checkGoogleGroups(userGroups, GroupCheckConfig(allowedGroups = Some(allowedGroups))) + result shouldEqual false + } + + "returns false if the user has other groups but is missing all allowed groups" in { + val userGroups = Set("example-group", "another-group") + val result = Actions.checkGoogleGroups(userGroups, GroupCheckConfig(allowedGroups = Some(allowedGroups))) + result shouldEqual false + } + + "returns true if the user has one group and is missing another group" in { + val userGroups = Set(allowedGroups.head) + val result = Actions.checkGoogleGroups(userGroups, GroupCheckConfig(allowedGroups = Some(allowedGroups))) + result shouldEqual true + } + + "returns false if the user has an allowed group but is missing required groups" in { + val userGroups = Set(allowedGroups.head) + val result = Actions.checkGoogleGroups(userGroups, GroupCheckConfig(allowedGroups = Some(allowedGroups), requiredGroups = Some(requiredGroups))) + result shouldEqual false + } + + "returns true if the user has an allowed group and has required groups" in { + val userGroups = Set(allowedGroups.head) ++ requiredGroups + val result = Actions.checkGoogleGroups(userGroups, GroupCheckConfig(allowedGroups = Some(allowedGroups), requiredGroups = Some(requiredGroups))) result shouldEqual true } } From d5ca3a5d6d96f66f3b18a9b2be384b2246ac29ad Mon Sep 17 00:00:00 2001 From: Tom Forbes Date: Fri, 20 Dec 2024 13:03:31 +0000 Subject: [PATCH 2/7] tidy --- play-v29/src/main/scala/com/gu/googleauth/actions.scala | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/play-v29/src/main/scala/com/gu/googleauth/actions.scala b/play-v29/src/main/scala/com/gu/googleauth/actions.scala index 7ef73c8..f1ed9cf 100644 --- a/play-v29/src/main/scala/com/gu/googleauth/actions.scala +++ b/play-v29/src/main/scala/com/gu/googleauth/actions.scala @@ -156,9 +156,9 @@ trait LoginSupport extends Logging { * Looks up user's Google Groups and ensures they belong to any that are required. Redirects to * `failureRedirectTarget` if the user is not a member of any required group. */ - def enforceGoogleGroups(userIdentity: UserIdentity, groupChecker: GoogleGroupChecker, groupCheckConfig: GroupCheckConfig, errorMessage: String = "Login failure. You do not belong to the required Google groups") + def enforceGoogleGroups(userIdentity: UserIdentity, googleGroupChecker: GoogleGroupChecker, groupCheckConfig: GroupCheckConfig, errorMessage: String = "Login failure. You do not belong to the required Google groups") (implicit request: RequestHeader, ec: ExecutionContext): EitherT[Future, Result, Unit] = { - groupChecker.retrieveGroupsFor(userIdentity.email).attemptT + googleGroupChecker.retrieveGroupsFor(userIdentity.email).attemptT .leftMap { t => logger.warn("Login failure, Could not look up user's Google groups", t) redirectWithError(failureRedirectTarget, "Login failure. Unable to look up Google Group membership") @@ -189,7 +189,8 @@ trait LoginSupport extends Logging { * * Also ensures the user belongs to *all* the (provided) required Google Groups, and *at least one of* the allowedGoogleGroups */ - def processOauth2Callback(groupChecker: GoogleGroupChecker, groupCheckConfig: GroupCheckConfig)(implicit request: RequestHeader, ec: ExecutionContext): Future[Result] = { + def processOauth2Callback(groupCheckConfig: GroupCheckConfig, groupChecker: GoogleGroupChecker) + (implicit request: RequestHeader, ec: ExecutionContext): Future[Result] = { (for { identity <- checkIdentity() _ <- enforceGoogleGroups(identity, groupChecker, groupCheckConfig) From 8767e0e6f05880ca4c980695f8d916f62079b3ad Mon Sep 17 00:00:00 2001 From: Tom Forbes Date: Fri, 20 Dec 2024 13:06:17 +0000 Subject: [PATCH 3/7] comment --- play-v29/src/main/scala/com/gu/googleauth/actions.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/play-v29/src/main/scala/com/gu/googleauth/actions.scala b/play-v29/src/main/scala/com/gu/googleauth/actions.scala index f1ed9cf..22cccef 100644 --- a/play-v29/src/main/scala/com/gu/googleauth/actions.scala +++ b/play-v29/src/main/scala/com/gu/googleauth/actions.scala @@ -156,7 +156,7 @@ trait LoginSupport extends Logging { * Looks up user's Google Groups and ensures they belong to any that are required. Redirects to * `failureRedirectTarget` if the user is not a member of any required group. */ - def enforceGoogleGroups(userIdentity: UserIdentity, googleGroupChecker: GoogleGroupChecker, groupCheckConfig: GroupCheckConfig, errorMessage: String = "Login failure. You do not belong to the required Google groups") + def enforceGoogleGroups(userIdentity: UserIdentity, groupCheckConfig: GroupCheckConfig, googleGroupChecker: GoogleGroupChecker, errorMessage: String = "Login failure. You do not belong to the required Google groups") (implicit request: RequestHeader, ec: ExecutionContext): EitherT[Future, Result, Unit] = { googleGroupChecker.retrieveGroupsFor(userIdentity.email).attemptT .leftMap { t => @@ -187,7 +187,7 @@ trait LoginSupport extends Logging { /** * Handle the OAuth2 callback, which logs the user in and redirects them appropriately. * - * Also ensures the user belongs to *all* the (provided) required Google Groups, and *at least one of* the allowedGoogleGroups + * Also ensures the user is in the correct Google Groups, as defined by the given GroupCheckConfig */ def processOauth2Callback(groupCheckConfig: GroupCheckConfig, groupChecker: GoogleGroupChecker) (implicit request: RequestHeader, ec: ExecutionContext): Future[Result] = { From 6c74b38a2a725fde4a1063388757cca13e301864 Mon Sep 17 00:00:00 2001 From: Tom Forbes Date: Fri, 20 Dec 2024 13:07:24 +0000 Subject: [PATCH 4/7] order --- play-v29/src/main/scala/com/gu/googleauth/actions.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/play-v29/src/main/scala/com/gu/googleauth/actions.scala b/play-v29/src/main/scala/com/gu/googleauth/actions.scala index 22cccef..8ce15c3 100644 --- a/play-v29/src/main/scala/com/gu/googleauth/actions.scala +++ b/play-v29/src/main/scala/com/gu/googleauth/actions.scala @@ -193,7 +193,7 @@ trait LoginSupport extends Logging { (implicit request: RequestHeader, ec: ExecutionContext): Future[Result] = { (for { identity <- checkIdentity() - _ <- enforceGoogleGroups(identity, groupChecker, groupCheckConfig) + _ <- enforceGoogleGroups(identity, groupCheckConfig, groupChecker) } yield { setupSessionWhenSuccessful(identity) }).merge From b07b41ac23b060578cc838446cff79e387d9effd Mon Sep 17 00:00:00 2001 From: Tom Forbes Date: Fri, 20 Dec 2024 13:12:49 +0000 Subject: [PATCH 5/7] Update example --- .../src/sbt-test/example/webapp/app/controllers/Login.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/play-v29/src/sbt-test/example/webapp/app/controllers/Login.scala b/play-v29/src/sbt-test/example/webapp/app/controllers/Login.scala index 9cf297d..3c45ea7 100644 --- a/play-v29/src/sbt-test/example/webapp/app/controllers/Login.scala +++ b/play-v29/src/sbt-test/example/webapp/app/controllers/Login.scala @@ -1,6 +1,7 @@ package controllers import com.gu.googleauth.{GoogleAuthConfig, GoogleGroupChecker, LoginSupport} +import com.gu.googleauth.Actions.GroupCheckConfig import play.api.libs.ws.WSClient import play.api.mvc._ @@ -34,7 +35,7 @@ class Login(requiredGoogleGroups: Set[String], val authConfig: GoogleAuthConfig, */ def oauth2Callback = Action.async { implicit request => // processOauth2Callback() // without Google group membership checks - processOauth2Callback(requiredGoogleGroups, googleGroupChecker) // with optional Google group checks + processOauth2Callback(GroupCheckConfig(requiredGroups = requiredGoogleGroups), googleGroupChecker) // with optional Google group checks } def logout = Action { implicit request => From 2d77b4acc02ad5087f74266c1536540a3f08f7db Mon Sep 17 00:00:00 2001 From: Tom Forbes Date: Fri, 20 Dec 2024 13:30:30 +0000 Subject: [PATCH 6/7] Some --- .../src/sbt-test/example/webapp/app/controllers/Login.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/play-v29/src/sbt-test/example/webapp/app/controllers/Login.scala b/play-v29/src/sbt-test/example/webapp/app/controllers/Login.scala index 3c45ea7..b6a72cd 100644 --- a/play-v29/src/sbt-test/example/webapp/app/controllers/Login.scala +++ b/play-v29/src/sbt-test/example/webapp/app/controllers/Login.scala @@ -35,7 +35,7 @@ class Login(requiredGoogleGroups: Set[String], val authConfig: GoogleAuthConfig, */ def oauth2Callback = Action.async { implicit request => // processOauth2Callback() // without Google group membership checks - processOauth2Callback(GroupCheckConfig(requiredGroups = requiredGoogleGroups), googleGroupChecker) // with optional Google group checks + processOauth2Callback(GroupCheckConfig(requiredGroups = Some(requiredGoogleGroups)), googleGroupChecker) // with optional Google group checks } def logout = Action { implicit request => From 7d69347876a61315c51108affc493d9603b65939 Mon Sep 17 00:00:00 2001 From: Tom Forbes Date: Fri, 10 Jan 2025 10:40:15 +0000 Subject: [PATCH 7/7] Keep old method and deprecate --- .../src/main/scala/com/gu/googleauth/actions.scala | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/play-v29/src/main/scala/com/gu/googleauth/actions.scala b/play-v29/src/main/scala/com/gu/googleauth/actions.scala index 8ce15c3..6196807 100644 --- a/play-v29/src/main/scala/com/gu/googleauth/actions.scala +++ b/play-v29/src/main/scala/com/gu/googleauth/actions.scala @@ -184,6 +184,17 @@ trait LoginSupport extends Logging { }).merge } + /** + * Handle the OAuth2 callback, which logs the user in and redirects them appropriately. + * + * Also ensures the user belongs to the (provided) required Google Groups. + */ + @deprecated("Prefer to pass in a GroupCheckConfig object instead.") + def processOauth2Callback(requiredGoogleGroups: Set[String], groupChecker: GoogleGroupChecker) + (implicit request: RequestHeader, ec: ExecutionContext): Future[Result] = { + val groupCheckConfig = GroupCheckConfig(requiredGroups = Some(requiredGoogleGroups)) + processOauth2Callback(groupCheckConfig, groupChecker) + } /** * Handle the OAuth2 callback, which logs the user in and redirects them appropriately. *