Feeds HTTPFetcher AA (Advanced Authentication) [#1928682]

Description:

This module provides support for more advanced authentication mechanisms
using the Drupal Feeds module. http://drupal.org/project/feeds

Features

Most notably, this can be used for feeds requiring a 2-step authentication
where the first request passes authentication credentials, and then the
server returns a form of security tokens to be passed in future reqeusts.

The credentials, and return token keys to look for is fully configurable
in the Fetcher settings for your Feed. The communication protocol is also
configurable, plain &url=var and json posting is supported currently.

Use Cases

Your feed requires a more advanced authentication mechanism than what Feeds Basic HTTP Fetcher provides.
Your feed requires specific key|value pairs for authentication.
Your feed requires a JSON post of credentials to authenticate.
Your feed uses a 2-step authentication. You post credentials, it authenticates and returns security tokens to you which you then have to pass in subsequent calls.

Sandbox page: http://drupal.org/sandbox/drastik/1928654
Git clone: git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/drastik/1928654.git
Drupal version: 7.x
Coder report: http://ventral.org/pareview/httpgitdrupalorgsandboxdrastik1928654git

Notes about Coder report:
"all functions should be prefixed with your module":
It follows this in everywhere except the http_aa_request.inc where it follows Feeds' pattern of naming.

"Class property $file_path should use lowerCamel naming without underscores":
Again, here it is following Feeds' other classes naming patterns.

Other project reviews (PAReview):

http://drupal.org/node/1920606#comment-7113902
http://drupal.org/node/1495600#comment-7118774
http://drupal.org/node/1914096#comment-7118800

Comment	File	Size	Author
#7	FeedsHTTPFetcherAA-validation.png	96.27 KB	jwjoshuawalker
	Feeds HTTP AA.png	77.84 KB	jwjoshuawalker

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jwjoshuawalker CreditAttribution: jwjoshuawalker commented 28 February 2013 at 04:14

Other project reviews (for bonus):

http://drupal.org/node/1920606#comment-7113902
http://drupal.org/node/1495600#comment-7118774
http://drupal.org/node/1914096#comment-7118800

Comment #2

jwjoshuawalker CreditAttribution: jwjoshuawalker commented 28 February 2013 at 04:15

Issue tags:

+PAreview: review bonus

Tagging with review bonus.

Comment #2.0

jwjoshuawalker CreditAttribution: jwjoshuawalker commented 28 February 2013 at 04:15

Issue summary:

View changes

Added Use Cases to see the value of this module.

Comment #3

aendra CreditAttribution: aendra commented 28 February 2013 at 12:11

✓ Automated Review

✓ Clean; excepting the two notices you mentioned.

✗ Manual Review

ⓘ README.txt -- You should probably add to this. While configuring the fetcher, I found the "Authentication response fields" section particularly confusing -- that said, I don't really have any authenticated feeds to try this with (Providing a step-by-step case-study would be super helpful). You should also explain what "Test mode" does here.
✗ libraries/http_aa_request.inc -- You use drupal_http_request() as a fallback, noting it won't do JSON post. In plugins/FeedsHTTPFetcherAA.inc, ln. ~170, you should do the same check for cURL and disable the JSON option if cURL is not available (If I'm understanding those comments correctly).
✗ I got this Notice (only appeared in dblog, not on-screen) when I tried to import my (probably misconfigured) feed:

Notice: Undefined variable: auth_post_format in FeedsHTTPFetcherAAResult->getRaw() (line 37 of /Users/aendrew/Sites/drupal-7.x-dev/sites/all/modules/feeds_httpfetcher_aa__advanced_authentication_/plugins/FeedsHTTPFetcherAA.inc).
✓ Everything else looks great, though!

Comment #4

aendra CreditAttribution: aendra commented 28 February 2013 at 12:12

Status:

Needs review

» Needs work

Forgot to change status.

Comment #5

jwjoshuawalker CreditAttribution: jwjoshuawalker commented 2 March 2013 at 09:23

Status:

Needs work

» Needs review

@aendrew, nice catch and thank you.

That getRaw() typo/oversight bugs me. When I got the job specs, it was one of those situations where I had almost every line of the module written in my head before I started. I knocked it out one evening, very excited knowing how useful it actually was, so I wanted to contribute back. I guess my hands couldn't keep up :)

CHANGELOG:

Made drupal_http_request() work with JSON posts.
Properly access $this->auth_post_format in getRaw().
Other code design improvements.
Clarified descriptions of fields.
Added several form state reactions, option to toggle 2-step authentication.
Added more details in README.txt and Project page.

I've double checked everything and added several features while at it. I believe it to be solid. Also coder review of new version is clean like the last.

You said: "that said, I don't really have any authenticated feeds to try this with"
I will put up a services-powered feed (or something similar) to http://drastikbydesign.com 's blog that requires authentication, to test with.

Comment #6

klausi

he/him||they/them

German

🇦🇹 Vienna

CreditAttribution: klausi commented 2 March 2013 at 10:40

Assigned:	Unassigned	» cubeinspire
Status:	Needs review	» Reviewed & tested by the community
Issue tags:	-PAreview: review bonus

manual review:

I still would say that the fetcher function should be prefixed with your module's name, to stay out of the namespace of a potential other module. Yes, in this regard Feeds does it wrong and you should not follow. Same for the class properties.
"header('HTTP/1.1 503 "Not Found"', NULL, 503);": do not use header(), use drupal_add_http_header() instead.
feeds_httpfetcher_aa.info: dependency to Feefs is missing?

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to cubeinspire as he might have time to take a final look at this.

Comment #7

jwjoshuawalker CreditAttribution: jwjoshuawalker commented 2 March 2013 at 17:50

File	Size
FeedsHTTPFetcherAA-validation.png	96.27 KB

Thanks klausi.

Applied everything from #6, as well as some extra validation on settings form to save user's time in the case of mis-configuration.
Ventral clean: http://ventral.org/pareview/httpgitdrupalorgsandboxdrastik1928654git

CHANGELOG:

Add Feeds dependency.
Change .inc file & function names to prefix this module's name
Replace header() calls with drupal_add_http_header().
Perform validation on settings form to check for login_url when using 2-step.
Replace Feeds-style named class properties with lowerCamel style.

Comment #8

jthorson CreditAttribution: jthorson commented 6 March 2013 at 21:42

Status:

Reviewed & tested by the community

» Fixed

Thanks for your contribution, drastik!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.