Remove unused userinfo and online token validation #3239
Conversation
- Removes the get_userinfo functionality from the auth module - Removes the online token validation capability from the server - OIDC token validation will only happen locally PBENCH-1074
| try: | ||
| token_payload = oidc_client.token_introspect_offline(token=auth_token) | ||
| token_payload = oidc_client.token_introspect(token=auth_token) | ||
| except OpenIDTokenInvalid: | ||
| token_payload = None | ||
| except Exception: | ||
| current_app.logger.exception( | ||
| "Unexpected exception occurred while verifying the auth token {}", | ||
| auth_token, | ||
| ) | ||
|
|
||
| # Offline token verification resulted in some unexpected error, | ||
| # perform the online token verification. | ||
|
|
||
| # Note: Online verification should NOT be performed frequently, and it | ||
| # is only allowed for non-public clients. | ||
| try: | ||
| token_payload = oidc_client.token_introspect_online(token=auth_token) | ||
| except OpenIDClientError as exc: | ||
| current_app.logger.debug( | ||
| "Can not perform OIDC online token verification, '{}'", exc | ||
| ) | ||
| token_payload = None | ||
|
|
||
| token_payload = None |
There was a problem hiding this comment.
Arguably, this is beyond the scope of this PR, but I'll make the comment anyway, in case you're interested.
Since we (now) have the same response, other than the logger call, regardless of what exception we receive, there's not much reason to have two separate except clauses here.
Moreover, the fact that there was an unexpected exception should probably not be noted here (we're in the "middle" of the call stack).
So, it strikes me that lines 197-204 should be slimmed down to just:
except Exception:
token_payload = None
And the catch-all with the logger call should be moved to the end of token_introspect().
There was a problem hiding this comment.
I think the initial intention was that we didn't want to log for these valid jwt exceptions jwt.ExpiredSignatureError, jwt.InvalidSignatureError, jwt.InvalidAudienceError. These exceptions mean that the token decode was invalid and realistically we will get a lot of tokens that are invalid because of these 3. If we encounter any of these exceptions then we will raise OpenIDTokenInvalid error and move on.
However, if some other exception happened other than these 3 then that means something with our decode has actually failed (not invalid) so we catch that with a generic exception and log the message and move on.
There was a problem hiding this comment.
Indeed: we want to log if we or something on which we depend (like Keycloak itself) does something "wrong" or "unexpected", but we don't want to complain every time a client (or "would-be client") does something wrong, because that'll flood the logs and serve no real purpose.
And for authentication failures, we probably don't ever want to return more information to the caller than just "unauthorized".
There was a problem hiding this comment.
@npalaska, I agree; however, the catch-generic-log-and-move-on should be done in token_introspect() and not here in verify_auth_oidc().
| try: | ||
| token_payload = oidc_client.token_introspect_offline(token=auth_token) | ||
| token_payload = oidc_client.token_introspect(token=auth_token) | ||
| except OpenIDTokenInvalid: | ||
| token_payload = None | ||
| except Exception: | ||
| current_app.logger.exception( | ||
| "Unexpected exception occurred while verifying the auth token {}", | ||
| auth_token, | ||
| ) | ||
|
|
||
| # Offline token verification resulted in some unexpected error, | ||
| # perform the online token verification. | ||
|
|
||
| # Note: Online verification should NOT be performed frequently, and it | ||
| # is only allowed for non-public clients. | ||
| try: | ||
| token_payload = oidc_client.token_introspect_online(token=auth_token) | ||
| except OpenIDClientError as exc: | ||
| current_app.logger.debug( | ||
| "Can not perform OIDC online token verification, '{}'", exc | ||
| ) | ||
| token_payload = None | ||
|
|
||
| token_payload = None |
There was a problem hiding this comment.
Indeed: we want to log if we or something on which we depend (like Keycloak itself) does something "wrong" or "unexpected", but we don't want to complain every time a client (or "would-be client") does something wrong, because that'll flood the logs and serve no real purpose.
And for authentication failures, we probably don't ever want to return more information to the caller than just "unauthorized".
I will refactor that in the functional test PR. |
…m-analysis#3239) * Remove unused userinfo and online token validation - Removes the get_userinfo functionality from the auth module - Removes the online token validation capability from the server - OIDC token validation will only happen locally PBENCH-1074
* Remove unused userinfo and online token validation - Removes the get_userinfo functionality from the auth module - Removes the online token validation capability from the server - OIDC token validation will only happen locally PBENCH-1074
…m-analysis#3239) * Remove unused userinfo and online token validation - Removes the get_userinfo functionality from the auth module - Removes the online token validation capability from the server - OIDC token validation will only happen locally PBENCH-1074
…m-analysis#3239) * Remove unused userinfo and online token validation - Removes the get_userinfo functionality from the auth module - Removes the online token validation capability from the server - OIDC token validation will only happen locally PBENCH-1074
…m-analysis#3239) * Remove unused userinfo and online token validation - Removes the get_userinfo functionality from the auth module - Removes the online token validation capability from the server - OIDC token validation will only happen locally PBENCH-1074
…m-analysis#3239) * Remove unused userinfo and online token validation - Removes the get_userinfo functionality from the auth module - Removes the online token validation capability from the server - OIDC token validation will only happen locally PBENCH-1074
…m-analysis#3239) * Remove unused userinfo and online token validation - Removes the get_userinfo functionality from the auth module - Removes the online token validation capability from the server - OIDC token validation will only happen locally PBENCH-1074
* Remove unused userinfo and online token validation - Removes the get_userinfo functionality from the auth module - Removes the online token validation capability from the server - OIDC token validation will only happen locally PBENCH-1074
PBENCH-1074