-
Notifications
You must be signed in to change notification settings - Fork 11
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
Cache results of group checker #279
Conversation
Looks good, but have you tested this with an application in CODE? |
I've tested with an app running locally (using sbt publishLocal), which I think is as good as CODE? |
Difficult to test with multiple users locally? It would be good to demonstrate that a user logging in with the correct group membership doesn't then cause subsequent requests from a user who is not in the group to be accepted. I've kicked off a preview release so you can ensure this works as expected in CODE. |
@davidfurey has published a preview version of this PR with release workflow run #56, based on commit 6a9d92e: 18.0.0-PREVIEW.tf-cache-groups.2025-01-09T1115.6a9d92ef Want to make another preview release?Click 'Run workflow' in the GitHub UI, specifying the tf-cache-groups branch, or use the GitHub CLI command: gh workflow run release.yml --ref tf-cache-groups Want to make a full release after this PR is merged?Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command: gh workflow run release.yml |
Thanks @davidfurey |
} yield resp.getGroups.asScala.map(_.getEmail).toSet | ||
|
||
type Email = String | ||
private val cache: LoadingCache[Email, Set[String]] = CacheBuilder.newBuilder() |
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.
Only authed users will end up in the cache, so the unbounded cache size is fine.
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.
lgtm
The
GoogleGroupChecker
class makes requests to the Google directory service to find each user's groups.Currently if the
requireGroup
Filter is used to check group memberships, a request is made to the Google API every time. I have found that these requests can take ~600ms each time, which can really add up if a webapp needs to make multiple requests, or if therequireGroup
Filter is chained.This PR introduces a cache to the
GoogleGroupChecker
class to reduce the number of calls to the Google directory. The cache duration is now a class parameter, and defaults to 1 minute.Note - I've used Google's guava cache here because it's already available in this library.