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

Logout #44

Closed
eabili0 opened this issue Oct 18, 2019 · 6 comments · Fixed by #51
Closed

Logout #44

eabili0 opened this issue Oct 18, 2019 · 6 comments · Fixed by #51

Comments

@eabili0
Copy link
Contributor

eabili0 commented Oct 18, 2019

Whisper should adhere to the logout flow proposed by Hydra.

We need to

  1. open a /logout api to work as Hydra's Logout Provider;
  2. adapt whisper-client to ask for a logout_redirect_uri on registration;
  3. improve our docs to show how logout can be implemented;
  4. provide a working example for it.
@claudiosegala
Copy link
Contributor

@abilioesteves I do not know if I am getting confused but I stumble upon some obstacles.

  1. I did not find a native method for each client to set a post_logout_redirect_url
  2. Even though I arrive in the default logout page of Hydra, the access token continues valid.

For the first problem, I found some interesting things in the issue client: Whitelist logout redirect URL per client #1004 which singlewind comments a way that might help. I do not know if I understand it correctly but I believe that it involves the part in the logout procedure where I (the logout provider) can check the request.

The logout provider uses the challenge query parameter to fetch metadata about the request. The logout provider may choose to show a UI where the user has to accept the logout request. Alternatively, the logout provider MAY choose to silently accept the logout request. ~Logout, Hydra

Given that whisper become the default post logout URL. With access to the request, you could identify which client and store it. So that when hydra redirects to whisper again, whisper knows where to redirect. I believe that this might need the state that identifies the conversations with Whisper.

For the second problem, I believe that it might be that unless I add a post logout URL, it won't logout the user but I am not sure.

@eabili0
Copy link
Contributor Author

eabili0 commented Nov 17, 2019

Well, I took a deeper look at the problem, and I have to agree with you, unfortunately. Whisper needs to have an endpoint to become the default post logout uri, which will then redirect the browser to the correct client endpoint on logout. That endpoint should be informed in the client registration with whisper.

It is a good thing that we already started to separate hydra's interfaces from whisper's interfaces in the
whisper-client library.

For the second problem, I believe that it might be that unless I add a post logout URL, it won't logout the user but I am not sure.

Could please test this behaviour before going deeper on the issue? Maybe there's an unidentified bug with Hydra that we don't know about. Thanks!

@eabili0
Copy link
Contributor Author

eabili0 commented Nov 18, 2019

@claudiosegala, have you taken a look at the post_logout_redirect_uris parameter for Hydra's client creation API?

@claudiosegala
Copy link
Contributor

@abilioesteves I will test further to see if the second problem is a bug. I have taken a look at post_logout_redirect_uris. What about it? If I am not mistaken, I am using it now to send for each client the possible post logout redirect URIs.

// createOAuth2Client calls hydra to create an oauth2 client
func (client *hydraClient) createOAuth2Client() (result *OAuth2Client, err error) {
	p := path.Join(client.admin.BaseURL.Path, "/clients")
	payloadData, _ := json.Marshal(
		OAuth2Client{
			ClientID:                client.clientID,
			ClientSecret:            client.clientSecret,
			TokenEndpointAuthMethod: client.tokenEndpointAuthMethod,
			Scopes:                  strings.Join(client.scopes, " "),
			GrantTypes:              client.grantTypes,
			RedirectURIs:            client.RedirectURIs,
			PostLogoutRedirectURIs:  client.PostLogoutRedirectURIs,
		})

	logrus.Debugf("CreateOAuth2Client - POST payload: '%v'", payloadData)
	resp, data, err := client.admin.Post(p, payloadData)
	if err == nil {
		if resp != nil {
			if resp.StatusCode == 201 {
				err = json.Unmarshal(data, &result)
				return result, err
			} else if resp.StatusCode == 409 {
				return nil, fmt.Errorf("Conflict")
			}
			return nil, fmt.Errorf("Internal server error")
		}
		return nil, fmt.Errorf("Expecting response payload to be not null")
	}
	return nil, err
}

@eabili0
Copy link
Contributor Author

eabili0 commented Nov 20, 2019

@claudiosegala, just like the login flow, you should inform a valid post_logout_redirect_uri on the mounted logout url (that the user will click). Also, form the Hydra docs, you should inform an id_token_hint on the same clickable url. Could you please take a look at that and see if it works? Thanks!

@claudiosegala
Copy link
Contributor

claudiosegala commented Nov 20, 2019

@abilioesteves yeah, I gave a deeper look and saw that. The PR is coming.

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

Successfully merging a pull request may close this issue.

2 participants