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.

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

Respond with a 204 HTTP status code after cancelling a Session #142

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Respond with a 204 HTTP status code after cancelling a Session #142

wants to merge 3 commits into from

Conversation

stefanvermaas
Copy link

According to the official HTTP/1.1 specs a successful DELETE request can be handled in various ways.

If a DELETE method is successfully applied, the origin server SHOULD send a 202 (Accepted) status code if the action will likely succeed but has not yet been enacted, a 204 (No Content) status code if the action has been enacted and no further information is to be supplied, or a 200 (OK) status code if the action has been enacted and the response message includes a representation describing the status.

This will make sure a 204 HTTP status code is returned after successfully cancelling the session and it fixes #140

I assumed the session will be cancelled right-away and it's not some sort of asynchronous task. If that do is the case (the session is not yet cancelled when the response is already returned) we should change the HTTP response code to a 202.

I'm looking for some help on writing the tests for the HTTP status code. It seems there are none yet, but I'm not entirely sure what is the best way to add them and how to add them.

According to the official HTTP/1.1 specs a successful DELETE request
can be handled in various ways.

> If a DELETE method is successfully applied, the origin server SHOULD send a 202 (Accepted) status code if the action will likely succeed but has not yet been enacted, a 204 (No Content) status code if the action has been enacted and no further information is to be supplied, or a 200 (OK) status code if the action has been enacted and the response message includes a representation describing the status.

This commit will make sure a 204 HTTP status code is returned
after successfully cancelling the session.
@ivard
Copy link
Member

ivard commented Mar 17, 2021

This is indeed a good idea. More of a technical detail before this can be merged. The IRMA client (the app) also uses this endpoint and that part of the code cannot deal with 204's at the moment. I introduce support for this in #98, so after that one is merged, we can consider merging this too.

@stefanvermaas
Copy link
Author

I introduce support for this in #98, so after that one is merged, we can consider merging this too.

Sounds good! Let's wait for it to finish. In the meantime it would make sense to add tests, but I'm not entirely sure how to add these. Do you have a good resource for writing tests for go applications or would one of you be willing to add tests to this PR?

@ivard
Copy link
Member

ivard commented Mar 17, 2021

We already have some unit tests in irmago. The testing code is situated here.

It would maybe be nice to have a unit test that starts a session, cancels it at some point and checks whether it is properly deleted. I cannot find such an unit test now indeed. That test should be in this set of tests I think.

@stefanvermaas
Copy link
Author

Would you mind explaining how I can create a IRMA disclosure session? I've been messing around with the tests and tried to find references on how to do it, but I can't get it to work :)! Any help would be appreciated.

@ivard
Copy link
Member

ivard commented Mar 18, 2021

Ah now you send the DELETE request manually. It also an option to use the irmaclient code (for the IRMA app) for this. Then we test that part of the code too. Of course this test will fail then because, like I said, the client does not support 204's yet.

You can maybe look to sessionHelper function. This helper starts a IRMA server and an irma client (the backend of the app) and performs a session. This one uses a TestHandler which stubs all user input of the app for the happy flow. We can make a new handler with the same behaviour that cancels the session.

type CancelTestHandler struct {
	TestHandler
}

func (th *CancelTestHandler) Success(result string) {
	th.Failure(&irma.SessionError{ErrorType: irma.ErrorType("Session was not cancelled as expected")})
}

func (th *CancelTestHandler) RequestVerificationPermission(request *irma.DisclosureRequest, satisfiable bool, candidates [][]irmaclient.DisclosureCandidates, ServerName *irma.RequestorInfo, callback irmaclient.PermissionHandler) {
	callback(false, nil)
}

Disclaimer: I did not test this code, so there can be mistakes in it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cancel a IRMA session
3 participants