Fix #468 by implementing short-time cache for authorization middleware #502

seratch · 2020-06-24T09:15:34Z

Summary

This pull request fixes #468 by introducing new options as below.

AppConfig#authTestCacheEnabled (default: false)
AppConfig#authTestCacheExpirationMillis (default: 3000)

The default behavior won't be changed. Only when a Bolt app turns the flag on, the cache will be enabled.

The cache layer doesn't support distributed cache implementations (e.g., the ones using Memcached, Redis) by design. As mentioned in #468, the purpose of this cache is to reduce the number of auth.test API calls in Bolt apps that tend to receive lots of incoming requests from Slack in a short time of period.

TODO: Update document when releasing v1.1

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

seratch · 2020-06-24T09:37:49Z

Will fix the build failure. It’s just a matter of test code.

codecov · 2020-06-24T14:04:58Z

Codecov Report

Merging #502 into master will increase coverage by 0.16%.
The diff coverage is 85.71%.

@@             Coverage Diff              @@
##             master     #502      +/-   ##
============================================
+ Coverage     83.77%   83.93%   +0.16%     
- Complexity     2638     2665      +27     
============================================
  Files           294      294              
  Lines          7043     7097      +54     
  Branches        579      592      +13     
============================================
+ Hits           5900     5957      +57     
  Misses          783      783              
+ Partials        360      357       -3

Impacted Files	Coverage Δ	Complexity Δ
...lt/src/main/java/com/slack/api/bolt/AppConfig.java	`82.35% <ø> (ø)`	`9.00 <0.00> (+1.00)`
...lt/middleware/builtin/MultiTeamsAuthorization.java	`72.72% <79.48%> (+6.06%)`	`26.00 <14.00> (+13.00)`
...lt/middleware/builtin/SingleTeamAuthorization.java	`91.83% <100.00%> (+6.98%)`	`15.00 <9.00> (+8.00)`
bolt/src/main/java/com/slack/api/bolt/App.java	`72.11% <0.00%> (+0.69%)`	`113.00% <0.00%> (+3.00%)`
...i/methods/metrics/impl/MemoryMetricsDatastore.java	`92.25% <0.00%> (+0.70%)`	`42.00% <0.00%> (+1.00%)`
...pi/methods/metrics/impl/RedisMetricsDatastore.java	`89.50% <0.00%> (+2.46%)`	`38.00% <0.00%> (+1.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cd2da3...62c102d. Read the comment docs.

seratch · 2020-06-26T16:29:43Z

bolt/src/main/java/com/slack/api/bolt/middleware/builtin/MultiTeamsAuthorization.java

+    }
+
+    // token -> auth.test response
+    private final ConcurrentMap<String, CachedAuthTestResponse> tokenToAuthTestCache = new ConcurrentHashMap<>();


We may want to clean up very old cache data to avoid excessive memory usage in extreme cases. As I don't want to add any dependencies only for this, we can implement a simple mechanism on our own.

seratch · 2020-06-26T16:37:42Z

bolt/src/main/java/com/slack/api/bolt/middleware/builtin/SingleTeamAuthorization.java

@@ -22,6 +28,9 @@
    private final AppConfig appConfig;
    private final InstallationService installationService;

+    private Optional<AuthTestResponse> cachedAuthTestResponse = Optional.empty();
+    private AtomicLong lastCachedMillis = new AtomicLong(0L);


Thanks to @aoberoi 's suggestion here: #468 (comment), we can remove this TTL and store the response when booting a Bolt app as with Bolt for JS.

I came to think removing TTL from only single team auth may be confusing. I decided to add a "permanent" option by giving a negative TTL value instead.

seratch · 2020-06-30T08:24:16Z

This PR is ready for review. I'll wait for other maintianers' responses for a few business days.

…iddleware

…horization

@emanguy

* [kotlin-extension] slackapi#428 slackapi#469 slackapi#501 slackapi#503 slackapi#504 slackapi#513 Add Kotlin DSL modules for constructing Block Kit payloads - thnaks @emanguy @seratch * [bolt] slackapi#502 slackapi#468 short-time cache for authorization middleware - thanks @eamelink @seratch * [slack-api-client] slackapi#508 Bump okhttp version from 4.7.2 to 4.8.0 - thanks @seratch * [slack-api-client] slackapi#500 slackapi#499 Add proxy system properties support (http.proxyHost / http.proxyPort) - thanks @seratch * [slack-api-client] slackapi#512 Redact authorization header from debug logging outputs - thanks @seratch * [slack-api-client] slackapi#507 Add admin.conversations.restrictAccess.* APIs - thanks @seratch * [slack-api-client] slackapi#509 Add conversations.mark API - thanks @seratch * [slack-api-client] slackapi#505 Add calls.participants.remove, admin.usergroups.addTeams API - thanks @seratch * [slack-app-backend] slackapi#494 slackapi#496 Add file_share message events & files in message_changed events - thanks @Hariprasad-Ramakrishnan @seratch * [bolt-micronaut] slackapi#508 Bump micronaut from 1.3 to 2.0 - thanks @seratch

@emanguy

* [kotlin-extension] slackapi#428 slackapi#469 slackapi#501 slackapi#503 slackapi#504 slackapi#513 Add Kotlin DSL modules for constructing Block Kit payloads - thnaks @emanguy @seratch * [bolt] slackapi#502 slackapi#468 short-time cache for authorization middleware - thanks @eamelink @seratch * [slack-api-client] slackapi#508 Bump okhttp version from 4.7.2 to 4.8.0 - thanks @seratch * [slack-api-client] slackapi#500 slackapi#499 Add proxy system properties support (http.proxyHost / http.proxyPort) - thanks @seratch * [slack-api-client] slackapi#512 Redact authorization header from debug logging outputs - thanks @seratch * [slack-api-client] slackapi#507 Add admin.conversations.restrictAccess.* APIs - thanks @seratch * [slack-api-client] slackapi#509 Add conversations.mark API - thanks @seratch * [slack-api-client] slackapi#505 Add calls.participants.remove, admin.usergroups.addTeams API - thanks @seratch * [slack-app-backend] slackapi#494 slackapi#496 Add file_share message events & files in message_changed events - thanks @Hariprasad-Ramakrishnan @seratch * [bolt-micronaut] slackapi#508 Bump micronaut from 1.3 to 2.0 - thanks @seratch

@emanguy

* [kotlin-extension] slackapi#428 slackapi#469 slackapi#501 slackapi#503 slackapi#504 slackapi#513 Add Kotlin DSL modules for constructing Block Kit payloads - thnaks @emanguy @seratch * [bolt] slackapi#502 slackapi#468 short-time cache for authorization middleware - thanks @eamelink @seratch * [slack-api-client] slackapi#508 Bump okhttp version from 4.7.2 to 4.8.0 - thanks @seratch * [slack-api-client] slackapi#500 slackapi#499 Add proxy system properties support (http.proxyHost / http.proxyPort) - thanks @seratch * [slack-api-client] slackapi#512 Redact authorization header from debug logging outputs - thanks @seratch * [slack-api-client] slackapi#507 Add admin.conversations.restrictAccess.* APIs - thanks @seratch * [slack-api-client] slackapi#509 Add conversations.mark API - thanks @seratch * [slack-api-client] slackapi#505 Add calls.participants.remove, admin.usergroups.addTeams API - thanks @seratch * [slack-app-backend] slackapi#494 slackapi#496 Add file_share message events & files in message_changed events - thanks @Hariprasad-Ramakrishnan @seratch * [bolt-micronaut] slackapi#508 Bump micronaut from 1.3 to 2.0 - thanks @seratch

@emanguy

* [kotlin-extension] slackapi#428 slackapi#469 slackapi#501 slackapi#503 slackapi#504 slackapi#513 Add Kotlin DSL modules for constructing Block Kit payloads - thnaks @emanguy @seratch * [bolt] slackapi#502 slackapi#468 short-time cache for authorization middleware - thanks @eamelink @seratch * [slack-api-client] slackapi#508 Bump okhttp version from 4.7.2 to 4.8.0 - thanks @seratch * [slack-api-client] slackapi#500 slackapi#499 Add proxy system properties support (http.proxyHost / http.proxyPort) - thanks @seratch * [slack-api-client] slackapi#512 Redact authorization header from debug logging outputs - thanks @seratch * [slack-api-client] slackapi#507 Add admin.conversations.restrictAccess.* APIs - thanks @seratch * [slack-api-client] slackapi#509 Add conversations.mark API - thanks @seratch * [slack-api-client] slackapi#505 Add calls.participants.remove, admin.usergroups.addTeams API - thanks @seratch * [slack-app-backend] slackapi#494 slackapi#496 Add file_share message events & files in message_changed events - thanks @Hariprasad-Ramakrishnan @seratch * [bolt-micronaut] slackapi#508 Bump micronaut from 1.3 to 2.0 - thanks @seratch

seratch added enhancement M-T: A feature request for new functionality project:bolt labels Jun 24, 2020

seratch added this to the 1.1.0 milestone Jun 24, 2020

seratch requested review from stevengill and aoberoi June 24, 2020 09:15

seratch self-assigned this Jun 24, 2020

seratch commented Jun 26, 2020

View reviewed changes

seratch force-pushed the issue-468 branch from b9e70a7 to a249af2 Compare July 2, 2020 22:15

seratch added 3 commits July 8, 2020 10:28

Fix slackapi#468 by implementing short-time cache for authorization m…

8f17e2f

…iddleware

Stabilize the tests

d09b4f5

Add permanent cache option and cleanup thread for multi workspace aut…

62c102d

…horization

seratch force-pushed the issue-468 branch from a249af2 to 62c102d Compare July 8, 2020 01:29

seratch merged commit a973bb4 into slackapi:master Jul 8, 2020

seratch deleted the issue-468 branch July 8, 2020 06:13

seratch mentioned this pull request Jul 13, 2020

version 1.1.0 release #514

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #468 by implementing short-time cache for authorization middleware #502

Fix #468 by implementing short-time cache for authorization middleware #502

seratch commented Jun 24, 2020

seratch commented Jun 24, 2020

codecov bot commented Jun 24, 2020 •

edited

Loading

seratch Jun 26, 2020

seratch Jun 30, 2020

seratch Jun 26, 2020

seratch Jun 30, 2020

seratch commented Jun 30, 2020

Fix #468 by implementing short-time cache for authorization middleware #502

Fix #468 by implementing short-time cache for authorization middleware #502

Conversation

seratch commented Jun 24, 2020

Summary

Requirements (place an x in each [ ])

seratch commented Jun 24, 2020

codecov bot commented Jun 24, 2020 • edited Loading

Codecov Report

seratch Jun 26, 2020

Choose a reason for hiding this comment

seratch Jun 30, 2020

Choose a reason for hiding this comment

seratch Jun 26, 2020

Choose a reason for hiding this comment

seratch Jun 30, 2020

Choose a reason for hiding this comment

seratch commented Jun 30, 2020

Requirements (place an `x` in each `[ ]`)

codecov bot commented Jun 24, 2020 •

edited

Loading