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

document token and auth endpoints for OAuth2 clients that don't support OIDC discovery #712

Open
evanluc opened this issue Dec 1, 2016 · 3 comments

Comments

@evanluc
Copy link
Contributor

evanluc commented Dec 1, 2016

I'm not sure if I've misunderstood the purpose of this function but in the handleConnectorCallback the state is checked for in the handler, passed to getAuthRequest but in the case of the sql storage option state is never queried for, just id, so this query always fails to retrieve a row as the state != req id.

If this is indeed the expected behaviour could I get an explanation as to why. It seems counter intuitive.

@ericchiang
Copy link
Contributor

We happen to use the ID of the auth request (a random string) as the state when passing clients onto upstream OAuth2 based identity providers. That auth request happens to also have an internal field called "state" that reflects the downstream state dex's client passed. These two aren't the same "state" so the code is correct, even if counter intuitive.

Is there a behavioral bug that you're seeing such as logins with OAuth2 providers not working? Or is this just lack of documentation in the code?

ericchiang pushed a commit to ericchiang/dex that referenced this issue Dec 1, 2016
Use a non-empty state in the example-app to ensure dex is properly
preserving the state for the code flow.

Updates dexidp#712
@evanluc
Copy link
Contributor Author

evanluc commented Dec 2, 2016

That clears things up.

My problem was caused by trying to get my token at the route for that handler. It was fixed by redeeming the token at the right route /token. I'm using dex paired with python application using oauthlib to communicate between the dex server and my app instead of the golang client.

Really enjoying this project, my suggestion is to maybe have clearer documentation for the /auth and /token routes and where in the authentication flow they they fit. It takes some digging in the source code for folks that are not using the golang client.

Eg.
/auth : To get the authentication code
/token: To redeem our token

I may be up for doing this over the weekend. Does this sound like a good fit for the getting-started doc?

@ericchiang
Copy link
Contributor

Ah, thanks for clarifying.

Most OpenID Connect clients get auth and token endpoints from discovery[0], but yeah I can understand OAuth2 needing dex to use that value and stick to it. Beyond documentation, part of this is writing tests or conveying that these shouldn't change too.

We can definitely do that. Thanks for clarifying.

[0] https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata

@ericchiang ericchiang changed the title possible bug: handleConnectorCallback with sql storage fails -> id queried instead of state document token and auth endpoints for OAuth2 clients that don't support OIDC discovery Dec 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants