Update django 4.2

[hina-shah]

The PR makes necessary changes to upgrade Django to 4.2, and it's related libraries.

4.2 is the next LTS according to this, and will be supported till beginning of 2026.

[waTeim]

Ok I have some questions:

What's the purpose of this?

CSRF_TRUSTED_ORIGINS = [
        "https://localhost",
        "https://127.0.0.1",
        "https://*.renci.org",
    ]

And we resolved that this matches what we have set up with UNC ITS:

 "USE_JWT": False,
    'WANT_ASSERTIONS_SIGNED': True,
    'AUTHN_REQUESTS_SIGNED': False,
    'WANT_RESPONSE_SIGNED': False,
    'TOKEN_REQUIRED': False,

I wonder if this is even necessary (or for that matter anything in migrations) since we never bother to maintain the user database from one version of appstore to the other.
True, I am making an assumption on what the purpose of this is: appstore/core/migrations/0002_auto_20200430_1711.py

[hina-shah]

Ok I have some questions:

What's the purpose of this?

CSRF_TRUSTED_ORIGINS = [
        "https://localhost",
        "https://127.0.0.1",
        "https://*.renci.org",
    ]

CSRF_TRUSTED_ORIGINS is a list os trusted domains for POST requests for eg. We had this setting before enabled only for DEBUG mode, and only localhost was in the list. But we were getting CSRF_TOKEN errors, and hence not able to access the appstore page at all. Pulling out CSRF_TOKEN out of the DEBUG mode and adding renci.org as a trusted domain resolved that issue.

And we resolved that this matches what we have set up with UNC ITS:

"USE_JWT": False,
'WANT_ASSERTIONS_SIGNED': True,
'AUTHN_REQUESTS_SIGNED': False,
'WANT_RESPONSE_SIGNED': False,
'TOKEN_REQUIRED': False,

Yes, we have checked that. Any changes in this messes up with SAML.

I wonder if this is even necessary (or for that matter anything in migrations) since we never bother to maintain the user database from one version of appstore to the other. True, I am making an assumption on what the purpose of this is: appstore/core/migrations/0002_auto_20200430_1711.py

I'm not sure on this. Some migrations run by default even when the databases are generated when the appstore is deployed. But this one in particular was just renaming the model, and I did not see a reason for it. I think I can keep on investigating when working on whitelisting.

[waTeim]

Ok if there is a problem with CSRF on renci.org sites if it's not in the list, will there be a problem for eduhelx on ashe (when it's unc.edu)?

Should these things be in the values file rather than hard-coded?

[hina-shah]

Ok if there is a problem with CSRF on renci.org sites if it's not in the list, will there be a problem for eduhelx on ashe (when it's unc.edu)?

Should these things be in the values file rather than hard-coded?

That's a good point, renci.unc.edu should be added to that list.

I'm not sure if it should be in values file.. If we want to "strictly" keep the deployments within the renci domain, then hardcoded values make sense. If we want to have it truly open-source, and let folks do their own deployments, then yes - definitely should be a list value in values file.

[frostyfan109]

@waTeim pretty sure CSRF_TRUSTED_ORIGIN means that cross-site requests from that origin do not require a CSRF token on the post request. @hina-shah I think hardcoded domains (renci.org, renci.unc.edu) should be moved to values and make the current list the default value if unspecified.

[waTeim]

Still not a fan:

if CSRF_TRUSTED_ORIGINS == 0:
    CSRF_TRUSTED_ORIGINS = [
            "https://*.renci.org",
            "https://*.renci.unc.edu"
        ]

I get it, the intention is to make the default behavior backward compatible, otherwise, when you use the previous values file it would not work. Ok, I'm not sure that's a good idea, but even if we want that. I say these things should go into the chart's default values.

And/Or simply in the release notes point out that this value must be set.

Also, this is kind of a problem if the env variable is not set, you'll get an array containing a 0-length string instead of a 0-length array:

>>> "".split(',')
['']

[joshua-seals]

I think it is probably fine this way honestly since the deployment customization from the chart necessarily allows for changing this setting (assuming that is a settable value in appstore-chart).

I wonder though, in settings/base.py if there is some other value that could be coupled to this one to introspect the context of the deployment. 🤔
if CSRF_TRUSTED_ORIGINS == 0: CSRF_TRUSTED_ORIGINS := append(CSRF_TRUSTED_ORIGINS, EMAIL_HOST)
or some other value that infers our deployment context as a default value setting`. This way, we keep it general to the email host (or better domain context) if that is changed for any domain deployment like it is set to nih.gov but offer additional addons.

[joshua-seals]

Typo

[hina-shah]

Thanks for catching that!