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.

Sign up for GitHub

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

Jump to bottom

server: fixes for the implicit and hybrid flow #766

Merged
merged 1 commit into from
Jan 11, 2017
Merged

server: fixes for the implicit and hybrid flow #766

merged 1 commit into from
Jan 11, 2017

Conversation

ericchiang
Copy link
Contributor

@ericchiang ericchiang commented Jan 9, 2017 •
edited

Accept the following response_type for the implicit flow:

id_token
token id_token

And the following for hybrid flow

code id_token
code token
code token id_token

This corrects the previous behavior of the implicit flow, which
only accepted "token" (now correctly rejected).

cc @rithujohn191 @xeonx

@xeonx can you see if this branch works with your client?

closes #761

@simonhege
Copy link
Contributor

Thanks for this PR.
I was able to follow the flow up to token retrieval by client, thus it is a great improvement.
I'm now facing new issues regarding id_token validation on client side:

  • I had to allow CORS on /keys endpoint (I will send a PR after cleaning the code, certainly next weekend)
  • at_hash is required in id_token by the oidc-client-js client even if optional according to oidc spec

@ericchiang
Copy link
Contributor Author

ericchiang commented Jan 10, 2017 •
edited

I had to allow CORS on /keys endpoint (I will send a PR after cleaning the code, certainly next weekend)

Why don't we make the current CORS option encompass that.

at_hash is required in id_token by the oidc-client-js client even if optional according to oidc spec

opened #771

@ericchiang
Copy link
Contributor Author

going to add a few inline comments then merge

@ericchiang
Copy link
Contributor Author

does this client use the userinfo_endpoint? we'll have to implement that too (which won't be impossible) #376

@simonhege
Copy link
Contributor

Yes userinfo_endpoint is the following call in the flow but it is optional, configurable with a flag (resulting in using claims from the id_token.profile only), thus having it implemented is not mandatory for this client.

server: fixes for the implicit and hybrid flow
f926d74 
Accept the following response_type for the implicit flow:

    id_token
    token id_token

And the following for hybrid flow

    code id_token
    code token
    code token id_token

This corrects the previous behavior of the implicit flow, which
only accepted "token" (now correctly rejected).
@ericchiang ericchiang merged commit c66cce8 into dexidp:master Jan 11, 2017
@ericchiang ericchiang deleted the implicit-flow branch January 11, 2017 00:50
rithujohn191
rithujohn191 reviewed Jan 11, 2017
View reviewed changes
server/handlers.go
// Otherwise render the error to the user.
//
// TODO(ericchiang): Should we just always render the error?
s.renderError(w, err.Status(), err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason why we should not just render the error always?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the oauth2 spec is returning errors to the client https://tools.ietf.org/html/rfc6749#section-4.1.2.1

@simonhege simonhege mentioned this pull request Jan 14, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rithujohn191 rithujohn191 rithujohn191 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Response type for implicit flow
3 participants
@ericchiang @simonhege @rithujohn191