fix: LANGUAGE_COOKIE_NAME is not set by default #507

cmltaWt0 · 2021-10-07T21:51:26Z

Regression caused by https://github.com/edx/edx-platform/pull/28796

Details:

response.delete_cookie(
    settings.LANGUAGE_COOKIE_NAME,
    domain=settings.SHARED_COOKIE_DOMAIN
)
caused an Exception due to absence of LANGUAGE_COOKIE_NAME settings variable

> production.py

LANGUAGE_COOKIE_NAME = ENV_TOKENS.get('LANGUAGE_COOKIE', None) or ENV_TOKENS.get('LANGUAGE_COOKIE_NAME')

Exception:

2021-10-07 20:26:57,299 INFO 8 [audit] [user 3] [ip 172.19.0.1] models.py:2723 - Login success - user.id: 3
2021-10-07 20:26:57,318 INFO 8 [openedx_events.tooling] [user 3] [ip 172.19.0.1] tooling.py:160 - Responses of the Open edX Event <org.openedx.learning.auth.session.login.completed.v1>:
[]
2021-10-07 20:26:57,429 ERROR 8 [root] [user 3] [ip 172.19.0.1] signals.py:22 - Uncaught exception from None
Traceback (most recent call last):
  File "/openedx/venv/lib/python3.8/site-packages/django/core/handlers/exception.py", line 34, in inner
    response = get_response(request)
  File "/openedx/venv/lib/python3.8/site-packages/django/utils/deprecation.py", line 96, in __call__
    response = self.process_response(request, response)
  File "./openedx/core/djangoapps/lang_pref/middleware.py", line 93, in process_response
    response.delete_cookie(
  File "/openedx/venv/lib/python3.8/site-packages/django/http/response.py", line 216, in delete_cookie
    secure = key.startswith(('__Secure-', '__Host-'))
AttributeError: 'NoneType' object has no attribute 'startswith'

pcliupc

Looks good to me. May also need to update Changelog accordingly.

regisb

Thanks for your PR @cmltaWt0! I understand that you are trying to address a misconfiguration of the edx-platform production settings. Indeed, in the production settings of edx-platform, we currently have (source):

LANGUAGE_COOKIE_NAME = ENV_TOKENS.get('LANGUAGE_COOKIE', None) or ENV_TOKENS.get('LANGUAGE_COOKIE_NAME')

This is all the more unfortunate that the common settings provide a good default value for this setting (source):

LANGUAGE_COOKIE_NAME = "openedx-language-preference"

So I think it would be better if the edx-platform settings could be fixed upstream:

LANGUAGE_COOKIE_NAME = ENV_TOKENS.get('LANGUAGE_COOKIE') or ENV_TOKENS.get('LANGUAGE_COOKIE_NAME', LANGUAGE_COOKIE_NAME)

I understand that it might be easier to get a PR merged in the Tutor nightly branch than in edx-platform. So what I suggest is that you open a PR on the edx-platform repo; if it cannot be immediately merged, get back here and we'll merge this one. Once your PR is merged in edx-platform, revert your change in the Tutor nightly branch.

What do you think?

regisb · 2021-10-11T07:46:12Z

tutor/templates/apps/openedx/config/cms.env.json

@@ -35,6 +35,7 @@
  "EMAIL_USE_TLS": {{ "true" if SMTP_USE_TLS else "false" }},
  "HTTPS": "{{ "on" if ENABLE_HTTPS else "off" }}",
  "LANGUAGE_CODE": "{{ LANGUAGE_CODE }}",
+  "LANGUAGE_COOKIE_NAME": "{{ LANGUAGE_COOKIE_NAME }}",


I would rather avoid adding too much stuff to the env.json files. Could we replace these two additions (to lms.env.json and cms.env.json) by a single addition to the apps/openedx/settings/partials/common_all.py template?

Will try this.

regisb · 2021-10-11T07:48:01Z

tutor/templates/config.yml

@@ -48,6 +48,7 @@ JWT_COMMON_SECRET_KEY: "{{ OPENEDX_SECRET_KEY }}"
 JWT_RSA_PRIVATE_KEY: "{{ 2048|rsa_private_key }}"
 K8S_NAMESPACE: "openedx"
 LANGUAGE_CODE: "en"
+LANGUAGE_COOKIE_NAME: "openedx-language-preference"


We should avoid adding Tutor configuration variables for every edx-platform setting. My rule of thumb is that if a setting can be easily overridden by a Tutor plugin (e.g: with at most 2-3 patches) then we should not create a new Tutor setting. Instead, Tutor should provide a good default out of the box.

Great. Totally agree.

cmltaWt0 · 2021-10-11T11:16:55Z

Thanks for your PR @cmltaWt0! I understand that you are trying to address a misconfiguration of the edx-platform production settings. Indeed, in the production settings of edx-platform, we currently have (source):
LANGUAGE_COOKIE_NAME = ENV_TOKENS.get('LANGUAGE_COOKIE', None) or ENV_TOKENS.get('LANGUAGE_COOKIE_NAME')
This is all the more unfortunate that the common settings provide a good default value for this setting (source):
LANGUAGE_COOKIE_NAME = "openedx-language-preference"
So I think it would be better if the edx-platform settings could be fixed upstream:
LANGUAGE_COOKIE_NAME = ENV_TOKENS.get('LANGUAGE_COOKIE') or ENV_TOKENS.get('LANGUAGE_COOKIE_NAME', LANGUAGE_COOKIE_NAME)
I understand that it might be easier to get a PR merged in the Tutor nightly branch than in edx-platform. So what I suggest is that you open a PR on the edx-platform repo; if it cannot be immediately merged, get back here and we'll merge this one. Once your PR is merged in edx-platform, revert your change in the Tutor nightly branch.

What do you think?

Thanks a lot in such a good analysis. I was thinking the same way and didn't want to add any line of code to the tutor codebase but fix it in upstream.

Will create a PR to the edx-platform and let's hope it will be merged quickly. Simultaneously will changes this PR (to have plan B) based on the best practices you mentioned.

regisb · 2021-10-14T11:06:23Z

@cmltaWt0 I've had to git push --force the nightly branch -- bad, I know, but I only recently officialised the existence of the nightly branch by merging #494. Can you please rebase?

Regression caused by https://github.com/edx/edx-platform/pull/28796 Details: ``` response.delete_cookie( settings.LANGUAGE_COOKIE_NAME, domain=settings.SHARED_COOKIE_DOMAIN ) caused an Exception due to absence of LANGUAGE_COOKIE_NAME settings variable > production.py LANGUAGE_COOKIE_NAME = ENV_TOKENS.get('LANGUAGE_COOKIE', None) or ENV_TOKENS.get('LANGUAGE_COOKIE_NAME') ```

cmltaWt0 · 2021-10-19T07:29:33Z

@cmltaWt0 I've had to git push --force the nightly branch -- bad, I know, but I only recently officialised the existence of the nightly branch by merging #494. Can you please rebase?

Done. Yet not started to work on your review. Will try to finish this week.

cmltaWt0 · 2021-10-23T12:59:10Z

Adding upstream PR https://github.com/edx/edx-platform/pull/29096

Have to keep in mind that maple quick fix should be reverted after upstream PR is merged due to losing the ability to use $LMS_CFG as a point to override the LANGUAGE_COOKIE_NAME for different envs easily and dynamically.

cmltaWt0 · 2021-10-23T12:59:55Z

Actually I'm closing this PR as not needed anymore.

See also: #507 Upstream fix: https://github.com/edx/edx-platform/pull/29096

cmltaWt0 marked this pull request as ready for review October 7, 2021 22:11

pcliupc reviewed Oct 11, 2021

View reviewed changes

regisb requested changes Oct 11, 2021

View reviewed changes

regisb force-pushed the nightly branch from 6c89173 to c3c914f Compare October 14, 2021 11:05

cmltaWt0 force-pushed the cmltaWt0/language_cookie_name branch from ea0b4a6 to a3e436e Compare October 19, 2021 07:28

cmltaWt0 mentioned this pull request Oct 23, 2021

fix: use common default for LANGUAGE_COOKIE_NAME openedx/edx-platform#29096

Merged

cmltaWt0 closed this Oct 23, 2021

regisb added a commit that referenced this pull request Oct 25, 2021

fix: lms 500 error caused by missing LANGUAGE_COOKIE_NAME setting

bc9fc9a

See also: #507 Upstream fix: https://github.com/edx/edx-platform/pull/29096

regisb added a commit that referenced this pull request Oct 25, 2021

fix: lms 500 error caused by missing LANGUAGE_COOKIE_NAME setting

4f034f8

See also: #507 Upstream fix: https://github.com/edx/edx-platform/pull/29096

regisb added a commit that referenced this pull request Dec 20, 2021

fix: lms 500 error caused by missing LANGUAGE_COOKIE_NAME setting

694ddfe

See also: #507 Upstream fix: https://github.com/edx/edx-platform/pull/29096

regisb added a commit that referenced this pull request Dec 20, 2021

fix: lms 500 error caused by missing LANGUAGE_COOKIE_NAME setting

670552c

See also: #507 Upstream fix: https://github.com/edx/edx-platform/pull/29096

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: LANGUAGE_COOKIE_NAME is not set by default #507

fix: LANGUAGE_COOKIE_NAME is not set by default #507

cmltaWt0 commented Oct 7, 2021 •

edited

Loading

pcliupc left a comment

regisb left a comment •

edited

Loading

regisb Oct 11, 2021

cmltaWt0 Oct 11, 2021

regisb Oct 11, 2021

cmltaWt0 Oct 11, 2021

cmltaWt0 commented Oct 11, 2021

regisb commented Oct 14, 2021

cmltaWt0 commented Oct 19, 2021

cmltaWt0 commented Oct 23, 2021

cmltaWt0 commented Oct 23, 2021

fix: LANGUAGE_COOKIE_NAME is not set by default #507

fix: LANGUAGE_COOKIE_NAME is not set by default #507

Conversation

cmltaWt0 commented Oct 7, 2021 • edited Loading

pcliupc left a comment

Choose a reason for hiding this comment

regisb left a comment • edited Loading

Choose a reason for hiding this comment

regisb Oct 11, 2021

Choose a reason for hiding this comment

cmltaWt0 Oct 11, 2021

Choose a reason for hiding this comment

regisb Oct 11, 2021

Choose a reason for hiding this comment

cmltaWt0 Oct 11, 2021

Choose a reason for hiding this comment

cmltaWt0 commented Oct 11, 2021

regisb commented Oct 14, 2021

cmltaWt0 commented Oct 19, 2021

cmltaWt0 commented Oct 23, 2021

cmltaWt0 commented Oct 23, 2021

cmltaWt0 commented Oct 7, 2021 •

edited

Loading

regisb left a comment •

edited

Loading