Verification to dashboard#6516
Conversation
| if(@needs_verification?, | ||
| do: [verify_installation: true, flow: PlausibleWeb.Flows.provisioning()], | ||
| else: [] |
There was a problem hiding this comment.
suggestion, non-blocking: Can we avoid the verification being triggerable with users manually entering these URL params?
I think it'd be better if this server side process started with a user interaction, not navigation.
Because the redirect is handled by the server, maybe we can pass the fact that the verification banner should happen server-side, in conn.assigns?
If we pass this knowledge server-side, the user refreshing the dashboard while verification is happening would clear the verification banner (unless the verification is forced on them by server because they don't have stats_start_date) - they'd have to click browser back and click verify again to see it again.
There was a problem hiding this comment.
suggestion, non-blocking: Can we avoid the verification being triggerable with users manually entering these URL params?
Could you elaborate on why? Is this about security, or more about code architecture/cleanliness?
I don't think there's an actual security concern here -- verify_installation? already requires an authenticated user with access to that specific site, the check itself is rate-limited, and "re-verification" for old sites is already reachable through Settings -> Review installation anyway. Also, already triggered by navigation (the /:domain/verification route), so I don't think doing that based on a query parameter on the dashboard exposes any new vulnerability.
Because the redirect is handled by the server, maybe we can pass the fact that the verification banner should happen server-side, in conn.assigns?
conn.assigns is just a map scoped to a single request/response cycle — it doesn't survive a redirect at all, so it can't do this. Conn.put_session could keep the trigger hidden from the user instead (or Phoenix.Controller.put_flash to limit it to a single page load).
put_session could technically work, but it would get tricky — LiveView sockets can't write to the session directly, so both submit action on the Installation LV (to set it) and the dismiss action on the Verification banner LV (to clear it) would need to go through controller-level round-trips. Not worth the hustle I think.
If we pass this knowledge server-side, the user refreshing the dashboard while verification is happening would clear the verification banner (unless the verification is forced on them by server because they don't have stats_start_date) - they'd have to click browser back and click verify again to see it again
I don't think limiting verification to a single page load is good UX — I imagine many users would have the intuition to refresh the page to re-trigger a failed verification (instead of clicking "Check again"). So I'd leave verification visible until the user decides to dismiss it, OR once verification succeeds and the installation is confirmed working — that's the point at which we'd hide the banner on a refresh.
Also, the "server-side forcing verification" part is exactly what I wanted to avoid — the user should be able to dismiss it and not see it again even after a page refresh.
There was a problem hiding this comment.
Thanks for the detailed reply!
The concern is security. It's in the same broader category that it shouldn't be possible to build links that delete site or user when people click them. Basically, any mutation should need a user interaction. Verification isn't a mutation, but it's close.
Regarding implementation, thanks for considering it, looks like put_flash would be the way to do it, not ideal but not entirely custom either.
You're right to point out that it's already possible to build links that trigger verification so this PR wouldn't expose anything new, but it makes previously safe-looking links unsafe: https://plausible.io/plausible.io?f=is,page,/blog&f=is,source,Direct%20/%20None&f=is,screen,Desktop&verify_installation=true&flow=domain_change&f=is,os,Windows&f=is,browser,Firefox&f=is,region,DE-HE&f=is,country,DE&l=DE-HE,Hesse&l=DE,Germany
Also, the "server-side forcing verification" part is exactly what I wanted to avoid — the user should be able to dismiss it and not see it again even after a page refresh.
Agreed, dismiss should dismiss. Should be possible with flash.
Regarding users potentially refreshing the page and losing the server side grant to verification, you're probably right. But then they can press the back button and click whatever they clicked to start verification and it will restart. Some tradeoffs are to be expected.
There was a problem hiding this comment.
Anyways, this is non-blocking, just wanted to put this option on the table
* Refine verification banner UI and copy - Simplify failure CTAs: keep "Check again" as primary, add ghost "Review installation" (or "Try another URL" when a custom URL retry is offered); remove installation-guide, change-method, and view-snippet expandable logic - Render inline "verify your installation manually" / "review your installation" links inside recommendation text (offer_custom_url_input scenarios) via safe HTML helpers, avoiding HEEx whitespace pitfalls - Tighten diagnostics title and body copy - Move the "Setup pending" pill in the sites list to replace the percentage indicator; use the generic pill component - Extend the notice component with title_class override and a spinner slot; swap success icon to solid check-circle * improve templating logic --------- Co-authored-by: Robert Joonas <robertjoonas16@gmail.com>
3 participants
Changes
Please describe the changes made in the pull request here.
Below you'll find a checklist. For each item on the list, check one option and delete the other.
Tests
Changelog
Documentation
Dark mode