Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

add is memory token property to sessions #2004

Merged
merged 1 commit into from
Jul 19, 2017
Merged

add is memory token property to sessions #2004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions docs/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1112,6 +1112,12 @@ for the authenticated user.

<!--end-response-body-get-accountdevices-pushAuthKey-->

* `isMemoryToken`: *boolean, required*

<!--begin-response-body-get-accountdevices-isMemoryToken-->
Signifies whether the token was received from redis (i.e an up to date token) if true, or from db if false.
<!--end-response-body-get-accountdevices-isMemoryToken-->


#### GET /account/sessions

Expand Down Expand Up @@ -1202,6 +1208,12 @@ for the authenticated user.

<!--end-response-body-get-accountsessions-isCurrentDevice-->

* `isMemoryToken`: *boolean, required*
Copy link
Contributor

Choose a reason for hiding this comment

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

With appologies for the late drive-by comment, this seems like it's piercing a layer of abstraction - it's taking an internal implementation detail of the server and exposing it in the API for consumption by the client. To display the correct information, the content-server needs to know that the auth-server sometimes stores tokens in redis, and that these tokens are the only ones that are up-to-date.

I wonder if a better abstraction here would involve us sending two separate timestamps:

  • lastAccessTime: the value from redis, but it might be null if we don't have a correct value to show
  • createdTime: the "started syncing" timestamp which we know we can always get from the db

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if a better abstraction here would involve us sending two separate timestamps

Yeah we had that idea earlier, but I Think @philbooth wanted to keep the fields from redis and mysql consistent (to avoid confusion, etc)

Copy link
Contributor

Choose a reason for hiding this comment

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

from mtg:

lastAccessTimeFormatted - redis token
createdTimeFormatted - created token


<!--begin-response-body-get-accountsessions-isMemoryToken-->
Signifies whether the token was received from redis (i.e an up to date token) if true, or from db if false.
<!--end-response-body-get-accountsessions-isMemoryToken-->


#### POST /account/device/destroy

Expand Down
6 changes: 4 additions & 2 deletions lib/db.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,8 @@ module.exports = (
uaBrowserVersion: mergedInfo.uaBrowserVersion,
uaOS: mergedInfo.uaOS,
uaOSVersion: mergedInfo.uaOSVersion,
uaDeviceType: mergedInfo.uaDeviceType
uaDeviceType: mergedInfo.uaDeviceType,
isMemoryToken: mergedInfo.isMemoryToken || false
}
})
})
Expand Down Expand Up @@ -526,7 +527,8 @@ module.exports = (
uaOSVersion: token.uaOSVersion,
uaDeviceType: token.uaDeviceType,
lastAccessTime: token.lastAccessTime,
createdAt: token.createdAt
createdAt: token.createdAt,
isMemoryToken: true
Copy link
Contributor

Choose a reason for hiding this comment

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

@udaraweerasinghege we also need to update the response schemas for /sessions and /devices, to handle this optional property. I'm hoping API.md will auto update with this

}]
let sessionTokens = []
// get the array of session tokens associated with the given uid
Expand Down
10 changes: 6 additions & 4 deletions lib/routes/devices-sessions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,8 @@ module.exports = (log, db, config, customs, push, devices) => {
type: isA.string().max(16).required(),
pushCallback: isA.string().uri({ scheme: 'https' }).max(255).optional().allow('').allow(null),
pushPublicKey: isA.string().max(88).regex(URL_SAFE_BASE_64).optional().allow('').allow(null),
pushAuthKey: isA.string().max(24).regex(URL_SAFE_BASE_64).optional().allow('').allow(null)
pushAuthKey: isA.string().max(24).regex(URL_SAFE_BASE_64).optional().allow('').allow(null),
isMemoryToken: isA.boolean().required()
}).and('pushPublicKey', 'pushAuthKey'))
}
},
Expand All @@ -290,7 +291,6 @@ module.exports = (log, db, config, customs, push, devices) => {
device.lastAccessTime,
request.headers['accept-language']
)

delete device.sessionToken
delete device.uaBrowser
delete device.uaBrowserVersion
Expand Down Expand Up @@ -326,7 +326,8 @@ module.exports = (log, db, config, customs, push, devices) => {
deviceCallbackPublicKey: isA.string().max(88).regex(URL_SAFE_BASE_64).optional().allow('').allow(null),
deviceCallbackAuthKey: isA.string().max(24).regex(URL_SAFE_BASE_64).optional().allow('').allow(null),
isDevice: isA.boolean().required(),
isCurrentDevice: isA.boolean().required()
isCurrentDevice: isA.boolean().required(),
isMemoryToken: isA.boolean().required()
}))
}
},
Expand Down Expand Up @@ -372,7 +373,8 @@ module.exports = (log, db, config, customs, push, devices) => {
request.headers['accept-language']
),
os: session.uaOS,
userAgent
userAgent,
isMemoryToken: session.isMemoryToken || false
}
}))
},
Expand Down
26 changes: 17 additions & 9 deletions test/local/routes/devices-sessions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,9 +447,9 @@ describe('/account/devices', function () {
var unnamedDevice = { sessionToken: crypto.randomBytes(16).toString('hex') }
var mockDB = mocks.mockDB({
devices: [
{ name: 'current session', type: 'mobile', sessionToken: mockRequest.auth.credentials.tokenId },
{ name: 'has no type', sessionToken: crypto.randomBytes(16).toString('hex') },
{ name: 'has device type', sessionToken: crypto.randomBytes(16).toString('hex'), uaDeviceType: 'wibble' },
{ name: 'current session', type: 'mobile', sessionToken: mockRequest.auth.credentials.tokenId, isMemoryToken: true },
{ name: 'has no type', sessionToken: crypto.randomBytes(16).toString('hex'), isMemoryToken: false },
{ name: 'has device type', sessionToken: crypto.randomBytes(16).toString('hex'), uaDeviceType: 'wibble', isMemoryToken: false },
unnamedDevice
]
})
Expand All @@ -468,11 +468,13 @@ describe('/account/devices', function () {
assert.equal(response[0].type, 'mobile')
assert.equal(response[0].sessionToken, undefined)
assert.equal(response[0].isCurrentDevice, true)
assert.equal(response[0].isMemoryToken, true)

assert.equal(response[1].name, 'has no type')
assert.equal(response[1].type, 'desktop')
assert.equal(response[1].sessionToken, undefined)
assert.equal(response[1].isCurrentDevice, false)
assert.equal(response[1].isMemoryToken, false)

assert.equal(response[2].name, 'has device type')
assert.equal(response[2].type, 'wibble')
Expand All @@ -498,7 +500,8 @@ describe('/account/devices', function () {
isCurrentDevice: true,
lastAccessTime: 0,
name: 'test',
type: 'test'
type: 'test',
isMemoryToken: false
}
]
isA.assert(res, route.config.response.schema)
Expand All @@ -514,13 +517,15 @@ describe('/account/sessions', () => {
tokenId: tokenIds[0], uid: 'qux', createdAt: times[0], lastAccessTime: times[1],
uaBrowser: 'Firefox', uaBrowserVersion: '50', uaOS: 'Windows', uaOSVersion: '10',
uaDeviceType: null, deviceId: null, deviceCreatedAt: times[2],
deviceCallbackURL: 'callback', deviceCallbackPublicKey: 'publicKey', deviceCallbackAuthKey: 'authKey'
deviceCallbackURL: 'callback', deviceCallbackPublicKey: 'publicKey', deviceCallbackAuthKey: 'authKey',
isMemoryToken: true
},
{
tokenId: tokenIds[1], uid: 'wibble', createdAt: times[3], lastAccessTime: times[4],
uaBrowser: 'Nightly', uaBrowserVersion: null, uaOS: 'Android', uaOSVersion: '6',
uaDeviceType: 'mobile', deviceId: 'deviceId', deviceCreatedAt: times[5],
deviceCallbackURL: null, deviceCallbackPublicKey: null, deviceCallbackAuthKey: null
deviceCallbackURL: null, deviceCallbackPublicKey: null, deviceCallbackAuthKey: null,
isMemoryToken: true
},
{
tokenId: tokenIds[2], uid: 'blee', createdAt: times[6], lastAccessTime: times[7],
Expand Down Expand Up @@ -559,7 +564,8 @@ describe('/account/sessions', () => {
lastAccessTime: times[1],
lastAccessTimeFormatted: 'a few seconds ago',
os: 'Windows',
userAgent: 'Firefox 50'
userAgent: 'Firefox 50',
isMemoryToken: true
},
{
deviceId: 'deviceId',
Expand All @@ -574,7 +580,8 @@ describe('/account/sessions', () => {
lastAccessTime: times[4],
lastAccessTimeFormatted: 'a few seconds ago',
os: 'Android',
userAgent: 'Nightly'
userAgent: 'Nightly',
isMemoryToken: true
},
{
deviceId: 'deviceId',
Expand All @@ -589,7 +596,8 @@ describe('/account/sessions', () => {
lastAccessTime: times[7],
lastAccessTimeFormatted: 'a few seconds ago',
os: null,
userAgent: ''
userAgent: '',
isMemoryToken: false
},
])
})
Expand Down
2 changes: 2 additions & 0 deletions test/remote/db_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ describe('remote db', function() {
assert.equal(sessions[0].uaOSVersion, '10.10', 'uaOSVersion property is correct')
assert.equal(sessions[0].uaDeviceType, null, 'uaDeviceType property is correct')
assert.equal(sessions[0].lastAccessTime, sessions[0].createdAt, 'lastAccessTime property is correct')
assert.equal(sessions[0].isMemoryToken, undefined, 'isMemoryToken property is correct')
return db.sessionToken(tokenId)
})
.then(function(sessionToken) {
Expand Down Expand Up @@ -199,6 +200,7 @@ describe('remote db', function() {
assert.equal(token.uaBrowserVersion, '41')
assert.equal(token.uaOS, 'Mac OS X')
assert.equal(token.uaOSVersion, '10.10')
assert.equal(token.isMemoryToken, true)
assert.ok(token.lastAccessTime)
assert.ok(token.createdAt)
return db.sessionToken(tokenId)
Expand Down