search

sul-dlss-deprecated / triannon

Rails engine for working with storage of OpenAnnotations stored in Fedora4
Other
13 stars 1 forks source link

Feature/app auth #200

Closed dazza-codes closed 9 years ago

dazza-codes commented 9 years ago

This PR is for review of the current state of the auth_controller, which is or should be close to the requirements of the IIIF image authentication, see http://image-auth.iiif.io/api/image/2.1/authentication.html. (The main exception to that spec is missing support for JSONP requests.)

Further work to adapt this for Triannon will be done in another branch:

ndushay commented 9 years ago

BTW, it's possible to compare branches in github without creating a PR (but I don't think you can create the artifact - only do it live)

dazza-codes commented 9 years ago

I'm proposing a merge of this feature into the develop branch, to pursue the next phase of this auth development, by issuing a PR for the auth-ish branch. Unless I hear otherwise, I'll proceed with this merge on Thu or Fri (June 18-19).

dazza-codes commented 9 years ago

@ndushay - this PR is ready for review, as I think the coveralls failure is trivial and, if not, it looks like something for you to review (I've taken care of things that I've modified, I think).

dazza-codes commented 9 years ago

@azaroth42 - this PR represents an integration of the multi-roots with a "IIIF-spec" for auth. Once we merge this PR, additional revisions to auth will be in the 'auth-ish' branch (and eventual PR).

ndushay commented 9 years ago

Overall: I can't offer anything useful on the guts of the actual authentication code; the auth code functionality seems okay to my ignorant eyeball overview.

My main concerns: auth timeouts should be a config setting, not hardcoded HTTP verb handling should be done with routes, not in controller, if possible routing specs are conventionally put in spec/routing, not made part of controller specs.

I don't think the .gitattributes thing worked as the vcr diffs are still showing in this PR. Too bad.

BTW, rails routes are really sensitive to ordering, so it might have worked to put your auth routes at the end, but I'm fine with your routing changes (caveat: need the fix to not do id constraints when not needed)

dazza-codes commented 9 years ago

I think the coveralls failure is trivial, so this PR should now have addressed most of the points raised in @ndushay's review. Additional points require consideration and advice from @azaroth42.

dazza-codes commented 9 years ago

@azaroth42 - please review the auth-code, as @ndushay is unfamiliar with the IIIF auth-spec. For reference, see https://github.com/IIIF/auth/blob/master/server/pi3f.oauth.py

azaroth42 commented 9 years ago

It looks correct to me, but NB I'm just following the high level logic, not the code itself.

dazza-codes commented 9 years ago

In the interests of moving on, I'll merge this and address additional issues in the auth-ish branch, once these changed are merged/integrated there.