Wire up GET /login/oauth/authorize by apata · Pull Request #6712 · plausible/analytics

apata · 2026-09-24T12:42:47Z

Changes

Wires up GET /login/oauth/authorize. Needs feature flag mcp on user to work.

Basic visuals for starters:

Tests

Automated tests have been added

Changelog

This PR does not make a user-facing change

Documentation

This change does not need a documentation update

Dark mode

The UI has been tested both in dark and light mode

github-actions · 2026-09-24T13:27:27Z

Preview environment👷🏼‍♀️🏗️
PR-6712

zoldar · 2026-09-30T13:54:17Z

+
+      <div class="flex flex-col gap-y-4">
+        <div class="flex flex-col gap-y-2" data-test-id="grant-team">
+          <%= if match?([_, _ | _], @grantable_teams) do %>


This is a very obtuse way to express if length(@grantable_teams) > 1. Was credo complaining? I'd honestly consider make it stfu if it did.

zoldar · 2026-09-30T14:00:23Z

@@ -0,0 +1,157 @@
+defmodule PlausibleWeb.Live.OAuthAuthorize do


Unless I'm missing something, I don't see anything that would explicitly require making this a LiveView. A static view with a form would work just as well if not better (selection update goes via server and the interaction might be jarring on a more laggy connection). Is that meant for some other, more dynamic features getting added some time later?

Hmm, I didn't think about potential lagginess. Regarding selecting scopes and stuff, it's probably going to happen, but I haven't yet thought about it.

It was a form initially. CIMD was fetched on form load. The server validated that the document checks out, self-referential and all that. (A sample document that's valid: https://gist.githubusercontent.com/apata/7387339c3c81540524bc670da0b05f00/raw/client-metadata.json)

The user manually validated that they want to authorize based on the info from CIMD and the query params.

They click "Authorize". POST happens for the form.

On POST, we need to save who the user was, which team they selected, the client id, client name, client redirect URI, resource, scopes, code challenge and the challenge method. Where do we get client name from? Hidden form input? Can we trust that value server-side?

In the form solution, to have this trust, we fetched CIMD again on POST. However, that created the situation where the CIMD might have changed between rendering and submitting the form: since we can't possibly allow the user to press authorize for one application and receive grant for another, there's a new class of errors to handle.

I thought LV solved this quite elegantly. Now I'm not sure though.

We could go back to a form if we figured out a way to avoid a second fetch. Sign the whole thing on GET (Phoenix.Token.sign(conn, @salt, %{user_id: user.id, ctx: ctx})), then verify with short TTL on POST (Phoenix.Token.verify(conn, @salt, token, max_age: @max_age_seconds))? Apparently, this is similar to how LV does it, but if we handroll it, we get more control over the exact contents, TTL and salt.

PS: In a separate PR, I plan to cache CIMDs as suggested in the OAuth 2.1 draft. With N app servers, do the GET page and POST form hit the same server (and thus cache)?
PPS: I also considered the fact that LV necessitates JS as positive. This flow is supposed to be completed by a human.

Indeed, signing and verifying is a good point. However, going all in with LV just for that might be perhaps a slight overkill?

The mere necessitation of JS is not a strong argument IMHO. It can be worked around easily, especially with huge corpora of code doing this that LLMs can draw from (scrapers are probably significant percentage of the overall crap they hoover up 😅).

Agreed that "has JS => more likely to be human" is not a strong argument in favor. UX lag is a stronger one against LV (though I wonder if all our live views have gone through this justification check).

Therefore I've refactored back to the form in 7a8f07f. The signing and verifying part wasn't too bad.

One advantage is that it allows me to withdraw the PR with e2e tests for this. Will do so once we agree this is the way forward

zoldar · 2026-09-30T14:02:33Z

@@ -0,0 +1,116 @@
+defmodule PlausibleWeb.OAuth.AuthorizationRequest do
+  @moduledoc """
+  Validates an incoming [authorization request](https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1#name-authorization-request)


Suggested change

Validates an incoming [authorization request](https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1#name-authorization-request)

Validates and turns an incoming [authorization request](https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1#name-authorization-request)

zoldar · 2026-09-30T14:03:26Z

+  into the context the consent screen renders and the decision is taken against.
+
+  `build/1` is the only place the client's metadata document is fetched during a
+  consent round trip. The context it returns travels from the controller into


Suggested change

consent round trip. The context it returns travels from the controller into

consent round trip. The context it returns travels from the controller to

zoldar · 2026-09-30T14:04:01Z

+
+  `build/1` is the only place the client's metadata document is fetched during a
+  consent round trip. The context it returns travels from the controller into
+  the LiveView that takes the decision, so nothing downstream re-reads a


Suggested change

the LiveView that takes the decision, so nothing downstream re-reads a

the LiveView that makes the decision, so nothing downstream re-reads a

?

Speaking of, @apata would you consider simplifying the robo-talk? Those comments/moduledocs are exhausting to read.

Agreed, @aerosol! Generally I do, but this snuck through.

Moduledoc rewritten in 7a8f07f

zoldar · 2026-09-30T14:14:33Z

+  alias Plausible.OAuth.CIMD
+  alias Plausible.OAuth.ProtectedResources
+
+  @type t() :: %{


WDYT about using an embedded_schema here with a changeset that would cast and validate all that?

Is the idea to bring in Ecto so we'd be able to use its tested validation logic? I hadn't even considered that. Now that I am thinking of it, I can see your point, but TBH I'm a bit wary of it.

Some of its features related to changesets are not ideal IMO.

Just as an example that I stumbled upon already in previous PRs: when validating field F 1) length and 2) matching regex, it ends up running both validators, even if the length validation fails. That means there's an open door for regex denial of service (ReDoS) attacks, though "common sense" and glance at code would not reveal it.

Potentially risky checks can be wrapped in something like maybe_validate_format(changeset, ...), where the wrapper runs the validation only when changeset.valid? is true. It's not a must, though it could perhaps help reduce the boilerplate somewhat.

I tried to refactor to embedded schema, but the module got bigger, not smaller, and IMO harder to understand ':) It's possible I didn't try hard enough.

By boilerplate, do you mean the type and the remap from string keys to atom keys?

Since 7a8f07f, this module has better defined responsibilities, so I think the boilerplate isn't that jarring.

zoldar · 2026-09-30T14:24:29Z

+  @moduledoc """
+  The teams an authorization request may bind a grant to.
+
+  `current_team` is deliberately not consulted: on the authorize route the URL


current_team can't be set to anything the current user does not have access to already. https://github.com/plausible/analytics/blob/dynamic-funnel-poc/lib/plausible_web/plugs/auth_plug.ex#L30-L33 Besides, I don't see any relevance of current_team in this context - so maybe it's best to remove this paragraph to avoid confusion?

You're right, will check this moduledoc, it's a bit loose. Let's discuss the actual implementation here: https://github.com/plausible/analytics/pull/6712/changes/BASE..78454561932ab7d06c190acd814bb1ea716168ff#paneldiscussion_r4153052032

I've removed this module and cleaned up the moduledocs of authorization request and authorization response. They were indeed confusing, hopefully it's a bit more clear now.

zoldar · 2026-09-30T14:26:00Z

+  def resolve(assigns, identifier) do
+    teams = list(assigns)
+
+    Enum.find(teams, &(&1.identifier == identifier)) || List.first(teams)


Suggested change

Enum.find(teams, &(&1.identifier == identifier)) || List.first(teams)

Enum.find(teams, List.first(teams), &(&1.identifier == identifier))

Thanks! Fixed in 7a8f07f!

zoldar · 2026-09-30T14:31:23Z

@@ -0,0 +1,21 @@
+defmodule PlausibleWeb.Plugs.IgnoreTeamParam do


Again, __team can't cause current_team override to an arbitrary team just like that. Are we sure this is necessary?

Thanks! I'm aware it can't switch to random team, but it can switch the user's current team. That switch affects the other Plausible tabs they have open: https://3.basecamp.com/5308029/buckets/36789884/card_tables/cards/10217223382

The screen where I'm turning this parameter off is not supposed to be linked to from Plausible, right? It's only for outside applications (they build this link with the appropriate query params based on our advertised OAuth metadata).

Picking the right team to authorize is the user's responsibility. I want the default value to be their currently active team.

Is there a legitimate reason an outside application needs to be able to switch the user's current team programmatically?

If it switched only for this tab, then maybe I'd be OK with it, but at the moment, the switch has a wider impact and I'm not OK with it.

Theoretically, we could look at the unintentional switching as a wider bug and I can remove this extra plug from this PR.

Yeah, how team switching works is not great but is also limited by the fact we had to leave URL scheme intact. AFAIK the only way to make it resilient to multi-tab usage scenario, would be including team in every URL (either as a parameter or part of the path), which is not an option.

What's the benefit of selectively switching it off here though? This particular view completely ignores current team while it can still be switched from another tab.

Removed the plug in 7a8f07f
The fix was too narrow and led to worse UX for legitimate cases: we do want to read current team as the default selected one.

apata added 11 commits September 24, 2026 10:50

Implement authorize screen

356c8ca

Require email verification for OAuth /authorize

b8e99cd

Add rate limits

47a1ffb

Fix issue with team being accepted from the outside

a63b598

Rewrite authorize screen to be a live view

d7f63d0

Remove token endpoint tests

7ab7394

Enforce :mcp feature flag

bce4f68

Refactor test name

55eac09

Make AuthorizeControllerTest more simple

a0685b8

Remove extra test

77c9202

Solve credo issue

e5c6b1d

apata added the preview label Sep 24, 2026

apata marked this pull request as ready for review September 24, 2026 13:48

apata changed the title ~~Wires up /authorize to OAuth core~~ Wires up /login/oauth/authorize Sep 24, 2026

apata changed the title ~~Wires up /login/oauth/authorize~~ Wire up GET /login/oauth/authorize Sep 24, 2026

apata requested a review from a team September 30, 2026 06:18

Merge remote-tracking branch 'origin/master' into mcp/oauth-controllers

7845456

zoldar reviewed Sep 30, 2026

View reviewed changes

Refactor to form submit action

7a8f07f

		@@ -0,0 +1,157 @@
		defmodule PlausibleWeb.Live.OAuthAuthorize do

	Validates an incoming [authorization request](https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1#name-authorization-request)
	Validates and turns an incoming [authorization request](https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1#name-authorization-request)

	consent round trip. The context it returns travels from the controller into
	consent round trip. The context it returns travels from the controller to

	the LiveView that takes the decision, so nothing downstream re-reads a
	the LiveView that makes the decision, so nothing downstream re-reads a

	Enum.find(teams, &(&1.identifier == identifier)) \|\| List.first(teams)
	Enum.find(teams, List.first(teams), &(&1.identifier == identifier))

		@@ -0,0 +1,21 @@
		defmodule PlausibleWeb.Plugs.IgnoreTeamParam do

Uh oh!

Conversation

apata commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Changelog

Documentation

Dark mode

Uh oh!

github-actions Bot commented Sep 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apata Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

apata commented Sep 24, 2026 •

edited

Loading

apata Oct 1, 2026 •

edited

Loading