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

Adding route dependencies module. #251

Merged
merged 36 commits into from
Jun 22, 2024

Conversation

rhysrevans3
Copy link
Contributor

@rhysrevans3 rhysrevans3 commented May 10, 2024

Description:

Generate a list of route dependencies from the environment variable STAC_FASTAPI_ROUTE_DEPENDENCIES or the file that this environment variable points to. Feed these route dependencies into the STACApi on initialisation.

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
  • Changes are added to the changelog

@rhysrevans3
Copy link
Contributor Author

@jonhealy1 sorry I've been at a conference so have only just caught up on the authentication work. The basic auth looks great. Am I right in saying that all endpoints are now closed unless they're included in the public_endpoints list in the BASIC_AUTH environment variable?

@jonhealy1
Copy link
Collaborator

@pedro-cf Hi Pedro. Are you able to look at this pr and respond to @rhysrevans3? I think you would do a better job with this. Thanks!

@pedro-cf
Copy link
Collaborator

pedro-cf commented May 13, 2024

@jonhealy1 sorry I've been at a conference so have only just caught up on the authentication work. The basic auth looks great. Am I right in saying that all endpoints are now closed unless they're included in the public_endpoints list in the BASIC_AUTH environment variable?

Greetings,

if BASIC_AUTH is enabled the endpoints outside of public_endpoints will be protected.
BASIC_AUTH is optional and off by default. You enable it by setting the BASIC_AUTH env variable with your desired configuration.

@rhysrevans3
Copy link
Contributor Author

rhysrevans3 commented May 14, 2024

@jonhealy1 @pedro-cf thanks both I think I understand now. Could I get your opinion on this pull request as it also deals with auth. Are you happy that this and basic auth can co-exist? Or would you like me to try and merge the two? I currently don't have a way to protect all endpoints without explicitly writing them all out. You could add wildcards but this would probably need changes to https://github.com/stac-utils/stac-fastapi

@jonhealy1
Copy link
Collaborator

I definitely like the idea of supporting OAuth2.

@jonhealy1
Copy link
Collaborator

Wondering about the best way to integrate this with basic auth? This will need instructions added to the readme as well and a docker-compose demo file I think.

@rhysrevans3
Copy link
Contributor Author

I've been thinking the same thing I will add instructions to the readme and a docker-compose demo file. They do currently work together but it might be confusing to have both in their current implementation.

I could update basic auth to use this pull requests methods but it would change the config and would remove the * option. I could add the star option to this pull request but that would require changes to stac-fastapi.

another thought I had was this would fit better in the stac-fastapi so it can be used in all of the backends?

@rhysrevans3
Copy link
Contributor Author

rhysrevans3 commented Jun 10, 2024

I've made a pull request in stac-fastapi that would allow for default route dependencies to be added. This would make the merging of basic auth and this pull request much simpler.

@rhysrevans3 rhysrevans3 marked this pull request as ready for review June 13, 2024 13:06
@jonhealy1 jonhealy1 requested a review from pedro-cf June 14, 2024 10:14
Copy link
Collaborator

@pedro-cf pedro-cf left a comment

Choose a reason for hiding this comment

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

Good work, I would suggest:

  • add jsonschema validation for the route_dependencies.json
  • add real complete working examples route_dependencies.json

@rhysrevans3
Copy link
Contributor Author

Good work, I would suggest:

  • add jsonschema validation for the route_dependencies.json
  • add real complete working examples route_dependencies.json

I've add some jsonschema validation for route_dependencies. And added a second docker compose file that use example/route_dependencies/route_dependencies.json and gives an example of an Oauth2 route dependency.

@pedro-cf pedro-cf requested a review from jonhealy1 June 20, 2024 13:25
Copy link
Collaborator

@jonhealy1 jonhealy1 left a comment

Choose a reason for hiding this comment

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

Incredible work here

docker-compose.basic_auth_protected.yml Show resolved Hide resolved
README.md Show resolved Hide resolved
docker-compose.route_dependencies_env.yml Outdated Show resolved Hide resolved
docker-compose.route_dependencies_file.yml Outdated Show resolved Hide resolved
docker-compose.route_dependencies_file.yml Show resolved Hide resolved
examples/route_dependencies/stac-realm.json Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@jonhealy1
Copy link
Collaborator

Nice changelog updates thanks!

Moved changelog entries to Unreleased section.
@jonhealy1
Copy link
Collaborator

@rhysrevans3 I moved the changelog entries to the Unreleased section in the changelog.

@jonhealy1
Copy link
Collaborator

Once we release a new version here and on PyPI then we move the changelog entries to the appropriate section which will hopefully be v3.0.0. Waiting for stac-fastapi v3.0.0

@jonhealy1 jonhealy1 self-requested a review June 22, 2024 04:02
Copy link
Collaborator

@jonhealy1 jonhealy1 left a comment

Choose a reason for hiding this comment

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

Great work!

@jonhealy1 jonhealy1 merged commit de78fbc into stac-utils:main Jun 22, 2024
5 checks passed
@pedro-cf
Copy link
Collaborator

@rhysrevans3 didn't notice but the README needs some correcting:

Docker Compose Configurations
See docker-compose.basic_auth_protected.yml and docker-compose.basic_auth_public.yml for basic authentication configurations.

@jonhealy1
Copy link
Collaborator

@pedro-cf Can you open up a new pr to fix this? Good catch.

@pedro-cf
Copy link
Collaborator

pedro-cf commented Jun 23, 2024

@pedro-cf Can you open up a new pr to fix this? Good catch.

@jonhealy1

  • Would it make sense to simply remove the section? or Add a description for every file? @rhysrevans3 could perhaps help with this?
  • Also would it make sense to move the docker-compose files to the examples directory?

@jonhealy1
Copy link
Collaborator

It does make some sense to move them to the examples folder I think.

@rhysrevans3 rhysrevans3 deleted the route_dependencies branch June 25, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants