consent: Add logout api endpoint#971
Conversation
ory#970 Signed-off-by: Michael DeRazon <mderazon@gmail.com>
|
@arekkas the fix is very small, however since it's my first time dealing with Go, I was hitting some issues running tests (missing packages, even after I checked and the changes were forced even after I closed VSC, very bizzare. No way to revert them, they just keep popping back. Anyway, would probably take me a lot more time to understand the tooling around it than to actually make the changes. Regarding the changes themselves:
Anyway, hope it's not creating more work than it would for you to write it :-/ |
There was a problem hiding this comment.
That's very weird (regarding the changes). I also use VSC but not extensively, primarily because the Go support is not as good as in GoLand. Not sure what's happening there but (as you did) just commit the relevant files.
Regarding to test, you actually need Docker installed and it's a bit slow. You can skip slow tests with go test -short ./... (no docker needed) which should work much better.
Regarding go get, you actually need Go's dependency management golang/dep (can't wait for Go 1.11 where this works natively):
go get -d -u github.com/ory/hydra
cd $(go env GOPATH)/src/github.com/ory/hydra
dep ensure -vendor-only
go test -short ./...
So regarding writing tests, this would be very cool! The idea would be to create a new file consent/handler_test.go and the first few lines should probably be something like this. Maybe set the redirect URL to something like https://www.ory.sh. Also make sure that ConsentStartegy is set (example here - I think you can leave everything empty/nil except for the cookiestore).
Then you create an HTTP client with a cookiejar:
cj, err := cookiejar.New()
require.NoErr(t, err)
client := http.Client{
Jar: cj,
}You would obviously need to set a cookie in the cookiejar because we are testing if the cookie is actually removed. Two options:
- Just add a cookie to the cookiejar with URL
u, err := url.Parse(ts.URL)and a cookie that has the name of the authentication cookie (and some random value I think). - I think you could
t.Log("+v")thepersistentCJsomewhere around here to get the data and mimic it in your test.
I'd try the first one before doing the second.
Then, once the request is done client.Get(ts.URL+"/oauth2/auth/logout") (I think), you should check for an error and then see if the status code is 200 and if yes, check if the url is http://www.ory.sh (the redirect url we set) and if that is also true, check if the cookie has been removed from the cookiejar.
If you have more questions let me know!
| }) | ||
| } | ||
|
|
||
| // swagger:route DELETE /oauth2/auth/logout oAuth2 logoutUser |
There was a problem hiding this comment.
Let's rename this to swagger:route GET /oauth2/auth/logout oAuth2 userLogout
|
|
||
| func (h *Handler) LogoutUser(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { | ||
| err := revokeAuthenticationSession(w, r) | ||
| if err != nil { |
There was a problem hiding this comment.
Perfect! You can actually write this in one line:
if err := revokeAuthenticationSession(w, r); err != nil {| // listed in `LOGOUT_REDIRECT_URL` environment variable. | ||
| // | ||
| // | ||
| // Consumes: |
There was a problem hiding this comment.
You can remove Consumes because we don't consume anything
| // Consumes: | ||
| // - text/html | ||
| // | ||
| // Produces: |
There was a problem hiding this comment.
It produces JSON in case of an error, no text/html
| return | ||
| } | ||
|
|
||
| http.Redirect(w, r, os.Getenv("LOGOUT_REDIRECT_URL"), 302) |
There was a problem hiding this comment.
This solution is ok, but it makes setting defaults and testing the configuration harder. Typically what we do here is:
- Add a config item in config/config.go
- Add a field (e.g.
LogoutRedirectURL) to set this in the handler.go and the respective factors (new...) - Bind and set a default in
cmd/root.golike here. - Use
LogoutRedirectURLhere instead ofos.GetEnv()
Additionally, we should set up a default handler for the log out screen, similar to this one and add it to the frontend router here. The layout of the page doesn't matter. It should read something "You have been logged out successfully. ... Admin forgot to set up the redirect URL after logout ... to fix that set environment variable LogoutRedirectURL, e.g. export LogoutRedirectURL=http://... or on windows set LogoutRedirectURL=http://... ..."
| backend.DELETE("/oauth2/auth/sessions/consent/:user", h.DeleteUserConsentSession) | ||
| backend.DELETE("/oauth2/auth/sessions/consent/:user/:client", h.DeleteUserClientConsentSession) | ||
|
|
||
| frontend.GET("/oauth2/auth/logout", h.LogoutUser) |
There was a problem hiding this comment.
Let's rename this to UserLogout (because it's UserConsent :) )
|
One more thing, please document |
|
Thanks for the valuable feedback.
|
See this comment: #971 (comment)
|
|
I know now why this (reformat) in the SDK happens. Apparently, |
|
Were you able to disable it ? It might be something with the repo, because if I clone the repo on a new location I am getting errors |
|
It was really weird, I commited all the changes to a new branch, then switched back. Only then did they disappear. I honestly have no idea what's going on here... |
|
Did you manage to get it fixed? Maybe it was a broken git index? I had the issue several times too. I finally fixed it though, I think it had something to do with lower/uppercase of the filename. If this blocks you from working on the PR let me know. |
|
Just letting you know that I'll take your changes and integrate my feedback so this can be released with beta.8 today |
|
Thank you for your hard work - new PR is #984 |
|
Thanks a lot of this @arekkas . It seems your changes ended up being more substantial and you also added the tests, so I guess it's for the better ;-) Thanks again for all the hard work for this really great open source jewel |
|
No problem! |
2 participants
#970