Add endpoint to delete current session #923
Conversation
|
This new endpoint should help with getodk/central#400. |
| @@ -403,6 +403,26 @@ Logging out is not strictly necessary for Web Users; all sessions expire 24 hour | |||
| + Response 403 (application/json) | |||
| + Attributes (Error 403) | |||
|
|
|||
| #### Logging out current session [DELETE /v1/sessions/current] | |||
|
|
|||
| This endpoint causes the current session to log itself out. Logging out is not strictly necessary for Web Users; all sessions expire 24 hours after they are created. But it can be a good idea, in case someone else manages to steal your token. | |||
There was a problem hiding this comment.
"it can be a good idea..." e.g. on a shared computer?
There was a problem hiding this comment.
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.
| .then(() => (_, response) => { | ||
| // revoke the cookie associated w the session, if the session was used to | ||
| // terminate itself. | ||
| // TODO: repetitive w above. |
There was a problem hiding this comment.
what does this TODO imply?
There was a problem hiding this comment.
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.
| .expect(401); | ||
| })); | ||
|
|
||
| it('should not allow app users to delete their own sessions', testService(async (service) => { |
There was a problem hiding this comment.
Why not? How do app users log out?
There was a problem hiding this comment.
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.
docs/api.md
Outdated
| + Request | ||
| + Headers | ||
|
|
||
| Authorization: Bearer lSpAIeksRu1CNZs7!qjAot2T17dPzkrw9B4iTtpj7OoIJBmXvnHM8z8Ka4QPEjR7 |
There was a problem hiding this comment.
For web users, the token will also be sent as a cookie, and perhaps in more cases. Would it be helpful to also allow supplying the token-to-delete via cookie?
There was a problem hiding this comment.
The token could be supplied via a cookie. Frontend might do that. However, we don't encourage API users to use cookie auth.
| .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.
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.
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.
matthew-white
force-pushed
the
logout-current
branch
from
c585433
to
03bc87e
Compare
matthew-white
force-pushed
the
logout-current
branch
from
03bc87e
to
c169631
Compare
This PR adds an endpoint to delete the current session, making it easy to log out without specifying the token of the current session in the URL. We will probably use this endpoint in Frontend, but I also imagine that some API users would find it convenient.
What has been done to verify that this works as intended?
New tests. I copied some of the tests of DELETE /v1/sessions/:token. I didn't copy all the tests, since the two endpoints call a shared function: I think the tests of DELETE /v1/sessions/:token provide good coverage of that shared behavior.
Why is this the best possible solution? Were any other approaches considered?
I created a shared function because I think there's enough logic shared between DELETE /v1/sessions/:token and the new DELETE /v1/sessions/current to warrant one. DELETE /v1/sessions/current ends up being quite short and looks very similar to GET /v1/sessions/restore.
I chose the path …/current to be similar to another endpoint we have, /v1/users/current.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
API users will be able to use the new endpoint. The DELETE /v1/sessions/:token endpoint was modified, but much of its code was copied as-is to the shared function. Existing tests continue to pass.
Before submitting this PR, please make sure you have:
make test-fulland confirmed all checks still pass OR confirm CircleCI build passes