Skip to content
This repository has been archived by the owner on Aug 26, 2022. It is now read-only.

jinja2/react_base.html template uses request.user #6039

Closed
peterbe opened this issue Oct 24, 2019 · 5 comments
Closed

jinja2/react_base.html template uses request.user #6039

peterbe opened this issue Oct 24, 2019 · 5 comments
Assignees
Labels
comp: frontend Component: Frontend p2 Medium Priority: One of the next sprints

Comments

@peterbe
Copy link
Contributor

peterbe commented Oct 24, 2019

Summary says it all. The read-only pages should never depend on the users.

Our CDN is configured to never pass through any sessionid cookie values to the Django views so hopefully all pages now are always forcibly anonymous.

We should audit the trail to make sure nothing in the read-only site depends on the user.
Including waffle flags.

@peterbe peterbe added the status: needs triage Status: Untriaged label Oct 24, 2019
@tobinmori tobinmori added comp: frontend Component: Frontend p2 Medium Priority: One of the next sprints and removed status: needs triage Status: Untriaged labels Oct 25, 2019
peterbe added a commit to peterbe/kuma that referenced this issue Oct 25, 2019
@peterbe
Copy link
Contributor Author

peterbe commented Oct 25, 2019

Let's start with waffle flags. One place where request.user is used is

  {% if waffle.flag('developer_needs') %}
    {% stylesheet 'banners' %}
  {% endif %}

the implementation of waffle.flag depends on users in some django-waffle way.
But what's so wrong is that the rendering of the HTML is always anonymous (because our CloudFront will strip sessionid cookie values). So the HTML will never ship with the necessary CSS into the CSSOM but it could be that a users /api/v1/whoami says that the developer_needs is on so banners.jsx decides to render something to the DOM. But the CSS won't be there.

The minified banners.css is 5,095 bytes which isn't insignificant. So let's aim to NOT include it if there's no reason not to.

Thing is, developer_needs is one of N possible uses of the banners.css. In jinja this might be expressed as:

{% if waffle.flag('developer_needs') or waffle.flag('devportal_promotion') or waffle.flag('firefox_partnership') or waffle.flag('some_other_reason') %}
    {% stylesheet 'banners' %}
  {% endif %} 

So it's not fair to connect banners.css to a particular waffle flag.

A possible solution is to use Constance instead. So like this:

{% if config.ENABLE_BANNERS %}

Then, if you enable ANY waffle flag that needs banners.css you remember to go to https://developer.mozilla.org/admin/constance/config/ and enable it.

Another option is to inspect any defined waffle flag that might need it. If there is greater than 0% chance that it might be enabled, you include it. E.g.

# the view function
possible_flags = ['developer_needs']
context['include_banners_css'] = False
for flag in Flag.objects.filter(name__in=possible_flags):
   if flag.everyone or flag.percent > 0 or flag.superusers or flag.staff or flag.authenticated or flag. ...:
      context['include_banners_css'] = True
      break
{# e.g. react_base.html #}
{% if include_banners_css %}
  {% stylesheet 'banners' %}
{% endif %}

peterbe added a commit to peterbe/kuma that referenced this issue Oct 25, 2019
peterbe added a commit that referenced this issue Oct 25, 2019
* don't use data-login-service in react_base.html

Part of #6039

* react homepage and react document to share ga config

Part of #6039

* correction
@peterbe
Copy link
Contributor Author

peterbe commented Oct 29, 2019

Actually, a more ideal solution would be something like this:
https://gist.github.com/peterbe/1740864dfa1e2ec02e90063eeec396f4

It's more complicated but might perform better since it's less CSS upfront. We'd need to do it more properly with better error handling and we'd also need to figure out a way to "connect" this to django-pipeline.

@peterbe
Copy link
Contributor Author

peterbe commented Oct 29, 2019

@Gregoor For the record, this relates to how we'd do the CSS for BCD tables too.
I think my Django solution would tie us over but it would be even better if we could properly lazy load it.
Again, I'm not convinced I want to improve Kuma since there are no good ways to test if it works.

@tobinmori
Copy link

#6108 was done, but needs to be redone after some consideration.

@tobinmori tobinmori added this to the Tracy Chapman (Q4 Sprint 4) milestone Dec 3, 2019
@jmswisher jmswisher added p1 High Priority: Current sprint p2 Medium Priority: One of the next sprints and removed p2 Medium Priority: One of the next sprints p1 High Priority: Current sprint labels Dec 4, 2019
@jmswisher jmswisher removed this from the Tracy Chapman (Q4 Sprint 4) milestone Dec 5, 2019
@peterbe
Copy link
Contributor Author

peterbe commented Mar 20, 2020

We can close this now. Somewhere in the read-only site stuff do we do things that depend on request.user. For example, the Subscription banner depends on the user but we're side-stepping that entirely by including the subscription banner CSS on every possible page no matter what the waffle flag might say.

@peterbe peterbe closed this as completed Mar 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
comp: frontend Component: Frontend p2 Medium Priority: One of the next sprints
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants