Distributed claims by rohe · Pull Request #420 · CZ-NIC/pyoidc

rohe · 2017-09-04T07:26:21Z

Fixed bug in handling of distributed claims.

Thanks for contributing to this library! Please include the following check
list in your pull request submission (you can delete this message). If your
changes don't need a change log or documentation update, please ignore this.

Any changes relevant to users are recorded in the CHANGELOG.md.
The documentation has been updated, if necessary.

coveralls · 2017-09-04T07:37:42Z

Coverage increased (+0.1%) to 59.937% when pulling 43e0a77 on rohe:distributed_claims into f220947 on OpenIDC:master.

coveralls · 2017-09-04T09:50:12Z

Coverage increased (+0.1%) to 59.937% when pulling 007244d on rohe:distributed_claims into f220947 on OpenIDC:master.

tpazderka · 2017-09-04T11:25:40Z

src/oic/oic/__init__.py

-                    _uinfo = self.do_user_info_request(
-                        token=callback(csrc),
-                        userinfo_endpoint=spec["endpoint"])
+                    if callback:


This part is not covered by tests.

Does the recent changes meet your requirements ?

Sorry for not responding earlier. I am having a busy week at work. I will have a look today.

Here again, comparison to None and test without callback is missing.

OK now ? Need to get this out the door soon.

tpazderka · 2017-09-04T11:28:44Z

src/oic/oic/__init__.py

            if 'as_query_parameter' == _behav:
                method = 'GET'
-            else:
+            elif token:


Comparison to None should be explicit.

In tests, this condition is always true, so a test with token = None should be added.

The comparison is still not explicit. It believe that it should be elif token is not None: as token is previously assigned None.

I don't think that is good enough. token='' should be handled the same way as token=None
hence the use of elif token which implicitly include token is not ''.
And I'd rather not use elif token is not None and token is not ''

And is there any case where token would be an empty string?
If yes, then OK, leave the code as is.

coveralls · 2017-09-06T09:07:17Z

Coverage increased (+0.2%) to 60.012% when pulling eed4b70 on rohe:distributed_claims into f220947 on OpenIDC:master.

coveralls · 2017-09-06T09:07:17Z

Coverage increased (+0.3%) to 60.088% when pulling eed4b70 on rohe:distributed_claims into f220947 on OpenIDC:master.

tpazderka · 2017-09-08T09:10:36Z

src/oic/oic/__init__.py

-                    _ttype = token.token_type
-                except AttributeError:
-                    raise MissingParameter("Unspecified token type")
+                if token:


The same goes here: if token is not None:

tpazderka · 2017-09-08T09:15:47Z

src/oic/oic/__init__.py

+        if state:
+            # Verify userinfo sub claim against what's returned in the ID Token
+            idt = self.grant[state].get_id_token()
+            if idt:


This seems to never be triggered in tests.

tpazderka · 2017-09-08T09:16:37Z

I know this is a lot of tests to be added, but if you are already poking into the code, we might as well add test to cover the changes...

coveralls · 2017-09-10T07:48:47Z

Coverage increased (+0.3%) to 60.114% when pulling 323f074 on rohe:distributed_claims into f220947 on OpenIDC:master.

coveralls · 2017-09-10T07:48:47Z

Coverage increased (+0.3%) to 60.114% when pulling 323f074 on rohe:distributed_claims into f220947 on OpenIDC:master.

coveralls · 2017-09-11T07:11:39Z

Coverage increased (+0.3%) to 60.114% when pulling 1013ede on rohe:distributed_claims into f220947 on OpenIDC:master.

tpazderka · 2017-09-14T10:24:44Z

I would like to see more tests, but I understand that any progress is better than none.

decentral1se · 2017-09-14T10:59:15Z

I would like to see more tests, but I understand that any progress is better than none.

Agreed!

If we feel like we're missing coverage, just create an issue with a link to this PR and we can track that. If a bug is ever raised, we will be motivated to come back to cover the remaining cases.

Nice work @rohe.

rohe · 2017-09-15T07:07:44Z

Sounds good to me.
So, can one of you merge the PR.
Since I'm the author I shouldn't do it myself.
After the merge I'd like us to release a new version to pypi.
The next release of the OpenID Connect tool has a dependency on this PR and I want to get that out the door as soon as possible.

decentral1se · 2017-09-15T08:05:30Z

🍰

Since I'm the author I shouldn't do it myself.

Hey, you may be VIP but you're not getting out of merging your own PRs ;)

I won't get time to make a release, please someone, go for it and keep #419 in mind.

rohe · 2017-09-15T10:31:36Z

Excellent new! Thanks !
VIP, lol :-)
I'll probably not have time to do a release today. At the earliest tomorrow afternoon.

* Modified fetch_distributed_claims * Use HTTP GET for all requests

rohe added 2 commits August 31, 2017 11:34

Modified fetch_distributed_claims

5fd587a

Use HTTP GET for all requests

43e0a77

Missing empty last line

007244d

tpazderka requested changes Sep 4, 2017

View reviewed changes

Dealt with tpazderkas requests

eed4b70

tpazderka reviewed Sep 8, 2017

View reviewed changes

Added a couple of more tests.

323f074

Removed empty line and distinguish between token/no token.

1013ede

zandbelt mentioned this pull request Sep 13, 2017

Claim/Scope-Tests do not support Distributed Claims openid-certification/oidctest#51

Closed

tpazderka approved these changes Sep 14, 2017

View reviewed changes

tpazderka merged commit d840b0b into CZ-NIC:master Sep 15, 2017

rohe deleted the distributed_claims branch September 15, 2017 10:31

andrewkrug pushed a commit to mozilla-iam/pyoidc that referenced this pull request Jun 6, 2019

Distributed claims (CZ-NIC#420)

6083d3f

* Modified fetch_distributed_claims * Use HTTP GET for all requests

Conversation

rohe commented Sep 4, 2017

Uh oh!

coveralls commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rohe Sep 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 6, 2017

Uh oh!

coveralls commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tpazderka commented Sep 8, 2017

Uh oh!

coveralls commented Sep 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 10, 2017

Uh oh!

coveralls commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tpazderka commented Sep 14, 2017

Uh oh!

decentral1se commented Sep 14, 2017

Uh oh!

rohe commented Sep 15, 2017

Uh oh!

decentral1se commented Sep 15, 2017

Uh oh!

rohe commented Sep 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Sep 4, 2017 •

edited

Loading

coveralls commented Sep 4, 2017 •

edited

Loading

rohe Sep 10, 2017 •

edited

Loading

coveralls commented Sep 6, 2017 •

edited

Loading

coveralls commented Sep 10, 2017 •

edited

Loading

coveralls commented Sep 11, 2017 •

edited

Loading