Skip to content

feat(templates): add oath consent template#7454

Open
nadahlberg wants to merge 5 commits into
mainfrom
oauth-consent-template
Open

feat(templates): add oath consent template#7454
nadahlberg wants to merge 5 commits into
mainfrom
oauth-consent-template

Conversation

@nadahlberg

Copy link
Copy Markdown
Member

Fixes

Fixes #7228

Summary

This PR adds a branded template to the oath2_provider apps authorize view.

Deployment

This PR should:

  • skip-deploy (skips everything below)
    • skip-web-deploy
    • skip-celery-deploy
    • skip-cronjob-deploy
    • skip-daemon-deploy

Screenshots

Desktop

Screenshot 2026-06-16 at 11 44 57 AM

Mobile

Screenshot 2026-06-16 at 11 45 02 AM

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — straightforward branded template for the OAuth2 consent view.

Extended reasoning...

Overview

This PR adds a single new template file at cl/assets/templates/oauth2_provider/authorize.html that overrides django-oauth-toolkit's default authorize view with a CourtListener-branded version. It extends base.html, renders the application name, requested scopes, redirect URI, and hidden form fields, and provides a fallback error rendering branch.

Security risks

Minimal. The template displays values that come from a trusted form (application.name, scopes_descriptions, form.redirect_uri.value) through Django's auto-escaped templating. CSRF is included. The user-visible <code> block wrapping redirect_uri is escaped by Django and renders as text only. noindex meta is correctly applied to prevent indexing of the consent page.

Level of scrutiny

Low — this is a pure presentation-layer change with no Python logic, no view changes, and no auth flow changes. The behavior is entirely driven by django-oauth-toolkit's existing AuthorizationView, which already supplies the same context variables to its stock template.

Other factors

The bug hunter flagged a single cosmetic nit: {{ form.errors }} already renders non-field errors (it's an ErrorDict), so the following {{ form.non_field_errors }} line duplicates them in the error alert. This is purely visual and only shows on the AllowForm validation-failure path, which is rare. It's already posted as an inline comment for the author to optionally fix; the rest of the template is correct, and screenshots in the PR description confirm the happy path renders as intended.

Comment on lines +47 to +52
{% if form.errors %}
<div class="alert alert-danger">
{{ form.errors }}
{{ form.non_field_errors }}
</div>
{% endif %}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 {{ form.errors }} renders Django's ErrorDict (via as_ul()), which already includes non-field errors under the __all__ key — so the immediately-following {{ form.non_field_errors }} prints those same messages a second time inside the alert box. Since django-oauth-toolkit's AllowForm validation failures (invalid redirect_uri, scope, etc.) are almost exclusively non-field errors, the duplication will be visible in essentially every error case. Drop one of the two lines (typically {{ form.non_field_errors }}).

Extended reasoning...

What goes wrong

Django's Form.errors is an ErrorDict. Its default rendering (ErrorDict.__str__as_ul()) emits a single <ul class="errorlist"> containing every error on the form, including non-field errors keyed under the special __all__ key (NON_FIELD_ERRORS). form.non_field_errors() is then just self.errors.get(NON_FIELD_ERRORS, self.error_class(...)) — it returns the same ErrorList that form.errors already rendered.

So this block:

{% if form.errors %}
  <div class="alert alert-danger">
    {{ form.errors }}
    {{ form.non_field_errors }}
  </div>
{% endif %}

renders the non-field errors twice inside the same alert div.

Why this matters for AllowForm

The form used here is django-oauth-toolkit's AllowForm. Validation failures from clean() / add_error(None, ...) (invalid redirect_uri, invalid scope, etc.) are non-field errors, so practically every error message a user would see on this page will appear duplicated.

Step-by-step proof

  1. User submits the consent form with an invalid scope.
  2. AllowForm.clean() raises a ValidationError, which is stored under __all__.
  3. Template hits {% if form.errors %} → True; enters the alert div.
  4. {{ form.errors }}ErrorDict.as_ul() emits e.g.
    <ul class="errorlist nonfield"><li>Invalid scope.</li></ul>.
  5. {{ form.non_field_errors }} → returns the same ErrorList and renders
    <ul class="errorlist nonfield"><li>Invalid scope.</li></ul> again.
  6. The user sees the message twice in the same alert box.

Why this slipped past

Both Django snippets look complementary at a glance — "errors and non-field errors" reads naturally — but form.errors is a superset, not a sibling, of form.non_field_errors. The standard Django idiom is to render either {{ form.errors }} alone, or {{ form.non_field_errors }} plus per-field errors — never both.

Impact

Cosmetic only — no functional or security impact, and only visible on the error path. The visible OAuth error code path mostly flows through the {% else %} branch (which renders error.error / error.description), so this duplication shows up only when AllowForm itself fails validation.

Fix

Delete the {{ form.non_field_errors }} line; {{ form.errors }} already covers it:

{% if form.errors %}
  <div class="alert alert-danger">
    {{ form.errors }}
  </div>
{% endif %}

@nadahlberg

Copy link
Copy Markdown
Member Author

Updates the errors to mirror the login template. Using error|escape instead of error|safe since the error might contain a redirect uri from the client, which could be unsafe.

@nadahlberg

Copy link
Copy Markdown
Member Author

@elisa-a-v could you review?

@nadahlberg nadahlberg requested a review from elisa-a-v June 16, 2026 17:40
@nadahlberg nadahlberg assigned elisa-a-v and unassigned nadahlberg Jun 16, 2026
@mlissner

Copy link
Copy Markdown
Member

@nadahlberg be sure to add the item to the Web team project board and set it as Todo so that Elisa sees it there too. I think that's the latest approach unless I missed something.

@ERosendo ERosendo self-requested a review June 23, 2026 01:30
@ERosendo ERosendo moved this from To Do to In progress in Sprint (Web Team) Jun 23, 2026

@ERosendo ERosendo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mlissner @nadahlberg code looks good to me. One thing I'd like to double-check: do we feel strongly about using the blue background for the confirmation and error messages?

My impression is that a white background might be a bit easier to read and would also align better with some of our other pages (for example, the 404 page). I put together a couple of screenshots showing how it looks with a white background:

  • Confirmation message:
image
  • Error message:
image

And for reference, here's our 404 page, which also uses a white background:

image

Curious to hear what you both think!

@ERosendo ERosendo moved this from In progress to To Do in Sprint (Web Team) Jun 23, 2026
@ERosendo ERosendo assigned mlissner and nadahlberg and unassigned ERosendo Jun 23, 2026
@mlissner

Copy link
Copy Markdown
Member

Yeah, I agree!

@ERosendo

Copy link
Copy Markdown
Contributor

Thanks, @mlissner! Back to you, @nadahlberg. I think the only change needed is removing the well class.

@nadahlberg nadahlberg moved this from To Do to In progress in Sprint (Web Team) Jun 23, 2026

@elisa-a-v elisa-a-v left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and agreed with Eduardo on the white background 🫡

@mlissner mlissner moved this from In progress to To Do in Sprint (Web Team) Jul 6, 2026
@nadahlberg nadahlberg moved this from To Do to H 🍔 in Sprint (Web Team) Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Hana Vu 🗡️

Development

Successfully merging this pull request may close these issues.

Add branded template to OAuth consent page

4 participants