@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

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

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

@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

@louise-davies I am not sure if you read through the description of the PR but can I get your thoughts on the below two points please?

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

@VKTB sorry, missed those two questions

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.

It's probably acceptable for now to have them read at start up - admin usernames change rarely and e.g. require a restart to ICAT so it's no different here. The token blacklist hasn't really been used but would make more sense to be hot-editable. But equally, doing an apache reload is not a massive downtime really so I'm happy keeping the functionality as it is here and if it needs changing in the future we can change it then.

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

Setting a timeout is probably sensible - not sure what a sensible timeout would be but since it's configurable it's fine.

@louise-davies That's great, thank you!