Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🎨 Refactor Multitenant Manager errors and exception handling #3323

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented Oct 31, 2024

Relates to #3322

This PR refactors the exceptions that are raised in the MultitenantManager, and handles them in the server middleware.


The primary objective is to reduce unnecessary stack traces being printed upon client errors or bad requests.

In line with this, this PR now handles the following cases, no longer printing stack traces / error logs, and instead provides info logs of the following:

  • Unauthorized errors:
    • invalid header structure
    • token could not be decoded by jwt library
    • token can be decoded, but is invalid
  • Bad requests:
    • wallet name already exists
    • wallet key is missing
    • missing profile context

The unauthorized errors, and "wallet name already exists" errors are confirmed to be the same as before (still provides the same status code + client message) -- see next comment.

"wallet key is missing" I couldn't test, since unmanaged mode is not yet implemented, and "missing profile context" I'm not sure how to produce that. But given how it's not handled, they should produce the same http result as before.


web.HTTPBadRequests are now also handled in general. Instead of printing stack traces, they will now be logged at info level with a message such as (samples of new logs):

Bad request during POST /multitenancy/wallet: Wallet with name TestWalletName already exists.

Bad request during POST /didexchange/create-request: No public DID configured.

Bad request during POST /present-proof-2.0/records/6b94bd90-a8e7-4e81-949f-5a41c7fa9f08/send-presentation: Error creating presentation. Invalid state: Predicate is not satisfied.

Much more readable!


A follow-up PR will add handling of NotFound errors, UnprocessableEntity errors, etc, so they don't print stack traces either

ff137 added 7 commits October 31, 2024 14:28
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
…ingProfileError`, `WalletAlreadyExistsError`, `WalletKeyMissingError`

Signed-off-by: ff137 <ff137@proton.me>
… default message

Signed-off-by: ff137 <ff137@proton.me>
@ff137 ff137 self-assigned this Oct 31, 2024
@ff137
Copy link
Contributor Author

ff137 commented Oct 31, 2024

The following is for record keeping. Just shows that client messages remain the same, now without stack traces:


Before:
Passing an incorrectly formatted token (without "Bearer" at the start) results in:

=================
2024-10-31 12:12:28,694 acapy_agent.admin.server ERROR Handler error with exception: Invalid Authorization header structure
Traceback (most recent call last):
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/admin/server.py", line 141, in ready_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/admin/server.py", line 212, in debug_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/admin/server.py", line 306, in setup_context
    raise web.HTTPUnauthorized(
aiohttp.web_exceptions.HTTPUnauthorized: Invalid Authorization header structure

Now:

2024-10-31 15:09:38,335 acapy_agent.admin.server INFO Unauthorized access during GET /connections: Invalid Authorization header structure

Before:
Passing a bad tenant token (cannot be decoded by jwt library) results in:

2024-10-31 12:07:33,884 acapy_agent.admin.server ERROR Handler error with exception: Unauthorized
=================
Traceback (most recent call last):
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/admin/server.py", line 310, in setup_context
    profile = await self.multitenant_manager.get_profile_for_token(
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/multitenant/base.py", line 365, in get_profile_for_token
    token_body = jwt.decode(token, jwt_secret, algorithms=["HS256"], leeway=1)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/jwt/api_jwt.py", line 211, in decode
    decoded = self.decode_complete(
              ^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/jwt/api_jwt.py", line 152, in decode_complete
    decoded = api_jws.decode_complete(
              ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/jwt/api_jws.py", line 210, in decode_complete
    self._verify_signature(signing_input, header, signature, key, algorithms)
  File "/home/aries/.local/lib/python3.12/site-packages/jwt/api_jws.py", line 317, in _verify_signature
    raise InvalidSignatureError("Signature verification failed")
jwt.exceptions.InvalidSignatureError: Signature verification failed

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/admin/server.py", line 141, in ready_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/admin/server.py", line 212, in debug_middleware
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/acapy_agent/admin/server.py", line 328, in setup_context
    raise web.HTTPUnauthorized()
aiohttp.web_exceptions.HTTPUnauthorized: Unauthorized

Now:

2024-10-31 15:38:51,569 acapy_agent.admin.server INFO Unauthorized access during GET /connections: Unauthorized

Before:
Passing an old token that has been rotated:

Handler error with exception: Token not valid.
=================
Traceback (most recent call last):
  File "/home/aries/.local/lib/python3.12/site-packages/aries_cloudagent/admin/server.py", line 310, in setup_context
    profile = await self.multitenant_manager.get_profile_for_token(
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/aries_cloudagent/multitenant/base.py", line 381, in get_profile_for_token
    raise MultitenantManagerError("Token not valid")
aries_cloudagent.multitenant.base.MultitenantManagerError: Token not valid
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/home/aries/.local/lib/python3.12/site-packages/aries_cloudagent/admin/server.py", line 141, in ready_middleware
    return await handler(request)
    ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/aries_cloudagent/admin/server.py", line 212, in debug_middleware
    return await handler(request)
    ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/aries/.local/lib/python3.12/site-packages/aries_cloudagent/admin/server.py", line 326, in setup_context
    raise web.HTTPUnauthorized(reason=err.roll_up)
aiohttp.web_exceptions.HTTPUnauthorized: Token not valid.

Now:

2024-10-31 15:38:53,385 acapy_agent.admin.server INFO Unauthorized access during GET /connections: Token not valid.

ff137 added 2 commits October 31, 2024 20:43
Signed-off-by: ff137 <ff137@proton.me>
@ff137 ff137 marked this pull request as ready for review October 31, 2024 19:02
Copy link

@ff137 ff137 requested a review from jamshale October 31, 2024 20:18
@ff137
Copy link
Contributor Author

ff137 commented Oct 31, 2024

Note: the multitenant_provider plugin will need to be rebased with these changes (mainly MultitenantManagerError has moved)

Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

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

Nice. There's a lot of stuff like this that needs to be improved.

After this is merged we should have a draft PR in plugin repo for multitenant_provider with the required changes, just so we don't forget about it next release.

This should also be considered a breaking change as it affects a supported plugin.

@jamshale jamshale merged commit a4455dd into openwallet-foundation:main Nov 1, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants