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

Add support for Resource Owner Password Credentials Grant #1163

Closed
wants to merge 1 commit into from

Conversation

snowzach
Copy link

@snowzach snowzach commented Jan 3, 2018

This PR adds support for Resource Owner Password Credentials Grant.
https://www.oauth.com/oauth2-servers/access-tokens/password-grant/

You enable it in the config by setting:

oauth2:
   passwordConnector: local

or replace local with whatever connector you wish to use for password grants.

You then request a token via a

POST /dex/token
grant_type=password&client_id=example-app&client_secret=ZXhhbXBsZS1hcHAtc2VjcmV0&[email protected]&password=password&scope=openid profile email&nonce=2OSwiYXRfaGFzaCI6Il9rMVBZT0ppcGFQc0pHSmlZV

@snowzach snowzach changed the title Add support for password grant #926 Add support for Resource Owner Password Credentials Grant Jan 3, 2018
Copy link
Contributor

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

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

Thanks, this look pretty good.

I'm really surprised that handlePasswordGrant is just a huge method with seemingly no shared code with the other token handlers. Can you comment on that before proceeding?

@@ -132,6 +135,9 @@ type Server struct {
// If enabled, don't prompt user for approval after logging in through connector.
skipApproval bool

// Used for password grant
passwordConnector string
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, this should be a connector.PasswordConnector

@@ -67,6 +67,9 @@ type Config struct {
// Logging in implies approval.
SkipApprovalScreen bool

// If set, the server will use this connector to handle password grants
PasswordConnector string
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, the connector should be initialized in cmd so this would be of type connector.PasswordConnector

@jwntrs
Copy link
Contributor

jwntrs commented Mar 19, 2018

@snowzach are you still looking at this? Really excited for password grant

@snowzach
Copy link
Author

snowzach commented Mar 19, 2018

@pivotal-jwinters I wanted to finish this quick but the requested changes are a bigger refactor for the code than I have time at the moment. I will get to it eventually but $dayjob has taken over for the time being. Sorry. :(

@jwntrs
Copy link
Contributor

jwntrs commented Mar 19, 2018

@snowzach no worries. If there's anything I can do to help let me know.

guirochart added a commit to objenious/dex that referenced this pull request Feb 7, 2019
@yadzhang
Copy link

@snowzach @ericchiang Any progress on this? I am really need on password grant.

johnharris85 pushed a commit to johnharris85/dex that referenced this pull request Jun 9, 2019
@xtremerui
Copy link

xtremerui commented Jan 9, 2020

Hi, @bonifaido. I went through recent PRs and found you are assigned most of them so I figured you might be the right one to talk with. Sorry in advance.

@pivotal-jwinters and I work for ConcourseCI who uses Dex as its auth backend. We are currently using our own fork of Dex and password grant in this PR has been merged into our fork for almost one year without any issue.

We would love to help here. Could you review the current status of the PR and provide some guidance? @snowzach if you dont mind could we pick up your work? If necessary we could move on to a new PR of password grant, which will be based on @snowzach 's first commit.

Thx!

@snowzach
Copy link
Author

snowzach commented Jan 9, 2020

@xtremerui by all means you may do anything you like this with merge. I kept meaning to come back to it but just never had time.

@bonifaido
Copy link
Member

Hi @xtremerui ! I think this PR looks quite good, we just need to rebase it on current master and resolve the conflicts. Would you guys do that?

@xtremerui
Copy link

@bonifaido perfecto! We will update the PR and submit it again from our fork.

@xtremerui
Copy link

#1621 is submitted. This PR might be closed. Thx!

@snowzach
Copy link
Author

Close in favor of #1621

@snowzach snowzach closed this Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants