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.
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
Fix auth0 management tokens #1140
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I created tokens from user settings, I was prompted to sign into auth0 each time. Is that expected?
I added some off-label testing:
- does the cache work? do i get the same token if i ask for the same entry in the cache several times, even if it's a stupid value to ask for like
12345
in a cache of size 1) - does this sequence:
cache.get(1)
,cache.get(2)
,cache.get(1)
yield different tokens for all three values (it should, since the cache size is 1, socache.get(2)
invalidates the key1
) - does expiration work / can we expect expiring every 12 hours to ensure that we always have a fresh token (i changed expiration to 30 seconds from 12 hours, then did
cache.get(1)
... wait 30 seconds...cache.get(1)
)
def listRefreshTokens(user: User): Future[List[DeviceCredential]] = { | ||
for { | ||
bearerToken <- authBearerTokenCache.get(1) | ||
deviceCredentialsList <- requestDeviceTokens(user, bearerToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
way to use the secret flatMap in for comprehensions 😎
Logging in each time is expected behavior |
Ok then |
Thanks. Will hold off on merging until @hectcastro gives this one the ok. |
@@ -6,8 +6,10 @@ import akka.http.scaladsl.model._ | |||
import akka.http.scaladsl.model.Uri.{Path, Query} | |||
import akka.http.scaladsl.model.headers.{Authorization, GenericHttpCredentials} | |||
import akka.http.scaladsl.unmarshalling.Unmarshal | |||
import com.github.blemale.scaffeine.{ AsyncLoadingCache, Scaffeine } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick here about spacing before and after {}
.
@@ -41,6 +41,12 @@ auth0 { | |||
# Auth0 Bearer with proper permissions for Management API | |||
bearer = "" | |||
bearer = ${?AUTH0_MANAGEMENT_BEARER} | |||
|
|||
managementClientId = "" | |||
managementClientId = ${?AUTH0_MANAGEMENT_CLIENT_ID} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These settings correspond to the Auth0 non-interactive client, right? How did you authorize it to use the management API? I think we'll have to do that across the three Auth0 accounts for RF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. I had to enable the APIs section (under advanced account settings). I believe that the management API authorization is done through there by creating a non-interactive client. I set credential scopes of that client to only allow CRUD of device_tokens. I do think we'll have to do that for each account and then thread those credentials through Terraform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that I captured the necessary details in #1117 (comment).
I went through and created Auth0 management API clients for each domain. They're all named Web Management API
.
@@ -27,8 +30,44 @@ object TokenService extends Config { | |||
Authorization(GenericHttpCredentials("Bearer", auth0Bearer)) | |||
) | |||
|
|||
val authBearerTokenCache: AsyncLoadingCache[Int, ManagementBearerToken] = | |||
Scaffeine() | |||
.expireAfterWrite(12.hour) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clever idea. My instincts are to take this down to an hour, but not really sure I can come up with a scenario where 12 would lead to a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of a shorter expiration time as the cost is negligible.
@@ -40,4 +40,5 @@ object Dependencies { | |||
val scalacacheCaffeine = "com.github.cb372" %% "scalacache-caffeine" % Version.scalacache | |||
val elasticacheClient = "com.amazonaws" % "elasticache-java-cluster-client" % Version.elasticacheClient | |||
val shapeless = "com.chuusai" %% "shapeless" % Version.shapeless | |||
val findbugAnnotations = "com.google.code.findbugs" % "annotations" % Version.findbugAnnotations % "compile" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What relevance does this library have in the context of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This library is needed (for some reason that I don't fully grasp), to allow for the compiler to understand certain annotations within the caching libraries at compile time...I believe.
97e665d
to
7b245e1
Compare
Misc other profile related updates TODO: allow changing profile metadata such as name. This depends on #1140
This commit alters the approach to obtaining a bearer token that granted access to the auth0 management api. We utlize the management api to create device_tokens which we leveraged when created user's api tokens. Auth0 moved to a more secure token implementation which adopts a 24-hour lifetime token rather than the long-lived token used previously. As a result, we can no longer set an environment variable with the token and use that when doing device_token CRUD operations. This commit uses an async-cache with a 12-hour expiration time (adjustable) to handle getting and storing the current management bearer token.
d65be01
to
3711306
Compare
Misc other profile related updates TODO: allow changing profile metadata such as name. This depends on #1140
Overview
This commit alters the approach to obtaining a bearer token that granted access to the auth0 management api. We utilize the management api to create device_tokens which we leveraged when created user's api tokens.
Auth0 moved to a more secure token implementation which adopts a 24-hour lifetime token rather than the long-lived token used previously. As a result, we can no longer set an environment variable with the token and use that when doing device_token CRUD operations.
See the following links for more details:
This commit uses an async-cache with a 12-hour expiration time (adjustable) to handle getting and storing the current management bearer token.
Checklist
- [ ] Styleguide updated, if necessary- [ ] Swagger specification updated, if necessary- [ ] Symlinks from new migrations present or corrected for any new migrationsNotes
@hectcastro This will require some adjustments operationally. I added environment variables to account for the fact that we now need to add a new Auth0 non-interactive client for all environments. I have not updated the
.env
file itself. The dev account has been adjusted to handle this (although I don't know if that is how you would like to do it).I don't know what the ideal expiration time would be for the tokens. I set it to half of the actual lifetime of the tokens.
This doesn't do any hard token revocation via auth0. This could be something we would want when the token is removed from the cache.
Testing Instructions
This is a little difficult to test. You'll need additional credentials which you can get from me out of band
Connects #1082
Connects #1117