Use client_id and client_secret as basic auth credentials if provided

[poswald]

This should fix issue #227

Open to review and suggestions if this will conflict with other grant flows.

[Lukasa]

I think I'm happy with this change. @synasius, thoughts?

[synasius]

@poswald @Lukasa
I think you can remove the part where username and password are encoded as basic auth credentials. I don't know if this code is used elsewhere but there is no mention about user credentials being encoded in basic auth in the RFC6749.

Also it is worth checking if client_secret is empty before encoding client_id and client_secret.

if (not auth) and client_id:
    if not client_secret:
        raise ValueError(...)
    auth = requests.auth.HTTPBasicAuth(client_id, client_secret)

what do you think?

[Lukasa]

Agreed that we should check for the empty client_secret. I'm less sure about removing the other blog: it did fix some users' problems, and I'm reluctant to remove it unconditionally.

[synasius]

@Lukasa makes sense! 👍

[Lukasa]

@poswald Want to add that check for empty client_secret?

[poswald]

OK, I didn't realize that username/password are never used as the basic auth header. I'll look into making that change. As for the client_secret I may need some more explanation. You'll see in the original issue I had raised on DOT that was what I was trying to get working.

So when using the password grant type (Resource Owner Password Credentials Grant) and a public client, the client_secret isn't sent along. In this grant type you're authenticating the client and the user separately. If the client is a public client, you really can't make a very strong assertion that the request is authentic and the assumption is that the user should enter their credentials into the app because they trust it (for example it was downloaded from your website or the iOS app store under your name).

The wording is a bit odd in that it's required but only if it's not blank:

client_secret
     REQUIRED.  The client secret.  The client MAY omit the
     parameter if the client secret is an empty string.

It also is allowed to be blank in the DOT Application object. I was planning on opening a DOT issue regarding this because there's some assumptions built into it that kind of make it not work very cleanly. This issue came up for me while testing some code related to that.

[Lukasa]

Ugh I hate the OAuth specification. Yes, you're right, it's not required. In that case, we want a defensive client_secret = client_secret if client_secret is not None else ''.

[poswald]

How are we looking now?

[Lukasa]

@poswald Sorry, I think the summary of the above conversation is that we wanted to keep the fallback to the username:password case: it clearly helped some users.

[poswald]

Gah, sorry I missed your comment there. OK, let me look at the spec again too while I'm at it. You're certainly not alone in thinking the spec is annoying.

[Lukasa]

@poswald Heh, yeah, working with OAuth2 is always "fun". Thanks for putting in so much effort though, it's really appreciated! 🍮

[poswald]

So yeah, while it does look like authenticating user credentials in basic auth is not technically in the spec, who knows what servers are doing in the wild for people. This version fixes the issue without changing the behavior too much for people who weren't passing client_id in. There is a chance this could cause breakage for some people who used this implementation as a reference for what to do on the server side instead of going to the spec but I also think it fixes an issue where the client was not following the spec for the password grant type so it's good to do.

[Lukasa]

This line is actually superfluous with the .get call above, and can be removed.

[poswald]

Strictly speaking I think it's not if your goal is to protect from passing None to the basic auth function. The get with a default is the default used if the key is missing. If someone calls fetch_token( ... client_secret=None, ... ) then after client_secret = kwargs.get('client_secret', '') you'll find that client_secret is None:

>>> client_secret = {'client_secret': None}.get('client_secret', '')
>>> print client_secret
None
>>> client_secret = {}.get('client_secret', '')
>>> print client_secret
>>>

Unless I'm mistaken about None not being possible or not needing to be protected against I think this is best. The other way I was considering was:

client_secret = {'client_secret': None}.get('client_secret', '') or ''

What do you think?

[Lukasa]

Mmm, yeah, so I was running under the assumption that client_secret wouldn't be passed as None, but now that you raise the possibility it occurs to me that I think some calls may default it that way. Let's leave this code in as defense-in-depth.

[Lukasa]

There is a chance this could cause breakage for some people who used this implementation as a reference for what to do on the server side instead of going to the spec but I also think it fixes an issue where the client was not following the spec for the password grant type so it's good to do.

Oh god I hope no-one implements their server with reference to this codebase, there are so many hacks to get around non-standard behaviours that the client codebase doesn't make it at all clear what a server is supposed to do!

One small note, and then I'll pull the trigger on this. =)

[Lukasa]

Thanks @poswald! ✨ 🍰 ✨