Skip to content
This repository was archived by the owner on Jun 23, 2023. It is now read-only.

Conversation

nsklikas
Copy link
Contributor

@nsklikas nsklikas commented Jun 9, 2021

I encounter a bug were the exception defined in https://github.com/IdentityPython/oidc-op/blob/master/src/oidcop/token/__init__.py#L117 was raised when I was trying to exchange an authorization code for an access token.

Perhaps we should handle all exceptions there? Or simply create an OidcopError exception from which all other exceptions will inherit and we can simply catch that?

What do you think?

Closes #97

@nsklikas nsklikas requested review from peppelinux and rohe June 9, 2021 16:15
@rohe
Copy link
Collaborator

rohe commented Jun 9, 2021

As you can see in oidcop/exception.py the idea was that there would be a package specific exception which all the exceptions in that package inherited from.
Now OidcEndpointException should probably be renamed to OidcOPException/OidcOPError and possible package exception that wasn't following that pattern should be hunted down and made to comply.

@rohe
Copy link
Collaborator

rohe commented Jun 9, 2021

Having said that we still will have exception that belong to OidcMsg and CryptoJWT that will crop up in OidcOP.

@peppelinux
Copy link
Member

Can you share the test that make this exception?
I'm interested about why this exception was not handled by unit tests

@nsklikas
Copy link
Contributor Author

Ok I looked some more into it and it seems that the bug is elsewhere.

I try to run a regular code flow. When I try to exchange the authorization code with an access_token the following error is raised:

  File "/srv/venv/lib/python3.7/site-packages/oidcop/oauth2/token.py", line 407, in process_request
    _access_token, grant=True
  File "/srv/venv/lib/python3.7/site-packages/oidcop/session/manager.py", line 459, in get_session_info_by_token
    _token_info = self.token_handler.info(token_value)
  File "/srv/venv/lib/python3.7/site-packages/oidcop/token/handler.py", line 55, in info
    _handler, item_info = self.get_handler(item, order)
  File "/srv/venv/lib/python3.7/site-packages/oidcop/token/handler.py", line 75, in get_handler
    res = self.handler[typ].info(token)
  File "/srv/venv/lib/python3.7/site-packages/oidcop/token/__init__.py", line 117, in info
    raise WrongTokenClass(_res["token_class"])
oidcop.token.exception.WrongTokenClass: access_token

The bug is cause from the token minting. In

item.value = token_handler(session_id=session_id, **token_payload)
it calls Token without passing the token_class. The function called is
def __call__(self, session_id: Optional[str] = "", token_class: Optional[str] = "", **payload) -> str:

Which expects a token_class and if it isn't present then it defaults to authorization_code. It's weird that we haven't stumble on it all this time.

@nsklikas
Copy link
Contributor Author

nsklikas commented Jun 11, 2021

Having said that we still will have exception that belong to OidcMsg and CryptoJWT that will crop up in OidcOP.

To solve this we can have a common exception base that will be shared across the libraries, e.g.:

OIDCError
OidcMsgError(extends OIDCError)
CryptoJWTError(extends OIDCError)
OidcOPError(extends OIDCError)

OIDCError could live in any of these modules (perhaps oidc-msg is the most appropriate?)

@nsklikas nsklikas closed this Jun 11, 2021
@nsklikas nsklikas reopened this Jun 11, 2021
@rohe
Copy link
Collaborator

rohe commented Jun 12, 2021

Part of this is dealt with in #99 .

@rohe
Copy link
Collaborator

rohe commented Jun 12, 2021

If we add OidcError to oidcmsg it will not cover CryptoJWT but I don't see that as a major problem.
Actually I think it's the right way to go.

Copy link
Member

@peppelinux peppelinux left a comment

Choose a reason for hiding this comment

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

@nsklikas overall It LGTM, so here my approval. Please fill evetually the remaining gaps, It seems that @rohe told us something ..

@nsklikas nsklikas closed this Jun 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants