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 gitlab and github token Auth support #14 #18

Merged
merged 23 commits into from
Feb 12, 2021
Merged

add gitlab and github token Auth support #14 #18

merged 23 commits into from
Feb 12, 2021

Conversation

vgropp
Copy link
Contributor

@vgropp vgropp commented Feb 5, 2021

i am awaiting your comments, this works to support github and gitlab private repos in my Tests #14

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

This looks great! I have an idea which I can describe better in a commit. I'm gonna push it up to your feature branch and get your thoughts, we can always revert later.

src/main/java/com/diffplug/blowdryer/BlowdryerSetup.java Outdated Show resolved Hide resolved
@nedtwigg
Copy link
Member

nedtwigg commented Feb 5, 2021

There are two things I like about this fluent-configurator pattern:

  1. discoverability
  2. built-in error-checking for incompatible modes

For security's sake, you want the auth to be specific only to the repository which the user has configured, and you never want to send credentials that the user intended for https to get sent over http. If the user has to coordinate those method calls themselves, then they might get it wrong. By putting it all into a fluent configurator, we can do all that work for the user, and guarantee that the inputs all stay consistent and secure.

Lemme know what you think. If you're okay with the basic idea but prefer different method names, feel free to change them and/or add javadoc. Please test the auth locally on your machine too, I didn't do any manual testing.

Once you're satisfied with the API and that the auth works, please add a changelog entry and I'll merge and release. I'm happy to update the docs myself, once it's released I can put in links to the javadoc for BlowdryerSetup.GitHub/GitLab.

@vgropp
Copy link
Contributor Author

vgropp commented Feb 7, 2021

I really like the fluent-configurator idea.

But I had problems with your first approach because Blowdryer.setResourcePlugin was called twice when using gitlab('private/repo', 'tag', '" + tag + "').authToken('foobar'). What do you think of solution to encapsolate it in a interface?

I only checked this yet with the (locally customized) ignored unit-test

@nedtwigg
Copy link
Member

nedtwigg commented Feb 8, 2021

I'm not inclined to to turn AuthToken support into an interface, because it bakes "Authorization", "Bearer " + authToken as the authentication model, and there are lots of other ones out there. I try to be very stingy about public interfaces in open-source plugins. Stupid-simple functions work really well for maintaining back-compat in a public API, but interfaces grow stale pretty quick as different needs arise.

You raised a great point about double-setting the plugins. That is a security feature (once a user sets up blowdryer, a malicious plugin should not be able to redirect it). The Auth should really be set atomically with the repo for the same reason, so I reverted your Authable interface and made the Resource/Auth pair atomic.

Once you are satisfied with the tests, I am satisified to merge and release.

@vgropp
Copy link
Contributor Author

vgropp commented Feb 10, 2021

I'm fine with your changes. The unit tests for auth against github and gitlab are working, but i have trouble setting up local Test in a real gradle project. I dont think these troubles are caused by these changes.

my changelog entries have been reverted with the interface commit, maybe you want to restore those, and documentation is probably missing

@vgropp
Copy link
Contributor Author

vgropp commented Feb 12, 2021

ok, i was finally able to successfully test the changes in a local real world project.
In addition to make the chnage sin contributing.md i had to use apply(plugin: "com.diffplug.blowdryerSetup") in the setting.gradle instead of plugins { id 'com.diffplug.blowdryerSetup' }
That is with gradle 6.8, maybe this helps as well or we can add it to CONTRIBUTING.md

Any chance to merge this soon and release a new version so we can start to actually use it? thanks

@nedtwigg nedtwigg merged commit 33fe34c into diffplug:master Feb 12, 2021
@nedtwigg
Copy link
Member

Published in 1.1.0.

@vgropp vgropp deleted the auth-support-#14 branch February 13, 2021 09:53
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.

None yet

2 participants