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

Add view helpers #22

Open
frm opened this issue Nov 13, 2020 · 10 comments
Open

Add view helpers #22

frm opened this issue Nov 13, 2020 · 10 comments
Labels
discussion Open for discussion before implementing enhancement New feature or request

Comments

@frm
Copy link
Contributor

frm commented Nov 13, 2020

Sometimes we want to conditionally render elements based on the user having permissions or not. We should implement this somehow.

A basic idea:

= if has_permissions?(@conn, :show, @post)

Any thoughts on this?

@frm frm added discussion Open for discussion before implementing enhancement New feature or request help wanted and removed help wanted labels Nov 13, 2020
@naps62
Copy link
Member

naps62 commented Nov 13, 2020

makes sense, but why has_permission? instead of can?

@frm
Copy link
Contributor Author

frm commented Nov 13, 2020

Honestly, just not to confuse with calling the policy directly. Especially since the API will be different. I'd prefer can? but I can see some confusion arising.

@frm
Copy link
Contributor Author

frm commented Nov 13, 2020

Another question: how would we handle custom policies? When plugging, we either infer the policy or have the user explicitly provide it.

We can add an optional parameter in the end that allows the policy to be specified. Basically all the options supported by the plug would be sent there. has_permissions?(@conn, :show, @post, policy: MyPolicy, ...)

Another would be to do put_policy in the controller but that sounds a bit odd to me. It would avoid having to carry the policy: MyPolicy option around in the HTML.

@naps62
Copy link
Member

naps62 commented Nov 13, 2020

Honestly, just not to confuse with calling the policy directly.

This is very weak api design, in my opinion. If you're reaching for synonyms just to avoid confusion, then you're just causing a different kind of confusion, because people know have to remember a different name, for no clear reason

In this case in particular, since one is a view helper, and the other is a policy function, I don't really see a problem with them having the same name. The benefit of Elixir vs Ruby in these situations is that you don't have to worry about naming conflicts

Another question: how would we handle custom policies? When plugging, we either infer the policy or have the user explicitly provide it.

This one is tricky. put_policy in the controller doesn't work. You can easily call has_permission?(@conn, :edit, @post_comment) within the PostController, which would need the CommentPolicy, and not the PostPolicy
You also end up with problems in partials that may be called from multiple controllers

Forcing a parameters on every call seems to cumbersome, in my opinion. How about if instead we plug that metadata into each resource module?

defmodule Post do
  use Dictator.Resource, policy: ...
end

This would be a big shift from the current API, but also seems to make sense along with the other graphql/liveview discussions

@frm
Copy link
Contributor Author

frm commented Nov 13, 2020

because people know have to remember a different name, for no clear reason

Yeah, I agree, but it's mostly because the API would be different. Don't you think that could cause confusion? Same function, completely different parameters?

How about if instead we plug that metadata into each resource module?

That one I strongly disagree, for two reasons:

  1. you're making core logic aware of authorisation and in particular, making it aware of controller functions. If a developer makes a non-default controller function (like callback or something), the resource will have to be aware of that.
  2. you're binding a resource to a single behaviour, when you can have multiple web clients in the same app and thus different policies for each.

At most we could use something like decorators but it doesn't work as well when you're not doing inheritance, does it?

You can easily call has_permission?(@conn, :edit, @post_comment) within the PostController, which would need the CommentPolicy, and not the PostPolicy. You also end up with problems in partials that may be called from multiple controllers

I actually think that passing that metadata to the conn is a good idea. Dictator could call put_private repeatedly to define that data. Then the view helper could use that private data. It would be on a per conn basis so same partials in multiple controllers wouldn't be an issue.

@conn
Copy link

conn commented Nov 13, 2020

I think I have room somewhere for some metadata, so feel free to pass me some.

Also, it's a little unclear to me: do I have the permission or not?

@naps62
Copy link
Member

naps62 commented Nov 13, 2020

I just hope you chose that username on purpose for these moments. because if so, you now have my full respect 😄

@naps62
Copy link
Member

naps62 commented Nov 13, 2020

Yeah, I agree, but it's mostly because the API would be different. Don't you think that could cause confusion? Same function, completely different parameters?

I don't think it would, no. I might be being naive, though. But it feels much more intuitive to me to use the same name, even with a different API, than having to use two synonyms, and having to remember which one goes where.

Either way, these are my 0.02$. Do as you feel its better 🖖

you're making core logic aware of authorisation and in particular, making it aware of controller functions. If a developer makes a non-default controller function (like callback or something), the resource will have to be aware of that.

Not exactly. The Post resource wouldn't be aware of each callback you need. It would just know it should refer to a PostPolicy, which in turn knows those details.
You can disagree with that as well, and that's fine, but it's not exactly like as put it.

Also, besides that, I'm out of good ideas

I actually think that passing that metadata to the conn is a good idea. Dictator could call put_private repeatedly to define that data. Then the view helper could use that private data. It would be on a per conn basis so same partials in multiple controllers wouldn't be an issue.

I don't think you understood my point here. In a PostController I expect to need to point to a PostPolicy. But I'm checking permissions for comments somewhere down in the views, so now I need to reference a CommentPolicy as well?
That is technically doable, but doesn't seem to make any sense conceptually.
And you also end up in a situation where you have to define multiple random policies on each controller, based solely on what views you happen to render, which seems confusing and error-prone

@frm
Copy link
Contributor Author

frm commented Nov 17, 2020

I'm down with can?/3.

I'm still against making the resource aware of policies, I think you're threading a dangerous line when it comes to responsibilities.

Ah, you're saying when checking a PostPolicy to render something for the current post, but then CommentPolicy to check if we should render something regarding the comments?

@naps62
Copy link
Member

naps62 commented Nov 18, 2020

Ah, you're saying when checking a PostPolicy to render something for the current post, but then CommentPolicy to check if we should render something regarding the comments?

yes. once you start checking permissions on views, you no longer have a 1-to-1 relationship between views and policies

What you do have is a 1-1 relationship between resources and policies (or at least, mostly. you could argue that it would be useful to have multiple policies depending on the context)
That's why I suggested attaching them to the resource

If we're not going with that, the only other sensible options I see are:

  1. Use can?/4 instead, and make the policy a mandatory argument
  2. Define some sort of policy_for(conn, resource) overridable function, which would default to let me guess the policy from the module name, but could be overriden. Not sure how this would coexist with the controller plug options, though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Open for discussion before implementing enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants