Enable runtime CORS configuration via environment variables

[captaincoordinates]

Related Issue(s):
#340

Description:
Support runtime CORS configuration via JSON configuration and environment variable. Missing environment variable, missing config file, un-parseable config file all result in no CORS configuration. Runtime CORS configuration will be ignored if the application has already added CORSMiddleware.

New tests have been added to ensure this functionality works as designed. Minor changes to testing infrastructure to support setup behaviour prior to some pytest fixtures running.

Documentation updated to recommend this approach to CORS configuration over the previous code modification approach.

PR Checklist:

• Code is formatted and linted (run pre-commit run --all-files)
• Tests pass (run make test)
• Documentation has been updated to reflect changes, if applicable, and docs build successfully (run make docs)
• Changes are added to the CHANGELOG.

[geospatial-jeff]

@captaincoordinates Thanks for the PR! 🙏

This feels a little overloaded. Using a JSON file to store CORS configuration is a code leak and overly complex, typically this sort of configuration is the responsibility of the deployment and not the application code. I would much rather reuse the FastAPI CORS middleware as-is, parameterizing the middleware settings w/ environment variables. This provides a more flexible and robust way to configure/deploy the app and aligns better with microservice best practices (see https://12factor.net/config).

Baking CORS into stac-fastapi does resolve #340, as a user will be able to configure CORS in stac-fastapi without forking the code or implementing their own app. Since the stac-api spec does recommend enabling CORS, I think it is totally fine to include it as a default middleware (by appending to StacApi.middlewares - https://github.com/stac-utils/stac-fastapi/blob/master/stac_fastapi/api/stac_fastapi/api/app.py#L90). No need to implement our own custom MiddlewareConfig since FastAPI already provides a way to add middleware to the application.

This will close #127 as well.

[vincentsarago]

we could simply do something like

from starlette.middleware import cors

class CORSMiddleware(cors.CORSMiddleware):
    """Starlette CORS Middleware with default."""

    def __init__(
        self,
        app: ASGIApp,
        allow_origins: Sequence[str] = ("*",),
        allow_methods: Sequence[str] = ("OPTIONS, POST, GET",),
        allow_headers: Sequence[str] = ("Content-Type",),
        allow_credentials: bool = False,
        allow_origin_regex: str = None,
        expose_headers: Sequence[str] = (),
        max_age: int = 600,
    ) -> None:
        """Create CORSMiddleware Object."""
        super().__init__(
            app,
            allow_origins=allow_origins,
            allow_methods=allow_methods,
            allow_headers=allow_headers,
            allow_credentials=allow_credentials,
            allow_origin_regex=allow_origin_regex,
            expose_headers=expose_headers,
            max_age=max_age,
        )

# And then in
# https://github.com/stac-utils/stac-fastapi/blob/master/stac_fastapi/api/stac_fastapi/api/app.py#L90
middlewares: List = attr.ib(
    default=attr.Factory(lambda: [BrotliMiddleware, CORSMiddleware]),
)

Note: each backend application will need to update the CORSMiddleware if they need transaction methods (

stac-fastapi/stac_fastapi/types/stac_fastapi/types/core.py

Line 25 in 5dc8e0e

class BaseTransactionsClient(abc.ABC):

)

[captaincoordinates]

@geospatial-jeff @vincentsarago thanks for the feedback. If you're each happy for CORS to be a default middleware I will follow @vincentsarago's approach above with the addition of env var configuration for each of the options, defaulting to the values provided in that example. BRB with an updated PR

[captaincoordinates]

PR updated with a variation on the suggested approach to permit configuration via environment variables. Some additional tests are required for the various CORS configuration options but I was hoping to get initial feedback on the overall approach before committing that effort (hence draft PR status).

[captaincoordinates]

I'm working on aligning env var configuration with the ApiSettings sub-class approach after realising that there was a good convention already in place

[captaincoordinates]

FYI I'm working on the docs build failure. I checked make test before pushing but not make docs

[moradology]

@captaincoordinates Curious about the status on this

[captaincoordinates]

@moradology PR feedback went quiet, so I'm not sure

[moradology]

Cool, I've just been away from the lib for a bit and am trying to get back on top of the PRs I've missed! I'll poke this today to keep it moving!

[moradology]

@captaincoordinates Hey Tom, so I've got a rebased branch here which you might look at and force onto your @sparkgeo feature/1 branch (I appear to not have the permissions to do this). Let me know if I can be of further assistance

[captaincoordinates]

@moradology thanks, I'm pinned by project work at the moment but I will hopefully get to it during the next few days

[captaincoordinates]

@moradology should be good to go. Thanks for fixing those test errors, for a moment there I thought I was going to have to figure it out myself

[jlev]

Any chance this will land soon? Would love to get CORS headers configured the right way with environment variables, instead of hacking on a fork of the code.

[captaincoordinates]

@jlev I don't know why this has been hanging for so long. Now there are merge conflicts that are difficult to resolve and I don't know when I'll be able to fix them

[alexgleith]

Would love to see this merged. Happy to help out if I can.

[geospatial-jeff]

I managed to resolve most of the merge conflicts but I don't think we should merge this PR as the same may be accomplished with a lot less code by forcing user to configure/attach middleware themselves.

from stac_fastapi.api.app import StacApi
from fastapi.middleware.cors import CORSMiddleware

# Create you api.
api = StacApi(...)

# Add the middleware
# https://fastapi.tiangolo.com/tutorial/cors/
api.app.add_middleware(
    CORSMiddleware,
    allow_methods=["*"],
    allow_headers=os.getenv("CORS_ALLOW_HEADERS")
)

I see why the PR is written this way; adding of middlewares is currently done in the post-init and doesn't currently expose passing of positional args which requires overloading the middleware class itself to expose these arguments. This is a bad pattern, and I'd rather fix the underlying pattern than overloading each middleware. I think we should probably remove the StacApi.middlewares attribute all-together and write a StacApi.add_middleware method which just wraps FastApi.add_middleware and allows users to add their own middlewares w/ less abstraction.

This is discussed more here: #127 (comment)

[geospatial-jeff]

Closing this in favor of #435 and #436