Hotfix/#754 client secret basic remove quote plus by gbmarc1 · Pull Request #755 · CZ-NIC/pyoidc

gbmarc1 · 2020-07-16T21:42:05Z

Any changes relevant to users are recorded in the CHANGELOG.md.
The documentation has been updated, if necessary.
New code is annotated.
Changes are covered by tests.

tpazderka · 2020-07-20T13:32:06Z

Are you willing to change this to do not use the quote_plus based on a kwarg as mentioned in #754 ?

gbmarc1 · 2020-07-20T16:35:06Z

Yes I will do. However, would it be more flexible to add an optional argument where you can specify the pre-encoding (before base64) of the user-pass instead of a boolean to quote_plus or not.
This would enable any encoding as per the point 3 of the RFC7617 spec.

   To receive authorization, the client

   1.  obtains the user-id and password from the user,

   2.  constructs the user-pass by concatenating the user-id, a single
       colon (":") character, and the password,

   3.  encodes the user-pass into an octet sequence (see below for a
       discussion of character encoding schemes),

   4.  and obtains the basic-credentials by encoding this octet sequence
       using Base64 ([RFC4648], Section 4) into a sequence of US-ASCII
       characters ([RFC0020]).

   The original definition of this authentication scheme failed to
   specify the character encoding scheme used to convert the user-pass
   into an octet sequence.  In practice, most implementations chose
   either a locale-specific encoding such as ISO-8859-1 ([ISO-8859-1]),
   or UTF-8 ([RFC3629]).  For backwards compatibility reasons, this
   specification continues to leave the default encoding undefined, as
   long as it is compatible with US-ASCII (mapping any US-ASCII
   character to a single octet matching the US-ASCII character code).

…sic-remove-quote_plus' into hotfix/CZ-NIC#754-ClientSecretBasic-remove-quote_plus # Conflicts: # src/oic/utils/authn/client.py # tests/test_client.py

codecov-commenter · 2020-07-20T17:41:00Z

Codecov Report

Merging #755 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #755      +/-   ##
==========================================
- Coverage   63.66%   63.65%   -0.02%     
==========================================
  Files          64       64              
  Lines       11826    11822       -4     
  Branches     2091     2091              
==========================================
- Hits         7529     7525       -4     
  Misses       3701     3701              
  Partials      596      596

Impacted Files	Coverage Δ
src/oic/utils/authn/user.py	`62.96% <ø> (-0.46%)`	⬇️
src/oic/utils/authn/client.py	`69.75% <100.00%> (-0.10%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53a246a...d2aae60. Read the comment docs.

tpazderka · 2020-07-21T08:26:50Z

+        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


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

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.

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.

tpazderka · 2020-07-22T13:53:31Z

Here is an overview of what got changed by this pull request:

Issues
======
- Added 1

See the complete overview on Codacy

tpazderka

Looks good code-wise.

There are still two reported errors from mypy that need to be fixed:

tests/test_client.py:88: error: Incompatible types in assignment (expression has type "bytes", variable has type "str")
303tests/test_client.py:90: error: Argument 1 to "join" of "bytes" has incompatible type "Tuple[str, str]"; expected "Iterable[Union[ByteString, memoryview]]"

gbmarc1 · 2020-07-23T14:06:45Z

Looks good code-wise.

There are still two reported errors from mypy that need to be fixed:

tests/test_client.py:88: error: Incompatible types in assignment (expression has type "bytes", variable has type "str")
303tests/test_client.py:90: error: Argument 1 to "join" of "bytes" has incompatible type "Tuple[str, str]"; expected "Iterable[Union[ByteString, memoryview]]"

I don't see any problem with this code

tpazderka · 2020-07-23T14:35:45Z

+
+        http_args = csb.construct(cis, **kwargs)
+
+        user, passwd = user.encode(encoding), passwd.encode(encoding)


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.

That should actually solve both mypy complains...

Marc-Antoine Belanger added 2 commits July 16, 2020 17:39

remove quote_plus

d7bc4b4

added non friendly character to password test

8bd7390

gbmarc1 mentioned this pull request Jul 16, 2020

ClientSecretBasic should not url encode credentials before encoding in base64 #754

Open

remove extra quote_plus

58a1b39

Marc-Antoine Belanger added 3 commits July 20, 2020 13:25

optional encoding before base64 encoding

d81bb20

Merge remote-tracking branch 'origin/hotfix/CZ-NIC#754-ClientSecretBa…

2bf41f4

…sic-remove-quote_plus' into hotfix/CZ-NIC#754-ClientSecretBasic-remove-quote_plus # Conflicts: # src/oic/utils/authn/client.py # tests/test_client.py

kwargs in contruct

d2aae60

Marc-Antoine Belanger added 4 commits July 20, 2020 13:45

cahngelog + encoding on the password

b2ef337

encoding to bytes in join

1518714

refactoring encoding

5b87466

black format

fa50d4a

tpazderka requested changes Jul 21, 2020

View reviewed changes

Marc-Antoine Belanger added 2 commits July 21, 2020 09:56

Fix with PR comments

7b70b07

cis None

571cf13

tpazderka reviewed Jul 23, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Hotfix/#754 client secret basic remove quote plus#755
gbmarc1 wants to merge 12 commits into
CZ-NIC:masterfrom
gbmarc1:hotfix/#754-ClientSecretBasic-remove-quote_plus

gbmarc1 commented Jul 16, 2020 •

edited

Loading

Uh oh!

tpazderka commented Jul 20, 2020

Uh oh!

gbmarc1 commented Jul 20, 2020

Uh oh!

codecov-commenter commented Jul 20, 2020

Uh oh!

Uh oh!

tpazderka Jul 21, 2020

Uh oh!

tpazderka Jul 22, 2020

Uh oh!

tpazderka Jul 22, 2020

Uh oh!

tpazderka commented Jul 22, 2020

Uh oh!

tpazderka left a comment

Uh oh!

gbmarc1 commented Jul 23, 2020 •

edited

Loading

Uh oh!

tpazderka Jul 23, 2020

Uh oh!

tpazderka Jul 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


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

		user, passwd = user.encode(encoding), passwd.encode(encoding)

Conversation

gbmarc1 commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tpazderka commented Jul 20, 2020

Uh oh!

gbmarc1 commented Jul 20, 2020

Uh oh!

codecov-commenter commented Jul 20, 2020

Codecov Report

Uh oh!

Uh oh!

tpazderka Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

tpazderka Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

tpazderka Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

tpazderka commented Jul 22, 2020

Uh oh!

tpazderka left a comment

Choose a reason for hiding this comment

Uh oh!

gbmarc1 commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tpazderka Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

tpazderka Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gbmarc1 commented Jul 16, 2020 •

edited

Loading

gbmarc1 commented Jul 23, 2020 •

edited

Loading