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

Confusion about password grant_flow example in the wiki #561

Closed
lukasnagl opened this issue Jan 26, 2015 · 14 comments
Closed

Confusion about password grant_flow example in the wiki #561

lukasnagl opened this issue Jan 26, 2015 · 14 comments

Comments

@lukasnagl
Copy link

When trying to implement the “password” OAuth grant flow, I noticed the “Testing” example in the wiki. I was under the impression that it was necessary to send client_id and client_secret with the request after reading it. This approach can also be found to be accepted on stack overflow.

Looking at the RFC at http://tools.ietf.org/html/rfc6749#section-4.3 and especially http://tools.ietf.org/html/rfc6749#section-4.3.2, I don’t see why client_id and client_secret would be sent for this grant flow.

Why do both of the examples above send client_id and client_secret, then? This may not be an issue of doorkeeper/the doorkeeper wiki at all, but merely a confusion on my part regarding the OAuth specification or the wiki example. In any case, I would be happy if someone could provide further information about this.

@tute
Copy link
Contributor

tute commented Jan 27, 2015

Client for password flow is optional as of #296. Answer in StackOverflow and wiki page may be updated.
Thank you for noting! :)

@miletbaker
Copy link

@j4zz @tute

I note this as closed, but my understanding is that client_id and secret_id are indeed required as part of the spec.

If you look under: http://tools.ietf.org/html/rfc6749#section-4.3.2, in particular at this statement:

If the client type is confidential or the client was issued client
credentials (or assigned other authentication requirements), the
client MUST authenticate with the authorization server as described
in Section 3.2.1.

As the client is issued with credentials these should be authenticated as per 3.2.1 which references 2.3 which states client_id and client_secret are required. (I think the confusion is in how the spec has been written, it would make more sense having this clearer in section 4.3.2). However logically I can see that If these are not validated, it leaves us with the problem of how would you revoke access to that client for example. In fact the spec lists three reasons why the client should be validated here http://tools.ietf.org/html/rfc6749#section-3.2.1

I am not sure whether doorkeeper needs to remain un-opinionated about this? I suppose there must be cases where an implementor may be using doorkeeper without creating clients, but for those that are authentication is required as per the spec.

I guess as is, we can manually authenticate the client in the resource_owner_from_credentials block. I am not sure if this would be the best way but a quick and dirty way would be to check the client first by finding an application using Application. by_uid_and_secret(params[:client_id], params[:client_secret]) and only then authenticate by username and password if the client is valid. Though I suspect this would be better handled using the same process as for other grant_types.

What do you think?

@tute
Copy link
Contributor

tute commented Feb 11, 2015

My opinion on this is: don't use OAuth2 password flow. Clients will then be authenticated. I started encoding this opinion in 2115a52. You can see why it makes no sense on that flow to authenticate in this answer: https://stackoverflow.com/questions/6190381/how-to-keep-the-client-credentials-confidential-while-using-oauth2s-resource-o/6217429#6217429. Doorkeeper is the authorization server, it can't control what happens in the client.

Thank you for your input!

@itskingori
Copy link

In case anyone is curious, I still feel that it's important to authenticate the client ... and there are benefits to this like being able to know the application associated with an access token. Doorkeeper only set application_id if client credentials were passed in.

That said my configuration authenticates client credentials using Basic Authorisation only (no params for security considerations) and this is how I did it ...

resource_owner_from_credentials do |routes|
  # Since we are using Basic Authorizaion we parse the Authorization header
  # and check if the credentials are valid by fetching the application
  # matching those credentials
  authorization = request.authorization
  if authorization.present? && authorization =~ /^Basic (.*)/m
    credentials = Base64.decode64(Regexp.last_match[1]).split(/:/, 2)
    uid         = credentials.first
    secret      = credentials.second
    application = Doorkeeper::Application.by_uid_and_secret uid, secret
  end
  # Use Devise's find_for_database_authentication method in User model.
  user = User.find_for_database_authentication(email: params[:username])
  # Only accept if we have a valid application, user and valid credentials
  user if application && user && (user.valid_password? params[:password])
end

@dhyegocalota
Copy link

Is there any way to make required again client_id + client_secret? That's because I don't really wanna let any request use this flow (just mine). I'm trying to figure it out yet.
Thanks.

@itskingori
Copy link

@dhyegofernando You can modify the check in the sample code I've just shared. Specifically this part that performs the actual checking from the DB.

application = Doorkeeper::Application.by_uid_and_secret uid, secret

@dhyegocalota
Copy link

@itsmrwave my bad I didn't see. Thank u so much.

@devin909
Copy link

devin909 commented Jul 1, 2015

I don't think client credentials are required for password flow because the specs clearly says this flow should only be used when there is a high degree of trust between the oAuth provider and the client in 4.3. 4.3.2 says that client authentication is required if the client type is confidential or the client was issued client credentials (or assigned other authentication requirements). What is neither if statements are true? What is the client type isn't confidential and the client wasn't issued client credentials?

At any rate, if you want to use refresh_tokens with password workflow, it looks like you will still need to provide client credentials, because the refresh_token endpoint call still requires client credentials parameters.

@dhyegocalota
Copy link

@itskingori I found a new way to do that

Create a file in config/initializers/doorkeeper/password_strategy_client_validator.rb

module Doorkeeper
  module PasswordStrategyClientValidator
    private

    def validate_client
      !!client
    end
  end
end

Doorkeeper::OAuth::PasswordAccessTokenRequest.prepend Doorkeeper::PasswordStrategyClientValidator

@enewbury
Copy link

enewbury commented Jan 22, 2019

According to the Oauth2 Spec, client_id and client_secret should be set on password grant if the client is confidential, i.e. if a server is authenticating with password flow. Also the oauth2 gem includes these by default even in password auth, so this does not work with doorkeeper
https://tools.ietf.org/html/rfc6749#section-3.2.1
https://tools.ietf.org/html/rfc6749#section-4.3.2

@duende84
Copy link

Hey people, I'm trying to implement the password grant flow, but it only works when I send the client_id and client_secret as params for /oauth/token endpoint. is there any way to implement password grant flow without sending those params (for security reasons) from the client app?.

@warrenlalata
Copy link

warrenlalata commented Jan 19, 2020

Hey people, I'm trying to implement the password grant flow, but it only works when I send the client_id and client_secret as params for /oauth/token endpoint. is there any way to implement password grant flow without sending those params (for security reasons) from the client app?.

same. when leaving the client_id and client_secret as params empty, error says:
{
"error": "invalid_grant",
"error_description": "The provided authorization grant is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client."
}

image

but when I re-included it, it proceeds in OK:
image

this is confusing.

@nbulaj
Copy link
Member

nbulaj commented Apr 13, 2020

For anybody who are looking here in 2020:

Looking at the RFC at http://tools.ietf.org/html/rfc6749#section-4.3 and especially http://tools.ietf.org/html/rfc6749#section-4.3.2, I don’t see why client_id and client_secret would be sent for this grant flow.

4.3.2. Access Token Request section says:

If the client type is confidential or the client was issued client credentials (or assigned other authentication requirements), the client MUST authenticate with the authorization server as described in Section 3.2.1.

Also

The authorization server MUST:

  • require client authentication for confidential clients or for any client that was issued client credentials (or with other authentication requirements)

As all Doorkeeper applications have credentials - it's required to use client_id and client_secret with the request and conform the specs.

@ThisIsMissEm
Copy link
Contributor

As all Doorkeeper applications have credentials - it's required to use client_id and client_secret with the request and conform the specs.

If the client type is public, then a client_secret should not be generated.

To be honest, it would probably be very wise to consider deprecating Password Grant Flow, as has been done in OAuth 2.1: https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-10#name-grant-types

Tao-Galasse added a commit to Progeser/progeser-api that referenced this issue Sep 17, 2024
BREAKING CHANGE: authentication now requires a `client_id` due to OAuth 2 RFC
Read doorkeeper-gem/doorkeeper#561 (comment) for more details.
Tao-Galasse added a commit to Progeser/progeser-api that referenced this issue Sep 17, 2024
BREAKING CHANGE: authentication now requires a `client_id` due to OAuth 2 RFC
Read doorkeeper-gem/doorkeeper#561 (comment) for more details.
Lelievre-david pushed a commit to Progeser/progeser-api that referenced this issue Sep 23, 2024
BREAKING CHANGE: authentication now requires a `client_id` due to OAuth 2 RFC
Read doorkeeper-gem/doorkeeper#561 (comment) for more details.
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

No branches or pull requests