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

Hotfix/#754 client secret basic remove quote plus #755

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,13 @@ The format is based on the [KeepAChangeLog] project.
## Unreleased

### Added
- [#755] Kwargs for ClientCredential encoding and url_encoded. If `url_encoded` is `True` or is not
provided, the user-pass will be encoded using `quote_plus`. The `encoding` kwargs will be used to `encode` to bytes
before encoding in base64. Then, the base64 will be decoded using this encoding also.
- [#739] Better error message for providers which return HTTP Error 405 on userinfo
- [#723] Add settings class to handle settings related to Client and Server

[#755]: https://github.com/OpenIDC/pyoidc/pull/755/
[#723]: https://github.com/OpenIDC/pyoidc/pull/723/
[#739]: https://github.com/OpenIDC/pyoidc/pull/739/

Expand Down
19 changes: 16 additions & 3 deletions src/oic/utils/authn/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,23 @@ class ClientSecretBasic(ClientAuthnMethod):
Section 3.2.1 of OAuth 2.0 [RFC6749] using HTTP Basic authentication scheme.
"""

def construct(self, cis, request_args=None, http_args=None, **kwargs):
def construct(
self,
cis=None,
request_args=None,
http_args=None,
url_encoded: bool = True,
encoding: str = "utf-8",
**kwargs
):
"""
Create the request.

:param cis: Request class instance
:param request_args: Request arguments
:param http_args: HTTP arguments
:param url_encoded: url encode the user-pass before converting to base64
:param encoding: encoding format of user-pass before converting to base64
:return: dictionary of HTTP arguments
"""
if http_args is None:
Expand All @@ -121,8 +131,11 @@ def construct(self, cis, request_args=None, http_args=None, **kwargs):
if "headers" not in http_args:
http_args["headers"] = {}

credentials = "{}:{}".format(quote_plus(user), quote_plus(passwd))
authz = base64.b64encode(credentials.encode("utf-8")).decode("utf-8")
if url_encoded:
user, passwd = quote_plus(user), quote_plus(passwd)

credentials = b":".join((user.encode(encoding), passwd.encode(encoding)))
authz = base64.b64encode(credentials).decode(encoding)
http_args["headers"]["Authorization"] = "Basic {}".format(authz)

try:
Expand Down
34 changes: 29 additions & 5 deletions tests/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,16 +58,40 @@ def client():


class TestClientSecretBasic(object):
def test_construct(self, client):
cis = AccessTokenRequest(code="foo", redirect_uri="http://example.com")
@pytest.mark.parametrize(
"url_encoded, encoding",
((False, "latin1"), (True, "latin1"), (None, None), (False, None)),
)
def test_construct(self, client, url_encoded, encoding):

cis = AccessTokenRequest(code="foo", redirect_uri="http://example.com")
csb = ClientSecretBasic(client)
http_args = csb.construct(cis)
cred = "{}:{}".format(quote_plus("A"), quote_plus("boarding pass"))

user, passwd = "A", "boarding pass"
kwargs = {}
if url_encoded is None or url_encoded:
user, passwd = (
quote_plus(user),
quote_plus(passwd),
)

if url_encoded is not None:
kwargs.update({"url_encoded": url_encoded})

if encoding is None:
encoding = "utf-8"
else:
kwargs.update({"encoding": encoding})

http_args = csb.construct(cis, **kwargs)

user, passwd = user.encode(encoding), passwd.encode(encoding)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This assigns bytes to what was preciously str, mypy doesn't like that it may cause programming errors.

It should be solved by using a new variable, or encoding directly in L90.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That should actually solve both mypy complains...


cred = b":".join((user, passwd))
assert http_args == {
"headers": {
"Authorization": "Basic {}".format(
base64.b64encode(cred.encode("utf-8")).decode("utf-8")
base64.b64encode(cred).decode(encoding)
)
}
}
Expand Down
10 changes: 5 additions & 5 deletions tests/utils/test_authn_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
from oic.utils.authn.client import get_client_id

CDB = {
"number5": {"client_secret": "drickyoughurt"},
"number5": {"client_secret": "drickyoughurt*"},
"token_client": {},
"expired": {"client_secret": "drickyoughurt", "client_secret_expires_at": 1},
"expired": {"client_secret": "drickyoughurt*", "client_secret_expires_at": 1},
"secret_token": "token_client",
"expired_token": "expired",
}
Expand Down Expand Up @@ -45,11 +45,11 @@ def test_wrong_authn(self):
get_client_id(self.cdb, AuthorizationRequest(), "mumbo jumbo")

def test_basic_authn_client_ok(self):
authn = "Basic " + b64encode(b"number5:drickyoughurt").decode()
authn = "Basic " + b64encode(b"number5:drickyoughurt*").decode()
assert get_client_id(self.cdb, AuthorizationRequest(), authn)

def test_basic_authn_client_missing(self):
authn = "Basic " + b64encode(b"missing:drickyoughurt").decode()
authn = "Basic " + b64encode(b"missing:drickyoughurt*").decode()
with pytest.raises(FailedAuthentication):
get_client_id(self.cdb, AuthorizationRequest(), authn)

Expand All @@ -59,7 +59,7 @@ def test_basic_authn_client_wrongpass(self):
get_client_id(self.cdb, AuthorizationRequest(), authn)

def test_basic_authn_client_invalid(self):
authn = "Basic " + b64encode(b"expired:drickyoughurt").decode()
authn = "Basic " + b64encode(b"expired:drickyoughurt*").decode()
with pytest.raises(FailedAuthentication):
get_client_id(self.cdb, AuthorizationRequest(), authn)

Expand Down