Skip to content

CORE-443 deny email patterns from access to auth domains driven by telemetry #1607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 3, 2025
Merged

CORE-443 deny email patterns from access to auth domains driven by telemetry #1607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8356f23
deny email patterns from auth domain access
dvoet Apr 3, 2025
0ddc072
fmt
dvoet Apr 3, 2025
577a467
logging
dvoet Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/main/resources/reference.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,41 @@ sam {

elasticsearch {
libraryEnabled = false
}

nih {
# NIH prohibits accounts from free email providers or emails from contries of concern
# (?i) is case insensitive
denyEmailPatterns = [
"(?i).+@(.+\\.)*126\\..+",
"(?i).+@(.+\\.)*163\\..+",
"(?i).+@(.+\\.)*aol\\..+",
"(?i).+@(.+\\.)*bk\\..+",
"(?i).+@(.+\\.)*foxmail\\..+",
"(?i).+@(.+\\.)*gmail\\..+",
"(?i).+@(.+\\.)*hotmail\\..+",
"(?i).+@(.+\\.)*icloud\\..+",
"(?i).+@(.+\\.)*inbox\\..+",
"(?i).+@(.+\\.)*live\\..+",
"(?i).+@(.+\\.)*mail\\..+",
"(?i).+@(.+\\.)*mailinator\\..+",
"(?i).+@(.+\\.)*outlook\\..+",
"(?i).+@(.+\\.)*proton\\..+",
"(?i).+@(.+\\.)*qq\\..+",
"(?i).+@(.+\\.)*rambler\\..+",
"(?i).+@(.+\\.)*sina\\..+",
"(?i).+@(.+\\.)*yeah\\..+",
"(?i).+@(.+\\.)*yahoo\\..+",
"(?i).+@(.+\\.)*yandex\\..+",
"(?i).+@(.+\\.)*ecohealthalliance\\.org",
"(?i).+@(.+\\.)+cn",
"(?i).+@(.+\\.)+hk",
"(?i).+@(.+\\.)+mo",
"(?i).+@(.+\\.)+ru",
"(?i).+@(.+\\.)+ir",
"(?i).+@(.+\\.)+nk",
"(?i).+@(.+\\.)+cu",
"(?i).+@(.+\\.)+ve",
"(?i)ydu@mrn\\.org ",
]
}
7 changes: 7 additions & 0 deletions src/main/scala/org/broadinstitute/dsde/firecloud/FireCloudConfig.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import java.util.concurrent.TimeUnit
import scala.concurrent.duration.FiniteDuration
import scala.jdk.CollectionConverters._
import scala.util.Try
import scala.util.matching.Regex

object FireCloudConfig {
private val config = ConfigFactory.load()
Expand Down Expand Up @@ -205,6 +206,12 @@ object FireCloudConfig {
DbGapPermission(phsId, consentGroup) -> groupName
}.toMap
}
lazy val denyEmailPatterns: Set[Regex] = {
val denyEmailPatternsConfigs = nih.getStringList("denyEmailPatterns")
denyEmailPatternsConfigs.asScala.toSet.map { pattern: String =>
pattern.r
}
}
}

object ElasticSearch {
Expand Down
16 changes: 14 additions & 2 deletions src/main/scala/org/broadinstitute/dsde/firecloud/service/NihService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ class NihService(val samDao: SamDAO,
dbGapGroupEmail <- Future.traverse(dbGapSamGroup.toList)(samDao.getGroupEmail(_)(getAdminAccessToken))
ecmEmails <- getNihAllowlistTerraEmailsFromEcm(allowlistUsers)
thurloeEmails <- getNihAllowlistTerraEmailsFromThurloe(allowlistUsers)
members = ecmEmails ++ thurloeEmails ++ dbGapGroupEmail
members = allowedNihMembers(ecmEmails ++ thurloeEmails ++ dbGapGroupEmail)
_ <- ensureAllowlistGroupsExists()
// The request to Sam to completely overwrite the group with the list of actively linked users on the allowlist
_ <- samDao.overwriteGroupMembers(nihAllowlist.groupToSync, ManagedGroupRoles.Member, members.toList)(
Expand All @@ -274,6 +274,18 @@ class NihService(val samDao: SamDAO,
} yield ()
}

private def allowedNihMembers(members: Set[WorkbenchEmail]): Set[WorkbenchEmail] = {
val allowedMembers =
members.filterNot(email => FireCloudConfig.Nih.denyEmailPatterns.exists(_.matches(email.value)))
val deniedMembers = members -- allowedMembers
if (deniedMembers.nonEmpty) {
logger.info(
s"NIH allowlist sync: ${deniedMembers.mkString(",")} were denied access to the NIH allowlist due to matching deny patterns"
)
}
allowedMembers
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recommendation, feel free to do in a follow-on PR: log when a user is denied because of this block list so we can follow up with some record of the blocklist behaving

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea

private def linkNihAccountEcm(userInfo: UserInfo, nihLink: NihLink): Future[Try[Unit]] =
ecmDao
.putLinkedEraAccount(LinkedEraAccount(userInfo.id, nihLink), getAdminAccessToken)
Expand Down Expand Up @@ -387,7 +399,7 @@ class NihService(val samDao: SamDAO,
): Future[Boolean] = {
val allowlistUsers = downloadNihAllowlist(nihAllowlist)

if (allowlistUsers contains linkedNihUserName) {
if (allowlistUsers.contains(linkedNihUserName) && allowedNihMembers(Set(userEmail)).contains(userEmail)) {
for {
_ <- samDao.addGroupMember(nihAllowlist.groupToSync, ManagedGroupRoles.Member, userEmail)(getAdminAccessToken)
} yield true
Expand Down
65 changes: 62 additions & 3 deletions src/test/scala/org/broadinstitute/dsde/firecloud/service/NihServiceUnitSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ class NihServiceUnitSpec extends AnyFlatSpec with Matchers with BeforeAndAfterEa
val userTcgaOnly = genSamUser();
val userTargetOnly = genSamUser();
val userDbGap = genSamUser();
val deniedUser = genSamUser().copy(email = WorkbenchEmail("someone@gmAil.com"))
val dbGapGroupEmail = WorkbenchEmail(UUID.randomUUID().toString + "@email.com")

// DateTimes must be modified in seconds instead of days to match implementation
Expand All @@ -93,13 +94,15 @@ class NihServiceUnitSpec extends AnyFlatSpec with Matchers with BeforeAndAfterEa
var userDbGapLinkedAccount =
LinkedEraAccount(userDbGap.id.value, "nihUsername5", new DateTime().plusSeconds(secondsIn30Days))

val samUsers = Seq(userNoLinkedAccount, userNoAllowlists, userTcgaAndTarget, userTcgaOnly, userTargetOnly, userDbGap)
val samUsers =
Seq(userNoLinkedAccount, userNoAllowlists, userTcgaAndTarget, userTcgaOnly, userTargetOnly, userDbGap, deniedUser)
val linkedAccounts = Seq(
userNoAllowlistsLinkedAccount,
userTcgaAndTargetLinkedAccount,
userTcgaOnlyLinkedAccount,
userTargetOnlyLinkedAccount,
userDbGapLinkedAccount
userDbGapLinkedAccount,
deniedUser
)

val idToSamUser = samUsers.groupBy(_.id).view.mapValues(_.head).toMap
Expand All @@ -109,7 +112,8 @@ class NihServiceUnitSpec extends AnyFlatSpec with Matchers with BeforeAndAfterEa
userTcgaAndTarget.id -> userTcgaAndTargetLinkedAccount,
userTcgaOnly.id -> userTcgaOnlyLinkedAccount,
userTargetOnly.id -> userTargetOnlyLinkedAccount,
userDbGap.id -> userDbGapLinkedAccount
userDbGap.id -> userDbGapLinkedAccount,
deniedUser.id -> userTcgaAndTargetLinkedAccount
)

val linkedAccountsByExternalId = Map(
Expand Down Expand Up @@ -428,6 +432,61 @@ class NihServiceUnitSpec extends AnyFlatSpec with Matchers with BeforeAndAfterEa
)(ArgumentMatchers.eq(UserInfo(adminAccessToken, "")))
}

it should "decode a JWT from Shibboleth and sync allowlists and remove a denied user" in {
mockShibbolethDAO()
mockEcmUsers()
mockThurloeUsers()
val user = deniedUser
val userInfo = UserInfo(user.email.value,
OAuth2BearerToken(user.id.value),
Instant.now().plusSeconds(60).getEpochSecond,
user.id.value
)
val linkedAccount = userTcgaOnlyLinkedAccount
val jwt = jwtForUser(linkedAccount)
val (statusCode, nihStatus) = Await
.result(nihService.updateNihLinkAndSyncSelf(userInfo, jwt), Duration.Inf)
.asInstanceOf[PerRequest.RequestComplete[(StatusCode, NihStatus)]]
.response

nihStatus.linkedNihUsername should be(Some(linkedAccount.linkedExternalId))
nihStatus.linkExpireTime should be(Some(linkedAccount.linkExpireTime.getMillis / 1000L))
nihStatus.datasetPermissions should be(
Set(
NihDatasetPermission("BROKEN", authorized = false),
NihDatasetPermission("TARGET", authorized = false),
NihDatasetPermission("TCGA", authorized = false),
NihDatasetPermission("RAS", authorized = false)
)
)

statusCode should be(StatusCodes.OK)
verify(googleDao, times(1)).getBucketObjectAsInputStream(FireCloudConfig.Nih.whitelistBucket, "tcga-whitelist.txt")
verify(googleDao, times(1)).getBucketObjectAsInputStream(FireCloudConfig.Nih.whitelistBucket,
"target-whitelist.txt"
)
verify(samDao, times(1)).removeGroupMember(
ArgumentMatchers.eq(WorkbenchGroupName("TARGET-dbGaP-Authorized")),
ArgumentMatchers.eq(ManagedGroupRoles.Member),
ArgumentMatchers.eq(WorkbenchEmail(user.email.value))
)(ArgumentMatchers.eq(UserInfo(adminAccessToken, "")))
verify(samDao, times(1)).removeGroupMember(
ArgumentMatchers.eq(WorkbenchGroupName("TCGA-dbGaP-Authorized")),
ArgumentMatchers.eq(ManagedGroupRoles.Member),
ArgumentMatchers.eq(WorkbenchEmail(user.email.value))
)(ArgumentMatchers.eq(UserInfo(adminAccessToken, "")))
verify(samDao, never()).addGroupMember(
ArgumentMatchers.eq(WorkbenchGroupName("this-doesnt-matter")),
ArgumentMatchers.eq(ManagedGroupRoles.Member),
ArgumentMatchers.eq(WorkbenchEmail(user.email.value))
)(ArgumentMatchers.eq(UserInfo(adminAccessToken, "")))
verify(samDao, never()).addGroupMember(
ArgumentMatchers.eq(WorkbenchGroupName("other-group")),
ArgumentMatchers.eq(ManagedGroupRoles.Member),
ArgumentMatchers.eq(WorkbenchEmail(user.email.value))
)(ArgumentMatchers.eq(UserInfo(adminAccessToken, "")))
}

it should "continue, but return an error of ECM returns an error" in {
mockShibbolethDAO()
mockThurloeUsers()
Expand Down
Loading