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

Port API to FastAPI #133 #134

Open
wants to merge 43 commits into
base: main
Choose a base branch
from
Open

Port API to FastAPI #133 #134

wants to merge 43 commits into from

Conversation

VKTB
Copy link
Contributor

@VKTB VKTB commented Nov 4, 2024

Description

  • Drops support for all Python versions below 3.11
  • Ports the API to FastAPI
  • Updates project dependencies to latest versions
  • Adds missing severity key value pair to the scheduled_maintenance.json file
  • Defines new custom exception classes to improve error handling
  • Logger is now configured using a logging.ini file
  • App is now configured using a .env file instead of the old config.json. The config is loaded when the first starts up which means that updates to the the AUTHENTICATION__JWT_REFRESH_TOKEN_BLACKLIST and AUTHENTICATION__ADMIN_USERS values now require the application to be restarted to take effect. If this is a problem then I suggest that we store these values in separate files like we do with the active usernames in LDAP JWT Auth.
  • Refactors the majority of the app logic including tests
  • Fixes a bug where any refresh token could refresh any access token
  • The linter was complaining about no use of timeout in requests calls so I added an option in the config to allow for this to be set. I set it to 5 seconds in the example file (hopefully this is enough?).
  • Updates the Dockerfile to have multistages:
    • dev for local development
    • test for running the tests
    • prod for production
  • Updates the tests CI job to run the tests in a Docker container

Testing Instructions

Add a set up instructions describing how the reviewer should test the code

  • Review code
  • Check GitHub Actions build
  • Review changes to test coverage
  • Test it with SciGateway, ensure it doesn't break anything
  • Test endpoints (swagger docs available at /docs) work as expected

Agile Board Tracking

closes #93
closes #94
closes #95
closes #129
closes #133

@VKTB
Copy link
Contributor Author

VKTB commented Nov 12, 2024

@louise-davies As I mentioned, earlier there are a few things I want to check

  • Do the Ansible scripts create the maintenance files? If so, does the scheduled_maintenace.json have a severity key value pair?
  • Do the Ansible scripts need to be updated to create a .env and logging.ini config files?
  • Are there any other changes that need to be made to the ansible scripts?
  • The response status code/message have changed for the below endpoints. Will this break SciGateway?
    • (For POST requests to /login) If the mnemonic field in the body is missing, it now returns 422 { "detail": [ { "type": "missing", "loc": [ "body", "mnemonic" ], "msg": "Field required", "input": {} } ] } instead of 400 Missing mnemonic. (Message is built by Pydantic)
    • (For POST requests to /refresh) If JWT refresh token (HTTP-only cookie) is missing, it now returns 400 No JWT refresh token found instead of 400 No refresh token cookie found.
    • (For POST requests to /refresh) If JWT refresh token (HTTP-only cookie) is blacklisted, it now returns 403 Unable to refresh access token instead of 403 Refresh token was not valid".
    • (For POST requests to /refresh If token field (which is the JWT access token) in the body is missing, it now returns 422 { "detail": [ { "type": "missing", "loc": [ "body", "token" ], "msg": "Field required", "input": null } ] } instead of 400 No access token found. (Message is built by Pydantic)
    • (For POST requests to /refresh) If anything goes wrong while it is trying to refresh the access token like invalid JWT tokens, the usernames in the tokens are not matching, or ICAT errors, it now returns 403 Unable to refresh access token instead of 403 Unable to refresh token.
    • (For POST requests to /verify) For 200 responses, it no longer returns a body . Previously, it returned an empty string (""). I think returning 204 instead would be more suitable here given no body is returned but not sure how that will impact SciGateway.
    • (For POST requests to /verify) If JWT token in the body is invalid, it now returns 403 Invalid JWT token provided instead of 403 Token could not be verified.
    • (For GET requests to /maintenance) If it fails to get the maintenance state, it now returns 500 Failed to get maintenance state instead of 500 Failed to retrieve maintenance mode state.
    • (For PUT requests to /maintenance) If there are validation errors as a result of invalid/missing JSON data in the body, it now returns 422 instead of 400. The message returned is different as it the validation is handled by Pydantic instead of the jsonschema library.
    • (For PUT requests to /maintenance) If JWT access token in the body is invalid, it now returns 403 Unable to update maintenance state instead of 403 Access token was not valid.
    • (For PUT requests to /maintenance) If user is not admin, it now returns 403 Unable to update maintenance state instead of 403 Unauthorized.
    • (For PUT requests to /maintenance) If anything goes wrong while it is trying to update the maintenance state like file write error, it now returns 500 Failed to update maintenance state instead of 500 Failed to update maintenance mode state.
    • (For PUT requests to /maintenance) For 200 responsnes, it now returns Maintenance state successfully updated instead of Maintenance mode state successfully updated.
    • (For GET requests to /scheduled_maintenance) If it fails to get the scheduled maintenance state, it now returns 500 Failed to get scheduled maintenance state instead of 500 Failed to return scheduled maintenance state.
    • (For PUT requests to /scheduled_maintenance) If there are validation errors as a result of invalid/missing JSON data in the body, it now returns 422 instead of 400. The message returned is different as it the validation is handled by Pydantic instead of the jsonschema library.
    • (For PUT requests to /scheduled_maintenance) If JWT access token in the body is invalid, it now returns 403 Unable to update scheduled maintenance state instead of 403 Access token was not valid.
    • (For PUT requests to /scheduled_maintenance) If user is not admin, it now returns 403 Unable to update scheduled maintenance state instead of 403 Unauthorized.
    • (For PUT requests to /scheduled_maintenance) If anything goes wrong while it is trying to update the scheduled maintenance state like file write error, it now returns 500 Failed to update scheduled maintenance state instead of 500 Failed to update scheduled maintenance mode state.
    • (For PUT requests to /scheduled_maintenance) For 200 responsnes, it now returns Maintenance state successfully updated instead of Maintenance mode state successfully updated.

@VKTB VKTB marked this pull request as ready for review November 12, 2024 18:10
@louise-davies
Copy link
Member

louise-davies commented Nov 14, 2024

@VKTB

Do the Ansible scripts create the maintenance files? If so, does the scheduled_maintenace.json have a severity key value pair?

Yes, they create them if they don't already exist. But no they weren't updated to have severity yet - this was added later. Can easily update the ansible to have that set to warning by default and I can update the files already installed on machines too. I presume this is because this PR adds validation to the JSON now - previously we didn't care and just stored any JSON iirc.

Do the Ansible scripts need to be updated to create a .env and logging.ini config files?

Yes they would do if they are required. The .env file replaces config.json right? Which is configured in the ansible so can just adapt it. Should not be hard to add, just standard templating stuff/copying files if they're supplied.

Are there any other changes that need to be made to the ansible scripts?

I wouldn't think so, unless it's installed differently all the ansible scripts do is set up directories/files, install packages into virtualenv & set up the apache config to run the WSGI script.

The response status code/message have changed for the below endpoints. Will this break SciGateway?

(For POST requests to /login) If the mnemonic field in the body is missing, it now returns 422 { "detail": [ { "type": "missing", "loc": [ "body", "mnemonic" ], "msg": "Field required", "input": {} } ] } instead of 400 Missing mnemonic. (Message is built by Pydantic)

Shouldn't matter - we don't really care how the login commands fail - we just ensure the user is logged out and display a generic "Failed to login" message (aka we don't parse the error from the API)

(For POST requests to /refresh) If JWT refresh token (HTTP-only cookie) is missing, it now returns 400 No JWT refresh token found instead of 400 No refresh token cookie found.

Again, we don't parse the error message so no concern, would just display a "Signed out due to token invalidation" message

(For POST requests to /refresh) If JWT refresh token (HTTP-only cookie) is blacklisted, it now returns 403 Unable to refresh access token instead of 403 Refresh token was not valid".

See above

(For POST requests to /refresh If token field (which is the JWT access token) in the body is missing, it now returns 422 { "detail": [ { "type": "missing", "loc": [ "body", "token" ], "msg": "Field required", "input": null } ] } instead of 400 No access token found. (Message is built by Pydantic)

See above

(For POST requests to /refresh) If anything goes wrong while it is trying to refresh the access token like invalid JWT tokens, the usernames in the tokens are not matching, or ICAT errors, it now returns 403 Unable to refresh access token instead of 403 Unable to refresh token.

See above

(For POST requests to /verify) For 200 responses, it no longer returns a body . Previously, it returned an empty string (""). I think returning 204 instead would be more suitable here given no body is returned but not sure how that will impact SciGateway.

We don't care about the specific body or code, as long as it's a 2XX code

(For POST requests to /verify) If JWT token in the body is invalid, it now returns 403 Invalid JWT token provided instead of 403 Token could not be verified.

Any failure to verify like refresh is not parsed, and will just result in a generic "Signed out due to token invalidation"

(For GET requests to /maintenance) If it fails to get the maintenance state, it now returns 500 Failed to get maintenance state instead of 500 Failed to retrieve maintenance mode state.

We don't parse the message so no concern

(For PUT requests to /maintenance) If there are validation errors as a result of invalid/missing JSON data in the body, it now returns 422 instead of 400. The message returned is different as it the validation is handled by Pydantic instead of the jsonschema library.

Again, we don't parse the message and the error code is still an error code so error handling will work the same

(For PUT requests to /maintenance) If JWT access token in the body is invalid, it now returns 403 Unable to update maintenance state instead of 403 Access token was not valid.

See above, we don't parse the message

(For PUT requests to /maintenance) If user is not admin, it now returns 403 Unable to update maintenance state instead of 403 Unauthorized.

See above

(For PUT requests to /maintenance) If anything goes wrong while it is trying to update the maintenance state like file write error, it now returns 500 Failed to update maintenance state instead of 500 Failed to update maintenance mode state.

See above

(For PUT requests to /maintenance) For 200 responses, it now returns Maintenance state successfully updated instead of Maintenance mode state successfully updated.

We don't parse success messages either, it just returns the body. So the message the admins see will change slightly but not a concern in my opinion.

(For GET requests to /scheduled_maintenance) If it fails to get the scheduled maintenance state, it now returns 500 Failed to get scheduled maintenance state instead of 500 Failed to return scheduled maintenance state.

We don't parse the error

(For PUT requests to /scheduled_maintenance) If there are validation errors as a result of invalid/missing JSON data in the body, it now returns 422 instead of 400. The message returned is different as it the validation is handled by Pydantic instead of the jsonschema library.

See above

(For PUT requests to /scheduled_maintenance) If JWT access token in the body is invalid, it now returns 403 Unable to update scheduled maintenance state instead of 403 Access token was not valid.

See above

(For PUT requests to /scheduled_maintenance) If user is not admin, it now returns 403 Unable to update scheduled maintenance state instead of 403 Unauthorized.

See above

(For PUT requests to /scheduled_maintenance) If anything goes wrong while it is trying to update the scheduled maintenance state like file write error, it now returns 500 Failed to update scheduled maintenance state instead of 500 Failed to update scheduled maintenance mode state.

See above

(For PUT requests to /scheduled_maintenance) For 200 responsnes, it now returns Maintenance state successfully updated instead of Maintenance mode state successfully updated.

Like with the maintenance endpoint, we don't parse success messages, it just returns the body. So the message the admins see will change slightly but not a concern in my opinion.

Overall, I think the changes to the API endpoints themselves don't require any changes in SG and overall look for the better, whereas scigateway-ansible will need updating but it's relatively easy to do so I think

@VKTB
Copy link
Contributor Author

VKTB commented Nov 14, 2024

@louise-davies Thank you for providing these answers! Sounds like we don't need to do anything in SciGateway which is good news.

I presume this is because this PR adds validation to the JSON now - previously we didn't care and just stored any JSON iirc.

That's right, it now uses Pydantic models which automatically validate the data stored in the JSON files.

The .env file replaces config.json right?

Correct. The configuration is now handled using Pydantic Settings which allows for loading config values from system environment variables or the .env file. It's up to us to decide whether we use system ENV VARs or a .env file. Please note that when both are used, Pydantic will still read system ENV VARs as well as the .env file but system ENV VARs will always take priority over values loaded from the .env file.

I wouldn't think so, unless it's installed differently all the ansible scripts do is set up directories/files, install packages into virtualenv & set up the apache config to run the WSGI script.

There is no WSGI script anymore as FastAPI is ASGI and in the world of containers the app is run in production mode using the FastAPI CLI command line program which internally uses a production-ready Uvicorn ASGI server. I have never run a FastAPI app on an apache server like that so we should look at how we get it to work.

@louise-davies
Copy link
Member

louise-davies commented Nov 14, 2024

@VKTB ah yeah, forgot about the WSGI to ASGI change. Will probably just have to set up a system service for the API, and use Apache to reverse proxy to whatever port we run it on - so slightly more changes to the ansible scripts than I thought

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants