Cookie authentication #71

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

rapidrapids commented May 15, 2012

Further to issue #65 it is unfortunately not possible to use request defaults for this.

Request will only use values held in its defaults that do not already exist in the current request. Because nano has to always pass over headers (for content-type for example), any 'default' headers will never be used by request.

This pull differs slightly from my original proposal in #65 - I think its cleaner, because you only pass in the cookie in nano's configuration object literal (not via params).

I'm unsure if the test I've added is sufficient, hope so! :)

Many thanks

Jonathan Mahoney added some commits May 15, 2012

Jonathan Mahoney Added support for cookie authentication
Can now add `cookie` to nano configuration (updated read me
accordingly). Also fixed a typo on line 815.
d99aa58
Jonathan Mahoney Added test for cookie authentication ac6113f
Contributor

rapidrapids commented May 15, 2012

Note that this pull request also fixes a very minor type on line 810 (now line 815) of nano.js

Owner

dscape commented May 15, 2012

@mikeal any ideas here?

Owner

dscape commented May 15, 2012

I have tried this and it failed, can you help me out please?

After starting a CouchDB instance:

node tests/shared/cookie.js 

The modified test:

var specify  = require("specify")
  , helpers  = require("../helpers")
  , timeout  = helpers.timeout
  , Nano     = helpers.Nano
  , nock     = helpers.nock
  , nano     = Nano({ url : helpers.couch, cookie: "abc123" })
  ;

var mock = nock(helpers.couch, "shared/cookie")
  , db   = nano.use("shared_cookie")
  ;

specify("shared_cookie:setup", timeout, function (assert) {
  nano.db.create("shared_cookie", function (error, response, headers) {
    console.log(headers)
    assert.equal(error, undefined, "Failed to create database");
    assert.equal(response.request.headers.cookie, "abc123", 
      "Cookie not sent in request headers");
  });
});

specify("shared_cookie:test", timeout, function (assert) {
  db.insert({"foo": "bar"}, null, function (error, response, headers) {
    console.log(headers)
    assert.equal(response.request.headers['X-CouchDB-WWW-Authenticate'],
      "Cookie", "Request header 'X-CouchDB-WWW-Authenticate' was missing");
    assert.equal(response.request.headers.cookie, "abc123", 
      "Cookie not sent in request headers");
  });
});

specify("shared_cookie:teardown", timeout, function (assert) {
  nano.db.destroy("shared_cookie", function (err) {
    assert.equal(err, undefined, "Failed to destroy database");
    assert.ok(mock.isDone(), "Some mocks didn't run");
  });
});

specify.run(process.argv.slice(2));
Owner

dscape commented May 15, 2012

You can check my branch (including your patches) at https://github.com/dscape/nano/tree/cookies.

This looks great btw. Much better than the first proposal. I'm disappointed that request can't do this, so waiting on @mikeal to confirm.

Owner

dscape commented May 15, 2012

Am I missing something here?

@dscape dscape added a commit that referenced this pull request May 15, 2012

@dscape dscape [fix] fixes using defaults in request
* detected in #71 by @beardtwizzle

* minor version 3.0.2
8bfe094
Contributor

rapidrapids commented May 16, 2012

To be fair I had only run the suite (I.e. npm test) - which all passes. Leave it with and I'll fix

Contributor

rapidrapids commented May 16, 2012

OK I've finally fixed this... note that you MUST have a server admin account on your couchdb that you're testing against - here I've just set the user to admin and password to password.

Note also that I've added in 'form' option for relax, which mimics the same functionality in request (I needed it not only for my test, but in general for the cookie login process):

form - sets body but to querystring representation of value and adds Content-type: application/x-www-form-urlencoded; charset=utf-8 header.

Owner

dscape commented May 16, 2012

I have a talk today at JSDay in Italy but will check this out asap

Owner

dscape commented May 22, 2012

Hey!

I'm trying to merge this but you seem to have deleted the branches??

I don't merge on github, so this is making the task quite difficult for me.

Can you please re-submit both pull request with a working branch? Or at least tell me where I can fetch this from??

Thank you

rapidrapids referenced this pull request May 22, 2012

Closed

Cookies #77

@dscape dscape added a commit that referenced this pull request May 23, 2012

@dscape dscape [cookies] fixes #77 #73 and #71 003127d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment