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 simple basic auth system #589

Merged
merged 3 commits into from Jul 3, 2018

Conversation

Projects
None yet
2 participants
@pierrehts
Contributor

pierrehts commented Jun 29, 2018

Hello,

This is a simple basic auth system that allow user to add these parameters into the yaml configuration :

  • http.basicauth.enabled: true
  • http.basicauth.user: "root"
  • http.basicauth.password: "root"

This way it allows stormcrawler to be used in most of the intranets.

If this PR is accepted I'll produce the associated documentation .

Thanks

@jnioche

Thanks for this PR, see comments

@jnioche

Thanks for the PR, much appreciated.

@@ -54,6 +54,7 @@
import org.apache.http.util.Args;
import org.apache.http.util.ByteArrayBuffer;
import org.apache.storm.Config;
import org.apache.storm.shade.org.yaml.snakeyaml.external.biz.base64Coder.Base64Coder;

This comment has been minimized.

@jnioche

jnioche Jul 2, 2018

Member

I'd rather we relied on non-shaded dependencies - snakeyaml is in our pom already - or even better on the standard Java classes e.g. https://docs.oracle.com/javase/8/docs/api/java/util/Base64.Encoder.html

@@ -112,6 +113,30 @@ public void configure(final Config conf) {
defaultHeaders.add(new BasicHeader("Accept", accept));
}
boolean useBasicAuth = ConfUtils.getBoolean(conf,

This comment has been minimized.

@jnioche

jnioche Jul 2, 2018

Member

Instead of having a separate configuration for activating or deactivating the authentication, what about checking if http.basicauth.user is not null? If it is set, we use it. That would keep things simpler.

@pierrehts

This comment has been minimized.

Contributor

pierrehts commented Jul 3, 2018

Thanks for the review, I adjusted the code accordingly,
Also, I modified the logic to allow user to set an empty password if necessary.

@jnioche

jnioche approved these changes Jul 3, 2018

@jnioche jnioche merged commit d8ace2a into DigitalPebble:master Jul 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jnioche

This comment has been minimized.

Member

jnioche commented Jul 3, 2018

Thanks @pierrehts. I have modified https://github.com/DigitalPebble/storm-crawler/wiki/Protocols to indicate that httpclient has a mechanism for basic authentication. I can give you write access to the WIKI if you want to update https://github.com/DigitalPebble/storm-crawler/wiki/Configuration.
Would be great to add the same mechanism to the okhttp implementation, would you be interested in having a look at that?
Thanks again

@jnioche jnioche added this to the 1.11 milestone Jul 3, 2018

@pierrehts

This comment has been minimized.

Contributor

pierrehts commented Jul 5, 2018

Thanks for accepting the PR.

Please, give me write access to the WIKI, I'll produce documentation for the new properties.
And yes, I'll implement this into okhttp as soon as I have some free time.

@jnioche

This comment has been minimized.

Member

jnioche commented Jul 5, 2018

Hi @pierrehts, have sent you an invite to become a collaborator. You should be able to write to the WIKI after that.
Thanks for documenting the new properties.
BTW if you are using StormCrawler for an organisation, why not add it to https://github.com/DigitalPebble/storm-crawler/wiki/Powered-By?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment