Skip to content

Conversation

PicchiSeba
Copy link
Contributor

@PicchiSeba PicchiSeba commented Jun 11, 2025

If we go into production right now, the FastAPI documentation endpoints (Swagger, Redoc, OpenAPI) are reachable by anyone.

There might be cases where this feature is not required or even prohibited.

With this PR we aim to hide these interfaces with a System Parameter

@OCA-git-bot
Copy link
Contributor

Hi @lmignon,
some modules you are maintaining are being modified, check this out!

Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

I would prefer a configuration parameter on the endpoint in place of a global config parameter.

@PicchiSeba PicchiSeba force-pushed the 16.0-hide-api-docs branch 2 times, most recently from dcc5ceb to 881bfdc Compare June 11, 2025 12:01
@PicchiSeba
Copy link
Contributor Author

PicchiSeba commented Jun 11, 2025

While testing it I found a problem, not necessarily related to this implementation.

If I enable the endpoint with expose_fastapi_docs set to True the docs are reachable as expected.
Then if I change expose_fastapi_docs to `False the docs are still reachable.

I found that the _prepare_fastapi_app_params method is never triggered after synching the endpoint. Perhaps it gets computed only before mounting the FastAPI app?

@PicchiSeba PicchiSeba changed the title [IMP]fastapi: expose FastAPI docs only if system parameter is set [IMP]fastapi: expose FastAPI docs only if field is set Jun 11, 2025
@PicchiSeba PicchiSeba force-pushed the 16.0-hide-api-docs branch from 881bfdc to de344b4 Compare June 11, 2025 12:14
@lmignon
Copy link
Contributor

lmignon commented Jun 11, 2025

While testing it I found a problem, not necessarily related to this implementation.

If I enable the endpoint with expose_fastapi_docs set to True the docs are reachable as expected. Then if I change expose_fastapi_docs to `False the docs are still reachable.

I found that the _prepare_fastapi_app_params method is never triggered after synching the endpoint. Perhaps it gets computed only before mounting the FastAPI app?

It's because you must add the new field into the list returned by the method _fastapi_app_fields. This will instruct the addon that the app must be rebuild when you new field is modified.

@PicchiSeba PicchiSeba force-pushed the 16.0-hide-api-docs branch from de344b4 to 649b406 Compare June 12, 2025 07:19
@PicchiSeba PicchiSeba changed the title [IMP]fastapi: expose FastAPI docs only if field is set [16.0][IMP]fastapi: expose FastAPI docs only if field is set Jun 13, 2025
Copy link

@SirPyTech SirPyTech left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
Good work 👏
I just have a few suggestions, mainly about documentation of the code, please have a look at the comments below.

@PicchiSeba PicchiSeba force-pushed the 16.0-hide-api-docs branch 2 times, most recently from 85bda5a to 8ca9d00 Compare June 13, 2025 10:02
Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

@PicchiSeba Thank you once again. LGTM

@PicchiSeba PicchiSeba force-pushed the 16.0-hide-api-docs branch from 95c8ca1 to 4eea801 Compare June 13, 2025 15:50
Copy link

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

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

Code review, LGTM

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@sebastienbeau sebastienbeau added this to the 16.0 milestone Sep 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants