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
2 changes: 1 addition & 1 deletion src/oic/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
)

__author__ = "Roland Hedberg"
__version__ = "1.2.0"
__version__ = "1.3.0"
tpazderka marked this conversation as resolved.
Show resolved Hide resolved


OIDCONF_PATTERN = "%s/.well-known/openid-configuration"
Expand Down
17 changes: 15 additions & 2 deletions src/oic/utils/authn/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,21 @@ 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 "encoding" in kwargs:
encoding = kwargs["encoding"]
else:
encoding = "utf-8"

if "url_encoded" in kwargs:
url_encoded = kwargs["url_encoded"]
else:
url_encoded = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather define the kwargs with their default values in the method signature (and also document in the docstring).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, it looks like adding the annotation triggers complains from mypy. I am not sure if there is an easy way out of here.
Will have a look.

There are some other quality issues that need to be solved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, the mypy complain is because of the cis arg that is not in the superclass. One way to fix this is to remove the typing annotations. The other way is to make the signature the same (by making the cis a kwarg rather than arg).

I would go with the latter. If you are not willing to do that, make a separate issue and I will deal with that and you can base your PR on top of that.


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