From 4e0e64e2aa9c99e1562e1546c51b04b56d251a5f Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Tue, 13 Dec 2022 15:01:40 -1000 Subject: [PATCH 01/20] feat(acls): basic permission check implmentation, needs refactoring --- .../controllers/mention_controller.ex | 4 +++- .../errors/custom_errors.ex | 18 ++++++++++++++++ lib/epochtalk_server_web/helpers/acl.ex | 21 +++++++++++++++++++ lib/epochtalk_server_web/router.ex | 11 ++++++++-- 4 files changed, 51 insertions(+), 3 deletions(-) create mode 100644 lib/epochtalk_server_web/helpers/acl.ex diff --git a/lib/epochtalk_server_web/controllers/mention_controller.ex b/lib/epochtalk_server_web/controllers/mention_controller.ex index 7bc1a3f1..376f41d4 100644 --- a/lib/epochtalk_server_web/controllers/mention_controller.ex +++ b/lib/epochtalk_server_web/controllers/mention_controller.ex @@ -8,14 +8,16 @@ defmodule EpochtalkServerWeb.MentionController do alias EpochtalkServer.Models.Mention alias EpochtalkServerWeb.ErrorHelpers alias EpochtalkServerWeb.Helpers.Validate + alias EpochtalkServerWeb.Helpers.ACL @doc """ Used to page `Mention` models for a specific `User` """ def page(conn, attrs) do - with page <- Validate.cast(attrs, "page", :integer, min: 1), + with page <- Validate.cast(attrs, "page", :integer, min: 1), limit <- Validate.cast(attrs, "limit", :integer, min: 1), extended <- Validate.cast(attrs, "extended", :boolean), + :ok <- ACL.allow(conn, "mentions.page"), {:auth, user} <- {:auth, Guardian.Plug.current_resource(conn)}, {:ok, mentions, data} <- Mention.page_by_user_id(user.id, page, per_page: limit, extended: extended), diff --git a/lib/epochtalk_server_web/errors/custom_errors.ex b/lib/epochtalk_server_web/errors/custom_errors.ex index 7c8baf12..478dee7e 100644 --- a/lib/epochtalk_server_web/errors/custom_errors.ex +++ b/lib/epochtalk_server_web/errors/custom_errors.ex @@ -1,4 +1,22 @@ defmodule EpochtalkServerWeb.CustomErrors do + # ACL Permissions + defmodule InvalidPermission do + @moduledoc """ + Exception raised when api request payload is incorrect + """ + @default_message "Forbidden, invalid permissions to perform this action" + defexception plug_status: 403, message: @default_message + + @impl true + def exception(value) do + case value do + [message: nil] -> %InvalidPermission{} + [message: message] -> %InvalidPermission{message: message} + _ -> %InvalidPermission{} + end + end + end + # API Payload Handling defmodule InvalidPayload do @moduledoc """ diff --git a/lib/epochtalk_server_web/helpers/acl.ex b/lib/epochtalk_server_web/helpers/acl.ex new file mode 100644 index 00000000..d4032908 --- /dev/null +++ b/lib/epochtalk_server_web/helpers/acl.ex @@ -0,0 +1,21 @@ +defmodule EpochtalkServerWeb.Helpers.ACL do + alias EpochtalkServer.Models.Role + alias EpochtalkServerWeb.CustomErrors.InvalidPermission + @moduledoc """ + Helper for checking authenticated user's permissions. + """ + + def allow(conn, permission_path), do: allow(conn, permission_path, nil) + def allow(conn, permission_path, error_msg) do + user = EpochtalkServer.Auth.Guardian.Plug.current_resource(conn) + authed_permissions = Role.get_masked_permissions(user.roles) + path_list = String.split(permission_path, ".") # convert path to array + path_list = if tl(path_list) != "allow", # append "allow" to array + do: path_list ++ ["allow"], + else: path_list + has_permission = Kernel.get_in(authed_permissions, path_list) + if has_permission, + do: :ok, + else: raise(InvalidPermission, message: error_msg) + end +end diff --git a/lib/epochtalk_server_web/router.ex b/lib/epochtalk_server_web/router.ex index 572ec949..2b035664 100644 --- a/lib/epochtalk_server_web/router.ex +++ b/lib/epochtalk_server_web/router.ex @@ -15,16 +15,23 @@ defmodule EpochtalkServerWeb.Router do end pipeline :enforce_auth do + plug Guardian.Plug.Pipeline, + module: EpochtalkServer.Auth.Guardian, + error_handler: EpochtalkServerWeb.GuardianErrorHandler + + plug Guardian.Plug.VerifyHeader, claims: %{"typ" => "access"} + plug Guardian.Plug.LoadResource, allow_blank: false plug Guardian.Plug.EnsureAuthenticated end + scope "/api", EpochtalkServerWeb do - pipe_through [:api, :maybe_auth, :enforce_auth] - get "/authenticate", UserController, :authenticate + pipe_through [:api, :enforce_auth] get "/users/preferences", PreferenceController, :preferences get "/mentions", MentionController, :page get "/notifications/counts", NotificationController, :counts post "/notifications/dismiss", NotificationController, :dismiss + get "/authenticate", UserController, :authenticate end scope "/api", EpochtalkServerWeb do From 2b45aa733af5e9483fcdf2702f990a5cea3c13b5 Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Thu, 15 Dec 2022 10:16:54 -1000 Subject: [PATCH 02/20] refactor(todo): add todo for session --- lib/epochtalk_server/auth/guardian.ex | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/epochtalk_server/auth/guardian.ex b/lib/epochtalk_server/auth/guardian.ex index 3bbb43af..1b8b8c4a 100644 --- a/lib/epochtalk_server/auth/guardian.ex +++ b/lib/epochtalk_server/auth/guardian.ex @@ -145,6 +145,8 @@ defmodule EpochtalkServer.Auth.Guardian do @doc """ Fetches the resource that is represented by claims. For JWT this would normally be found in the `sub` field. + + TODO(boka): refactor fetching of user session (L 161-202) into session.ex """ def resource_from_claims(%{"sub" => sub, "jti" => jti} = _claims) do # Here we'll look up our resource from the claims, the subject can be From dbc2a3a2822c97b71f9b38bf057ca374b397e1a8 Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Thu, 15 Dec 2022 10:17:37 -1000 Subject: [PATCH 03/20] refactor(notifications): implement permission check in notifications --- .../controllers/notification_controller.ex | 44 +++++++++++++------ 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/lib/epochtalk_server_web/controllers/notification_controller.ex b/lib/epochtalk_server_web/controllers/notification_controller.ex index 68050be2..d2e8db8f 100644 --- a/lib/epochtalk_server_web/controllers/notification_controller.ex +++ b/lib/epochtalk_server_web/controllers/notification_controller.ex @@ -8,26 +8,44 @@ defmodule EpochtalkServerWeb.NotificationController do alias EpochtalkServer.Models.Notification alias EpochtalkServerWeb.ErrorHelpers alias EpochtalkServerWeb.Helpers.Validate + alias EpochtalkServerWeb.Helpers.ACL @doc """ Used to retrieve `Notification` counts for a specific `User` """ def counts(conn, attrs) do - with max <- Validate.cast(attrs, "max", :integer, min: 1), - {:auth, user} <- {:auth, Guardian.Plug.current_resource(conn)}, - do: - render(conn, "counts.json", - data: Notification.counts_by_user_id(user.id, max: max || 99) - ), - else: - ({:auth, nil} -> - ErrorHelpers.render_json_error( - conn, - 400, - "Not logged in, cannot fetch notification counts" - )) + with {:auth, user} <- {:auth, Guardian.Plug.current_resource(conn)}, + :ok <- ACL.allow(conn, "notifications.counts"), + # {:ok, :allow} <- Authorization.grant(conn, counts_auth), + max <- Validate.cast(attrs, "max", :integer, min: 1) do + render(conn, "counts.json", data: Notification.counts_by_user_id(user.id, max: max || 99)) + else + {:auth, nil} -> + ErrorHelpers.render_json_error( + conn, + 400, + "Not logged in, cannot fetch notification counts" + ) + + {:access, false} -> + ErrorHelpers.render_json_error( + conn, + 400, + "Not logged in, cannot fetch notification counts" + ) + end end + # defp counts_auth() do + # one = Task.one("some error one") + # two = Task.two("some error two") + + # case Run.tasks(one, two) do + # :ok -> {:ok, :allow} + # {:error, msg} -> throw(msg) + # end + # end + @doc """ Used to dismiss `Notification` counts for a specific `User` """ From bff0f083c8263a5aacebc8c8d0f8ec2e8a5911d0 Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Thu, 15 Dec 2022 11:06:48 -1000 Subject: [PATCH 04/20] fix(acls): issue with acls call from notifications --- lib/epochtalk_server_web/controllers/mention_controller.ex | 4 ++-- .../controllers/notification_controller.ex | 2 +- lib/epochtalk_server_web/errors/custom_errors.ex | 1 + lib/epochtalk_server_web/router.ex | 1 - 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/epochtalk_server_web/controllers/mention_controller.ex b/lib/epochtalk_server_web/controllers/mention_controller.ex index 376f41d4..ac1994a8 100644 --- a/lib/epochtalk_server_web/controllers/mention_controller.ex +++ b/lib/epochtalk_server_web/controllers/mention_controller.ex @@ -14,10 +14,10 @@ defmodule EpochtalkServerWeb.MentionController do Used to page `Mention` models for a specific `User` """ def page(conn, attrs) do - with page <- Validate.cast(attrs, "page", :integer, min: 1), + with page <- Validate.cast(attrs, "page", :integer, min: 1), limit <- Validate.cast(attrs, "limit", :integer, min: 1), extended <- Validate.cast(attrs, "extended", :boolean), - :ok <- ACL.allow(conn, "mentions.page"), + :ok <- ACL.allow!(conn, "mentions.page"), {:auth, user} <- {:auth, Guardian.Plug.current_resource(conn)}, {:ok, mentions, data} <- Mention.page_by_user_id(user.id, page, per_page: limit, extended: extended), diff --git a/lib/epochtalk_server_web/controllers/notification_controller.ex b/lib/epochtalk_server_web/controllers/notification_controller.ex index d2e8db8f..f3d61eb8 100644 --- a/lib/epochtalk_server_web/controllers/notification_controller.ex +++ b/lib/epochtalk_server_web/controllers/notification_controller.ex @@ -15,7 +15,7 @@ defmodule EpochtalkServerWeb.NotificationController do """ def counts(conn, attrs) do with {:auth, user} <- {:auth, Guardian.Plug.current_resource(conn)}, - :ok <- ACL.allow(conn, "notifications.counts"), + :ok <- ACL.allow!(conn, "notifications.counts"), # {:ok, :allow} <- Authorization.grant(conn, counts_auth), max <- Validate.cast(attrs, "max", :integer, min: 1) do render(conn, "counts.json", data: Notification.counts_by_user_id(user.id, max: max || 99)) diff --git a/lib/epochtalk_server_web/errors/custom_errors.ex b/lib/epochtalk_server_web/errors/custom_errors.ex index 478dee7e..dcb2905a 100644 --- a/lib/epochtalk_server_web/errors/custom_errors.ex +++ b/lib/epochtalk_server_web/errors/custom_errors.ex @@ -1,5 +1,6 @@ defmodule EpochtalkServerWeb.CustomErrors do # ACL Permissions + # credo:disable-for-next-line defmodule InvalidPermission do @moduledoc """ Exception raised when api request payload is incorrect diff --git a/lib/epochtalk_server_web/router.ex b/lib/epochtalk_server_web/router.ex index 2b035664..e9928139 100644 --- a/lib/epochtalk_server_web/router.ex +++ b/lib/epochtalk_server_web/router.ex @@ -24,7 +24,6 @@ defmodule EpochtalkServerWeb.Router do plug Guardian.Plug.EnsureAuthenticated end - scope "/api", EpochtalkServerWeb do pipe_through [:api, :enforce_auth] get "/users/preferences", PreferenceController, :preferences From 3539c0226b96d2b2d214bfd94b8d03ddb41b9eff Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Thu, 15 Dec 2022 12:10:46 -1000 Subject: [PATCH 05/20] feat(acls): add acl to notification dismiss --- lib/epochtalk_server_web/controllers/notification_controller.ex | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/epochtalk_server_web/controllers/notification_controller.ex b/lib/epochtalk_server_web/controllers/notification_controller.ex index f3d61eb8..75268c9f 100644 --- a/lib/epochtalk_server_web/controllers/notification_controller.ex +++ b/lib/epochtalk_server_web/controllers/notification_controller.ex @@ -51,6 +51,7 @@ defmodule EpochtalkServerWeb.NotificationController do """ def dismiss(conn, %{"id" => id}) do with {:auth, user} <- {:auth, Guardian.Plug.current_resource(conn)}, + :ok <- ACL.allow!(conn, "notifications.dismiss"), {_count, nil} <- Notification.dismiss(id) do EpochtalkServerWeb.Endpoint.broadcast("user:#{user.id}", "refreshMentions", %{}) render(conn, "dismiss.json", data: %{success: true}) From f767b77f4aabe69a23c48b1d6b1846f3ba42009e Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Thu, 15 Dec 2022 14:42:28 -1000 Subject: [PATCH 06/20] refactor(authenticate-test): update authenticate test error message, now that authenticate is under the :ensure_auth pipeline --- test/epochtalk_server_web/controllers/user_controller_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/epochtalk_server_web/controllers/user_controller_test.exs b/test/epochtalk_server_web/controllers/user_controller_test.exs index b5779014..1afc469a 100644 --- a/test/epochtalk_server_web/controllers/user_controller_test.exs +++ b/test/epochtalk_server_web/controllers/user_controller_test.exs @@ -263,7 +263,7 @@ defmodule EpochtalkServerWeb.UserControllerTest do test "errors with 401 when user is not logged in but trying to authenticate", %{conn: conn} do conn = get(conn, Routes.user_path(conn, :authenticate)) - assert %{"error" => "Unauthorized", "message" => "Unauthenticated"} = + assert %{"error" => "Unauthorized", "message" => "No resource found", "status" => 401} = json_response(conn, 401) end end From 1f903aa9a058f496aded7c38ba4485acb8b145a0 Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Thu, 15 Dec 2022 14:43:51 -1000 Subject: [PATCH 07/20] refactor(acls): add docs and update allow! function in ACL module --- lib/epochtalk_server_web/helpers/acl.ex | 56 ++++++++++++++++++++----- 1 file changed, 45 insertions(+), 11 deletions(-) diff --git a/lib/epochtalk_server_web/helpers/acl.ex b/lib/epochtalk_server_web/helpers/acl.ex index d4032908..8680589d 100644 --- a/lib/epochtalk_server_web/helpers/acl.ex +++ b/lib/epochtalk_server_web/helpers/acl.ex @@ -1,21 +1,55 @@ defmodule EpochtalkServerWeb.Helpers.ACL do alias EpochtalkServer.Models.Role + alias EpochtalkServer.Models.User alias EpochtalkServerWeb.CustomErrors.InvalidPermission + @moduledoc """ Helper for checking authenticated user's permissions. """ - def allow(conn, permission_path), do: allow(conn, permission_path, nil) - def allow(conn, permission_path, error_msg) do - user = EpochtalkServer.Auth.Guardian.Plug.current_resource(conn) - authed_permissions = Role.get_masked_permissions(user.roles) - path_list = String.split(permission_path, ".") # convert path to array - path_list = if tl(path_list) != "allow", # append "allow" to array - do: path_list ++ ["allow"], - else: path_list + @doc """ + Checks if authenticated `User` is allowed to perform a specific action. + + Compares a mask all of user's roles into a masked permission set and checks + if that permission set contains the specified `permission_path`. + + Raises `CustomErrors.InvalidPermission` exception if the `User` does not have + the proper permissions. + + TODO(akinsey): figure out how to typespec this function. The requirement that + dialyzer has to use a valid authenticated connection, is preventing it from reaching + `:ok`. Currently when spec is added, dialyzer will only reach the `raise` + """ + def allow!(%Plug.Conn{} = conn, permission_path), do: allow!(conn, permission_path, nil) + + def allow!(%User{} = user, permission_path), do: allow!(%Plug.Conn{private: %{guardian_default_resource: user}}, permission_path, nil) + + @doc """ + Same as `ACL.allow!/2` but allows a custom error message to be raised if the + `User` does not have the proper permissions. + """ + def allow!( + %Plug.Conn{private: %{guardian_default_resource: user}} = _conn, + permission_path, + error_msg + ) do + authed_permissions = Role.get_masked_permissions(user.roles || []) + # convert path to array + path_list = String.split(permission_path, ".") + # append "allow" to array + path_list = + if List.last(path_list) != "allow", + do: path_list ++ ["allow"], + else: path_list + has_permission = Kernel.get_in(authed_permissions, path_list) - if has_permission, - do: :ok, - else: raise(InvalidPermission, message: error_msg) + if !has_permission, do: raise(InvalidPermission, message: error_msg) + :ok end + + def allow!( + %User{} = user, + permission_path, + error_msg + ), do: allow!(%Plug.Conn{private: %{guardian_default_resource: user}}, permission_path, error_msg) end From d034e5dcc143b2f870b87a9f8d7a3d665eba4f27 Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Thu, 15 Dec 2022 14:44:39 -1000 Subject: [PATCH 08/20] refactor(acl): run mix format --- lib/epochtalk_server_web/helpers/acl.ex | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/epochtalk_server_web/helpers/acl.ex b/lib/epochtalk_server_web/helpers/acl.ex index 8680589d..97688299 100644 --- a/lib/epochtalk_server_web/helpers/acl.ex +++ b/lib/epochtalk_server_web/helpers/acl.ex @@ -22,7 +22,8 @@ defmodule EpochtalkServerWeb.Helpers.ACL do """ def allow!(%Plug.Conn{} = conn, permission_path), do: allow!(conn, permission_path, nil) - def allow!(%User{} = user, permission_path), do: allow!(%Plug.Conn{private: %{guardian_default_resource: user}}, permission_path, nil) + def allow!(%User{} = user, permission_path), + do: allow!(%Plug.Conn{private: %{guardian_default_resource: user}}, permission_path, nil) @doc """ Same as `ACL.allow!/2` but allows a custom error message to be raised if the @@ -51,5 +52,11 @@ defmodule EpochtalkServerWeb.Helpers.ACL do %User{} = user, permission_path, error_msg - ), do: allow!(%Plug.Conn{private: %{guardian_default_resource: user}}, permission_path, error_msg) + ), + do: + allow!( + %Plug.Conn{private: %{guardian_default_resource: user}}, + permission_path, + error_msg + ) end From 93eca88442af98c778039d8e9d4c427db7da5f58 Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Fri, 16 Dec 2022 14:13:38 -1000 Subject: [PATCH 09/20] refactor(todo-acls): add todo for acls --- lib/epochtalk_server_web/helpers/acl.ex | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/epochtalk_server_web/helpers/acl.ex b/lib/epochtalk_server_web/helpers/acl.ex index 97688299..47507303 100644 --- a/lib/epochtalk_server_web/helpers/acl.ex +++ b/lib/epochtalk_server_web/helpers/acl.ex @@ -28,6 +28,10 @@ defmodule EpochtalkServerWeb.Helpers.ACL do @doc """ Same as `ACL.allow!/2` but allows a custom error message to be raised if the `User` does not have the proper permissions. + + TODO(akinsey): implement logic to handle if connection is not authenticated. + Default to `private` role if `frontend_configs.login_required` is enabled, otherwise + default to `anonymous` role. """ def allow!( %Plug.Conn{private: %{guardian_default_resource: user}} = _conn, From edb9cfe6cb60242bd0bc10efd2995157d81e81d5 Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Fri, 16 Dec 2022 14:14:29 -1000 Subject: [PATCH 10/20] test(acl): implement test for existing acl implementation --- .../epochtalk_server_web/helpers/acl_test.exs | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 test/epochtalk_server_web/helpers/acl_test.exs diff --git a/test/epochtalk_server_web/helpers/acl_test.exs b/test/epochtalk_server_web/helpers/acl_test.exs new file mode 100644 index 00000000..1bd56c8e --- /dev/null +++ b/test/epochtalk_server_web/helpers/acl_test.exs @@ -0,0 +1,54 @@ +defmodule EpochtalkServerWeb.ACLTest do + use EpochtalkServerWeb.ConnCase, async: true + alias EpochtalkServerWeb.Helpers.ACL + alias EpochtalkServerWeb.CustomErrors.InvalidPermission + + describe "allow!/2" do + @tag :authenticated + test "returns :ok with valid 'User' role permissions", %{user: user} do + assert ACL.allow!(user, "boards.allCategories") == :ok + assert ACL.allow!(user, "posts.create") == :ok + assert ACL.allow!(user, "threads.create") == :ok + assert ACL.allow!(user, "boards.allCategories.allow") == :ok + assert ACL.allow!(user, "posts.create.allow") == :ok + assert ACL.allow!(user, "threads.create.allow") == :ok + end + + @tag :authenticated + test "raises 'InvalidPermissions' error with invalid 'User' role permissions", %{user: user} do + assert_raise InvalidPermission, + ~r/^Forbidden, invalid permissions to perform this action/, + fn -> + ACL.allow!(user, "roles.update") + end + + assert_raise InvalidPermission, + ~r/^Forbidden, invalid permissions to perform this action/, + fn -> + ACL.allow!(user, "roles.create") + end + + assert_raise InvalidPermission, + ~r/^Forbidden, invalid permissions to perform this action/, + fn -> + ACL.allow!(user, "adminSettings.find") + end + end + + @tag :authenticated + test "raises 'InvalidPermissions' error with custom message with invalid 'User' role permissions", + %{user: user} do + assert_raise InvalidPermission, ~r/^You cannot update roles/, fn -> + ACL.allow!(user, "roles.update", "You cannot update roles") + end + + assert_raise InvalidPermission, ~r/^You cannot create roles/, fn -> + ACL.allow!(user, "roles.create", "You cannot create roles") + end + + assert_raise InvalidPermission, ~r/^You cannot fetch admin settings/, fn -> + ACL.allow!(user, "adminSettings.find", "You cannot fetch admin settings") + end + end + end +end From 5b2a983adcf8bd95cd7149b01b31fd223a33100e Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Tue, 17 Jan 2023 14:23:52 -1000 Subject: [PATCH 11/20] refactor: wip default permissions for acls check, need to test still --- lib/epochtalk_server_web/helpers/acl.ex | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/epochtalk_server_web/helpers/acl.ex b/lib/epochtalk_server_web/helpers/acl.ex index 47507303..98bd49d7 100644 --- a/lib/epochtalk_server_web/helpers/acl.ex +++ b/lib/epochtalk_server_web/helpers/acl.ex @@ -38,7 +38,16 @@ defmodule EpochtalkServerWeb.Helpers.ACL do permission_path, error_msg ) do - authed_permissions = Role.get_masked_permissions(user.roles || []) + # check if login is required to view forum + config = Application.get_env(:epochtalk_server, :frontend_config) + login_required = config["login_required"] + + # default to user's roles > anonymous > private + user_roles = if user == nil, + do: (if login_required, do: [Role.by_lookup("private")], else: [Role.by_lookup("anonymous")]), + else: user.roles + + authed_permissions = Role.get_masked_permissions(user_roles) # convert path to array path_list = String.split(permission_path, ".") # append "allow" to array From 6319faa0128a5a8a2d33b73f32b8f494e72a91da Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Thu, 19 Jan 2023 11:22:11 -1000 Subject: [PATCH 12/20] feat: acls, default to anonymous or private if user is not authenticated --- lib/epochtalk_server_web/helpers/acl.ex | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/lib/epochtalk_server_web/helpers/acl.ex b/lib/epochtalk_server_web/helpers/acl.ex index 98bd49d7..e956b66a 100644 --- a/lib/epochtalk_server_web/helpers/acl.ex +++ b/lib/epochtalk_server_web/helpers/acl.ex @@ -43,11 +43,14 @@ defmodule EpochtalkServerWeb.Helpers.ACL do login_required = config["login_required"] # default to user's roles > anonymous > private - user_roles = if user == nil, - do: (if login_required, do: [Role.by_lookup("private")], else: [Role.by_lookup("anonymous")]), - else: user.roles + user_roles = + if user == nil, + do: + if(login_required, do: [Role.by_lookup("private")], else: [Role.by_lookup("anonymous")]), + else: user.roles authed_permissions = Role.get_masked_permissions(user_roles) + # convert path to array path_list = String.split(permission_path, ".") # append "allow" to array @@ -61,6 +64,18 @@ defmodule EpochtalkServerWeb.Helpers.ACL do :ok end + def allow!( + %Plug.Conn{} = _conn, + permission_path, + error_msg + ), + do: + allow!( + %Plug.Conn{private: %{guardian_default_resource: nil}}, + permission_path, + error_msg + ) + def allow!( %User{} = user, permission_path, From 4116cd09c7713adb19c8eca934272c956684dd94 Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Thu, 19 Jan 2023 12:08:39 -1000 Subject: [PATCH 13/20] feat: add function spec for acls, guard clauses --- lib/epochtalk_server_web/helpers/acl.ex | 28 +++++++++++++------------ 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/lib/epochtalk_server_web/helpers/acl.ex b/lib/epochtalk_server_web/helpers/acl.ex index e956b66a..9469a42f 100644 --- a/lib/epochtalk_server_web/helpers/acl.ex +++ b/lib/epochtalk_server_web/helpers/acl.ex @@ -15,29 +15,29 @@ defmodule EpochtalkServerWeb.Helpers.ACL do Raises `CustomErrors.InvalidPermission` exception if the `User` does not have the proper permissions. - - TODO(akinsey): figure out how to typespec this function. The requirement that - dialyzer has to use a valid authenticated connection, is preventing it from reaching - `:ok`. Currently when spec is added, dialyzer will only reach the `raise` """ - def allow!(%Plug.Conn{} = conn, permission_path), do: allow!(conn, permission_path, nil) + @spec allow!(Plug.Conn.t() | User.t(), permission_path :: String.t()) :: no_return | :ok + def allow!(%Plug.Conn{} = conn, permission_path) when is_binary(permission_path), + do: allow!(conn, permission_path, nil) - def allow!(%User{} = user, permission_path), + def allow!(%User{} = user, permission_path) when is_binary(permission_path), do: allow!(%Plug.Conn{private: %{guardian_default_resource: user}}, permission_path, nil) @doc """ Same as `ACL.allow!/2` but allows a custom error message to be raised if the `User` does not have the proper permissions. - - TODO(akinsey): implement logic to handle if connection is not authenticated. - Default to `private` role if `frontend_configs.login_required` is enabled, otherwise - default to `anonymous` role. """ + @spec allow!( + %Plug.Conn{} | User.t(), + permission_path :: String.t(), + error_msg :: String.t() | nil + ) :: no_return | :ok def allow!( %Plug.Conn{private: %{guardian_default_resource: user}} = _conn, permission_path, error_msg - ) do + ) + when is_binary(permission_path) do # check if login is required to view forum config = Application.get_env(:epochtalk_server, :frontend_config) login_required = config["login_required"] @@ -68,7 +68,8 @@ defmodule EpochtalkServerWeb.Helpers.ACL do %Plug.Conn{} = _conn, permission_path, error_msg - ), + ) + when is_binary(permission_path) and is_binary(error_msg), do: allow!( %Plug.Conn{private: %{guardian_default_resource: nil}}, @@ -80,7 +81,8 @@ defmodule EpochtalkServerWeb.Helpers.ACL do %User{} = user, permission_path, error_msg - ), + ) + when is_binary(permission_path) and is_binary(error_msg), do: allow!( %Plug.Conn{private: %{guardian_default_resource: user}}, From 4279f9827233d11a1fc6bd6d9e5022d03267826a Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Thu, 19 Jan 2023 14:41:16 -1000 Subject: [PATCH 14/20] refactor(acl): clean up and refactor acl allow functions --- lib/epochtalk_server_web/helpers/acl.ex | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/epochtalk_server_web/helpers/acl.ex b/lib/epochtalk_server_web/helpers/acl.ex index 9469a42f..18151d7a 100644 --- a/lib/epochtalk_server_web/helpers/acl.ex +++ b/lib/epochtalk_server_web/helpers/acl.ex @@ -65,27 +65,27 @@ defmodule EpochtalkServerWeb.Helpers.ACL do end def allow!( - %Plug.Conn{} = _conn, + %User{} = user, permission_path, error_msg ) - when is_binary(permission_path) and is_binary(error_msg), + when (is_binary(permission_path) and is_binary(error_msg)) or is_nil(error_msg), do: allow!( - %Plug.Conn{private: %{guardian_default_resource: nil}}, + %Plug.Conn{private: %{guardian_default_resource: user}}, permission_path, error_msg ) def allow!( - %User{} = user, + _, permission_path, error_msg ) - when is_binary(permission_path) and is_binary(error_msg), + when (is_binary(permission_path) and is_binary(error_msg)) or is_nil(error_msg), do: allow!( - %Plug.Conn{private: %{guardian_default_resource: user}}, + %Plug.Conn{private: %{guardian_default_resource: nil}}, permission_path, error_msg ) From 1c25af3147ba20a67325340e59ec84a94ea95018 Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Thu, 19 Jan 2023 15:43:29 -1000 Subject: [PATCH 15/20] test(acl): implement more detailed testing --- .../epochtalk_server_web/helpers/acl_test.exs | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/test/epochtalk_server_web/helpers/acl_test.exs b/test/epochtalk_server_web/helpers/acl_test.exs index 1bd56c8e..917a5dd0 100644 --- a/test/epochtalk_server_web/helpers/acl_test.exs +++ b/test/epochtalk_server_web/helpers/acl_test.exs @@ -14,6 +14,20 @@ defmodule EpochtalkServerWeb.ACLTest do assert ACL.allow!(user, "threads.create.allow") == :ok end + test "raises 'InvalidPermissions' error with unauthenticated connection" do + assert_raise InvalidPermission, + ~r/^Forbidden, invalid permissions to perform this action/, + fn -> + ACL.allow!(%Plug.Conn{}, "posts.create") + end + + assert_raise InvalidPermission, + ~r/^Forbidden, invalid permissions to perform this action/, + fn -> + ACL.allow!(%Plug.Conn{}, "threads.create") + end + end + @tag :authenticated test "raises 'InvalidPermissions' error with invalid 'User' role permissions", %{user: user} do assert_raise InvalidPermission, @@ -34,6 +48,31 @@ defmodule EpochtalkServerWeb.ACLTest do ACL.allow!(user, "adminSettings.find") end end + end + + describe "allow!/3" do + @tag :authenticated + test "returns :ok with valid 'User' role permissions", %{user: user} do + assert ACL.allow!(user, "boards.allCategories", "You cannot query all categories") == :ok + assert ACL.allow!(user, "posts.create", "You cannot create posts") == :ok + assert ACL.allow!(user, "threads.create", "You cannot create threads") == :ok + + assert ACL.allow!(user, "boards.allCategories.allow", "You cannot query all categories") == + :ok + + assert ACL.allow!(user, "posts.create.allow", "You cannot create posts") == :ok + assert ACL.allow!(user, "threads.create.allow", "You cannot create threads") == :ok + end + + test "raises 'InvalidPermissions' error with unauthenticated connection" do + assert_raise InvalidPermission, ~r/^You cannot create posts/, fn -> + ACL.allow!(%Plug.Conn{}, "posts.create", "You cannot create posts") + end + + assert_raise InvalidPermission, ~r/^You cannot create threads/, fn -> + ACL.allow!(%Plug.Conn{}, "threads.create", "You cannot create threads") + end + end @tag :authenticated test "raises 'InvalidPermissions' error with custom message with invalid 'User' role permissions", From 31aec07ef98a9cf6ef4f3c652ee7413be504bda0 Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Fri, 20 Jan 2023 11:27:52 -1000 Subject: [PATCH 16/20] fix: credo issue with spec and bad code for login required --- lib/epochtalk_server_web/helpers/acl.ex | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/epochtalk_server_web/helpers/acl.ex b/lib/epochtalk_server_web/helpers/acl.ex index 18151d7a..e09d0fed 100644 --- a/lib/epochtalk_server_web/helpers/acl.ex +++ b/lib/epochtalk_server_web/helpers/acl.ex @@ -28,7 +28,7 @@ defmodule EpochtalkServerWeb.Helpers.ACL do `User` does not have the proper permissions. """ @spec allow!( - %Plug.Conn{} | User.t(), + Plug.Conn.t() | User.t() | any(), permission_path :: String.t(), error_msg :: String.t() | nil ) :: no_return | :ok @@ -40,8 +40,7 @@ defmodule EpochtalkServerWeb.Helpers.ACL do when is_binary(permission_path) do # check if login is required to view forum config = Application.get_env(:epochtalk_server, :frontend_config) - login_required = config["login_required"] - + login_required = config[:login_required] # default to user's roles > anonymous > private user_roles = if user == nil, From 0ecd95dde94433ee5f294a02314ec7f972f0dcbc Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Fri, 20 Jan 2023 14:41:32 -1000 Subject: [PATCH 17/20] tests(acl): add tests to check that the anonymous and private roles are appropriately assigned when unauthenticated --- .../epochtalk_server_web/helpers/acl_test.exs | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/test/epochtalk_server_web/helpers/acl_test.exs b/test/epochtalk_server_web/helpers/acl_test.exs index 917a5dd0..9e3bb5cf 100644 --- a/test/epochtalk_server_web/helpers/acl_test.exs +++ b/test/epochtalk_server_web/helpers/acl_test.exs @@ -14,6 +14,43 @@ defmodule EpochtalkServerWeb.ACLTest do assert ACL.allow!(user, "threads.create.allow") == :ok end + test "defaults to 'anonymous' role if not authenticated", %{conn: conn} do + assert ACL.allow!(conn, "boards.allCategories") == :ok + assert ACL.allow!(conn, "posts.byThread") == :ok + + assert_raise InvalidPermission, + ~r/^Forbidden, invalid permissions to perform this action/, + fn -> + ACL.allow!(%Plug.Conn{}, "posts.create") + end + end + + test "defaults to 'private' role if 'frontend_config.login_required' is true", %{conn: conn} do + frontend_config = + Application.get_env(:epochtalk_server, :frontend_config) + |> Map.put(:login_required, true) + + Application.put_env(:epochtalk_server, :frontend_config, frontend_config) + + assert_raise InvalidPermission, + ~r/^Forbidden, invalid permissions to perform this action/, + fn -> + ACL.allow!(conn, "boards.allCategories") + end + + assert_raise InvalidPermission, + ~r/^Forbidden, invalid permissions to perform this action/, + fn -> + ACL.allow!(conn, "posts.create") + end + + frontend_config = + Application.get_env(:epochtalk_server, :frontend_config) + |> Map.put(:login_required, false) + + Application.put_env(:epochtalk_server, :frontend_config, frontend_config) + end + test "raises 'InvalidPermissions' error with unauthenticated connection" do assert_raise InvalidPermission, ~r/^Forbidden, invalid permissions to perform this action/, @@ -74,6 +111,43 @@ defmodule EpochtalkServerWeb.ACLTest do end end + test "defaults to 'anonymous' role if not authenticated", %{conn: conn} do + assert ACL.allow!(conn, "boards.allCategories", "You cannot query all categories") == :ok + assert ACL.allow!(conn, "posts.byThread", "You cannot query threads") == :ok + + assert_raise InvalidPermission, + ~r/^You cannot create posts/, + fn -> + ACL.allow!(%Plug.Conn{}, "posts.create", "You cannot create posts") + end + end + + test "defaults to 'private' role if 'frontend_config.login_required' is true", %{conn: conn} do + frontend_config = + Application.get_env(:epochtalk_server, :frontend_config) + |> Map.put(:login_required, true) + + Application.put_env(:epochtalk_server, :frontend_config, frontend_config) + + assert_raise InvalidPermission, + ~r/^You cannot view categories/, + fn -> + ACL.allow!(conn, "boards.allCategories", "You cannot view categories") + end + + assert_raise InvalidPermission, + ~r/^You cannot create posts/, + fn -> + ACL.allow!(conn, "posts.create", "You cannot create posts") + end + + frontend_config = + Application.get_env(:epochtalk_server, :frontend_config) + |> Map.put(:login_required, false) + + Application.put_env(:epochtalk_server, :frontend_config, frontend_config) + end + @tag :authenticated test "raises 'InvalidPermissions' error with custom message with invalid 'User' role permissions", %{user: user} do From d368f858315b91c8de66e581ce5fddc5267e3167 Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Thu, 26 Jan 2023 12:10:49 -1000 Subject: [PATCH 18/20] tests(acls): make testing of acls more explicit --- test/epochtalk_server_web/helpers/acl_test.exs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/epochtalk_server_web/helpers/acl_test.exs b/test/epochtalk_server_web/helpers/acl_test.exs index 9e3bb5cf..282ba83f 100644 --- a/test/epochtalk_server_web/helpers/acl_test.exs +++ b/test/epochtalk_server_web/helpers/acl_test.exs @@ -14,9 +14,9 @@ defmodule EpochtalkServerWeb.ACLTest do assert ACL.allow!(user, "threads.create.allow") == :ok end - test "defaults to 'anonymous' role if not authenticated", %{conn: conn} do - assert ACL.allow!(conn, "boards.allCategories") == :ok - assert ACL.allow!(conn, "posts.byThread") == :ok + test "defaults to 'anonymous' role if not authenticated" do + assert ACL.allow!(%Plug.Conn{}, "boards.allCategories") == :ok + assert ACL.allow!(%Plug.Conn{}, "posts.byThread") == :ok assert_raise InvalidPermission, ~r/^Forbidden, invalid permissions to perform this action/, From 73b3fbaff012ae3a75a06569eac43b718e2ee420 Mon Sep 17 00:00:00 2001 From: Anthony Kinsey Date: Thu, 26 Jan 2023 12:14:27 -1000 Subject: [PATCH 19/20] tests(acls): make testing of acls more explicit for allowbrew list | less --- test/epochtalk_server_web/helpers/acl_test.exs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/epochtalk_server_web/helpers/acl_test.exs b/test/epochtalk_server_web/helpers/acl_test.exs index 282ba83f..f9056767 100644 --- a/test/epochtalk_server_web/helpers/acl_test.exs +++ b/test/epochtalk_server_web/helpers/acl_test.exs @@ -111,9 +111,9 @@ defmodule EpochtalkServerWeb.ACLTest do end end - test "defaults to 'anonymous' role if not authenticated", %{conn: conn} do - assert ACL.allow!(conn, "boards.allCategories", "You cannot query all categories") == :ok - assert ACL.allow!(conn, "posts.byThread", "You cannot query threads") == :ok + test "defaults to 'anonymous' role if not authenticated" do + assert ACL.allow!(%Plug.Conn{}, "boards.allCategories", "You cannot query all categories") == :ok + assert ACL.allow!(%Plug.Conn{}, "posts.byThread", "You cannot query threads") == :ok assert_raise InvalidPermission, ~r/^You cannot create posts/, From 528d90cf5be4b7ea088d559b7b602bca577c8d6b Mon Sep 17 00:00:00 2001 From: unenglishable Date: Thu, 26 Jan 2023 14:24:58 -0800 Subject: [PATCH 20/20] refactor(acl test): mix format --- test/epochtalk_server_web/helpers/acl_test.exs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/epochtalk_server_web/helpers/acl_test.exs b/test/epochtalk_server_web/helpers/acl_test.exs index f9056767..999725da 100644 --- a/test/epochtalk_server_web/helpers/acl_test.exs +++ b/test/epochtalk_server_web/helpers/acl_test.exs @@ -112,7 +112,9 @@ defmodule EpochtalkServerWeb.ACLTest do end test "defaults to 'anonymous' role if not authenticated" do - assert ACL.allow!(%Plug.Conn{}, "boards.allCategories", "You cannot query all categories") == :ok + assert ACL.allow!(%Plug.Conn{}, "boards.allCategories", "You cannot query all categories") == + :ok + assert ACL.allow!(%Plug.Conn{}, "posts.byThread", "You cannot query threads") == :ok assert_raise InvalidPermission,