Skip to content
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

Allow configuring AuthorizeSiteAccess plug site param #4597

Merged
merged 12 commits into from
Sep 26, 2024

Conversation

apata
Copy link
Contributor

@apata apata commented Sep 19, 2024

Changes

  • Allow auth site access plug to specify that domain is found in request body at some key, needed for api/docs/query endpoint where :domain or :website does not appear in path params
  • Nuisance: Stop typescript --watch command clearing previous server stdout when running make server

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

  • This PR does not change the UI

@apata apata force-pushed the auth-plug-branch-where-to-find-domain branch from 348e4c1 to 8907b2a Compare September 19, 2024 09:44
end

defp get_site_with_role(conn, current_user, conn_params_domain_accessor) do
domain = get_domain_from_conn(conn, conn_params_domain_accessor)
Copy link
Contributor

Choose a reason for hiding this comment

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

When the param is not provided at all (site_id, for instance), the plug will crash when executing the query, AFAICT. Is site_id optional here? If not, perhaps we should return 404, like in other cases?

lib/plausible_web/plugs/authorize_site_access.ex Outdated Show resolved Hide resolved
@@ -12,7 +12,7 @@ defmodule PlausibleWeb.Plugs.AuthorizeSiteAccess do

def init([]), do: @all_roles

def init(allowed_roles) do
def init(allowed_roles) when is_list(allowed_roles) do
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

lib/plausible_web/plugs/authorize_site_access.ex Outdated Show resolved Hide resolved
@apata apata force-pushed the auth-plug-branch-where-to-find-domain branch from 8907b2a to 76085fa Compare September 19, 2024 16:51
@zoldar zoldar changed the base branch from master to unify-domain-website September 20, 2024 12:36
@zoldar zoldar force-pushed the auth-plug-branch-where-to-find-domain branch from f4914ed to 61be182 Compare September 26, 2024 08:55
@zoldar zoldar changed the base branch from unify-domain-website to master September 26, 2024 09:01
@zoldar zoldar added the preview label Sep 26, 2024
Copy link

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

@zoldar zoldar changed the title Allow auth site access plug to specify that domain is found in request body at some key Allow configuring AuthorizeSiteAccess plug site param Sep 26, 2024
@zoldar zoldar added this pull request to the merge queue Sep 26, 2024
Merged via the queue into master with commit cac4ad2 Sep 26, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants