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

Add logout param support #10

Merged
merged 2 commits into from
May 23, 2017
Merged

Add logout param support #10

merged 2 commits into from
May 23, 2017

Conversation

Jaywoods2
Copy link
Contributor

after logout,i can redirect to the login page and login.

@geoffgarside
Copy link
Contributor

From the current CAS spec it looks like it should be an option of the CAS client application whether or not to send a service parameter with the /logout request. It would be better if this was therefore an option rather than always sending a service URL.

@Jaywoods2
Copy link
Contributor Author

After listening to your opinions,I have modified the code that adds a judgment parameters,and it can choose whether to send the service parameters。Do you think so? I hope you can give me more advice!

@geoffgarside
Copy link
Contributor

I was thinking of leaving the LogoutUrlForRequest as it is at the moment and adding LogoutUrlForRequestWithService(r *http.Request, serviceURL *url.URL) (string, error) allowing the serviceURL to be nil to enable the URL to be extracted from the http.Request.

@geoffgarside
Copy link
Contributor

Actually I think having the client-wide option is good. What would happen though if someone was to hit the applications "/logout" route, would this not result in the user being sent to something like

https://sso.example.com/logout?service=https://app.example.com/logout

which would just result in another logout, so would the logout URL code need to sanitise the URL so the service URL would be sent as https://app.example.com/ instead?

casey-chow added a commit to casey-chow/tigertrade that referenced this pull request Apr 13, 2017
problem was that the underlying thing didn't support redirects.
there's a PR for it, so I switched to the PR. I'm subscribed to
the PR so when it gets merged I'll use that instead

go-cas/cas#10

good shit
@geoffgarside geoffgarside merged commit 2722b1d into go-cas:master May 23, 2017
@geoffgarside
Copy link
Contributor

Taken me far too long to come back to this...

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.

2 participants