Skip to content

Commit 1afc22a

Browse files
jeromepochatjeromepochat
authored
Merge pull request #890 from jeromepochat/BEE-62243-update-migration-path
[BEE-62243] Use `AccessSpecifiedRepositories` as default access strategy for GitHub App credentials
2 parents e66cbb9 + 0991d93 commit 1afc22a

File tree

12 files changed

+149
-51
lines changed

12 files changed

+149
-51
lines changed

‎docs/github-app.adoc‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -187,12 +187,15 @@ For now, in this case, you either need to use a less restrictive strategy for th
187187

188188
==== Backwards compatibility
189189

190-
[IMPORTANT]
191-
The new configuration options are not fully backwards compatible.
190+
The existing credentials is migrated to the “Specify accessible repositories” mode described in the documentation:
191+
192+
* the owner is set with the original credentials owner
193+
* the list of repository is set with empty list
192194

193-
For existing GitHub App credentials which do not have the owner field set, the migration to the new format is not fully compatible. These credentials migrate to the “Infer owner and allow access to all owned repositories” mode described in the documentation, which means that they will only work in contexts where the owner can be inferred, such as Organization Folders and Multibranch Pipelines.
195+
In case of empty owner and when the credentials are used in a context where inference is supported, such as Organization Folders and Multibranch Pipelines, the inferred owner is used to compute permissions.
194196

195-
If you are using the credentials in a context where inference is not supported, you will need to reconfigure these credentials to use the “Specify accessible repositories” mode instead, specifying the appropriate owner (or leaving it blank if the app is installed in a single GitHub organization).
197+
[IMPORTANT]
198+
If you are using the credentials in a context where inference is not supported, you will need to reconfigure these credentials to specify the appropriate owner (or leaving it blank if the app is installed in a single GitHub organization).
196199

197200
=== Help?
198201

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

Lines changed: 27 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@
3939
import jenkins.util.JenkinsJVM;
4040
import net.sf.json.JSONObject;
4141
import org.apache.commons.lang3.StringUtils;
42-
import org.jenkinsci.plugins.github_branch_source.app_credentials.AccessInferredOwner;
4342
import org.jenkinsci.plugins.github_branch_source.app_credentials.AccessSpecifiedRepositories;
4443
import org.jenkinsci.plugins.github_branch_source.app_credentials.AccessibleRepositories;
4544
import org.jenkinsci.plugins.github_branch_source.app_credentials.DefaultPermissionsStrategy;
@@ -165,19 +164,18 @@ public String getOwner() {
165164
/** Do not call this method, use {@link #setRepositoryAccessStrategy} instead. */
166165
public void setOwner(String owner) {
167166
owner = Util.fixEmptyAndTrim(owner);
168-
if (owner != null) {
169-
setRepositoryAccessStrategy(new AccessSpecifiedRepositories(owner, List.of()));
170-
} else {
171-
setRepositoryAccessStrategy(new AccessInferredOwner());
172-
}
167+
LOGGER.log(Level.FINE, null, new Exception("setOwner method called with owner: " + owner));
168+
setRepositoryAccessStrategy(new AccessSpecifiedRepositories(owner, List.of()));
173169
// We only expect this to be called by CasC and by a few plugins which implement variants of this class based on
174170
// external credential providers, so we still count it as a migration.
175171
MigrationAdminMonitor.addMigratedCredentialId(getId());
176172
}
177173

178174
@NonNull
179175
public RepositoryAccessStrategy getRepositoryAccessStrategy() {
180-
return repositoryAccessStrategy == null ? new AccessInferredOwner() : repositoryAccessStrategy;
176+
return repositoryAccessStrategy == null
177+
? new AccessSpecifiedRepositories(owner, List.of())
178+
: repositoryAccessStrategy;
181179
}
182180

183181
@DataBoundSetter
@@ -206,6 +204,10 @@ Map<String, GHPermissionType> getPermissions() {
206204
return getDefaultPermissionsStrategy().getPermissions();
207205
}
208206

207+
private GitHubAppUsageContext getContext() {
208+
return context;
209+
}
210+
209211
@SuppressWarnings("deprecation")
210212
AuthorizationProvider getAuthorizationProvider() {
211213
return new CredentialsTokenProvider(this);
@@ -273,7 +275,8 @@ static AppInstallationToken generateAppInstallationToken(
273275
String apiUrl,
274276
String owner,
275277
List<String> repositories,
276-
Map<String, GHPermissionType> permissions) {
278+
Map<String, GHPermissionType> permissions,
279+
String inferredOwner) {
277280
JenkinsJVM.checkJenkinsJVM();
278281
// We expect this to be fast but if anything hangs in here we do not want to block indefinitely
279282

@@ -293,12 +296,15 @@ static AppInstallationToken generateAppInstallationToken(
293296
if (appInstallations.isEmpty()) {
294297
throw new IllegalArgumentException(String.format(ERROR_NOT_INSTALLED, appId));
295298
}
296-
GHAppInstallation appInstallation;
297-
if (StringUtils.isEmpty(owner) && appInstallations.size() == 1) {
299+
final String ownerOrNull = Util.fixEmpty(owner);
300+
final GHAppInstallation appInstallation;
301+
if (ownerOrNull == null && appInstallations.size() == 1) {
298302
// This case is only used when AccessSpecifiedRepositories.getOwner is empty.
299303
appInstallation = appInstallations.get(0);
300304
} else {
301-
final String ownerOrEmpty = owner != null ? owner : "";
305+
// Use the possibly inferred owner from the context
306+
final String ownerOrEmpty =
307+
Util.fixNull(Optional.ofNullable(ownerOrNull).orElse(inferredOwner));
302308
Optional<GHAppInstallation> appInstallationOptional = appInstallations.stream()
303309
.filter(installation -> installation
304310
.getAccount()
@@ -390,7 +396,8 @@ private AppInstallationToken getToken(GitHub gitHub) {
390396
actualApiUri(),
391397
accessibleRepositories.getOwner(),
392398
accessibleRepositories.getRepositories(),
393-
getPermissions());
399+
getPermissions(),
400+
getContext().getInferredOwner());
394401
LOGGER.log(Level.FINER, "Retrieved GitHub App Installation Token for app ID {0}", getAppID());
395402
}
396403
} catch (Exception e) {
@@ -592,19 +599,7 @@ long getTokenStaleEpochSeconds() {
592599
private Object readResolve() {
593600
cachedCredentials = new ConcurrentHashMap<>();
594601
if (repositoryAccessStrategy == null || defaultPermissionsStrategy == null) {
595-
if (owner != null) {
596-
// In this case, the migration should result in identical behavior.
597-
setRepositoryAccessStrategy(new AccessSpecifiedRepositories(owner, List.of()));
598-
} else {
599-
// There is a choice here: We can either preserve compatibility for users who have
600-
// the app installed in multiple orgs and only use the credentials in contexts
601-
// where owner inference is supported by using AccessInferredOwner, _or_ we can
602-
// preserve compatibility for users who have the app installed in a single org and
603-
// use it in contexts where inference is not supported by using
604-
// AccessSpecifiedRepositories with a null owner.
605-
// None of the new strategies support these two use cases simultaneously.
606-
setRepositoryAccessStrategy(new AccessInferredOwner());
607-
}
602+
setRepositoryAccessStrategy(new AccessSpecifiedRepositories(owner, List.of()));
608603
setDefaultPermissionsStrategy(DefaultPermissionsStrategy.INHERIT_ALL);
609604
MigrationAdminMonitor.addMigratedCredentialId(getId());
610605
}
@@ -662,6 +657,7 @@ private static final class DelegatingGitHubAppCredentials extends BaseStandardCr
662657
j.put("owner", accessibleRepositories.getOwner());
663658
j.put("repositories", accessibleRepositories.getRepositories());
664659
j.put("permissions", onMaster.getPermissions());
660+
j.put("inferredOwner", onMaster.getContext().getInferredOwner());
665661
tokenRefreshData = Secret.fromString(j.toString()).getEncryptedValue();
666662

667663
// Check token is valid before sending it to the agent.
@@ -772,7 +768,8 @@ public AppInstallationToken call() throws RuntimeException {
772768
(String) fields.get("apiUri"),
773769
(String) fields.get("owner"),
774770
(List<String>) fields.get("repositories"),
775-
(Map<String, GHPermissionType>) fields.get("permissions"));
771+
(Map<String, GHPermissionType>) fields.get("permissions"),
772+
(String) fields.get("inferredOwner"));
776773
LOGGER.log(
777774
Level.FINER,
778775
"Retrieved GitHub App Installation Token for app ID {0} for agent",
@@ -814,6 +811,10 @@ public ListBoxModel doFillApiUriItems() {
814811
return getPossibleApiUriItems();
815812
}
816813

814+
public static RepositoryAccessStrategy getDefaultRepositoryAccessStrategy() {
815+
return new AccessSpecifiedRepositories(null, List.of());
816+
}
817+
817818
public FormValidation doCheckAppID(@QueryParameter String appID) {
818819
if (!appID.isEmpty()) {
819820
try {

‎src/main/resources/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials/config.jelly‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
</f:entry>
2323

2424
<f:advanced>
25-
<f:dropdownDescriptorSelector title="${%Repository access strategy}" field="repositoryAccessStrategy" />
25+
<f:dropdownDescriptorSelector title="${%Repository access strategy}" field="repositoryAccessStrategy" default="${descriptor.defaultRepositoryAccessStrategy}" />
2626
<f:entry title="${%Default permissions strategy}" field="defaultPermissionsStrategy">
2727
<f:enum default="INHERIT_ALL">
2828
${it.displayName}

‎src/main/resources/org/jenkinsci/plugins/github_branch_source/app_credentials/AccessSpecifiedRepositories/help-owner.html‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
<p>
22
The organization or user that this credential is to be used for.
3-
If left blank, and the GitHub App is only installed in a single organization, then these
4-
credentials will be able to access that organization in any context in Jenkins.
3+
If left blank:
4+
<ul>
5+
<li>If the GitHub App is installed in a single organization, then these credentials will be able to access that organization in any context in Jenkins.</li>
6+
<li>If the GitHub App is installed in multiple organizations, then the owner will be inferred from the context where inference is supported.</li>
7+
</ul>
58
</p>

‎src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentialsContextualizationTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ public void setUpWireMock() throws Exception {
3636
githubApi.stubFor(get(urlEqualTo("/app/installations"))
3737
.willReturn(aResponse()
3838
.withHeader("Content-Type", "application/json; charset=utf-8")
39-
.withBodyFile("../AppCredentials/files/body-mapping-githubapp-installations.json")));
39+
.withBodyFile("../AppCredentials/files/body-mapping-githubapp-installations-multiple.json")));
4040
githubApi.stubFor(post(urlEqualTo("/app/installations/654321/access_tokens"))
4141
.withRequestBody(equalToJson(
4242
"{\"permissions\":{\"pull_requests\":\"write\",\"metadata\":\"read\",\"checks\":\"write\",\"contents\":\"read\"}}",

‎src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentialsJCasCCompatibilityTest.java‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ public void should_support_configuration_as_code() throws Exception {
5151
List<Credentials> credentials = domainCredentials.get(0).getCredentials();
5252
assertThat(credentials.size(), is(7));
5353

54-
assertGitHubAppCredential(credentials.get(0), "github-app", "GitHub app 1111");
54+
assertGitHubAppCredential(
55+
credentials.get(0), "github-app", "GitHub app 1111", new AccessSpecifiedRepositories(null, List.of()));
5556
assertGitHubAppCredential(
5657
credentials.get(1), "old-owner", "", new AccessSpecifiedRepositories("test", List.of()));
5758
assertGitHubAppCredential(
@@ -106,10 +107,6 @@ private CNode getCredentials() throws Exception {
106107
return configNode;
107108
}
108109

109-
private static void assertGitHubAppCredential(Credentials credentials, String id, String description) {
110-
assertGitHubAppCredential(credentials, id, description, new AccessInferredOwner());
111-
}
112-
113110
private static void assertGitHubAppCredential(
114111
Credentials credentials, String id, String description, RepositoryAccessStrategy repoStrategy) {
115112
assertGitHubAppCredential(credentials, id, description, repoStrategy, DefaultPermissionsStrategy.INHERIT_ALL);

‎src/test/java/org/jenkinsci/plugins/github_branch_source/GithubAppCredentialsTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ public void setUpWireMock() throws Exception {
113113
githubApi.stubFor(get(urlEqualTo("/app/installations"))
114114
.willReturn(aResponse()
115115
.withHeader("Content-Type", "application/json; charset=utf-8")
116-
.withBodyFile("../AppCredentials/files/body-mapping-githubapp-installations.json")));
116+
.withBodyFile("../AppCredentials/files/body-mapping-githubapp-installations-multiple.json")));
117117

118118
final String scenarioName = "credentials-accesstoken";
119119

‎src/test/java/org/jenkinsci/plugins/github_branch_source/app_credentials/MigrationAdminMonitorTest.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,16 +41,19 @@ public void smokes() throws Throwable {
4141
CredentialsProvider.lookupCredentialsInItemGroup(GitHubAppCredentials.class, r.jenkins, ACL.SYSTEM2),
4242
CredentialsMatchers.withId("old-credentials-with-owner"));
4343
assertThat(credentialsWithOwner.getOwner(), nullValue());
44-
var strategy = (AccessSpecifiedRepositories) credentialsWithOwner.getRepositoryAccessStrategy();
45-
assertThat(strategy.getOwner(), is("cloudBeers"));
46-
assertThat(strategy.getRepositories(), empty());
44+
var strategyWithOwner = (AccessSpecifiedRepositories) credentialsWithOwner.getRepositoryAccessStrategy();
45+
assertThat(strategyWithOwner.getOwner(), is("cloudBeers"));
46+
assertThat(strategyWithOwner.getRepositories(), empty());
4747
assertThat(credentialsWithOwner.getDefaultPermissionsStrategy(), is(DefaultPermissionsStrategy.INHERIT_ALL));
4848

4949
var credentialsNoOwner = CredentialsMatchers.firstOrNull(
5050
CredentialsProvider.lookupCredentialsInItemGroup(GitHubAppCredentials.class, r.jenkins, ACL.SYSTEM2),
5151
CredentialsMatchers.withId("old-credentials-no-owner"));
5252
assertThat(credentialsNoOwner.getOwner(), nullValue());
53-
assertThat(credentialsNoOwner.getRepositoryAccessStrategy(), instanceOf(AccessInferredOwner.class));
53+
assertThat(credentialsNoOwner.getRepositoryAccessStrategy(), instanceOf(AccessSpecifiedRepositories.class));
54+
var strategyWithNoOwner = (AccessSpecifiedRepositories) credentialsNoOwner.getRepositoryAccessStrategy();
55+
assertThat(strategyWithNoOwner.getOwner(), nullValue());
56+
assertThat(strategyWithNoOwner.getRepositories(), empty());
5457
assertThat(credentialsNoOwner.getDefaultPermissionsStrategy(), is(DefaultPermissionsStrategy.INHERIT_ALL));
5558

5659
var monitor = ExtensionList.lookupSingleton(MigrationAdminMonitor.class);

‎src/test/java/org/jenkinsci/plugins/github_branch_source/app_credentials/RunWithCredentialsTest.java‎

Lines changed: 55 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -127,12 +127,6 @@ public void setup() throws IOException, FormException, InterruptedException, Exe
127127
.withHeaders(HEADERS)
128128
.withBodyFile("../AppCredentials/files/body-mapping-githubapp-app.json")));
129129

130-
// Stub app installation
131-
githubApi.stubFor(get(urlEqualTo("/app/installations"))
132-
.willReturn(aResponse()
133-
.withHeaders(HEADERS)
134-
.withBodyFile("../AppCredentials/files/body-mapping-githubapp-installations.json")));
135-
136130
// Stub app installation access token
137131
githubApi.stubFor(post(urlEqualTo("/app/installations/654321/access_tokens"))
138132
.withRequestBody(equalToJson(
@@ -206,6 +200,22 @@ public void setup() throws IOException, FormException, InterruptedException, Exe
206200
"{\"repositories\":[{\"id\":1,\"name\":\"repository-a\",\"full_name\":\"cloudbeers/repository-a\"},{\"id\":2,\"name\":\"repository-b\",\"full_name\":\"cloudbeers/repository-b\"}]}")));
207201
}
208202

203+
private void simpleInstallations() {
204+
// Stub app installation
205+
githubApi.stubFor(get(urlEqualTo("/app/installations"))
206+
.willReturn(aResponse()
207+
.withHeaders(HEADERS)
208+
.withBodyFile("../AppCredentials/files/body-mapping-githubapp-installations-single.json")));
209+
}
210+
211+
private void multipleInstallations() {
212+
// Stub app installation
213+
githubApi.stubFor(get(urlEqualTo("/app/installations"))
214+
.willReturn(aResponse()
215+
.withHeaders(HEADERS)
216+
.withBodyFile("../AppCredentials/files/body-mapping-githubapp-installations-multiple.json")));
217+
}
218+
209219
@After
210220
public void cleanup() throws IOException, InterruptedException {
211221
if (project != null) {
@@ -218,6 +228,8 @@ public void cleanup() throws IOException, InterruptedException {
218228

219229
@Test
220230
public void inferredRepository() throws Exception {
231+
multipleInstallations();
232+
221233
credentials.setRepositoryAccessStrategy(new AccessInferredRepository());
222234

223235
final var build = r.buildAndAssertSuccess(project);
@@ -228,6 +240,8 @@ public void inferredRepository() throws Exception {
228240

229241
@Test
230242
public void inferredOwner() throws Exception {
243+
multipleInstallations();
244+
231245
credentials.setRepositoryAccessStrategy(new AccessInferredOwner());
232246

233247
final var build = r.buildAndAssertSuccess(project);
@@ -238,6 +252,8 @@ public void inferredOwner() throws Exception {
238252

239253
@Test
240254
public void specifiedRepositoriesA() throws Exception {
255+
multipleInstallations();
256+
241257
credentials.setRepositoryAccessStrategy(
242258
new AccessSpecifiedRepositories("cloudbeers", Arrays.asList("repository-a")));
243259

@@ -249,6 +265,8 @@ public void specifiedRepositoriesA() throws Exception {
249265

250266
@Test
251267
public void specifiedRepositoriesB() throws Exception {
268+
multipleInstallations();
269+
252270
credentials.setRepositoryAccessStrategy(
253271
new AccessSpecifiedRepositories("cloudbeers", Arrays.asList("repository-b")));
254272

@@ -260,6 +278,8 @@ public void specifiedRepositoriesB() throws Exception {
260278

261279
@Test
262280
public void specifiedRepositoriesAB() throws Exception {
281+
multipleInstallations();
282+
263283
credentials.setRepositoryAccessStrategy(
264284
new AccessSpecifiedRepositories("cloudbeers", Arrays.asList("repository-a", "repository-b")));
265285

@@ -271,6 +291,8 @@ public void specifiedRepositoriesAB() throws Exception {
271291

272292
@Test
273293
public void specifiedRepositoriesABC() throws Exception {
294+
multipleInstallations();
295+
274296
credentials.setRepositoryAccessStrategy(new AccessSpecifiedRepositories(
275297
"cloudbeers", Arrays.asList("repository-a", "repository-b", "repository-c")));
276298

@@ -282,13 +304,40 @@ public void specifiedRepositoriesABC() throws Exception {
282304
build);
283305
}
284306

307+
@Test
308+
public void specifiedRepositoryWithOwner() throws Exception {
309+
simpleInstallations();
310+
311+
credentials.setRepositoryAccessStrategy(new AccessSpecifiedRepositories("cloudbeers", List.of()));
312+
313+
final var build = r.buildAndAssertSuccess(project);
314+
315+
// Only specified repositories should be accessible from the contextualized credentials (TOKEN_AB)
316+
assertAccessibleRepositories(build, "cloudbeers/repository-a", "cloudbeers/repository-b");
317+
}
318+
319+
@Test
320+
public void specifiedRepositoryWithoutOwner() throws Exception {
321+
simpleInstallations();
322+
323+
// Owner is inferred from the context in this case
324+
credentials.setRepositoryAccessStrategy(new AccessSpecifiedRepositories(null, List.of()));
325+
326+
final var build = r.buildAndAssertSuccess(project);
327+
328+
// Only specified repositories should be accessible from the contextualized credentials (TOKEN_AB)
329+
assertAccessibleRepositories(build, "cloudbeers/repository-a", "cloudbeers/repository-b");
330+
}
331+
285332
/**
286333
* Demonstrates how a user who only has access to edit a Jenkinsfile can abuse GithubProjectProperty in combination
287334
* with the properties step to access any repository accessible to the app installation, as long as the Pipeline is
288335
* not part of a MultiBranchProject.
289336
*/
290337
@Test
291338
public void propertiesStepAllowsAccessBypass() throws Exception {
339+
multipleInstallations();
340+
292341
credentials.setRepositoryAccessStrategy(new AccessInferredRepository());
293342

294343
project.setDefinition(new CpsFlowDefinition(

‎src/test/resources/api/AppCredentials/files/body-mapping-githubapp-installations.json‎ renamed to ‎src/test/resources/api/AppCredentials/files/body-mapping-githubapp-installations-multiple.json‎

File renamed without changes.

0 commit comments

Comments
 (0)