Navigation Menu
Conversation
…ns of the repositories and permissions available to installation access tokens in some contexts Co-authored-by: Jérôme Pochat <jpochat@cloudbees.com>
.../org/jenkinsci/plugins/github_branch_source/app_credentials/AccessSpecifiedRepositories.java
Dismissed
Show dismissed
Hide dismissed
.../org/jenkinsci/plugins/github_branch_source/app_credentials/AccessSpecifiedRepositories.java
Dismissed
Show dismissed
Hide dismissed
...enkinsci/plugins/github_branch_source/app_credentials/InstallationAccessTokenPermission.java
Fixed
Show fixed
Hide fixed
...enkinsci/plugins/github_branch_source/app_credentials/InstallationAccessTokenPermission.java
Fixed
Show fixed
Hide fixed
| * All permissions available to the app installation (default) | ||
| * The access tokens generated in untrusted contexts will have the same permissions as the app installation in GitHub |
There was a problem hiding this comment.
Whether this is the best default, I am not sure. I think for the default we just want to choose whatever is least surprising. Perhaps read-only access would be more appropriate as a default.
| */ | ||
| private transient Map<String, GitHubAppCredentials> byOwner; | ||
| private transient Map<GitHubAppUsageContext, GitHubAppCredentials> cachedCredentials = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
We still dynamically create different credentials for each context, but now the contexts are more complicated.
| // This method is not deprecated or restricted only to preserve compatibility with existing CasC YAML files. | ||
| /** Do not call this method, use {@link #setRepositoryAccessStrategy} instead. */ |
There was a problem hiding this comment.
Alternatively, we could mark this method @Deprecated, in which case all CasC users that configure GitHub App credentials would have to edit their YAML scripts when applying this plugin update to avoid startup failures. I do not know what approach is generally preferred when making significant configuration changes, so I went for trying to preserve CasC compatibility for now.
We do probably want to keep the method around though so that we do not need to do coordinated releases of plugins that currently call this method, which are mentioned in the PR description.
| GHAppCreateTokenBuilder builder = appInstallation.createToken(); | ||
| if (!repositories.isEmpty()) { | ||
| builder.repositories(repositories); | ||
| } | ||
| if (!permissions.isEmpty()) { | ||
| builder.permissions(permissions); | ||
| } else { | ||
| builder.permissions(appInstallation.getPermissions()); | ||
| } | ||
| GHAppInstallationToken appInstallationToken = builder.create(); |
There was a problem hiding this comment.
These new methods from https://github.com/hub4j/github-api are what actually change the accessible repositories and permissions for the generated tokens.
...enkinsci/plugins/github_branch_source/app_credentials/InstallationAccessTokenPermission.java
Outdated
Show resolved
Hide resolved
| @@ -22,12 +22,21 @@ | |||
| </f:entry> | |||
|
|
|||
| <f:advanced> | |||
There was a problem hiding this comment.
I'm not sure if we should still hide these options behind the advanced dropdown or not. Any thoughts?
...g/jenkinsci/plugins/github_branch_source/app_credentials/MigrationAdminMonitor/message.jelly
Show resolved
Hide resolved
| // TODO: CasC plugin incorrectly oversimplifies the YAML export for new-specific-empty by | ||
| // fully removing the repositoryAccessStrategy configuration because its inner | ||
| // configuration is all empty, but that means it no longer round-trips. |
There was a problem hiding this comment.
Essentially, if you have a Describable with multiple possible implementations, and a single implementation whose inner values are nullable or can be an empty list, a configuration like this:
outerContext:
whatever: foo
describable:
myDescribable1:
innerConfiguration: []
Will be exported as this:
outerContext:
whatever: foo
Which is not good if describable is heterogeneous and has multiple possible implementations! We'd like for it to be exported as the following, which is what happens if myDescribable1 has no nested configuration (I think because it ends up as a SCALAR in ConfigurationAsCode.toYaml instead of a MAPPING):
outerContext:
whatever: foo
describable: myDescribable1
| Note also that Jenkins users who have Job/Configure permission in a context where the credentials are available are considered trusted and can bypass these strategies by configuring jobs as desired. | ||
| In trusted contexts, the generated access tokens will have the same access as configured for the app installation in GitHub. | ||
|
|
||
| The following repository access strategies are available: |
There was a problem hiding this comment.
Do these strategies make sense? Do they work they way you would expect? Do you think the default is appropriate? Is there any other strategy that you think should be supported?
…Strategy for subclasses c.f. 5a78598
|
Hmm, some of the changes in 755fec8 might make the migration admin monitor trigger even for brand new app credentials. If so, we might need to swap back to setting the default values of the new fields in the constructor. |
Description
See JENKINS-75058. See also jenkins-infra/jenkins.io#7770 for greater context around this change. For now this is a draft PR. I will go through the PR and add review comments to help clarify things.
This PR adds new configuration options to GitHub App credentials that make use of the
repositoriesandpermissionsparameters when using the/app/installations/{installation_id}/access_tokensGitHub API endpoint to create installation access tokens.There are two new high-level options:
repositoriesparameter to control which repositories are available to the installation access tokens. There are three strategies, please see the changes to docs/github-app.adoc and the correspondinghelp-*.htmlfor details.permissionsparameter to control which permissions are available to the installation access tokens, but only when the tokens are generated and used in an untrusted context. There are three strategies, please see the changes to docs/github-app.adoc for details.If you have any recommendations for renaming of these options or any of their sub-options, please feel free to suggest them.
I will go through and add comments to the PR to various points of interest. Please feel free to ask about anything that is not clear.
Backwards compatibility:
ownerfield set, we can 1) either preserve compatibility for users who have the app installed in multiple orgs and only use the credentials in contexts where owner inference is supported by usingAccessInferredOwneras the migration, or 2) we can preserve compatibility for users who have the app installed in a single org and use it in contexts where inference is not supported by usingAccessSpecifiedRepositorieswith anullowner. None of the new strategies currnetly support these two use cases simultaneously.Notes for downstream plugins:
Connector.lookupScanCredentialsso that the retrieved credentials recognize that they are in a trusted context so that the generated token is not restricted. See for example Use Connector.lookupScanCredentials to contextualize GitHubAppCredentials github-checks-plugin#398.GitHubAppCredentials.getOwnerandsetOwnermethods should no longer be used. The following plugins along with a CloudBees-internal one will need (minor) updates to match this PR. I will file PRs for this soon:ownerfield in GitHub App Credentials kubernetes-credentials-provider-plugin#107Submitter checklist
Reviewer checklist
Documentation changes
Users/aliases to notify