New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preview mode breaks in Maple if PREVIEW_LMS_HOST is not a subdomain of LMS_HOST #557
Comments
Hey Florian, sorry about the delay replying to your issue. I believe that this should be triaged as an upstream bug. In a nutshell, and to paraphrase your description: users should not have to re-login to access the preview site, even when the preview site domain is not a subdomain of the LMS. I am not a big fan of modifying the session cookie domain just to address this issue. I believe that a proper resolution should include oAuth authentication between the preview site and the LMS. Let's pull @timmc-edx into the conversation here, since he is the one who helped me resolved this other issue. |
Oof, right. We actually got bit by this on edx.org, and what we ended up doing was moving the preview site ( Setting the session cookie domain to be broader would certainly work, although disrecommended for the noted reasons. Moving the preview domain would probably be the fastest fix, if acceptable. The idea of having the preview site be an OAuth client of the LMS is interesting, although I don't know if it would work. My understanding is that the preview site is the LMS, just addressed by a different domain, so I don't know if trying it would break the regular LMS. Some previous internal discussion... Kyle:
(It appears there was some temporary issue with certs in the move, which may or may not be a stumbling block for other deployments. I haven't looked at the details.) Dave:
|
Hmm... @kdmccormick @ormsbee how hard would it actually be to make this change? It looks like everything reads |
@timmc-edx Welp, edx-platform already shamelessly uses import crum
...
def in_preview_mode():
"""
Returns whether the user is in preview mode or not.
"""
hostname = get_current_request_hostname()
preview_lms_base = settings.FEATURES.get('PREVIEW_LMS_BASE', None)
if hostname and preview_lms_base:
if hostname.split(':')[0] == preview_lms_base.split(':')[0]:
return True # DEPRECATED: We're in preview mode, per the hostname.
request = crum.get_current_request()
if request:
if request.GET.get('course_preview') == '1'
return True # We're in preview mode, per the query param.
return False # Not in preview mode. allowing use of either the preview domain or a query param. |
@timmc-edx I just realized what you meant by that -- that hard part would be making sure that we each preview-mode courseware page links out to other preview-mode courseware pages, not published-mode courseware pages. Yeah, I agree; that does sound like the hard part. |
The preview URL is not something that people typically make permanent links/bookmarks to, so I would advocate for moving it to a sub-domain as was done in the edX case as the lowest risk change. I echo your concerns: modifying FWIW, I don't really know much about the state of preview in the LMS right now, and it's possible that it's already broken in this scenario. But at a high level, that's the kind of concern I'd have–changing everything at the domain level at least means that this sort of mode-management is automatically present by default. |
I'd like to reiterate that Tutor already does this (in other words, configures the preview domain name correctly by default), and only breaks if the user chooses to override the preview domain so that it is not a subdomain of the LMS, in a departure from Tutor's defaults. If I understand correctly, then preview in Tutor isn't broken out of the gate; it just breaks unexpectedly when the user overrides I'd posit that if changing to
(like I suggested upthread) is considered undesirable, then Tutor could maybe simply flag an error if |
Since I expect it'll be some time before the preview situation is improved (by changing edx-platform to use OAuth, or switching to a querystring param, or whatever), I like @fghaas 's suggestion here to just alert the user when the preview domain isn't a subdomain of LMS's domain. I don't know whether there's prior art for that sort of thing in Tutor; if not, we could simply raise an |
Another option would be for Tutor to allow overriding |
Your analysis of the situation is 100% correct @fghaas. I just want to point out that it's possible to override the I've been meaning for some time to perform some configuration checks as part of |
I'm not sure how we're able to override |
The
|
Hello there, I am interested in this because I am builiding a spesfic plugin2, of which it would used cloudflare tunnel, the thing is cloudflare has a restriction of it's (proxy/free ssl) featuer that is you need to be on a premium plan so they can cover two level subdomain3, i.e. Reading through the comments, it seems that setting the cookie domain to be the parent is discouraged, that being said, is possible to get more input on this?,as I intent to suggest doing that to mitigate the above caveta. Checking my cookies in dev tools on edx.org, it seems edx.org is setting the domain .edx.org for at least a dozen of cookies including the one refered above1. Secondaly I went through RFC for cookies4, And my conclusion is that it's discourged to run two untrusting services on same machine/host, and there always going to be a risk for cookies secuirty if DNS service is untrusted. In any case I would link to this issue, and mention the cloudflare free plan and Open edX integration as a cavete. And thus leave to the opreator best judement, and of course your input is most welcomed. Footnotes
|
Here's what I suggest:
Who wants to open a PR? |
If no one has taken charge of this issue, I am fully prepared and capable of making sure it is taken care of. |
Please do @CodeWithEmad :) |
So sorry for the late response.
@regisb Calling the action at the end of full load is not the best option since it's running twice on tutor config save. once in: tutor/tutor/commands/config.py Line 150 in 00c58e7
and again in: tutor/tutor/commands/config.py Line 186 in 00c58e7
we can either change where the action is called or modify the commands.config.save function. your call.
also, checking the |
Yes, can we move it to
In the tutor-mfe plugin. |
Well, There are a couple of other things I'd like to address here:
|
Sorry, moving to
Does that make sense?
The priority argument is optional. In our case I don't think we need to specify one -- but maybe I'm wrong about that? Let's try to implement the MFE feature without the priority arg and see if it works.
I'd rather avoid that.
No. Well, yes it's intentional, but it's not great :) I've been meaning to add a check in the past, but did not because there was no elegant way of doing it. After you've created the new action, we will be able to fix this. In particular, I'd like to avoid IP addresses, "localhost" and "mydomain:" values, which cause considerable confusion among users. |
I might be a little off on this one. let's discuss more on the PR. |
Bug description
As of Maple, if
PREVIEW_LMS_HOST
is set to something other than a subdomain ofLMS_HOST
, preview mode breaks since it unexpectedly prompts the user for authentication.How to reproduce
preview.lms.example.com
, setPREVIEW_LMS_HOST
to something likelms-preview.example.com
.Environment
Tutor 13.0.2.
Additional context
In Maple, as explained in the release notes, there's no Preview for the Learning MFE yet, and previews render in the legacy LMS.
However, we can also disable the Learning MFE, at which point we get the legacy learning experience throughout. This, I suppose, might be something that quite a few sites will want to opt to use, until they can offer a smooth course authoring experience where preview mode and the published course do look the same again.
Now, if I open a course unit in Studio and then hit the Preview button, I am prompted to authenticate. I don't think that that should be happening. (I should add that authentication also does not work in that scenario, so preview mode effectively isn't functional at all.)
I am guessing (please correct me if I'm wrong here) that unlike Studio which now uses OAuth2 authentication, preview mode is still relying on a shared session cookie. And in 7c157ec, Tutor followed the recommendation from the docs and changed the value of
SESSION_COOKIE_DOMAIN
from the domain shared betweenLMS_HOST
andCMS_HOST
, to justLMS_HOST
.That happens to work fine for preview mode as well, as long as
PREVIEW_LMS_HOST
is indeed a subdomain ofLMS_HOST
. When it isn't, however, things break.I am guessing that instead of bringing back
(which
templates/apps/openedx/config/lms.env.json
andcms.env.json
were using prior to 7c157ec), we could instead use... but this will of course mean that the session cookie will be exposed to "a potentially large number of subdomains", which is exactly what the Studio OAuth2 change is aiming to avoid. So I'm not sure if that's a good way forward.
@regisb, what are your thoughts on this? How wrong am I? :)
The text was updated successfully, but these errors were encountered: