-
Notifications
You must be signed in to change notification settings - Fork 386
Allow GitHub app to be installed in multiple orgs #290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
318b60a
f7ad933
9887b6f
83f1d74
aee2719
46c5a13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,7 +87,8 @@ Fill out the form: | |
| - App ID: the github app ID, it can be found in the 'About' section of your GitHub app in the general tab. | ||
| - API endpoint (optional, only required for GitHub enterprise this will only show up if a GitHub enterprise server is configured). | ||
| - Key: click add, paste the contents of the converted private key | ||
| - Passphrase: do not fill this field, it will be ignored | ||
| - Advanced: (optional) If you've installed your same GitHub app on multiple organisations you need the next step | ||
| * Owner: the name of the organisation or user, i.e. jenkinsci for https://github.com/jenkinsci | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So under this approach you cannot reuse the Jenkins credentials object across orgs, and you need to duplicate the private key. Not as smooth as having Jenkins figure out which installation to work with, but better than nothing, right?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah I couldn't see a way to do that, as we don't have the context from the build job at all
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, it would be a more intrusive patch since you would need to do something along the lines of defining a thread-local org and setting this from a new overload of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What about for GitHub Enterprise? It doesn't seem to be possible to install an app at the Enterprise level. My Jenkins instance works on repos across many organizations in a GitHub Enterprise instance.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not have GHE to test against but in that case with the current patch you would need to first install the app in each such org, then create a separate Jenkins credentials item for each installation.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, those were all public repos. is there any way that we can get this owner based on the job config as it would already have the github repo information ?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The credential isn’t tied to a job it’s independent, not safely, there may be some possible hacks to do it though
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My company uses Jenkins in a similar fashion as the company described by @nrayapati. We deployed a single We allow our engineers to create jobs as needed within their own Jenkins folders and leverage the single GitHub App Credential. Since any engineer can install our GitHub App on their organization, and any engineer can create an organization, we would be in the situation of needing to create a Credential for every organization as it's created. I, too, would like to see support for a single global Credential to work for jobs across multiple organizations. @nrayapati can you please file a Jira ticket (As this PR has already been merged and released) so that we can centralize the discussion about this use case on that ticket?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just use the org name in repo instead of the org name in credential config? |
||
| - Click OK | ||
|
|
||
| === link:https://github.com/jenkinsci/configuration-as-code-plugin[Configuration as Code Plugin] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,9 @@ | |
| public class GitHubAppCredentials extends BaseStandardCredentials implements StandardUsernamePasswordCredentials { | ||
|
|
||
| private static final String ERROR_AUTHENTICATING_GITHUB_APP = "Couldn't authenticate with GitHub app ID %s"; | ||
| private static final String NOT_INSTALLED = ", has it been installed to your GitHub organisation / user?"; | ||
|
|
||
| private static final String ERROR_NOT_INSTALLED = ERROR_AUTHENTICATING_GITHUB_APP + NOT_INSTALLED; | ||
|
|
||
| @NonNull | ||
| private final String appID; | ||
|
|
@@ -39,6 +42,8 @@ public class GitHubAppCredentials extends BaseStandardCredentials implements Sta | |
|
|
||
| private String apiUri; | ||
|
|
||
| private String owner; | ||
|
|
||
| @DataBoundConstructor | ||
| @SuppressWarnings("unused") // by stapler | ||
| public GitHubAppCredentials( | ||
|
|
@@ -72,27 +77,55 @@ public Secret getPrivateKey() { | |
| return privateKey; | ||
| } | ||
|
|
||
| /** | ||
| * Owner of this installation, i.e. a user or organisation, | ||
| * used to differeniate app installations when the app is installed to multiple organisations / users. | ||
| * | ||
| * If this is null then call listInstallations and if there's only one in the list then use that installation. | ||
| * | ||
| * @return the owner of the organisation or null. | ||
| */ | ||
| @CheckForNull | ||
| public String getOwner() { | ||
| return owner; | ||
| } | ||
|
|
||
| @DataBoundSetter | ||
| public void setOwner(String owner) { | ||
| this.owner = owner; | ||
timja marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| @SuppressWarnings("deprecation") // preview features are required for GitHub app integration, GitHub api adds deprecated to all preview methods | ||
| static String generateAppInstallationToken(String appId, String appPrivateKey, String apiUrl) { | ||
| static String generateAppInstallationToken(String appId, String appPrivateKey, String apiUrl, String owner) { | ||
| try { | ||
| String jwtToken = createJWT(appId, appPrivateKey); | ||
| GitHub gitHubApp = new GitHubBuilder().withEndpoint(apiUrl).withJwtToken(jwtToken).build(); | ||
|
|
||
| GHApp app = gitHubApp.getApp(); | ||
|
|
||
| List<GHAppInstallation> appInstallations = app.listInstallations().asList(); | ||
| if (!appInstallations.isEmpty()) { | ||
| GHAppInstallation appInstallation = appInstallations.get(0); | ||
| GHAppInstallationToken appInstallationToken = appInstallation | ||
| .createToken(appInstallation.getPermissions()) | ||
| .create(); | ||
|
|
||
| return appInstallationToken.getToken(); | ||
| if (appInstallations.isEmpty()) { | ||
| throw new IllegalArgumentException(String.format(ERROR_NOT_INSTALLED, appId)); | ||
| } | ||
| GHAppInstallation appInstallation; | ||
| if (appInstallations.size() == 1) { | ||
| appInstallation = appInstallations.get(0); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not verifying the owner?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've made an assumption that most installations only work with one org, so I've made it an advanced field and by default if and only if there's one installation the plugin will just pick that.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I get that, I am just saying that in case you did specify an owner, and there is exactly one installation, we are not verifying that the installation is in fact from that owner. Which feels wrong. |
||
| } else { | ||
| appInstallation = appInstallations.stream() | ||
| .filter(installation -> installation.getAccount().getLogin().equals(owner)) | ||
| .findAny() | ||
| .orElseThrow(() -> new IllegalArgumentException(String.format(ERROR_NOT_INSTALLED, appId))); | ||
| } | ||
|
|
||
| GHAppInstallationToken appInstallationToken = appInstallation | ||
| .createToken(appInstallation.getPermissions()) | ||
| .create(); | ||
|
|
||
| return appInstallationToken.getToken(); | ||
| } catch (IOException e) { | ||
| throw new IllegalArgumentException(String.format(ERROR_AUTHENTICATING_GITHUB_APP, appId), e); | ||
| } | ||
| throw new IllegalArgumentException(String.format(ERROR_AUTHENTICATING_GITHUB_APP, appId)); | ||
|
|
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -105,7 +138,7 @@ public Secret getPassword() { | |
| apiUri = "https://api.github.com"; | ||
| } | ||
|
|
||
| String appInstallationToken = generateAppInstallationToken(appID, privateKey.getPlainText(), apiUri); | ||
| String appInstallationToken = generateAppInstallationToken(appID, privateKey.getPlainText(), apiUri, owner); | ||
|
|
||
| return Secret.fromString(appInstallationToken); | ||
| } | ||
|
|
@@ -163,18 +196,19 @@ public ListBoxModel doFillApiUriItems() { | |
| public FormValidation doTestConnection( | ||
| @QueryParameter("appID") final String appID, | ||
| @QueryParameter("privateKey") final String privateKey, | ||
| @QueryParameter("apiUri") final String apiUri | ||
| @QueryParameter("apiUri") final String apiUri, | ||
| @QueryParameter("owner") final String owner | ||
|
|
||
| ) { | ||
| GitHubAppCredentials gitHubAppCredential = new GitHubAppCredentials( | ||
| CredentialsScope.GLOBAL, "test-id-not-being-saved", null, | ||
| appID, Secret.fromString(privateKey) | ||
| ); | ||
| gitHubAppCredential.setApiUri(apiUri); | ||
| gitHubAppCredential.setOwner(owner); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Commenting here since the Do you know if it's possible to use system scope credentials with the branch source plugin? One issue we ran into with our credentials plugin was making the credentials for an org only available to the builds within that org.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not possible to use system scope credentials, but you can define credentials on an org folder so they are not accessible outside that folder. |
||
|
|
||
| try { | ||
| GitHub connect = Connector.connect(apiUri, gitHubAppCredential); | ||
|
|
||
| return FormValidation.ok("Success, Remaining rate limit: " + connect.getRateLimit().getRemaining()); | ||
| } catch (Exception e) { | ||
| return FormValidation.error(e, String.format(ERROR_AUTHENTICATING_GITHUB_APP, appID)); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| <p> | ||
| The organisation or user that this app is to be used for, only required if this app is installed to multiple organisations. | ||
timja marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| </p> | ||
Uh oh!
There was an error while loading. Please reload this page.