Skip to content

feat: allow one app to authenticate #1053

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

Closed
wants to merge 1 commit into from

Conversation

carlossg
Copy link

@carlossg carlossg commented Mar 8, 2021

Description

allow one app to authenticate
against multiple app installations and orgs
by inspecting the url that it tries to access

Before submitting a PR:

We love getting PRs, but we hate asking people for the same basic changes every time.

  • Push your changes to a branch other than master. Create your PR from that branch.
  • Add JavaDocs and other comments
  • Write tests that run and pass in CI. See CONTRIBUTING.md for details on how to capture snapshot data.
  • Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.

When creating a PR:

  • Fill in the "Description" above.
  • Enable "Allow edits from maintainers".

@carlossg
Copy link
Author

carlossg commented Mar 8, 2021

@bitwiseman I started this to allow one app to authenticate against multiple orgs, and eventually try to solve JENKINS-62220
I would like to see if this is a good path forward before spending more time on it

/cc @nrayapati

against multiple app installations and orgs
by inspecting the url that it tries to access
@jglick
Copy link
Contributor

jglick commented Mar 8, 2021

See jenkinsci/github-branch-source-plugin#290 (comment) for background. You are not going to be able to use the App credentials as generic username/password credentials, for example in a withCredentials block, like you can with single-org credentials; they would only work via this library.

@@ -17,47 +23,75 @@
*/
public class OrgAppInstallationAuthorizationProvider extends GitHub.DependentAuthorizationProvider {

private final String organizationName;
private static final Pattern pattern = Pattern.compile("/repos/(.*)/.*");
Copy link
Member

@bitwiseman bitwiseman Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This provider has a reference to a GitHub instance, meaning we can get the base URL using getApiUrl() and then do simpler string matching. I'd rather not regex if we don't have to.

Also do GitHub App tokens only apply to /repos/* endpoints or can they interact with other endpoints? I'm pretty sure there are even if they are not accessed by Jenkins.

@bitwiseman
Copy link
Member

@carlossg
I think this is better implemented at the Jenkins level than here, but I'm open to being wrong about that.

In Jenkins Pipeline any one job is only ever dealing with one Org. The GitHubAppCredentials class in github-branch-source could keep track of the various tokens for each job/org/owner ... maybe? The GitHubAppCredentials have the api url set on them, but that isn't the repo url, right? Does the Jenkins Credentials system have the concept of returning a different password depending on job context?

You could implement a solution and test it here. The testing cycle will be faster and allow greater coverage. But this seems like it could get pretty hairy. If you implement it here, I would expect it to be implemented in a manner that would work in general, not just in the Jenkins case.

Hacking OrgAppInstallationAuthorizationProvider to make it guess which Org it should select seems brittle and also a potential security issue. I'd rather see a new "MultiOrgAppInstallationAuthorizationProviderclass that contains a HashSet ofOrgAppInstallationAuthorizationProvider`. Have take a default authorization that it would use (I'm not sure just JWT would work?) and a list of Orgs, then add internal hooks to know which Org you're interacting with. However, all this is a lot of work for something that isn't a huge issue outside of applications constrained by the Jenkins Credentials behaviors.

@jglick
Copy link
Contributor

jglick commented Mar 9, 2021

Does the Jenkins Credentials system have the concept of returning a different password depending on job context?

Not currently, but we could define something along the lines of

interface Contexualized<T extends Credentials> extends Credentials {
   T contextualize(Item context);
}

Have not studied in detail what would need to be patched to make this work.

@bitwiseman
Copy link
Member

@carlossg Do you plan to continue with this?

@bitwiseman bitwiseman closed this Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants