Skip to content

Commit d80588e

Browse files
committed
Update based on review feedback
1 parent 3ac6082 commit d80588e

File tree

3 files changed

+128
-174
lines changed

3 files changed

+128
-174
lines changed

src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import edu.umd.cs.findbugs.annotations.NonNull;
88
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
99
import hudson.Extension;
10+
import hudson.Functions;
1011
import hudson.Util;
1112
import hudson.remoting.Channel;
1213
import hudson.util.FormValidation;
@@ -121,7 +122,12 @@ static AppInstallationToken generateAppInstallationToken(String appId, String ap
121122
.withJwtToken(jwtToken)
122123
.build();
123124

124-
GHApp app = gitHubApp.getApp();
125+
GHApp app;
126+
try {
127+
app = gitHubApp.getApp();
128+
} catch (IOException e) {
129+
throw new IllegalArgumentException(String.format(ERROR_AUTHENTICATING_GITHUB_APP, appId), e);
130+
}
125131

126132
List<GHAppInstallation> appInstallations = app.listInstallations().asList();
127133
if (appInstallations.isEmpty()) {
@@ -134,30 +140,23 @@ static AppInstallationToken generateAppInstallationToken(String appId, String ap
134140
appInstallation = appInstallations.stream()
135141
.filter(installation -> installation.getAccount().getLogin().equals(owner))
136142
.findAny()
137-
.orElseThrow(() -> new IllegalArgumentException(String.format(
138-
ERROR_NOT_INSTALLED,
139-
appId)));
143+
.orElseThrow(() -> new IllegalArgumentException(String.format(ERROR_NOT_INSTALLED, appId)));
140144
}
141145

142146
GHAppInstallationToken appInstallationToken = appInstallation
143147
.createToken(appInstallation.getPermissions())
144148
.create();
145149

146150
long expiration = getExpirationSeconds(appInstallationToken);
147-
LOGGER.log(Level.FINEST,
148-
"Token raw expiration epoch seconds: {0,number,#}",
149-
expiration);
150-
151151
AppInstallationToken token = new AppInstallationToken(appInstallationToken.getToken(),
152152
expiration);
153-
LOGGER.log(Level.FINE,
153+
LOGGER.log(Level.FINER,
154154
"Generated App Installation Token for app ID {0}",
155155
appId);
156156

157157
return token;
158158
} catch (Exception e) {
159-
LOGGER.log(Level.WARNING, "Failed to retrieve GitHub App installation token for app ID " + appId, e);
160-
throw new IllegalArgumentException(String.format(ERROR_AUTHENTICATING_GITHUB_APP, appId), e);
159+
throw new IllegalArgumentException("Failed to generate GitHub App installation token for app ID " + appId , e);
161160
}
162161
}
163162

@@ -202,7 +201,8 @@ public Secret getPassword() {
202201
// This minimizes failures due to occasional network instability,
203202
// while only slightly increasing the chance that tokens will expire while in use.
204203
LOGGER.log(Level.WARNING,
205-
"Keeping cached GitHub App Installation Token for app ID {0}: token is stale but has not expired", appID);
204+
"Failed to generate new GitHub App Installation Token for app ID " + appID + ": cached token is stale but has not expired",
205+
e);
206206
} else {
207207
throw e;
208208
}
@@ -298,7 +298,7 @@ public AppInstallationToken(String token, long expirationEpochSeconds) {
298298
secondsUntilStale = maximumAllowedAge;
299299
}
300300

301-
LOGGER.log(Level.FINER, "Token will become stale after {0,number,#} seconds", secondsUntilStale);
301+
LOGGER.log(Level.FINER, "Token will become stale after " + secondsUntilStale + " seconds");
302302

303303
this.token = token;
304304
this.expirationEpochSeconds = expirationEpochSeconds;
@@ -377,7 +377,7 @@ private static final class DelegatingGitHubAppCredentials extends BaseStandardCr
377377
LOGGER.log(Level.FINEST, "Checking App Installation Token for app ID {0} before sending to agent", onMaster.appID);
378378
onMaster.getPassword();
379379
} catch (Exception e) {
380-
LOGGER.log(Level.WARNING, "Failed to refresh stale GitHub App installation token before sending to agent for app ID " + onMaster.getAppID(), e);
380+
LOGGER.log(Level.WARNING, "Failed to update stale GitHub App installation token for app ID " + onMaster.getAppID() + " before sending to agent", e);
381381
}
382382

383383
cachedToken = onMaster.getCachedToken();
@@ -405,19 +405,20 @@ public Secret getPassword() {
405405
synchronized (this) {
406406
try {
407407
if (cachedToken == null || cachedToken.isStale()) {
408+
LOGGER.log(Level.FINE, "Generating App Installation Token for app ID {0} on agent", appID);
408409
cachedToken = ch.call(new GetToken(tokenRefreshData));
409-
LOGGER.log(Level.INFO,
410-
"Retrieved GitHub App Installation Token for app ID {0} on agent",
411-
appID);
410+
LOGGER.log(Level.FINER, "Retrieved GitHub App Installation Token for app ID {0} on agent", appID);
412411
}
413412
} catch (Exception e) {
414413
if (cachedToken != null && !cachedToken.isExpired()) {
415414
// Requesting a new token failed. If the cached token is not expired, continue to use it.
416415
// This minimizes failures due to occasional network instability,
417416
// while only slightly increasing the chance that tokens will expire while in use.
418417
LOGGER.log(Level.WARNING,
419-
"Keeping cached GitHub App Installation Token for app ID {0} on agent: token is stale but has not expired",
420-
appID);
418+
"Failed to generate new GitHub App Installation Token for app ID " + appID + " on agent: cached token is stale but has not expired");
419+
// Logging the exception here caused a security exeception when trying to read the agent logs during testing
420+
// Added the exception to a secondary log message that can be viewed if it is needed
421+
LOGGER.log(Level.FINER, () -> Functions.printThrowable(e));
421422
} else {
422423
throw e;
423424
}
@@ -429,7 +430,6 @@ public Secret getPassword() {
429430

430431
return Secret.fromString(appInstallationToken);
431432
} catch (IOException | InterruptedException x) {
432-
LOGGER.log(Level.WARNING, "Failed to get GitHub App Installation token on agent: " + getId(), x);
433433
throw new RuntimeException(x);
434434
}
435435
}

0 commit comments

Comments
 (0)