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

Taks 1 - Refactor get_api_endpoints() #3

Merged
merged 3 commits into from
Feb 26, 2024

Conversation

Satoshi-Sh
Copy link
Collaborator

@Satoshi-Sh Satoshi-Sh commented Feb 24, 2024

Problem Definiton

Ref: Github Pull Request #36052 VladaZakharova commented on Jan 18

In the init_api_auth_provider method, we update the base path as follows:

def init_api_auth_provider(connexion_app: connexion.FlaskApp):
    """Initialize the API offered by the auth manager."""
    auth_mgr = get_auth_manager()
    blueprint = auth_mgr.get_api_endpoints(connexion_app)
    if blueprint:
        base_paths.append(blueprint.url_prefix if blueprint.url_prefix else "")
        flask_app = connexion_app.app
        flask_app.extensions["csrf"].exempt(blueprint)

However, the blueprint object obtained from auth_mgr.get_api_endpoints(connexion_app) will always be None if we are using ConnexionV3.

Proposed solution

Ref vincbeck commented on Jan 18

  • Rename get_api_endpoints to set_api_endpoints. The return type should be updated to None. Documentation should be updated as well to something like "Set API endpoint(s) definition for the auth manager.". This is a breaking change but nobody uses this interface yet, so it is a good time to do it.
  • This piece of code flask_app.extensions["csrf"].exempt(blueprint) should be moved in the set_api_endpoints method using appbuilder.app.extensions["csrf"].exempt(api.blueprint)

How to test

  • Run client tests python ./clients/python/test_python_client.py

@sudiptob2 sudiptob2 changed the title changed set with get Taks 1 - Refactor get_api_endpoints() Feb 24, 2024
@sudiptob2 sudiptob2 self-requested a review February 24, 2024 01:40
@sudiptob2 sudiptob2 marked this pull request as ready for review February 24, 2024 02:16
@@ -304,8 +304,4 @@ def init_api_experimental(app):
def init_api_auth_provider(connexion_app: connexion.FlaskApp):
"""Initialize the API offered by the auth manager."""
auth_mgr = get_auth_manager()
blueprint = auth_mgr.get_api_endpoints(connexion_app)
if blueprint:
base_paths.append(blueprint.url_prefix if blueprint.url_prefix else "")
Copy link
Owner

Choose a reason for hiding this comment

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

We do not have any blueprint here, so we can not update base_path But we need to handle base path update logic somewhere. Whats the impact of removing this line.

specification=specification,
resolver=_LazyResolver(),
base_path="/auth/fab/v1",
swagger_ui_options=swagger_ui_options,
strict_validation=True,
validate_responses=True,
)
return api.blueprint if api else None
return None
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
return None
self.appbuilder.app.extensions["csrf"].exempt(api.blueprint)
return None

According to the proposed solution, you it is suggested to move it here I guess.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't give you KeyError "crsf"?

Copy link
Owner

Choose a reason for hiding this comment

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

yeah, you have to add an if statement. But this does not solve the CSRF error

Copy link
Owner

@sudiptob2 sudiptob2 Feb 24, 2024

Choose a reason for hiding this comment

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

        if api:
            self.appbuilder.app.extensions["csrf"].exempt(api.blueprint)
        return None

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like this connexion_app.add_api() doesn't return an object, so I just removed "if" statement.

I kind of understanding Vlada's struggle now.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I think that is why, CSRF error is still there. In this comment I think Rob suggested some solution
apache#36052 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure what he meant here. I might need some time to understand it.

"Find a way to add csrf extension to a newly created blueprint using connexion: to retrieve blueprint object from connexion_app variable to save the current logic (flask_app.extensions["csrf"].exempt(blueprint)) or find a way to add this extension on connexion level(check the documentation for available options)."

Seems like we cannot get the blueprint, so we better focus on the second option(add this extension on the connexion level). I'm reading the connexion source code, but no luck yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably need to understand this repo. I will let you know if I find something useful.

@sudiptob2 sudiptob2 force-pushed the feat/migrate-to-connexion-v3 branch 2 times, most recently from f66f6d9 to de01bb4 Compare February 24, 2024 15:29
@Satoshi-Sh
Copy link
Collaborator Author

Satoshi-Sh commented Feb 24, 2024

Problem

Due to the upgrading connextion v3, we cannot access blueprints( they moved the blueprint registration code inside their codebase). We used the returned blueprint to make exemptions to accept HTTP(S) requests without "csrf token" in the header. When the client makes a post request, it doesn't include a csrf token. That's why we get csrf token missing error with test_python_client.py

@RobbeSneyders suggested utilizing the middleware library asgi-csrf to do the same without using blueprints.

def skip_api_paths(scope)
    return scope["path"].startswith("/api/")

app = asgi_csrf(
    app,
    signing_secret="secret-goes-here",
    skip_if_scope=skip_api_paths
)

This is a sample code to make csrf-token exemption.

  • What the score will be in the airflow project?
  • I'm not sure about siginig_secret. Do I need to generate it or can get it from somewhere?

I'm trying to implement this on "/api/v1/" and random signing_secret for now.

@Satoshi-Sh
Copy link
Collaborator Author

I just tried to use asgi_csrf but it doesn't work as expected. I might've made some mistakes in the implementation. I just pushed it so that you can have a look at it. @sudiptob2

@sudiptob2 sudiptob2 merged this pull request into feat/migrate-to-connexion-v3 Feb 26, 2024
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.

2 participants