diff --git a/src/main/scala/org/broadinstitute/dsde/firecloud/core/ProfileClient.scala b/src/main/scala/org/broadinstitute/dsde/firecloud/core/ProfileClient.scala index 2f7e9784c..a180804ba 100644 --- a/src/main/scala/org/broadinstitute/dsde/firecloud/core/ProfileClient.scala +++ b/src/main/scala/org/broadinstitute/dsde/firecloud/core/ProfileClient.scala @@ -34,6 +34,21 @@ object ProfileClient { case class SyncSelf(userInfo: UserInfo) def props(requestContext: RequestContext): Props = Props(new ProfileClientActor(requestContext)) + + def calculateExpireTime(profile:Profile): Long = { + (profile.lastLinkTime, profile.linkExpireTime) match { + case (Some(lastLink), Some(expire)) if (lastLink < DateUtils.nowMinus24Hours && expire > DateUtils.nowPlus24Hours) => + // The user has not logged in to FireCloud within 24 hours, AND the user's expiration is + // more than 24 hours in the future. Reset the user's expiration. + DateUtils.nowPlus24Hours + case (Some(lastLink), Some(expire)) => + // User in good standing; return the expire time unchanged + expire + case _ => + // Either last-link or expire is missing. Reset the user's expiration. + DateUtils.nowPlus24Hours + } + } } class ProfileClientActor(requestContext: RequestContext) extends Actor with FireCloudRequestBuilding { @@ -119,24 +134,18 @@ class ProfileClientActor(requestContext: RequestContext) extends Actor with Fire case Some(nihUsername) => // we have a linked profile. - // if we have no record of the user logging in or the user's expire time, default to 0 - val lastLinkSeconds = profile.lastLinkTime.getOrElse(0L) - var linkExpireSeconds = profile.linkExpireTime.getOrElse(0L) - - val howOld = DateUtils.hoursSince(lastLinkSeconds) - val howSoonExpire = DateUtils.secondsSince(linkExpireSeconds) + // get the current link expiration time + val profileExpiration = profile.linkExpireTime.getOrElse(0L) - // if the user's link has expired, the user must re-link. - // NB: we use a separate val here in case we need to change the logic. For instance, we could - // change the logic later to be "link has expired OR user hasn't logged in within 24 hours" - val loginRequired = (howSoonExpire >= 0) + // calculate the possible new link expiration time + val linkExpireSeconds = if (updateExpiration) { + calculateExpireTime(profile) + } else { + profileExpiration + } - // if the user has not logged in to FireCloud within 24 hours, AND the user's expiration is - // further out than 24 hours, reset the user's expiration. - if (updateExpiration && howOld >= 24 && Math.abs(howSoonExpire) >= (24*60*60)) { - // generate time now+24 hours - linkExpireSeconds = DateUtils.nowPlus24Hours - // update linkExpireTime in Thurloe for this user + // if the link expiration time needs updating (i.e. is different than what's in the profile), do the updating + if (linkExpireSeconds != profileExpiration) { val expireKVP = FireCloudKeyValue(Some("linkExpireTime"), Some(linkExpireSeconds.toString)) val expirePayload = ThurloeKeyValue(Some(userInfo.getUniqueId), Some(expireKVP)) @@ -158,6 +167,13 @@ class ProfileClientActor(requestContext: RequestContext) extends Actor with Fire } } + val howSoonExpire = DateUtils.secondsSince(linkExpireSeconds) + + // if the user's link has expired, the user must re-link. + // NB: we use a separate val here in case we need to change the logic. For instance, we could + // change the logic later to be "link has expired OR user hasn't logged in within 24 hours" + val loginRequired = (howSoonExpire >= 0) + getNIHStatusResponse(pipeline, loginRequired, profile, linkExpireSeconds) case _ => diff --git a/src/main/scala/org/broadinstitute/dsde/firecloud/utils/DateUtils.scala b/src/main/scala/org/broadinstitute/dsde/firecloud/utils/DateUtils.scala index 223affbcd..7bcf75e11 100644 --- a/src/main/scala/org/broadinstitute/dsde/firecloud/utils/DateUtils.scala +++ b/src/main/scala/org/broadinstitute/dsde/firecloud/utils/DateUtils.scala @@ -13,6 +13,10 @@ object DateUtils { nowDateTime.plusDays(30).getMillis / EPOCH } + def nowMinus30Days: Long = { + nowDateTime.minusDays(30).getMillis / EPOCH + } + def nowPlus24Hours: Long = { nowDateTime.plusHours(24).getMillis / EPOCH } @@ -21,6 +25,13 @@ object DateUtils { nowDateTime.minusHours(24).getMillis / EPOCH } + def nowPlus1Hour: Long = { + nowDateTime.plusHours(1).getMillis / EPOCH + } + + def nowMinus1Hour: Long = { + nowDateTime.minusHours(1).getMillis / EPOCH + } def hoursSince(seconds: Long): Int = { Hours.hoursBetween(dtFromSeconds(seconds), nowDateTime).getHours diff --git a/src/test/scala/org/broadinstitute/dsde/firecloud/service/ProfileClientSpec.scala b/src/test/scala/org/broadinstitute/dsde/firecloud/service/ProfileClientSpec.scala new file mode 100644 index 000000000..52e29af21 --- /dev/null +++ b/src/test/scala/org/broadinstitute/dsde/firecloud/service/ProfileClientSpec.scala @@ -0,0 +1,170 @@ +package org.broadinstitute.dsde.firecloud.service + +import org.broadinstitute.dsde.firecloud.model.Profile +import org.broadinstitute.dsde.firecloud.core.ProfileClient +import org.broadinstitute.dsde.firecloud.utils.DateUtils +import org.scalatest.FreeSpec + +class ProfileClientSpec extends FreeSpec { + + + def makeProfile(lastLinkTime: Option[Long] = None, linkExpireTime: Option[Long] = None): Profile = { + Profile ( + "firstName", + "lastName", + "title", + "institute", + "institutionalProgram", + "programLocationCity", + "programLocationState", + "programLocationCountry", + "pi", + "nonProfitStatus", + Some("billingAccountName"), + Some("linkedNihUsername"), + lastLinkTime, //lastLinkTime + linkExpireTime, //linkExpireTime + Some(false) //isDbGapAuthorized, unused + ) + } + + def assertExpireTimeWasUpdated(calculatedExpire: Long) = { + // we expect the calculated time to be now + 24 hours. We can't test this exactly because milliseconds + // may have elapsed in processing, so we rely on DateUtils rounding, and give a + val diff = DateUtils.hoursUntil(calculatedExpire) + + assert( diff == 23 || diff == 24, + "Expected expiration 24 hours in the future, found expiration " + diff + " hours away" ) + } + + + "ExpireTimeCalculationTests" - { + + + // following tests: lastLink MORE than 24 hours in the future + "lastLink > 24 hours in the past and expire > 24 hours in the future" - { + "should return expire as now+24hours" in { + val lastLink = DateUtils.nowMinus30Days + val expire = DateUtils.nowPlus30Days + + val profile = makeProfile(Some(lastLink), Some(expire)) + val calculatedExpire = ProfileClient.calculateExpireTime(profile) + + assertExpireTimeWasUpdated(calculatedExpire) + } + } + + "lastLink > 24 hours in the past and expire < 24 hours in the future" - { + "should return expire time unchanged" in { + val lastLink = DateUtils.nowMinus30Days + val expire = DateUtils.nowPlus1Hour + + val profile = makeProfile(Some(lastLink), Some(expire)) + val calculatedExpire = ProfileClient.calculateExpireTime(profile) + assertResult(expire) { calculatedExpire } + } + } + + "lastLink > 24 hours in the past and expire > 24 hours in the past" - { + "should return expire time unchanged" in { + val lastLink = DateUtils.nowMinus30Days + val expire = DateUtils.nowMinus30Days + + val profile = makeProfile(Some(lastLink), Some(expire)) + val calculatedExpire = ProfileClient.calculateExpireTime(profile) + assertResult(expire) { calculatedExpire } + } + } + + "lastLink > 24 hours in the past and expire < 24 hours in the past" - { + "should return expire time unchanged" in { + val lastLink = DateUtils.nowMinus30Days + val expire = DateUtils.nowMinus1Hour + + val profile = makeProfile(Some(lastLink), Some(expire)) + val calculatedExpire = ProfileClient.calculateExpireTime(profile) + assertResult(expire) { calculatedExpire } + } + } + + + // following tests: lastLink LESS than 24 hours in the future + "lastLink < 24 hours in the past and expire > 24 hours in the future" - { + "should return expire time unchanged" in { + val lastLink = DateUtils.nowMinus1Hour + val expire = DateUtils.nowPlus30Days + + val profile = makeProfile(Some(lastLink), Some(expire)) + val calculatedExpire = ProfileClient.calculateExpireTime(profile) + assertResult(expire) { calculatedExpire } + } + } + + "lastLink < 24 hours in the past and expire < 24 hours in the future" - { + "should return expire time unchanged" in { + val lastLink = DateUtils.nowMinus1Hour + val expire = DateUtils.nowPlus1Hour + + val profile = makeProfile(Some(lastLink), Some(expire)) + val calculatedExpire = ProfileClient.calculateExpireTime(profile) + assertResult(expire) { calculatedExpire } + } + } + + "lastLink < 24 hours in the past and expire > 24 hours in the past" - { + "should return expire time unchanged" in { + val lastLink = DateUtils.nowMinus1Hour + val expire = DateUtils.nowMinus30Days + + val profile = makeProfile(Some(lastLink), Some(expire)) + val calculatedExpire = ProfileClient.calculateExpireTime(profile) + assertResult(expire) { calculatedExpire } + } + } + + "lastLink < 24 hours in the past and expire < 24 hours in the past" - { + "should return expire time unchanged" in { + val lastLink = DateUtils.nowMinus1Hour + val expire = DateUtils.nowMinus1Hour + + val profile = makeProfile(Some(lastLink), Some(expire)) + val calculatedExpire = ProfileClient.calculateExpireTime(profile) + assertResult(expire) { calculatedExpire } + } + } + + "No lastLink or expire time" - { + "should return expire as now+24hours" in { + val profile = makeProfile() + val calculatedExpire = ProfileClient.calculateExpireTime(profile) + + assertExpireTimeWasUpdated(calculatedExpire) + } + } + + "No lastLink time" - { + "should return expire as now+24hours" in { + val expire = DateUtils.nowPlus1Hour + val profile = makeProfile(None, Some(expire)) + val calculatedExpire = ProfileClient.calculateExpireTime(profile) + + assertExpireTimeWasUpdated(calculatedExpire) + } + } + + "No expire time" - { + "should return expire as now+24hours" in { + val lastLink = DateUtils.nowMinus1Hour + val profile = makeProfile(Some(lastLink), None) + val calculatedExpire = ProfileClient.calculateExpireTime(profile) + + assertExpireTimeWasUpdated(calculatedExpire) + } + } + + + + + } + +}