Skip to content

Commit fbaf6f4

Browse files
benbroadawaybenbroadaway
committed
comparable cache key
1 parent 16c97b5 commit fbaf6f4

File tree

6 files changed

+116
-53
lines changed

6 files changed

+116
-53
lines changed

‎github-app-installation/src/main/java/com/walmartlabs/concord/github/appinstallation/AccessTokenProvider.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ public class AccessTokenProvider {
6060
public AccessTokenProvider(GitHubAppInstallationConfig cfg, ObjectMapper objectMapper) {
6161
this.httpTimeout = cfg.getHttpClientTimeout();
6262
this.objectMapper = objectMapper;
63+
// TODO: use thread pool for http client. JDK17 HttpClient creates a new thread per client even for synchronous execution
6364
this.httpClientSupplier = () -> HttpClient.newBuilder() // would be nice to re-use client thread-safely
6465
.connectTimeout(httpTimeout)
6566
.build();
@@ -96,14 +97,13 @@ private String getAccessTokenUrl(String apiBaseUrl, String installationRepo, Str
9697
.build();
9798

9899
var appInstallation = sendRequest(req, 200, AccessTokenProvider.GitHubAppInstallationResp.class, (code, body) -> {
99-
log.warn("getAccessTokenUrl ['{}'] -> error: {} : {}", installationRepo, code, body);
100-
101100
if (code == 404) {
102101
// not possible to discern between repo not found and app not installed for existing (private) repo
103102
log.warn("getAccessTokenUrl ['{}'] -> not found", installationRepo);
104103
return new GitHubAppException.NotFoundException("Repo not found or App installation not found for repo");
105104
}
106105

106+
log.warn("getAccessTokenUrl ['{}'] -> error: {} : {}", installationRepo, code, body);
107107
return new GitHubAppException("Unexpected error locating repo installation: " + code);
108108
});
109109

‎github-app-installation/src/main/java/com/walmartlabs/concord/github/appinstallation/CacheKey.java‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
package com.walmartlabs.concord.github.appinstallation;
2+
3+
/*-
4+
* *****
5+
* Concord
6+
* -----
7+
* Copyright (C) 2017 - 2025 Walmart Inc.
8+
* -----
9+
* Licensed under the Apache License, Version 2.0 (the "License");
10+
* you may not use this file except in compliance with the License.
11+
* You may obtain a copy of the License at
12+
*
13+
* http://www.apache.org/licenses/LICENSE-2.0
14+
*
15+
* Unless required by applicable law or agreed to in writing, software
16+
* distributed under the License is distributed on an "AS IS" BASIS,
17+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
18+
* See the License for the specific language governing permissions and
19+
* limitations under the License.
20+
* =====
21+
*/
22+
23+
import org.immutables.value.Value;
24+
25+
import javax.annotation.Nonnull;
26+
import javax.annotation.Nullable;
27+
import java.net.URI;
28+
import java.util.Objects;
29+
30+
@Value.Immutable
31+
@Value.Style(jdkOnly = true, redactedMask = "**redacted**")
32+
interface CacheKey {
33+
34+
URI repoUri();
35+
36+
@Nullable
37+
@Value.Redacted
38+
byte[] binaryDataSecret();
39+
40+
@Value.Default
41+
default int weight() {
42+
var weight = 1;
43+
44+
if (binaryDataSecret() != null) {
45+
var data = Objects.requireNonNull(binaryDataSecret());
46+
weight += 1;
47+
weight += data.length / 1024;
48+
}
49+
50+
return weight;
51+
}
52+
53+
static CacheKey from(URI repoUri) {
54+
return ImmutableCacheKey.builder()
55+
.repoUri(repoUri)
56+
.build();
57+
}
58+
59+
static CacheKey from(URI repoUri, @Nonnull byte[] secret) {
60+
return ImmutableCacheKey.builder()
61+
.repoUri(repoUri)
62+
.binaryDataSecret(secret)
63+
.build();
64+
}
65+
66+
}

‎github-app-installation/src/main/java/com/walmartlabs/concord/github/appinstallation/GitHubAppInstallation.java‎

Lines changed: 28 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,6 @@ public class GitHubAppInstallation implements AuthTokenProvider {
5454

5555
private final LoadingCache<CacheKey, Optional<ExternalAuthToken>> cache;
5656

57-
private record CacheKey(URI repoUri, Secret secret) {}
58-
5957
@Inject
6058
public GitHubAppInstallation(GitHubAppInstallationConfig cfg, ObjectMapper objectMapper) {
6159
this.cfg = cfg;
@@ -65,23 +63,11 @@ public GitHubAppInstallation(GitHubAppInstallationConfig cfg, ObjectMapper objec
6563
this.cache = CacheBuilder.newBuilder()
6664
.expireAfterWrite(cfg.getSystemAuthCacheDuration())
6765
.maximumWeight(cfg.getSystemAuthCacheMaxWeight())
68-
.weigher((Weigher<CacheKey, Optional<ExternalAuthToken>>) (key, value) -> {
69-
int weight = 1;
70-
71-
if (key.secret() != null) {
72-
weight += 1;
73-
74-
if (key.secret() instanceof BinaryDataSecret bds) {
75-
weight += bds.getData().length / 1024;
76-
}
77-
}
78-
79-
return weight;
80-
})
66+
.weigher((Weigher<CacheKey, Optional<ExternalAuthToken>>) (key, value) -> key.weight())
8167
.build(new CacheLoader<>() {
8268
@Override
8369
public @Nonnull Optional<ExternalAuthToken> load(@Nonnull CacheKey key) {
84-
return fetchToken(key.repoUri(), key.secret());
70+
return fetchToken(key.repoUri(), key.binaryDataSecret());
8571
}
8672
});
8773
}
@@ -91,10 +77,27 @@ public boolean supports(URI repo, @Nullable Secret secret) {
9177
return Utils.validateSecret(secret, objectMapper) || systemSupports(repo);
9278
}
9379

80+
private CacheKey createKey(URI repoUri, @Nullable Secret secret) {
81+
if (secret == null) {
82+
return CacheKey.from(repoUri);
83+
}
84+
85+
if (secret instanceof BinaryDataSecret bds) {
86+
return CacheKey.from(repoUri, bds.getData());
87+
}
88+
89+
return null;
90+
}
91+
9492
@Override
9593
public Optional<ExternalAuthToken> getToken(URI repo, @Nullable Secret secret) {
94+
var cacheKey = createKey(repo, secret);
95+
96+
if (cacheKey == null) {
97+
return Optional.empty();
98+
}
99+
96100
try {
97-
var cacheKey = new CacheKey(repo, secret);
98101
var activeToken = cache.get(cacheKey);
99102

100103
return activeToken.map(t -> refreshBeforeExpire(t, cacheKey));
@@ -120,9 +123,9 @@ private boolean systemSupports(URI repoUri) {
120123
return cfg.getAuthConfigs().stream().anyMatch(auth -> auth.canHandle(repoUri));
121124
}
122125

123-
private Optional<ExternalAuthToken> fetchToken(URI repo, @Nullable Secret secret) {
126+
private Optional<ExternalAuthToken> fetchToken(URI repo, @Nullable byte[] secret) {
124127
if (secret != null) {
125-
return fromSecret(repo, secret);
128+
return Optional.ofNullable(fromBinaryData(repo, secret));
126129
}
127130

128131
// no secret, see if system config has something for this repo
@@ -169,24 +172,16 @@ private ExternalAuthToken refreshBeforeExpire(@Nonnull ExternalAuthToken token,
169172
return token;
170173
}
171174

172-
private Optional<ExternalAuthToken> fromSecret(URI repo, @Nonnull Secret secret) {
173-
if (secret instanceof BinaryDataSecret bds) {
174-
return Optional.ofNullable(fromBinaryData(repo, bds));
175-
}
176-
177-
return Optional.empty();
178-
}
179-
180-
private ExternalAuthToken fromBinaryData(URI repo, BinaryDataSecret bds) {
181-
var appInfo = Utils.parseAppInstallation(bds, objectMapper);
175+
private ExternalAuthToken fromBinaryData(URI repo, byte[] data) {
176+
var appInfo = Utils.parseAppInstallation(data, objectMapper);
182177
if (appInfo.isPresent()) {
183178
// great, it's apparently a valid app installation config
184179
return getTokenFromAppInstall(appInfo.get(), repo);
185180
}
186181

187182
// hopefully it's just a token a plaintext token
188183
return GitHubInstallationToken.builder()
189-
.token(new String(bds.getData()).trim())
184+
.token(new String(data).trim())
190185
.build();
191186
}
192187

@@ -196,8 +191,9 @@ private ExternalAuthToken getTokenFromAppInstall(GitHubAppAuthConfig app, URI re
196191
try {
197192
var ownerAndRepo = Utils.extractOwnerAndRepo(app, repo);
198193
return accessTokenProvider().getRepoInstallationToken(app, ownerAndRepo);
199-
} catch (RepoExtractionException e) {
200-
log.warn("Error retrieving GitHub access token", e);
194+
} catch (RepoExtractionException | GitHubAppException e) {
195+
var msg = e.getMessage();
196+
log.warn("Error retrieving GitHub access token: {}", msg);
201197
}
202198

203199
return null;

‎github-app-installation/src/main/java/com/walmartlabs/concord/github/appinstallation/Utils.java‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ static boolean validateSecret(Secret secret, ObjectMapper mapper) {
5252
return false;
5353
}
5454

55-
var base = parseRawAppInstallation(bds, mapper);
55+
var base = parseRawAppInstallation(bds.getData(), mapper);
5656
if (base == null) {
5757
// It's not JSON, may be an oauth token
5858
return isPrintableAscii(bds.getData());
@@ -62,7 +62,7 @@ static boolean validateSecret(Secret secret, ObjectMapper mapper) {
6262
}
6363

6464
// App installation config format is either valid or not
65-
return parseAppInstallation(bds, mapper).isPresent();
65+
return parseAppInstallation(bds.getData(), mapper).isPresent();
6666
}
6767

6868
private static boolean isPrintableAscii(byte[] bytes) {
@@ -81,10 +81,9 @@ private static boolean isPrintableAscii(byte[] bytes) {
8181
return true;
8282
}
8383

84-
static Map<?, ?> parseRawAppInstallation(BinaryDataSecret bds,
85-
ObjectMapper mapper) {
84+
static Map<?, ?> parseRawAppInstallation(byte[] bds, ObjectMapper mapper) {
8685
try { // find out if it's at least valid JSON.
87-
var base = mapper.readValue(bds.getData(), Map.class);
86+
var base = mapper.readValue(bds, Map.class);
8887
if (base.containsKey("githubAppInstallation")) {
8988
return base;
9089
} else {
@@ -97,8 +96,7 @@ private static boolean isPrintableAscii(byte[] bytes) {
9796
}
9897
}
9998

100-
static Optional<GitHubAppAuthConfig> parseAppInstallation(BinaryDataSecret bds,
101-
ObjectMapper mapper) {
99+
static Optional<GitHubAppAuthConfig> parseAppInstallation(byte[] bds, ObjectMapper mapper) {
102100
Map<?, ?> base = parseRawAppInstallation(bds, mapper);
103101

104102
if (base == null || !base.containsKey("githubAppInstallation")) {

‎github-app-installation/src/test/java/com/walmartlabs/concord/github/appinstallation/UtilsTest.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ void testValidateSecret() {
5656

5757
@Test
5858
void parseAppInstallation_ValidJson() {
59-
var o = parseAppInstallation(new BinaryDataSecret(APP_INSTALL_CONTENT.getBytes()), MAPPPER);
59+
var o = parseAppInstallation(APP_INSTALL_CONTENT.getBytes(), MAPPPER);
6060

6161
assertTrue(o.isPresent());
6262
var result = o.get();
@@ -72,32 +72,32 @@ void parseAppInstallation_MissingElement() {
7272
"privateKey": "mock-key-data"
7373
}
7474
}""";
75-
var secret = new BinaryDataSecret(missingClientId.getBytes());
76-
var ex = assertThrows(GitHubAppException.class, () -> parseAppInstallation(secret, MAPPPER));
75+
var data = missingClientId.getBytes();
76+
var ex = assertThrows(GitHubAppException.class, () -> parseAppInstallation(data, MAPPPER));
7777

7878
assertTrue(ex.getMessage().contains("Invalid app installation definition"));
7979
}
8080

8181
@Test
8282
void parseAppInstallation_OtherJson() {
8383
var unexpectedJson = "{ \"valid\": \"but not usable here\"}";
84-
var result = parseAppInstallation(new BinaryDataSecret(unexpectedJson.getBytes()), MAPPPER);
84+
var result = parseAppInstallation(unexpectedJson.getBytes(), MAPPPER);
8585

8686
assertFalse(result.isPresent());
8787
}
8888

8989
@Test
9090
void parseRawAppInstallation_NotJson() {
9191
var unexpectedJson = "justText";
92-
var result = parseRawAppInstallation(new BinaryDataSecret(unexpectedJson.getBytes()), MAPPPER);
92+
var result = parseRawAppInstallation(unexpectedJson.getBytes(), MAPPPER);
9393

9494
assertNull(result);
9595
}
9696

9797
@Test
9898
void parseRawAppInstallation_OtherJson() {
9999
var unexpectedJson = "{ \"valid\": \"but not usable here\"}";
100-
var result = parseRawAppInstallation(new BinaryDataSecret(unexpectedJson.getBytes()), MAPPPER);
100+
var result = parseRawAppInstallation(unexpectedJson.getBytes(), MAPPPER);
101101

102102
assertNotNull(result);
103103
assertTrue(result.isEmpty());

‎server/impl/src/main/java/com/walmartlabs/concord/server/org/secret/SystemResource.java‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,16 @@
2222

2323
import com.walmartlabs.concord.common.ExternalAuthToken;
2424
import com.walmartlabs.concord.common.AuthTokenProvider;
25-
import com.walmartlabs.concord.repository.RepositoryException;
25+
import com.walmartlabs.concord.github.appinstallation.exception.GitHubAppException;
2626
import com.walmartlabs.concord.server.sdk.ConcordApplicationException;
2727
import com.walmartlabs.concord.server.sdk.rest.Resource;
28+
import com.walmartlabs.concord.server.sdk.validation.Validate;
2829
import com.walmartlabs.concord.server.security.Permission;
2930
import io.swagger.v3.oas.annotations.Operation;
3031
import io.swagger.v3.oas.annotations.tags.Tag;
3132

3233
import javax.inject.Inject;
34+
import javax.validation.constraints.NotNull;
3335
import javax.ws.rs.*;
3436
import javax.ws.rs.core.MediaType;
3537
import javax.ws.rs.core.Response;
@@ -52,18 +54,19 @@ public SystemResource(AuthTokenProvider authTokenProvider) {
5254
@GET
5355
@Path("/gitauth")
5456
@Produces(MediaType.APPLICATION_JSON)
57+
@Validate
5558
@Operation(description = "Retrieves system-provided auth for give repository URI. Requires externalTokenLookup permission. ", operationId = "getExternalToken")
56-
public ExternalAuthToken getExternalToken(@QueryParam("repoUri") URI repoUri) {
59+
public ExternalAuthToken getExternalToken(@QueryParam("repoUri") @NotNull URI repoUri) {
5760
assertSystemGitAuthPermission();
5861

5962
try {
6063
return authTokenProvider.getToken(repoUri, null)
6164
.orElseThrow(() -> new ConcordApplicationException("No system-provided auth found for the given repository URI: " + repoUri, Response.Status.NOT_FOUND));
62-
} catch (RepositoryException.NotFoundException e) {
63-
throw new ConcordApplicationException(e.getMessage(), Response.Status.BAD_REQUEST);
64-
} catch (RepositoryException e) {
65-
// TODO 500?
65+
} catch (GitHubAppException.NotFoundException e) {
66+
// repo issue, or app not installed
6667
throw new ConcordApplicationException(e.getMessage(), Response.Status.NOT_FOUND);
68+
} catch (GitHubAppException e) {
69+
throw new ConcordApplicationException(e.getMessage(), Response.Status.BAD_REQUEST);
6770
}
6871
}
6972

0 commit comments

Comments
 (0)