Feature/auth-ish

[dazza-codes]

This feature branch should encapsulate the authentication requirements, as discussed and documented in google doc at https://docs.google.com/a/stanford.edu/document/d/1ceBhDf71ah0ShWYVGntu8l5BsNfVuXV2JAYTzF0zoAE/edit?usp=sharing_eid and also based partially on the IIIF authentication spec (i.e. http://image-auth.iiif.io/api/image/2.1/authentication.html).

If this feature branch is reviewed and merged, the next step is to actually enable the authentication as a before filter on all the routes. This PR does not yet do that (one step at a time).

[ndushay]

I think this looks ok but I'm wondering if we want to accept logins via GET with the userid and password in the url? Are they somehow encrypted to avoid cleartext? I know this is really for our batch loading need but I've had "no passwords over the net in cleartext" drilled into me.

[dazza-codes]

@ndushay says, "I'm wondering if we want to accept logins via GET with the userid and password in the url?" The current implementation is using rails helpers for authentication, which uses HTTP header values that are encrypted. Combined with HTTPS, the assumption is that this will be secure. If not, then we should review alternatives that will be secure. More info on this is available at

• http://stackoverflow.com/questions/5101020/in-ruby-on-rails-what-does-authenticate-with-http-basic-do
• http://www.w3.org/Protocols/HTTP/1.0/spec.html#AA

[azaroth42]

:-1: to login/password with GET. Even over https the requested URL with params gets logged to the apache log file. An attacker with access to the log could then potentially bootstrap themselves with the information.

Especially as Triannon doesn't have any user/password management system, it would never actually be able to respond to the request sensibly.

[dazza-codes]

@azaroth42 - I'm not clear on whether your requesting any specific changes, WRT a GET request for /auth/login/. The HTTP basic authentication is a GET request that uses header data, not URI parameters. Is that OK or are you proposing to scrap any GET request? If it is scrapped, then that will disable any browser access to /auth/login and triannon is, in part, a browser-compatible app (at this time).

[azaroth42]

(2nd attempt, after rereading) Where does it get the username / password table from? Seems like it's out of scope for us at Stanford at the moment.

[dazza-codes]

@azaroth - agreed, triannon has no 'user' authentication, so we should disable GET /auth/login? That leaves POST /auth/login, which is used by an authenticated client to POST user data (e.g. from SW auth) and thereby fake a user login. I'm not yet clear about the consequences for disabling GET /auth/login, but they might be minimal.

[azaroth42]

Hope they're minimal :)

[dazza-codes]

Well, it breaks about half of all the auth specs, so there's work to fix all that.

[dazza-codes]

@ndushay & @azaroth42 - the last two commits (836754e & 22fbf60) should remove all traces of user authentication, while leaving the functionality for an authorized client to POST /auth/login with user login credentials (they are not authenticated by triannon, which trusts the authorized client to have done the authentication already, e.g. SW-webauth).

[dazza-codes]

If I can merge this to move on, we can discuss javascript and CORS support in a new feature branch. Before that, my next priority is getting some auth code into the ruby client.

[ndushay]

@darrenleeweber I wonder if it's related to the ruby version or a gem version. _run_save_callback is in the Annotation model code for an anno callback -- it's part of ActiveModel, I think.

[dazza-codes]

Commit 4364558 should improve the spec coverage because it tests the access token validation; @azaroth42 might have to review these specs to advise whether they are consistent with the desired behavior - note especially that the current code requires a login session to validate an access token.

[ndushay]

Here is a link (thanks, @cbeer) by one of the main rails developers about gemspec vs. Gemfile.lock and gems vs. apps - hopefully this will clarify why putting Gemfile.lock in the git repo for a gem isn't desirable:

http://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/

[ndushay]

I also dunno if you want to squash your commits at all before the merge, particularly commits that are removing files that originated in this PR.

[dazza-codes]

OK, reviewed the blog post about Gemfile.lock at

• http://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/

It all makes sense to me. Please note that Gemfile.lock will not be used by gem install, so it should have no impact at all on the triannon-app. I'm wondering whether we need to have a PR acceptance rule that the triannon-app is OK with the code changes, but that would require using a github source for testing the triannon gem in the triannon-app.

[ndushay]

I would say let's not have something test its downstream usage; let's instead hope and assume it will work in triannon-service, and if it doesn't, THEN you will update the triannon gem??

Looks like travis build is unhappy again, but perhaps now you can identify whatever gem pinning you might need to do in the gemspec file?

[ndushay]

@darrenleeweber I think you need to compare the specific gem versions used by travis (avail in the travis output for an individual ruby build) against the gem versions in use (bundle exec gem list or look at Gemfile.lock on your local machine), and figure out which gem(s) you need to pin to a particular version to get it to pass.

I'm sure this is really annoying to work on, but we shouldn't merge until travis passes.

[dazza-codes]

@ndushay - I could try to pin actionpack to 4.2.2, because the errors arise in 4.2.3, OR you might take a look at the Annotation#save method to update it for 4.2.3? (I am not sure why my gemset is so different from travis in this regard, given that I often run bundle update when working on a gem.)

1) Triannon::AnnotationsController#create creates a new annotation from the body of the request
     Failure/Error: post :create, ttl_data, anno_root: @root_container
     NoMethodError:
       undefined method `_run_save_callbacks' for #<Triannon::Annotation:0x0000000b00b5d0>
     # ./app/models/triannon/annotation.rb:43:in `save'
     # ./app/controllers/triannon/annotations_controller.rb:84:in `create'
     # /home/travis/.rvm/gems/ruby-2.2.2/gems/actionpack-4.2.3/lib/action_controller/metal/implicit_render.rb:4:in `send_action'

[ndushay]

Why don't you pin it for now and comment in the gemspec that I need to look at Annotation.save to get rid of pinning; also pls create a github issue to fix this and assign it to me. I'm in the middle of something else right now, but I'll be sure to take a look.

[dazza-codes]

Issue for rails 4.2.3 is in #206

[dazza-codes]

Travis is crashing even with the rails pin at 4.2.2 and I'm out of time and patience to fix it. It worked with Gemfile.lock and these failures are not related to this auth-ish code for this PR. Nevertheless, I'm not going to merge this PR until travis is fixed.

[ndushay]

I just did the following:

• checked out this branch
• checked out Gemfile and Rakefile from the master branch into my new branch based on this one
• rake engine_cart:clean
• rake ci

and the tests ran cleanly. I think your concerns about engine_cart are red herrings, and those changes are not required for travis or elsewhere.

For fun, I also commented out the line pinning the tilt gem in the Gemfile, and it ran - so that is no longer a problem - yay! (New gem releases probably resolved the conflict).

I can do a push with the Gemfile and Rakefile I used, if you like, or you could revise and remove those commits; whatever.

@atz mentioned at lunch that he would be willing to work with you on git rebase and show you how to squash commits - i think you'll find it straightforward enough.

[azaroth42]

Thanks @ndushay and @atz!

[dazza-codes]

@ndushay - revise your test using

rake engine_cart:clean
bundle install # or bundle update would be better
rake ci

Let me know if you get any problems using the Gemfile and Rakefile from master.

[dazza-codes]

This branch should be OK to merge, now that travis build passed and coveralls is OK.

[ndushay]

@darrenleeweber I just did what you asked:

• git co auth-ish
• git co master Gemfile
• git co master Rakefile
• rake engine_cart:clean
• bundle install
• rake ci

and it was smooth sailing - the tests all passed.

I say let's merge once you've got the simpler, original Gemfile and Rakefile and squash your commits. Yay - you solved it!

[dazza-codes]

@ndushay - just because it worked on your laptop doesn't mean it will work on travis. Whatever is working on travis in this PR is working on my laptop and travis, so why mess with it?

[dazza-codes]

I'd rather learn to squash commits another time. I don't want to mess with something that is working and has taken a lot longer than I anticipated to get it working. The last thing I need right now is to mess it up by learning something about git squash, I'd rather do that on throw-away code.

[dazza-codes]

In any case, this merge is into develop, not master.

[ndushay]

I'll commit my changes (Gemfile and Rakefile) and if it breaks travis, i'll uncommit 'em.

The squashing of commits is good practice and lots of us have done it many many times without a problem. It's something git supports well.

Also, I was mistaken about tilt - Gemfile still needs the version pinned.

[dazza-codes]

I don't want you to change the Gemfile and Rakefile, for reasons that I'll explain F2F.

[ndushay]

oops! sorry - didn't see your note. honest.

[dazza-codes]

Lets resolve the Gemfile and Rakefile tweaks in another PR. The point of getting them 'fixed' was to allow this PR to pass for the more important auth code.

[dazza-codes]

My laptop and travis pass specs with my Gemfile/Rakefile modifications, see https://travis-ci.org/sul-dlss/triannon/builds/69807916

[ndushay]

It's not that I doubt that your Gemfile and Rakefile mods function - they ran on my laptop also.

The issue is that your changes are unnecessary, won't make sense to anyone but you, and add complexity for no gain.

We have lots of rails engine gems created in DLSS using engine_cart with builds that run in travis and on developers laptops; none of those analogous gems have needed the changes you are fighting for.

[dazza-codes]

My changes are NOT unnecessary. So, I can explain why my changes are necessary. Can you explain why the 'tilt' hack is required?

[ndushay]

i'm done arguing with you over following accepted practices. I will let @azaroth42 weigh in.