indigo-dc / flaat

FLAsk with Access Tokens - FLAAT
MIT License
11 stars 6 forks source link

Avoid configuration at import time #32

Closed BorjaEst closed 1 month ago

BorjaEst commented 3 years ago

See the following recommendation from flask main page: Configuration Best Practices

Do not write code that needs the configuration at import time. If you limit yourself to request-only accesses to the configuration you can reconfigure the object later on as needed.

As example, this functions set_client_id/secret on the flask example are implemented to run at import time. Currenty I cannot factorise my application as I have to pass the value as env variables and at "import" time are not set (for example when testing).

I would consider a more "flask" aproach, for example creating an init_app which would read "CLIENT_ID" and "CLIENT_SECRET" from the environment variables. Or even better something like this: https://docs.authlib.org/en/stable/client/flask.html#configuration

marcvs commented 3 years ago

Not sure about the status, but since this was only in the example, I think it's closeable, right?

BorjaEst commented 3 years ago

I paste here a patch example of how to implement "lazy" configuration so if someones finds the same problem has a reference point:

authorization.py


from functools import wraps
from flaat import Flaat
from flask import current_app, request

class Authorization(Flaat):
    """Monkeypatch flaat to solve lazy configuration
    https://github.com/indigo-dc/flaat/issues/32

    For more information see:
    https://flask.palletsprojects.com/en/2.0.x/extensiondev/#the-extension-code
    """

    def __init__(self, app=None):
        self.app = app
        self.admin_entitlements = None
        if app is not None:
            self.init_app(app)

    def init_app(self, app):
        super().__init__()

        self.set_web_framework('flask')
        self.set_trusted_OP_list([
            'https://aai-dev.egi.eu/oidc'
        ])

        # Flaat timeout:
        timeout = app.config.get('FLAAT_TIMEOUT', 3)
        self.set_timeout(timeout)

        # verbosity:
        #     0: No output
        #     1: Errors
        #     2: More info, including token info
        #     3: Max
        verbosity = app.config.get('FLAAT_VERBOSITY', 0)
        self.set_verbosity(verbosity)

        # TLS verification:
        verify_tls = app.config.get('VERIFY_TLS', False)
        self.set_verify_tls(verify_tls)

        # Required for using token introspection endpoint:
        client_id = app.config['EGI_CLIENT_ID']
        self.set_client_id(client_id)

        client_secret = app.config['EGI_CLIENT_SECRET']
        self.set_client_secret(client_secret)

        admin_entitlements = app.config['ADMIN_ENTITLEMENTS']
        self.admin_entitlements = admin_entitlements

app.py


"""Main application package."""
...
from .authorization import Authorization
auth = Authorization()

def create_app(config_base="your_app.settings"):
    app = Flask(__name__.split(".")[0])
    app.config.from_object(config_base)
    register_extensions(app)
    ...
    return app

def register_extensions(app):
    """Register Flask extensions."""
    ....
    auth.init_app(app)

With this you can add the FLAAT configuration parameters to your application like any other extension. For example, if you load them from the ENV:

your_app.settings


from environs import Env, EnvError
env = Env()
env.read_env()

...
FLAAT_VERBOSITY = str("VERBOSITY", default=0)
EGI_CLIENT_ID = str("OIDC_CLIENT_ID")
EGI_CLIENT_SECRET = str("OIDC_CLIENT_SECRET")
ADMIN_ENTITLEMENTS = str("ADMIN_ENTITLEMENTS", default=[])
marcvs commented 3 years ago

Hey, now I (finally) understood what you meant. Many thanks for the example code. I shall add this as a new example.

BorjaEst commented 2 years ago

Solved for version 1.0.0

BorjaEst commented 2 years ago

@lburgey, do you know if you did manage to solve this issue?

lburgey commented 2 years ago

@lburgey, do you know if you did manage to solve this issue?

Not really. A few decorators may still check their parameters at import time. But if this is undesirable, this functionality could be changed.

BorjaEst commented 2 years ago

@lburgey, do you know if you did manage to solve this issue?

Not really. A few decorators may still check their parameters at import time. But if this is undesirable, this functionality could be changed.

I have figured that when I was fixing the tests for eosc-performance. Do you know of a simple way to collect the app.config["ADMIN_ENTITLEMENTS"] on an admin_required for example?

In your code I saw:

    def admin_required(self, on_failure=None):
        """make view_func only available for admin users"""
        # the requirement is loaded lazily, so we can set eduperson_entitlements at runtime
        return self.requires(self.get_admin_requirement, on_failure=on_failure)

But it raises 'function' object has no attribute 'is_satisfied_by' so I think you meant:

    def admin_required(self, on_failure=None):
        """make view_func only available for admin users"""
        # the requirement is loaded lazily, so we can set eduperson_entitlements at runtime
        return self.requires(self.get_admin_requirement(), on_failure=on_failure)

Which causes this race condition as get_admin_requirement executes before app.config["ADMIN_ENTITLEMENTS"].

It looks a big thing, so do not expend time on it, we will figure out something. I just wanted to know if I am missing something.