Skip to content

Conversation

@pxtxs
Copy link
Contributor

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

Copy link
Contributor

@jnioche jnioche left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, see comments

Copy link
Contributor

@jnioche jnioche left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, much appreciated.

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

Choose a reason for hiding this comment

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

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

defaultHeaders.add(new BasicHeader("Accept", accept));
}

boolean useBasicAuth = ConfUtils.getBoolean(conf,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@pxtxs
Copy link
Contributor Author

pxtxs 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 merged commit d8ace2a into apache:master Jul 3, 2018
@jnioche
Copy link
Contributor

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
@pxtxs
Copy link
Contributor Author

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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants