Skip to content
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

Add the option to configure redirect uri #111

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

philiplrb
Copy link

Copy link
Member

@samrocketman samrocketman left a comment

Choose a reason for hiding this comment

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

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

writer.startNode("redirectUri");
String redirectUriValue = DEFAULT_REDIRECT_URI;
if (null != realm.getRedirectUri()) {
redirectUriValue = realm.getRedirectUri();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Expanded the tabs.

*/
@DataBoundConstructor
public GithubSecurityRealm(String githubWebUri,
String githubApiUri,
String clientID,
String clientSecret,
String oauthScopes) {
String oauthScopes,
String redirectUri) {
Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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
Copy link
Author

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

@fred-vogt
Copy link

fred-vogt commented Sep 8, 2019

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

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

@fred-vogt
Copy link

fred-vogt commented Oct 4, 2019

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

@johnclyde
Copy link

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

@fred-vogt
Copy link

fred-vogt commented Feb 27, 2020

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

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
Copy link

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

github-oauth-old-link-01
github-oauth-settings-01

@fred-vogt
Copy link

@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
Copy link

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

@johnclyde
Copy link

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
Copy link

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
Copy link

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
Copy link

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

@Moofasax
Copy link

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
Copy link

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

@basil
Copy link
Member

basil commented Dec 25, 2021

@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.

@scurvydoggo scurvydoggo requested a review from a team as a code owner July 31, 2023 04:02
@MarkEWaite MarkEWaite added the enhancement A new feature label Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants