Skip to content

Conversation

davidangb
Copy link
Contributor

When logging in to FireCloud, if the current user:

  • has a linked NIH account
  • has not re-linked with NIH within the last 24 hours
  • has a NIH link expiration of more than 24 hours
    then set the user's NIH link expiration to be 24 hours.

The implementation of this PR is, at first glance (and maybe the fifth glance), unconventional. The OAuth login would need to query Thurloe for NIH link keys, then inspect those keys and act appropriately. We already had an endpoint that queried and inspected those keys ... so I added the check-for-expiration feature into that code path. I like this approach also because it makes it trivially easy to enable the check-for-expiration feature in the /api/nih/status endpoint; in turn this may make it easy to call the check more often.

ProfileClient.scala has a bunch of code that moved around, so the diff shows bigger than it really is. I'll try to comment inline with details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

most of the getProfile function is a cut-and-paste from the previous GetNIHStatus case. However, note I've removed the check on profile.isDbgapAuthorized, which added very little. See DSDEEPB-2404 for more context.

@rushtong rushtong self-assigned this Jan 6, 2016
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any usages for this - I'm guessing this is for upcoming work?

@rushtong
Copy link
Member

rushtong commented Jan 6, 2016

👍 Looks good and functions as expected. Merging.

rushtong added a commit that referenced this pull request Jan 6, 2016
DSDEEPB-2403: FireCloud OAuth login updates NIH link expiration time
@rushtong rushtong merged commit c0197e1 into develop Jan 6, 2016
@rushtong rushtong deleted the da_DSDEEPB-2403_setNIHExpire branch January 6, 2016 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants