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

feat: add individual resource policies #114

Merged
merged 11 commits into from
Sep 13, 2021

Conversation

loganhenson
Copy link

@loganhenson loganhenson commented Aug 25, 2021

Status: Awaiting Review

Specifications

Scott's thoughts how policies should work

High level tracking issue for policies

Initial work from Jeremy

Progress

  • view_any?/1 - Can the resource be viewed at all? Returning false should hide the resource entirely and return a 403 on api requests related to that resource
  • view?/2- Can a user view a specific schema?
  • create?/1 Can a user create a resource? I believe this one is covered by create_all/1
  • update?/2 Can a user update a specific schema?
  • delete?/2 Can a user delete a specific resource?
  • document default policies
  • document resource specific policies

PR Review:

  • Rename the meta fields to not include the ?
  • define a default ExTeal.Field.apply_options_for/3 that warns engineers of the deprecation and forwards the functionality to the new apply_options_for/4? Similar to how Select.with_options/2 functions
    • define a default for ExTeal.Resource.Fields.apply_values/4 with deprecation message
  • FF PR using this branch

Docs Additions

Things to consider after this PR

  • How to handle multi-select (via checkboxes) -> deletion
    • Right now it is simply returning a 403 with an error, in the future the deletion option can be hidden if any of the selected items are non-deleteable

Notable changes to initial spec

  • Non-implementation of create?/1
  • Addition of Manifest.default_policy
    • Falls back to ExTeal.OpenEverywherePolicy for a non-breaking change to existing apps
  • Keeps the signature of the new policy methods ...(conn, model) instead of ...(user, model) and instead opts to document the use of macros to abstract repeated use of large pattern matches

Example Policy

defmodule MyAppWeb.ExTeal.ThingPolicy do
  use ExTeal.Policy

  # Only allow the super_admin role to interact with "Thing"s

  def create_any?(conn) do
    conn.assigns.current_user.role == :super_admin
  end

  def view_any?(conn) do
    conn.assigns.current_user.role == :super_admin
  end

  def update_any?(conn) do
    conn.assigns.current_user.role == :super_admin
  end

  def delete_any?(conn) do
    conn.assigns.current_user.role == :super_admin
  end

  def view?(conn, _thing) do
    conn.assigns.current_user.role == :super_admin
  end

  def update?(conn, _thing) do
    conn.assigns.current_user.role == :super_admin
  end

  def delete?(conn, _thing) do
    conn.assigns.current_user.role == :super_admin
  end
end

Example of using plain Elixir to abstract inside the policy

defmodule FeastFettleWeb.ExTeal.PlanPolicy do
  @moduledoc """
  Custom Feast-Fettle Policy
  """

  use ExTeal.Policy

  defmacro super_admin do
    quote do
      %{
        assigns: %{
          current_user: %FeastFettle.Coherence.User{
            role: :super_admin
          }
        }
      }
    end
  end

  def create_any?(_), do: true
  def view_any?(_), do: true
  def update_any?(_), do: true
  def delete_any?(_), do: true

  # Only allow super_admin to view "Couples" plans
  def view?(super_admin() = _conn, %FeastFettle.Accounts.Plan{plan_type_id: 2}), do: true
  def view?(super_admin() = _conn, _), do: false
  def view?(_, _), do: true

  # Only allow super_admin to update tier 1 & 2 plans
  def update?(super_admin() = _conn, %FeastFettle.Accounts.Plan{tier: tier}) when tier in [1, 2], do: true
  def update?(super_admin() = _conn, _), do: false
  def update?(_, _), do: true

  # Only allow super_admin to delete tier 1 plans
  def delete?(super_admin() = _conn, %FeastFettle.Accounts.Plan{tier: 1}), do: true
  def delete?(super_admin() = _conn, _), do: false
  def delete?(_, _), do: true
end

@loganhenson loganhenson self-assigned this Aug 25, 2021
Copy link
Contributor

@staylorwr staylorwr left a comment

Choose a reason for hiding this comment

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

I finally took a first pass tonight Logan! This is looking 💯 . I took moveprice for a spin to see how the upgrade path looked and I didn't run into any problems.

Should we define a default ExTeal.Field.apply_options_for/3 that warns engineers of the depreciation and forwards the functionality to the new apply_options_for/4? Similar to how Select.with_options/2 functions

Finally, is there a branch of FF I can pull down to test the real policy implementations?

👏🏼 👏🏼 👏🏼 👏🏼

guides/Policies.md Show resolved Hide resolved
lib/ex_teal/resource/fields.ex Show resolved Hide resolved
@loganhenson
Copy link
Author

I finally took a first pass tonight Logan! This is looking 💯 . I took moveprice for a spin to see how the upgrade path looked and I didn't run into any problems.

Should we define a default ExTeal.Field.apply_options_for/3 that warns engineers of the depreciation and forwards the functionality to the new apply_options_for/4? Similar to how Select.with_options/2 functions

Finally, is there a branch of FF I can pull down to test the real policy implementations?

👏🏼 👏🏼 👏🏼 👏🏼

I can take a stab at adding the deprecation + forward for the apply_options_for 👍

We currently do not have a FF branch that implements these (beyond just my own testing), but maybe I can get one up 👍

@loganhenson loganhenson force-pushed the lh-add-individual-resource-policies branch from e592ffd to b05274f Compare September 8, 2021 18:18
@loganhenson
Copy link
Author

@staylorwr I've added your review requests! The FF PR is here: https://github.com/Feast-and-Fettle/feast-fettle/pull/1082 (have to swap to local ex_teal etc)

@staylorwr staylorwr self-requested a review September 13, 2021 13:58
Copy link
Contributor

@staylorwr staylorwr left a comment

Choose a reason for hiding this comment

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

💯

@loganhenson loganhenson marked this pull request as ready for review September 13, 2021 15:49
@staylorwr staylorwr merged commit b660f73 into master Sep 13, 2021
@staylorwr staylorwr deleted the lh-add-individual-resource-policies branch September 13, 2021 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants