mblackgeo / flask-cognito-lib

A Flask extension that supports protecting routes with AWS Cognito following OAuth 2.1 best practices
https://mblackgeo.github.io/flask-cognito-lib/
MIT License
57 stars 15 forks source link

feat: add `cognito_refresh_callback` and underlying plugin methods #39

Closed lokeoke closed 4 months ago

lokeoke commented 5 months ago

Addresses:

Depends on:

ToDo:

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (6f9740c) to head (86c024e).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #39 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 9 9 Lines 310 397 +87 ========================================= + Hits 310 397 +87 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lokeoke commented 4 months ago

@mblackgeo This one is ready for review

lokeoke commented 4 months ago

Had to make some adjustments in https://github.com/mblackgeo/flask-cognito-lib/pull/39/commits/1e36de647c490c2949301739c23009946e3eea31

  1. Noticed a mistake, as refresh flow does not provide new refresh_token each time. Cleaned that up.
  2. That lead to necessity of having separate AWS_COGNITO_REFRESH_COOKIE_AGE_SECONDS

My testing on the real app went good. I was able to log it, refresh access token multiple times, log out

planestoner commented 4 months ago

Guys thanks so much for this, it's been something I've been considering for a while. I'd like to fully understand how to implement it, am I correct in thinking that a refresh will not be called automatically and the cognito_refresh_callback needs to be manually called, or is it done automatically - somewhere perhaps I missed in a brief review?

My initial thoughts were to maybe create a custom decorator similar to the current @auth_required() which if the access is expired then attempts a refresh thus (or have I got the wrong end of the stick and there's a simpler approach?):

def my_auth_required(groups: Optional[Iterable[str]] = None, any_group: bool = False):
    def wrapper(fn):
        @wraps(fn)
        def decorator(*args, **kwargs):
            with app.app_context():
                if cfg.disabled:
                    return fn(*args, **kwargs)

                access_token = request.cookies.get(cfg.COOKIE_NAME)
                refresh_token = request.cookies.get(cfg.COOKIE_NAME_REFRESH)
                valid = False

                try:
                    # Attempt to verify the access token
                    claims = cognito_auth.verify_access_token(
                        token=access_token,
                        leeway=cfg.cognito_expiration_leeway,
                    )
                    valid = True
                except (TokenVerifyError, KeyError):
                    # Access token is invalid, try to refresh it
                    if refresh_token:
                        try:
                            new_tokens = cognito_auth.exchange_refresh_token(refresh_token)
                            access_token = new_tokens['access_token']  # Assume new access token is obtained
                            # Verify the new access token
                            claims = cognito_auth.verify_access_token(
                                token=access_token,
                                leeway=cfg.cognito_expiration_leeway,
                            )
                            # Update the cookies with new tokens
                            request.cookies.set('access_token', access_token)
                            if 'refresh_token' in new_tokens:
                                request.cookies.set('refresh_token', new_tokens['refresh_token'])
                            valid = True
                        except Exception as e:
                            valid = False

                # Check for required group membership if token is valid
                if valid and groups:
                    if any_group:
                        valid = any(g in claims.get("cognito:groups", []) for g in groups)
                    else:
                        valid = all(g in claims.get("cognito:groups", []) for g in groups)

                    if not valid:
                        raise CognitoGroupRequiredError

                if not valid:
                    raise AuthorisationRequiredError

                return fn(*args, **kwargs)

        return decorator

    return wrapper
lokeoke commented 3 months ago

Hi @planestoner,

I'd like to fully understand how to implement it, am I correct in thinking that a refresh will not be called automatically and the cognito_refresh_callback needs to be manually called, or is it done automatically - somewhere perhaps I missed in a brief review?

Any automation would be inappropriate as library then would dictate how the application should behave. As I mentioned in https://github.com/mblackgeo/flask-cognito-lib/pull/39#discussion_r1567221835

I have a React front end, that ~5 min before expiration shows user a modal with the button to prolong the session. If user clicks it - refresh flow happens. If not - user will be kicked-off.

My initial thoughts were to maybe create a custom decorator similar to the current @auth_required() which if the access is expired then attempts a refresh thus (or have I got the wrong end of the stick and there's a simpler approach?):

It's up-to-you. I just want to warn you that request.cookies.set('access_token', access_token) is not secure, see https://github.com/mblackgeo/flask-cognito-lib/pull/39/files#diff-5123dc7c7226ad96083297a735e20f71e19ad4b103579db333801451a9f9b6faR60-R82