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 redirect_uri oauth param and configuration #198

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

Conversation

tdaniely-dn
Copy link

@tdaniely-dn tdaniely-dn commented Aug 5, 2022

https://issues.jenkins.io/browse/JENKINS-43214

Continue work from #111

Add request_uri parameter according to:
https://docs.github.com/en/developers/apps/building-oauth-apps/authorizing-oauth-apps
https://docs.github.com/en/developers/apps/building-github-apps/identifying-and-authorizing-users-for-github-apps

Support:

  1. Shared GitHub apps with multiple Jenkins instances.
  2. Custom OAuth proxy setup.

Just to comment, GitHub App now does support multiple callback domains, so the proxy scenario is less likely.
Still made the value overridable just in case, but the default will be the correct route for the server.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!

  • Ensure that the pull request title represents the desired changelog entry

  • Please describe what you did

  • Link to relevant issues in GitHub or Jira

  • Link to relevant pull requests, esp. upstream and downstream changes

  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

  • Manually test mechanism (Tested redirect, manual conf, and JCasc with both valid an invalid values)

@tdaniely-dn
Copy link
Author

tdaniely-dn commented Aug 5, 2022

@basil can you take a look?
@fred-vogt @Moofasax fyi

@basil
Copy link
Member

basil commented Aug 5, 2022

Hi @tdaniely-dn, as of jenkins-infra/repository-permissions-updater#2590 I have removed myself from the list of maintainers for this plugin. I no longer have write access to merge PRs, and I no longer have Artifactory access to perform releases.

If the changes from this PR work, you can consider adopting the plugin to merge and release the PR. The "Contributing to Open Source" workshop from DevOps World 2021 is a useful starting point for new maintainers. That document includes links to a five part video series that illustrates many of the steps. If the plugin is crucial to your work, you may want to ask your employer to support your work efforts by allowing you to adopt the plugin.

@tdaniely-dn
Copy link
Author

tdaniely-dn commented Aug 5, 2022

Hi @tdaniely-dn, as of jenkins-infra/repository-permissions-updater#2590 I have removed myself from the list of maintainers for this plugin. I no longer have write access to merge PRs, and I no longer have Artifactory access to perform releases.

If the changes from this PR work, you can consider adopting the plugin to merge and release the PR. The "Contributing to Open Source" workshop from DevOps World 2021 is a useful starting point for new maintainers. That document includes links to a five part video series that illustrates many of the steps. If the plugin is crucial to your work, you may want to ask your employer to support your work efforts by allowing you to adopt the plugin.

@basil Thank you for the references, and thank you for the contributions. I'll look into adopting.

@Moofasax
Copy link

Oh nice @tdaniely-dn, we would love to have this as part of the standard!!!!

@idrislaw
Copy link

@tdaniely-dn, this is really cool feature. Hope we can get this soon!

@MarkEWaite
Copy link
Contributor

MarkEWaite commented May 22, 2023

@Moofasax and @idrislaw the comment from the earlier message applies to you as well. You could adopt the plugin, merge the pull request, and release it. It would help you and help the Jenkins community.

If the changes from this PR work, you can consider adopting the plugin to merge and release the PR. The "Contributing to Open Source" workshop from DevOps World 2021 is a useful starting point for new maintainers. That document includes links to a five part video series that illustrates many of the steps. If the plugin is crucial to your work, you may want to ask your employer to support your work efforts by allowing you to adopt the plugin.

If you'd prefer a more polished tutorial, the "Improve a Plugin" tutorial provides some of the same information in a more polished form.

If you're not able to adopt the plugin, you could also install the most recent build of the plugin on your controller and report the results of using it. That would meet your needs and provide feedback to others that you've tested the plugin and confirmed that the feature works in a way that meets your needs.

@Moofasax
Copy link

@MarkEWaite thanks for this, I will take a look. I don't know much Java, but I'll check out those links.

We have been using this PR in production with the latest LTS version (2.387.3-lts) of Jenkins. We run about 15 or 20 separate Jenkins instances, with tons of other plugins on them and haven't had any issues with this PR!

I can send screenshots of it installed, or if I can provide anymore feedback on its function, let me know!!

@itjobs-levi
Copy link

I'm trying to automate a lot of new jenkins to automatically launch and allow them to authenticate with github oauth.
For that, it seems that you need to set the redirect_url parameter mentioned in this issue.
This ticket has been updated until recently, how can I use it?

@itjobs-levi
Copy link

I am using the plugin that includes the redirect_uri function in jenkins 2.401.3 version.
It works very nice.
I hope that the plugin with this feature will be officially released soon :)

Copy link
Member

@scurvydoggo scurvydoggo left a comment

Choose a reason for hiding this comment

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

One comment around conventions, otherwise LGTM. I will aim to test this soon.

@@ -115,6 +118,8 @@ public class GithubSecurityRealm extends AbstractPasswordBasedSecurityRealm impl
private Secret clientSecret;
private String oauthScopes;
private String[] myScopes;
@NonNull
private 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.

While this is well intended, as we are not using @nonnull anywhere else in this project I don't think it makes sense to use it here. It would be 'unfair' to all of the other properties/fields, as this one effectively gets arbitrary treatment.

Could you please remove this annotation, and we can discuss adding it as a separate PR?

@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