-
Notifications
You must be signed in to change notification settings - Fork 25
Add pkce essential per client #115
Conversation
It looks good, I Just want to ask to you to put a brief description of this feature in the docs Probably this Is good Moment to review the general policy of oidc-op with pkce, before merging this |
What is the right place to document this? |
https://github.com/IdentityPython/oidc-op/blob/master/docs/source/contents/conf.rst#add_on What do you think to have a separate section for add_ons? |
Sure |
83554d2
to
c0e361e
Compare
c0e361e
to
bf8a63b
Compare
Do you think something like this https://github.com/nsklikas/oidc-op/blob/pkce-per-client/docs/source/contents/conf.rst#pkce ok? |
Yes, that's great! For now I'd like to have all the PR with some good contributions in the docs as well. |
src/oidcop/oidc/add_on/pkce.py
Outdated
if "pkce_essential" in client: | ||
essential = client["pkce_essential"] | ||
else: | ||
essential = endpoint_context.args["pkce"]["essential"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
essential = endpoint_context.args["pkce"].get("essential", False)
@nsklikas how does it sounds to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's set in https://github.com/IdentityPython/oidc-op/pull/115/files#diff-60c1d62c690192be5964946a0883424948797ac8682e8ef94d9a59a6c8414d82L135, so it will always be there.
Should I remove that line and add the get
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not so important neither urgent, only if it sounds good to you.
Yes explicit is better than implicit but an absent value (boolan) could be False by default.
That's my tastes, not mandatory for this project!
I guess the
line is a typo ?
|
066b92e
to
230d04e
Compare
Good catch. |
@rohe do we think that this could be merged as It Is or do we have to put something more in? |
I think we can go ahead with this as it is. |
406068c
to
78cae68
Compare
This allows us to have pkce(essential) activated for only some clients