aries-cloudagent-python icon indicating copy to clipboard operation
aries-cloudagent-python copied to clipboard

Feature: use decorators for admin api authentication

Open esune opened this issue 1 year ago • 7 comments

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

esune avatar Mar 28 '24 19:03 esune

Looks good upon first glance.

ianco avatar Mar 28 '24 20:03 ianco

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.

esune avatar Mar 28 '24 22:03 esune

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?

esune avatar Apr 02 '24 00:04 esune

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).

esune avatar Apr 08 '24 18:04 esune

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Apr 27 '24 00:04 sonarqubecloud[bot]

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.

esune avatar Apr 27 '24 00:04 esune

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.

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.

jamshale avatar Apr 29 '24 22:04 jamshale

This is - finally - ready for review.

esune avatar May 09 '24 00:05 esune

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.

jamshale avatar May 09 '24 15:05 jamshale

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.

jamshale avatar May 09 '24 16:05 jamshale

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

sonarqubecloud[bot] avatar May 09 '24 22:05 sonarqubecloud[bot]