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
59 stars 19 forks source link

Option to set domain for cookie #25

Closed iteratelance closed 12 months ago

iteratelance commented 1 year ago

Hello,

Thanks so much for maintaining this library. I think it would be useful to have a config option to set the domain the cookie responses is set to

Current code in decorators is

            resp.set_cookie(
                key=cfg.COOKIE_NAME,
                value=tokens.access_token,
                max_age=cfg.max_cookie_age_seconds,
                httponly=True,
                secure=True,
            )

Suggestion if cfg.COOKIE_DOMAIN_NAME is set

            resp.set_cookie(
                key=cfg.COOKIE_NAME,
                value=tokens.access_token,
                max_age=cfg.max_cookie_age_seconds,
                httponly=True,
                secure=True,
                domain=cfg.COOKIE_DOMAIN_NAME
            )

I believe that this would be useful option for those who are wanting to share the cookie across subdomains. I have a workaround at the moment but it's not pretty.

mblackgeo commented 1 year ago

Hey, thanks for the suggestion. Is this something you'd be willing to submit a PR for?

iteratelance commented 1 year ago

Hello @mblackgeo sure, I'd be happy to do but here is what I've learned as I just tested it. We'd also have to update the delete_cookie response.

resp.delete_cookie(key=cfg.COOKIE_NAME, domain=cfg.cookie_domain)

After playing around with a few things it appears my application does not even require specifying a cookie domain but I'm happy to help.

Additionally, any chance to get this working in latest version of Flask and updated dependencies? I downloaded a local version of this app without all the dependencies and used the following and have experienced no issues. I could submit a PR for this as well.

cryptography==41.0.4 
Flask==3.0.0
PyJWT==2.8.0

Please note it was extremely painful getting version of cryptography > 37.0.4 working on Lambda. You effectively have 3 options.

  1. pip install cryptography==37.0.4
  2. pip install \ --platform manylinux2014_x86_64 \ --implementation cp \ --python-version 3.9 \ --only-binary=:all: --upgrade \ --target ${DEST_DIR} \ cryptography==41.0.4
  3. use serverless-python-requirements in a docker container build for AWS
    custom:
    pythonRequirements:
    dockerizePip: true
    dockerImage: public.ecr.aws/sam/build-python3.9:latest
    dockerRunCmdExtraArgs: ['--platform', 'linux/amd64']
matardy commented 1 year ago

Hello,

Thanks so much for maintaining this library. I think it would be useful to have a config option to set the domain the cookie responses is set to

Current code in decorators is

            resp.set_cookie(
                key=cfg.COOKIE_NAME,
                value=tokens.access_token,
                max_age=cfg.max_cookie_age_seconds,
                httponly=True,
                secure=True,
            )

Suggestion if cfg.COOKIE_DOMAIN_NAME is set

            resp.set_cookie(
                key=cfg.COOKIE_NAME,
                value=tokens.access_token,
                max_age=cfg.max_cookie_age_seconds,
                httponly=True,
                secure=True,
                domain=cfg.COOKIE_DOMAIN_NAME
            )

I believe that this would be useful option for those who are wanting to share the cookie across subdomains. I have a workaround at the moment but it's not pretty.

Hey, this works for you? I trying to do the same but the domain is not getting the cookie. I want to do a SSR but the cognito_access_token cookie is not working.

mblackgeo commented 1 year ago

Hey, I'd be happy to see a PR that allows for an (optional) domain to be configured for which we can then set/delete with on the cookie.

Will make a separate issue around updating the dependencies of Flask and Cryptography.

matardy commented 1 year ago

Hi, I saw that @iteratelance already make a PR about domain cookie. @mblackgeo can you review it? I would be happy to help you in the maintaining of this repo.

mblackgeo commented 12 months ago

Hi, I saw that @iteratelance already make a PR about domain cookie. @mblackgeo can you review it? I would be happy to help you in the maintaining of this repo.

thanks, I have a been away recently - hopefully will get to it it in the next few days

mblackgeo commented 12 months ago

Released in v1.6.0 :tada:

thanks all!