Add the option to configure redirect uri

[philiplrb]

Added support for redirect uris as described at https://developer.github.com/enterprise/2.10/apps/building-oauth-apps/authorization-options-for-oauth-apps/#redirect-urls

[samrocketman]

Overall, not a bad change. There's a couple of other contributions I'm going to focus on merging before getting to this one.

[samrocketman]

Mixing tabs and spaces. There are a couple of places in your change where this happens so I'll just comment this once.

[philiplrb]

Expanded the tabs.

[samrocketman]

This may cause upgrade issues. I'm not sure just a hunch. I'll test and let you know. From what I recall it's no longer a good coding practice to modify the constructors of configuration classes. @jenkinsci/code-reviewers keep me honest!

[philiplrb]

If this needs to change, please give guidance on the approach you'd like to see.

[basil]

See Constructor vs. setters. Is redirectUri a mandatory or optional parameter? If optional, it should be a @DataBoundSetter rather than a @DataBoundConstructor.

[tdaniely-dn]

IMO, missing a test that a serialized older version deserializes into newer version with default value. Which it should looking at marshal/unmarshal.

The reason not to change the constructor is to not break deployments where jenkins instance is initialized in groovy hooks. So at least keep the old constructer signature as well.

[philiplrb]

@samrocketman Is there anything I can do to help get this moving? Thanks!

[fred-vogt]

https://issues.jenkins-ci.org/browse/JENKINS-43214

Left a comment on the issue how I think this could be used.

[fred-vogt]

@philiplrb I think what @samrocketman is referring to a some backwards compatible constructor overload.

[johnclyde]

I think you have to append the redirect_uri again in the access_token.

[fred-vogt]

@samrocketman @philiplrb I fixed the conflict and rebased with master here:

• https://github.com/vogtech/github-oauth-plugin/tree/JENKINS-43214

Also added the old constructor back for backwards compatibility with older scripts folks might have.

I'll be testing a build of this with the following url rewriting scheme:

Github "Authorization callback URL"
https://developer.github.com/apps/building-oauth-apps/authorizing-oauth-apps/#redirect-urls
=> https://jenkins.<domain>/securityRealm/finishLogin

Jenkins <instance> / manage / configure global security / security realm / "Redirect URI"
=> https://jenkins.<domain>/securityRealm/finishLogin/<instance>

Where I'll have a proxy that redirects:
https://jenkins.<domain>/securityRealm/finishLogin/<instance>
to
https://jenkins-<instance>.<domain>/securityRealm/finishLogin

[fred-vogt]

@samrocketman @philiplrb - I noticed when running this in the Jenkins UI the link in the help popup is old:

• https://developer.github.com/enterprise/2.10/apps/building-oauth-apps/authorizing-oauth-apps/

[fred-vogt]

@samrocketman @philiplrb - I tested Philip's PR in my AWS environment using 'API Gateway' + w/lambda.

---
swagger: "2.0"
host: "jenkins.<domain>.com"
schemes:
- "https"
paths:
  /securityRealm/finishLogin/{account}/{instance}:
    get:
      responses:
        302:
          description: "302 response"
          headers:
            Location:
              type: "string"

AWS_PROXY lambda integration:

import json
import re

from urllib.parse import urlencode

#
# Redicts: https://jenkins.<domain>.com/securityRealm/finishLogin/{account}/{instance}
#      to: https://jenkins-{instance}.{account}.<domain>.com/securityRealm/finishLogin
#

def lambda_handler(event, context):
    # /securityRealm/finishLogin/{account}/{instance}
    path = event['path']

    # ?code=...&state=...
    query_string = event['queryStringParameters']

    # extract account, instance name
    path_pattern = re.compile(r'/securityRealm\/finishLogin\/(?P<account>.+)\/(?P<instance>.+)')
    parsed = path_pattern.match(path)
    
    account = parsed.group('account')
    instance = parsed.group('instance')

    encoded_query_string = urlencode(query_string)

    # specific jenkins service instance Github OAuth callback URL
    location = f'https://jenkins-{instance}.{account}.<domain>.com/securityRealm/finishLogin?{encoded_query_string}'

    response = {
        'isBase64Encoded': False,
        'statusCode': 302,
        'headers': {
            'Location': location,
        },
        'body': '',
    }

    return response

[Moofasax]

Any update on merging this in? would be really nice to be able to configure the redirect_uri especially with JCasC

[johnclyde]

I don't remember specifically what prompted me to give up on configuring github oauth 8 months ago but I do remember that github's implementation did not behave the way I wanted it to. I think if I was trying to support multiple domains for one oauth app they basically didn't support that.

[Moofasax]

haha dang no worries, 8 months is a long time. it seems like the redirect_uri wasnt for multiple domains, but rather different paths behind the same domain if im reading their docs right.

[Moofasax]

In case this gives anyone else confidence, we have been using this PR for all our Jenkins servers for over a year now and it has been working great! Would be nice to have this PR as apart of the official, but we haven't run into any issues with it thus far!

[tdaniely-dn]

This is a major feature for anyone who's doing multi-master with github oauth.
What is needed to move this forward?

[Moofasax]

This is a major feature for anyone who's doing multi-master with github oauth.
What is needed to move this forward?

Yeah, we give a Jenkins master per dev team deployed into kubernetes, so this PR has worked great, I'm just worried about upgrading in the future haha

[fred-vogt]

@basil - any chance we can get this into a release eventually ?

[basil]

@basil - any chance we can get this into a release eventually ?

Seems reasonable enough, though I don't actually use this plugin and have no way of doing a meaningful review. If you can submit a new PR against tip-of-trunk and with this comment addressed I think that should be OK.