From bcbdad10f87e2aa323c9e334da51b95fd14c109d Mon Sep 17 00:00:00 2001
From: Fabian Heymann <fabian.heymann@finanzcheck.de>
Date: Fri, 13 Oct 2017 18:46:26 +0200
Subject: [PATCH] fix(authz/github): Add cache to team membership check to
 prevent excessive http requests (#200)

* fix(authz/github): Add cache to team membership check to prevent excessive http requests
* [squash later] review remarks
* [squash after] move cache initialization
---
 .../fiat/roles/github/GitHubProperties.java   |  2 +
 .../github/GithubTeamsUserRolesProvider.java  | 49 ++++++++++++++++---
 2 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GitHubProperties.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GitHubProperties.java
index 559097e7e..2ae0756b0 100644
--- a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GitHubProperties.java
+++ b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GitHubProperties.java
@@ -28,4 +28,6 @@ public class GitHubProperties {
   @Max(100L)
   @Min(1L)
   Integer paginationValue = 100;
+  @NotNull
+  Integer membershipCacheTTLSeconds = 60 * 1000;
 }
diff --git a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java
index 056497125..cd31f02b4 100644
--- a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java
+++ b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java
@@ -16,11 +16,15 @@
 
 package com.netflix.spinnaker.fiat.roles.github;
 
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
 import com.netflix.spinnaker.fiat.model.resources.Role;
 import com.netflix.spinnaker.fiat.roles.UserRolesProvider;
 import com.netflix.spinnaker.fiat.roles.github.client.GitHubClient;
 import com.netflix.spinnaker.fiat.roles.github.model.Team;
 import com.netflix.spinnaker.fiat.roles.github.model.TeamMembership;
+import lombok.Data;
 import lombok.Setter;
 import lombok.extern.slf4j.Slf4j;
 import lombok.val;
@@ -40,6 +44,8 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
 
 @Slf4j
@@ -61,9 +67,36 @@ public class GithubTeamsUserRolesProvider implements UserRolesProvider, Initiali
   @Setter
   private GitHubProperties gitHubProperties;
 
+  private LoadingCache<CacheKey, Boolean> teamMembershipCache;
+
+  private static final String ACTIVE = "active";
+
   @Override
   public void afterPropertiesSet() throws Exception {
     Assert.state(gitHubProperties.getOrganization() != null, "Supply an organization");
+    Assert.state(gitHubProperties.getBaseUrl() != null, "Supply a base url");
+
+    this.initializeTeamMembershipCache();
+  }
+
+  private void initializeTeamMembershipCache() {
+    this.teamMembershipCache = CacheBuilder.newBuilder()
+        .maximumSize(1000)
+        .expireAfterWrite(this.gitHubProperties.getMembershipCacheTTLSeconds(), TimeUnit.SECONDS)
+        .build(
+            new CacheLoader<CacheKey, Boolean>() {
+              public Boolean load(CacheKey key) {
+                try {
+                  TeamMembership response = gitHubClient.isMemberOfTeam(key.getTeamId(), key.getUsername());
+                  return (response.getState().equals(GithubTeamsUserRolesProvider.ACTIVE));
+                } catch (RetrofitError e) {
+                  if (e.getResponse().getStatus() != 404) {
+                    handleNon404s(e);
+                  }
+                }
+                return false;
+              }
+            });
   }
 
   @Override
@@ -151,14 +184,10 @@ private List<Team> getTeamsInOrgPaginated(int page) {
   }
 
   private boolean isMemberOfTeam(Team t, String username) {
-    String ACTIVE = "active";
     try {
-      TeamMembership response = gitHubClient.isMemberOfTeam(t.getId(), username);
-      return (response.getState().equals(ACTIVE));
-    } catch (RetrofitError e) {
-      if (e.getResponse().getStatus() != 404) {
-        handleNon404s(e);
-      }
+      return this.teamMembershipCache.get(new CacheKey(t.getId(), username));
+    } catch (ExecutionException e) {
+      log.error("Failed to read from cache when getting team membership", e);
     }
     return false;
   }
@@ -197,4 +226,10 @@ public Map<String, Collection<Role>> multiLoadRoles(Collection<String> userEmail
 
     return emailGroupsMap;
   }
+
+  @Data
+  private class CacheKey {
+    private final Long teamId;
+    private final String username;
+  }
 }