Synced implementation of token_endpoint #624
Conversation
tpazderka
force-pushed
the
refactor_token_endpoint
branch
2 times, most recently
from
0e46ea9
to
1d039c1
Compare
Codecov Report
@@ Coverage Diff @@
## master #624 +/- ##
==========================================
+ Coverage 61.29% 61.51% +0.21%
==========================================
Files 62 62
Lines 11175 11140 -35
Branches 1979 1973 -6
==========================================
+ Hits 6850 6853 +3
+ Misses 3757 3727 -30
+ Partials 568 560 -8
Continue to review full report at Codecov.
|
tpazderka
force-pushed
the
refactor_token_endpoint
branch
from
1d039c1
to
b7a7631
Compare
|
I will work on the few missing tests tomorrow. |
tpazderka
force-pushed
the
refactor_token_endpoint
branch
from
b7a7631
to
87965a0
Compare
tpazderka
force-pushed
the
refactor_token_endpoint
branch
from
3feb5cc
to
009d895
Compare
tpazderka
force-pushed
the
refactor_token_endpoint
branch
from
009d895
to
371923b
Compare
|
OK, finished the tests. There are two spots missing. They technically were not changed :) so I am not going to bother with them just yet. |
All three providers (oauth2, oic and extension) now share common code.
tpazderka
force-pushed
the
refactor_token_endpoint
branch
from
1c9ff70
to
c426082
Compare
There was a problem hiding this comment.
Some comments, nothing major.
I'm not sure if we should raise NotImplementedExceptions there instead of returning an error_response.
And at least RFC 6749 5.2 has a different idea how errors should be handled. It says it should trigger an error with 'unsupported_grant_type' code.
One small thing i noticed at the side: In the oic.Provider() init call, the AProvider is called without server_cls just to overwrite the self.server a few lines later, which does the same thing. Maybe move that to the superclass call instead.
src/oic/oauth2/provider.py
Outdated
| @@ -153,6 +154,8 @@ def re_authenticate(areq, authn): | |||
|
|
|||
| class Provider(object): | |||
| endp = [AuthorizationEndpoint, TokenEndpoint] | |||
| # Define the message class that in token_enpdoint | |||
There was a problem hiding this comment.
typo in comment 'endpoint'
src/oic/oauth2/provider.py
Outdated
| _info = self.sdb[areq["code"]] | ||
| except KeyError: | ||
| logger.error('Code not present in SessionDB') | ||
| error = TokenErrorResponse(error="unauthorized_client") |
There was a problem hiding this comment.
Do we have a reason to not set a error_description? Like we do in line 792.
It hides some information to omit more details, but we leak the info anyway, due to not being constant time here. So more helpful error messages might ease client debugging.
src/oic/oauth2/provider.py
Outdated
| # If redirect_uri was in the initial authorization request verify that they match | ||
| if "redirect_uri" in _info and areq["redirect_uri"] != _info["redirect_uri"]: | ||
| logger.error('Redirect_uri mismatch') | ||
| error = TokenErrorResponse(error="unauthorized_client") |
There was a problem hiding this comment.
same here, with error_description
src/oic/oauth2/provider.py
Outdated
| if 'state' in areq: | ||
| if _info['state'] != areq['state']: | ||
| logger.error('State value mismatch') | ||
| error = TokenErrorResponse(error="unauthorized_client") |
There was a problem hiding this comment.
And here too, error_description
|
Thanks for the review. I will probably do a separate issue for the refactoring of init calls. The refactoring is probably needed for most of the shared methods anyway and I would like to keep this focused on token_endpoint. |
All three providers (oauth2, oic and extension) now share common code.
CHANGELOG.md.Prep work for #615 and #617.
Also getting rid of duplicated code...