Skip to content
This repository was archived by the owner on Dec 13, 2018. It is now read-only.

Implement the hybrid flow, unify code and authorization flows #456

Closed
Tratcher opened this issue Sep 16, 2015 · 5 comments
Closed

Implement the hybrid flow, unify code and authorization flows #456

Tratcher opened this issue Sep 16, 2015 · 5 comments

Comments

@Tratcher
Copy link
Member

We don't really implement the hybrid flow, we just do the implicit flow and then fire AuthorizationCodeReceived at the end and let you do it yourself. Using AuthorizationCodeReceived here is confusing as it fires in a different order than it would in the code flow, and means something different. It looks like we should just implement the hybrid flow, and do so before doing all of the token validations.

I think we could unify HandleCodeOnlyFlow and HandleIdTokenFlows by doing things in the following order:

  1. check for a code, redeem it.
  2. validate the authorization and token responses.
  3. get claims from the user endpoint.
@Eilon
Copy link
Contributor

Eilon commented Sep 17, 2015

After discussion it sounds like this is important to support.

@brentschmaltz
Copy link
Contributor

@Tratcher @Eilon I agree the branching logic could be improved.
I think about the order slightly different since we should fault before firing the code received notification on a bad id_token.

  1. If a token is received, validate
  2. If a code is received, redeem code for accessToken validate,
  3. if redeem flag is on, hit UserInfoEndpoint, validate
  4. merge claims into claimsIdentity.

It is not clear where we fire the SecurityTokenValidated notification.

The main purpose of the 'authentication code received' notification in Katana was that we didn't redeem the code. Given that this is the OIDC handler, we could assume the code is for "user_info_endpoint' and NOT fire the event, but that may cut out some scenarios.

@muratg
Copy link
Contributor

muratg commented Jan 21, 2016

Moving this to Backlog as we will be in RC2 ask mode very soon. If you feel strongly about this issue, please ping me.

@muratg muratg modified the milestones: Backlog, 1.0.0-rc2 Jan 21, 2016
@brockallen
Copy link

Yes, this is important.

@leastprivilege
Copy link
Contributor

yes - absolutely!

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

No branches or pull requests

6 participants