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

Feature: use decorators for admin api authentication #2860

Merged
merged 43 commits into from
May 10, 2024

Conversation

esune
Copy link
Member

@esune esune commented Mar 28, 2024

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

@ianco
Copy link
Contributor

ianco commented Mar 28, 2024

Looks good upon first glance.

@esune
Copy link
Member Author

esune commented Mar 28, 2024

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.

Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

I like the flexibility and straightforward-ness of the approach. I like a nice function decorator lol. I always tend toward caution with them though; they can have some gotchas. Where these are being called by the aiohttp router, I think the gotcha potential should be minimal. LGTM so far.

Copy link
Contributor

@amanji amanji left a comment

Choose a reason for hiding this comment

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

So far, this all LGTM! Really liking how clean those decorators are.

aries_cloudagent/admin/server.py Show resolved Hide resolved
aries_cloudagent/admin/decorators/auth.py Show resolved Hide resolved
aries_cloudagent/admin/decorators/auth.py Show resolved Hide resolved
@swcurran swcurran added the 1.0.0 To be addressed for the ACA-Py 1.0.0 release label Apr 1, 2024
@esune
Copy link
Member Author

esune commented Apr 2, 2024

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 added 24 commits April 8, 2024 11:49
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
@esune
Copy link
Member Author

esune commented Apr 27, 2024

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.

@jamshale
Copy link
Contributor

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.

esune added 6 commits May 7, 2024 13:37
…t-python into feature/api-key-tweaks

Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
…t-python into feature/api-key-tweaks

Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
@esune esune marked this pull request as ready for review May 9, 2024 00:42
@esune
Copy link
Member Author

esune commented May 9, 2024

This is - finally - ready for review.

@esune esune requested review from dbluhm and amanji May 9, 2024 00:43
WadeBarnes
WadeBarnes previously approved these changes May 9, 2024
@jamshale
Copy link
Contributor

jamshale commented May 9, 2024

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.

poetry.lock Show resolved Hide resolved
@jamshale
Copy link
Contributor

jamshale commented May 9, 2024

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.

Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
esune added 2 commits May 9, 2024 22:00
This reverts commit ece22ce.

Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Signed-off-by: Emiliano Suñé <emiliano.sune@gmail.com>
Copy link

sonarcloud bot commented May 9, 2024

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

@jamshale jamshale merged commit 6b0aa4e into openwallet-foundation:main May 10, 2024
8 checks passed
@esune esune deleted the feature/api-key-tweaks branch May 13, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0.0 To be addressed for the ACA-Py 1.0.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multitenancy: do not require main wallet api-key when invoking subwallet functions
7 participants