-
Notifications
You must be signed in to change notification settings - Fork 76
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.
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
Add endpoint to delete current session #923
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,24 +48,30 @@ module.exports = (service, endpoint) => { | |
service.get('/sessions/restore', endpoint((_, { auth }) => | ||
auth.session.orElse(Problem.user.notFound()))); | ||
|
||
const logOut = (Sessions, auth, session) => | ||
auth.canOrReject('session.end', session.actor) | ||
.then(() => Sessions.terminate(session)) | ||
.then(() => (_, response) => { | ||
// revoke the cookie associated w the session, if the session was used to | ||
// terminate itself. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When do you foresee clients invalidating other sessions? Is there even any point in authorizing a request to invalidate a session? If a client knows a session token, then isn't that enough authority to invalidate that session? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DELETE /v1/sessions/{token} is used for multiple things. One of them is for logging yourself out, but another is revoking access for app users and public links. See the comment above about app users. Project managers can revoke app users' sessions, but someone with a token for an app user or public link shouldn't be able to revoke its access. On the other hand, DELETE /v1/sessions/current will probably just be used for logout. |
||
// TODO: repetitive w above. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does this TODO imply? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Heh I'm actually not 100% sure, mostly I'm just copying it over. I'm guessing that what's repetitive is the cookie attributes, which are repeated in the session create endpoint and the session delete endpoint. I'd be happy to try to resolve this comment by creating a function to return those two cookies with the correct attributes. Both endpoints would call that function. |
||
if (session.token === auth.session.map((s) => s.token).orNull()) | ||
response.cookie('__Host-session', 'null', { path: '/', expires: new Date(0), | ||
httpOnly: true, secure: true, sameSite: 'strict' }); | ||
|
||
return success; | ||
}); | ||
|
||
service.delete('/sessions/current', endpoint(({ Sessions }, { auth }) => | ||
auth.session.map(session => logOut(Sessions, auth, session)) | ||
.orElse(Problem.user.notFound()))); | ||
|
||
// here we always throw a 403 even if the token doesn't exist to prevent | ||
// information leakage. | ||
// TODO: but a timing attack still exists here. :( | ||
service.delete('/sessions/:token', endpoint(({ Sessions }, { auth, params }) => | ||
Sessions.getByBearerToken(params.token) | ||
.then(getOrReject(Problem.user.insufficientRights())) | ||
.then((session) => auth.canOrReject('session.end', session.actor) | ||
.then(() => Sessions.terminate(session)) | ||
.then(() => (_, response) => { | ||
// revoke the cookie associated w the session, if the session was used to | ||
// terminate itself. | ||
// TODO: repetitive w above. | ||
if (session.token === auth.session.map((s) => s.token).orNull()) | ||
response.cookie('__Host-session', 'null', { path: '/', expires: new Date(0), | ||
httpOnly: true, secure: true, sameSite: 'strict' }); | ||
|
||
return success; | ||
})))); | ||
|
||
.then(session => logOut(Sessions, auth, session)))); | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -269,6 +269,35 @@ describe('api: /sessions', () => { | |
}))))); | ||
}); | ||
|
||
describe('/current DELETE', () => { | ||
it('should return a 404 if no token was specified', testService(service => | ||
service.delete('/v1/sessions/current') | ||
.expect(404))); | ||
|
||
it('should invalidate the token if successful', testService(async (service) => { | ||
const { body: session } = await service.post('/v1/sessions') | ||
.send({ email: 'alice@getodk.org', password: 'alice' }) | ||
.expect(200); | ||
const { token } = session; | ||
const { body } = await service.delete('/v1/sessions/current') | ||
.set('Authorization', `Bearer ${token}`) | ||
.expect(200); | ||
body.should.eql({ success: true }); | ||
await service.get('/v1/users/current') | ||
.set('Authorization', `Bearer ${token}`) | ||
.expect(401); | ||
})); | ||
|
||
it('should not allow app users to delete their own sessions', testService(async (service) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not? How do app users log out? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. App users auth using a session token. When access is revoked for an app user, that session is deleted. Project managers can revoke access for app users, but app users cannot do so themselves. App users are granted very limited rights and are often shared by multiple people. |
||
const asBob = await service.login('bob'); | ||
const { body: appUser } = await asBob.post('/v1/projects/1/app-users') | ||
.send({ displayName: 'test app user' }) | ||
.expect(200); | ||
await service.delete(`/v1/key/${appUser.token}/sessions/current`) | ||
.expect(403); | ||
})); | ||
}); | ||
|
||
// this isn't exactly the right place for this but i just want to check the | ||
// whole stack in addition to the unit tests. | ||
describe('cookie CSRF auth', () => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"it can be a good idea..." e.g. on a shared computer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly I was just copying this from the docs above for DELETE /v1/sessions/{token}. I guess once you're done with your API session, you might as well delete your token if you're not going to use it again. Or yeah, on a shared computer. I'd be happy to remove or reword this.