Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

renew session after username / password change #74

Merged
merged 2 commits into from
Feb 15, 2016
Merged

renew session after username / password change #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a99ce20
test: renew session after username / password change
NickColley Feb 2, 2016
85596e6
fix: renew session after username / password change
NickColley Feb 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions lib/update.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ var clone = require('lodash/clone')
var Promise = require('lie')

var internals = module.exports.internals = {}
internals.deserialise = require('../utils/deserialise')
internals.request = require('../utils/request')
internals.saveAccount = require('../utils/save-account')
internals.serialise = require('../utils/serialise')
Expand All @@ -25,6 +26,19 @@ function update (state, options) {
})

.then(function () {
return internals.request({
url: state.url + '/session',
method: 'PUT',
body: internals.serialise('session', options)
})
})
Copy link
Member

Choose a reason for hiding this comment

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

I’m sorry I’ve missed a problem here. account.update(options) can be called with options.password only. That results in the PUT /session request to only send

{
    "data": {
        "type": "session",
        "attributes": {
            "password": "secretnew"
        }
    }
}

If options.username is not set, we must default it to account.username.

That also means that account.update({username: 'newusername'}) will not work, we must require the password to be always passed, as we require it for the PUT /session request

The alternative would be that the server can set a new session ID in the server response headers, so that we wouldn’t need the PATCH /session request altogether. It would also be good because a password change does not require a session renewal as far as I know.

Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

As an alternative to the header, the JSON API response could include the session in the body, in relationships.data.

{
  "links": {
    "self": "https://example.com/session/account"
  },
  "data": {
    "id": "abc4567",
    "type": "account",
    "attributes": {
      "username": "john-doe"
    },
    "relationships": {
      "profile": {
        "links": {
          "related": "https://example.com/session/account/profile"
        },
        "data": {
          "type": "profile",
          "id": "abc4567-profile"
        }
      },
      "session": {
        "data": [
          {
            "type": "session",
            "id": "newsessionid"
          }
        ]
      }
    }
  }
}

I think this is too complicated though, I’d prefer a x-set-session header or similar

Copy link
Member

Choose a reason for hiding this comment

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

I’ve created a new issue, let’s discuss there: #75


.then(function (response) {
var data = internals.deserialise(response.body, {
include: 'account'
})
state.account.session.id = data.id

merge(state.account, options)
internals.saveAccount({
cacheKey: state.cacheKey,
Expand Down
2 changes: 1 addition & 1 deletion test/integration/events-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ test('events', function (t) {
nock(baseURL)
.put('/session/account')
.reply(201, signUpResponse)
.put('/session').twice()
.put('/session').thrice()
.reply(201, signInResponse)
.patch('/session/account')
.reply(201, updateResponse)
Expand Down
14 changes: 11 additions & 3 deletions test/integration/update-account-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@ var options = {
password: 'secret'
}

var uniqueId = 1

test('sign in and change username', function (t) {
store.clear()

t.plan(10)
t.plan(11)

var account = new Account({
url: baseURL,
Expand All @@ -41,12 +43,17 @@ test('sign in and change username', function (t) {
.put('/session', function (body) {
return body.data.attributes.password === 'newsecret'
})
.reply(201, signInResponseAfterUpdate)
.thrice()
.reply(201, function () {
// Session is updated everytime a user puts
signInResponseAfterUpdate.data.id = 'session' + (uniqueId++)
return signInResponseAfterUpdate
})

account.signIn(options)

.then(function (signInResult) {
t.pass('signes in')
t.pass('signs in')
t.is(signInResult.username, 'chicken@docs.com')

return account.update({ username: 'newchicken@docs.com', password: 'newsecret' })
Expand All @@ -56,6 +63,7 @@ test('sign in and change username', function (t) {
t.pass('update resultResult received')
t.is(updateResult.username, 'newchicken@docs.com', 'new account name in result')
t.is(account.username, 'newchicken@docs.com', 'account username set to new one')
t.is(updateResult.session.id, account.get('session.id'), 'account session should be the same as the result')

return account.signOut()
})
Expand Down
24 changes: 15 additions & 9 deletions test/unit/update-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,7 @@ test('update without change', function (t) {
})

test('update with change', function (t) {
t.plan(5)

simple.mock(update.internals, 'request').resolveWith({
statusCode: 204,
body: null
})
simple.mock(update.internals, 'serialise').returnWith('jsonData')
simple.mock(update.internals, 'saveAccount').callFn(function () {})
t.plan(6)

var state = {
cacheKey: 'cacheKey123',
Expand All @@ -35,19 +28,32 @@ test('update with change', function (t) {
}
}

simple.mock(update.internals, 'request').resolveWith({
statusCode: 204,
body: null
})
simple.mock(update.internals, 'deserialise').returnWith(state.account.session)
simple.mock(update.internals, 'serialise').returnWith('jsonData')
simple.mock(update.internals, 'saveAccount').callFn(function () {})

update(state, {
username: 'treetrunks'
})

.then(function (account) {
t.deepEqual(update.internals.request.lastCall.arg, {
t.deepEqual(update.internals.request.calls[0].arg, {
method: 'PATCH',
url: 'http://example.com/session/account',
headers: {
authorization: 'Bearer abc1234'
},
body: 'jsonData'
})
t.deepEqual(update.internals.request.calls[1].arg, {
method: 'PUT',
url: 'http://example.com/session',
body: 'jsonData'
})
t.deepEqual(update.internals.saveAccount.lastCall.arg, {
cacheKey: 'cacheKey123',
account: {
Expand Down