Feature: use decorators for admin api authentication
Resolves #2318
Opening draft PR early for feedback while I work through the changes across the code (and fixing/updating/adding tests as needed).
The PR removes the authentication middleware and the logic they deal with, and implements two decorators:
admin_authentication: to be used for routes that should ONLY be invoked by an administrator, such as the multitenancy endpoints, the server endpoints and so on, independently of the mode the agent is running as.tenant_authentication: to be used to require authentication by either providing a tenant token (multi-tenant mode) or a valid api-key (single-tenant mode).
Both decorators account for unauthenticated options requests as well as insecure mode. Insecure paths will just not be decorated. Middleware code - currently commented-out - will be removed.
I think the bit of refactoring required for this to work (including plugins once released) is well worth the flexibility - looking for early feedback especially from @dbluhm, @ianco, @jamshale
Looks good upon first glance.
Looks good upon first glance.
Thanks @ianco . I've pushed updates to all the route handlers, now working on cleaning-up and fixing/adding tests and validating things really work as expected.
Fixed the route tests, still need to tackle removing the now failing middleware tests and add tests for the decorator functions. Unsure why the formatter check is failing as it looks (to me) like the change it suggests is the same as the change that is already there?
Rebased code off the latest main. I am currently fighting with failing unit tests: when calling endpoints in the server API namespace, I get the following error and have been so far unable to figure out why it is being triggered even after stepping through the code execution:
2024-04-08 18:21:29,365 aiohttp.server ERROR Error handling request
Traceback (most recent call last):
File "/home/vscode/.local/lib/python3.9/site-packages/aiohttp/web_protocol.py", line 433, in _handle_request
resp = await request_handler(request)
File "/home/vscode/.local/lib/python3.9/site-packages/aiohttp/web_app.py", line 504, in _handle
resp = await handler(request)
File "/home/vscode/.local/lib/python3.9/site-packages/aiohttp/web_middlewares.py", line 117, in impl
return await handler(request)
File "/workspaces/aries-cloudagent-python/aries_cloudagent/admin/server.py", line 180, in ready_middleware
return await handler(request)
File "/workspaces/aries-cloudagent-python/aries_cloudagent/admin/server.py", line 217, in debug_middleware
return await handler(request)
File "/workspaces/aries-cloudagent-python/aries_cloudagent/admin/server.py", line 362, in setup_context
return await task
File "/usr/local/lib/python3.9/asyncio/futures.py", line 284, in __await__
yield self # This tells Task to wait for completion.
File "/usr/local/lib/python3.9/asyncio/tasks.py", line 328, in __wakeup
future.result()
File "/usr/local/lib/python3.9/asyncio/futures.py", line 201, in result
raise self._exception
File "/usr/local/lib/python3.9/asyncio/tasks.py", line 256, in __step
result = coro.send(None)
File "/usr/local/lib/python3.9/site-packages/aiohttp_apispec/middlewares.py", line 45, in validation_middleware
return await handler(request)
TypeError: admin_auth() takes 1 positional argument but 2 were given
Stepping through the code with the debugger shows that the exception is thrown at this line.
An easy way to reproduce is to start the agent and call any of the endpoints protected with admin-authentication in the server namespace. Please note that calling similarly set-up endpoints in the multitenancy namespace does work as expected.
Any input is appreciated as this is now blocking progress (want to fix this scenario before creating new tests for the decorators).
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
No data about Coverage
No data about Duplication
All tests should be fixed now. I updated/synced the version of Black as three different versions were specified between pyproject.toml, .pre-commit-config.yaml and blackformat.yml and ran the formatter on the project, however it still raises potential changes - if anyone has an idea of why/how to address this please let me know.
Only thing left now is adding tests specific to the decorators.
All tests should be fixed now. I updated/synced the version of Black as three different versions were specified between
pyproject.toml,.pre-commit-config.yamlandblackformat.ymland ran the formatter on the project, however it still raises potential changes - if anyone has an idea of why/how to address this please let me know.
I haven't looked into the pre-commit or black format files. I use the ruff vscode extension to format my files and it seems to work well with the project. If I do get a fail from black on the PR I use the Run and Debug black or ruff process from the devcontainer and it re-formats the code correctly for me.
This is - finally - ready for review.
There's one failing unit test because of a ruff formatting error. aries_cloudagent/config/argparse.py:650:91: E501 Line too long.
You can just add # noqa: E501 to the end of that line.
Looks good and I will approve. I just had one comment about how there is a upgrade on all packages. I think it's better to only upgrade the packages related to the PR and do a general upgrade in a separate PR. In the case we do find a problem with an upgrade it will be easier to isolate.
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
No data about Coverage
No data about Duplication