Skip to content
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.

Sign up for GitHub

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 #468 by implementing short-time cache for authorization middleware #502

Merged
merged 3 commits into from
Jul 8, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f17e2f
Fix #468 by implementing short-time cache for authorization middleware
seratch Jun 24, 2020
d09b4f5
Stabilize the tests
seratch Jun 24, 2020
62c102d
Add permanent cache option and cleanup thread for multi workspace aut…
seratch Jun 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions bolt/src/main/java/com/slack/api/bolt/AppConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,21 @@ public boolean isDistributedApp() {
@Builder.Default
private boolean oAuthCallbackEnabled = false;


/**
* Returns true if auth.test call result cache in SingleTeamAuthorization or MultiTeamsAuthorization middleware
* is enabled. The default is false.
*/
@Builder.Default
private boolean authTestCacheEnabled = false;

/**
* Returns the millisecond value to keep cached auth.test response in cache.
* Negative value indicates the cache is permanent. The default is 3000 milliseconds.
*/
@Builder.Default
private long authTestCacheExpirationMillis = 3000L;

@Builder.Default
// https://api.slack.com/authentication/migration
private boolean classicAppPermissionsEnabled = false;
Expand Down
79 changes: 78 additions & 1 deletion bolt/src/main/java/com/slack/api/bolt/middleware/builtin/MultiTeamsAuthorization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,23 @@
import com.slack.api.bolt.response.Responder;
import com.slack.api.bolt.response.Response;
import com.slack.api.bolt.service.InstallationService;
import com.slack.api.methods.MethodsClient;
import com.slack.api.methods.SlackApiException;
import com.slack.api.methods.response.auth.AuthTestResponse;
import com.slack.api.model.block.LayoutBlock;
import com.slack.api.util.thread.ExecutorServiceFactory;
import lombok.AllArgsConstructor;
import lombok.Data;
import lombok.extern.slf4j.Slf4j;

import java.io.IOException;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;

import static com.slack.api.bolt.middleware.MiddlewareOps.isNoAuthRequiredRequest;
import static com.slack.api.bolt.response.ResponseTypes.ephemeral;
Expand All @@ -31,6 +41,17 @@ public class MultiTeamsAuthorization implements Middleware {
private final AppConfig config;
private final InstallationService installationService;

@Data
@AllArgsConstructor
static class CachedAuthTestResponse {
private AuthTestResponse response;
private long cachedMillis;
}

// token -> auth.test response
private final ConcurrentMap<String, CachedAuthTestResponse> tokenToAuthTestCache = new ConcurrentHashMap<>();
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

private final Optional<ScheduledExecutorService> tokenToAuthTestCacheCleaner;

private boolean alwaysRequestUserTokenNeeded;

public boolean isAlwaysRequestUserTokenNeeded() {
Expand All @@ -45,6 +66,40 @@ public MultiTeamsAuthorization(AppConfig config, InstallationService installatio
this.config = config;
this.installationService = installationService;
setAlwaysRequestUserTokenNeeded(config.isAlwaysRequestUserTokenNeeded());
if (config.isAuthTestCacheEnabled()) {
boolean permanentCacheEnabled = config.getAuthTestCacheExpirationMillis() < 0;
if (permanentCacheEnabled) {
this.tokenToAuthTestCacheCleaner = Optional.empty();
} else {
this.tokenToAuthTestCacheCleaner = Optional.of(buildTokenToAuthTestCacheCleaner(() -> {
long expirationMillis = System.currentTimeMillis() - config.getAuthTestCacheExpirationMillis();
for (Map.Entry<String, CachedAuthTestResponse> each : tokenToAuthTestCache.entrySet()) {
if (each.getValue() == null || each.getValue().getCachedMillis() < expirationMillis) {
tokenToAuthTestCache.remove(each.getKey());
}
}
}));
}
} else {
this.tokenToAuthTestCacheCleaner = Optional.empty();
}
}

private ScheduledExecutorService buildTokenToAuthTestCacheCleaner(Runnable task) {
String threadGroupName = MultiTeamsAuthorization.class.getSimpleName();
ScheduledExecutorService tokenToAuthTestCacheCleaner =
ExecutorServiceFactory.createDaemonThreadScheduledExecutor(threadGroupName);
tokenToAuthTestCacheCleaner.scheduleAtFixedRate(task, 120_000, 30_000, TimeUnit.MILLISECONDS);
log.debug("The tokenToAuthTestCacheCleaner (daemon thread) started");
return tokenToAuthTestCacheCleaner;
}

@Override
protected void finalize() throws Throwable {
if (this.tokenToAuthTestCacheCleaner.isPresent()) {
this.tokenToAuthTestCacheCleaner.get().shutdown();
}
super.finalize();
}

@Override
Expand Down Expand Up @@ -110,7 +165,7 @@ public Response apply(Request req, Response resp, MiddlewareChain chain) throws

try {
String token = botToken != null ? botToken : userToken;
AuthTestResponse authTestResponse = context.client().authTest(r -> r.token(token));
AuthTestResponse authTestResponse = callAuthTest(token, config, context.client());
if (authTestResponse.isOk()) {
context.setBotToken(botToken);
context.setRequestUserToken(userToken);
Expand All @@ -131,6 +186,28 @@ public Response apply(Request req, Response resp, MiddlewareChain chain) throws
}
}

protected AuthTestResponse callAuthTest(String token, AppConfig config, MethodsClient client) throws IOException, SlackApiException {
if (config.isAuthTestCacheEnabled()) {
CachedAuthTestResponse cachedResponse = tokenToAuthTestCache.get(token);
if (cachedResponse != null) {
boolean permanentCacheEnabled = config.getAuthTestCacheExpirationMillis() < 0;
if (permanentCacheEnabled) {
return cachedResponse.getResponse();
}
long millisToExpire = cachedResponse.getCachedMillis() + config.getAuthTestCacheExpirationMillis();
if (millisToExpire > System.currentTimeMillis()) {
return cachedResponse.getResponse();
}
}
AuthTestResponse response = client.authTest(r -> r.token(token));
CachedAuthTestResponse newCache = new CachedAuthTestResponse(response, System.currentTimeMillis());
tokenToAuthTestCache.put(token, newCache);
return response;
} else {
return client.authTest(r -> r.token(token));
}
}

protected Response handleAuthTestError(
String errorCode,
Bot foundBot,
Expand Down
34 changes: 33 additions & 1 deletion bolt/src/main/java/com/slack/api/bolt/middleware/builtin/SingleTeamAuthorization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,15 @@
import com.slack.api.bolt.request.Request;
import com.slack.api.bolt.response.Response;
import com.slack.api.bolt.service.InstallationService;
import com.slack.api.methods.MethodsClient;
import com.slack.api.methods.SlackApiException;
import com.slack.api.methods.response.auth.AuthTestResponse;
import lombok.extern.slf4j.Slf4j;

import java.io.IOException;
import java.util.Optional;
import java.util.concurrent.atomic.AtomicLong;

import static com.slack.api.bolt.middleware.MiddlewareOps.isNoAuthRequiredRequest;

/**
Expand All @@ -22,6 +28,9 @@ public class SingleTeamAuthorization implements Middleware {
private final AppConfig appConfig;
private final InstallationService installationService;

private Optional<AuthTestResponse> cachedAuthTestResponse = Optional.empty();
private AtomicLong lastCachedMillis = new AtomicLong(0L);
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


public SingleTeamAuthorization(AppConfig appConfig, InstallationService installationService) {
this.appConfig = appConfig;
this.installationService = installationService;
Expand All @@ -34,7 +43,7 @@ public Response apply(Request req, Response resp, MiddlewareChain chain) throws
}

Context context = req.getContext();
AuthTestResponse authResult = context.client().authTest(r -> r.token(appConfig.getSingleTeamBotToken()));
AuthTestResponse authResult = callAuthTest(appConfig, context.client());
if (authResult.isOk()) {
if (context.getBotToken() == null) {
context.setBotToken(appConfig.getSingleTeamBotToken());
Expand Down Expand Up @@ -72,4 +81,27 @@ public Response apply(Request req, Response resp, MiddlewareChain chain) throws
.build();
}
}

protected AuthTestResponse callAuthTest(AppConfig config, MethodsClient client) throws IOException, SlackApiException {
if (config.isAuthTestCacheEnabled()) {
if (cachedAuthTestResponse.isPresent()) {
boolean permanentCacheEnabled = config.getAuthTestCacheExpirationMillis() < 0;
if (permanentCacheEnabled) {
return cachedAuthTestResponse.get();
}
long millisToExpire = lastCachedMillis.get() + config.getAuthTestCacheExpirationMillis();
long currentMillis = System.currentTimeMillis();
if (millisToExpire > currentMillis) {
return cachedAuthTestResponse.get();
}
}
AuthTestResponse response = client.authTest(r -> r.token(config.getSingleTeamBotToken()));
cachedAuthTestResponse = Optional.of(response); // response here is not null for sure
lastCachedMillis.set(System.currentTimeMillis());
return response;
} else {
return client.authTest(r -> r.token(config.getSingleTeamBotToken()));
}
}

}
Loading