labbsr0x / whisper

A cloud-native Identity and OAuth Provider implemented with Golang and ORY Hydra
MIT License
12 stars 4 forks source link

Logout #44

Closed eabili0 closed 4 years ago

eabili0 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

claudiosegala commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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