Reformulate authorization in example app #1998
Conversation
pablobm
force-pushed
the
reformulate-authorization
branch
2 times, most recently
from
900ec15 to
6ebced6
Compare
Turns out that the example app loads the `pundit` gem only in the test
environment. As a result, test behaves differently from development,
causing confusion. The main difference is that the `Punditize` module
loads only when the gem is available.
Here I'm fixing the situation by making `pundit` available across
all environments. However this is not a perfect solution...
The app's `ApplicationController` has the following method:
```
def show_action?(action, resource)
action != :index || resource != ::Order
end
```
This is overridden by Punditize... but we only use Punditize in
`ProductsController`. So the "Orders" navigation link only appears
when we are visiting the "Products" section.
I'm going to follow this up with changes to how authorization works
in the example app, to remove this issue and bring some order to
the app in general.
Our collection tables show two actions for each record: "Edit" and "Destroy". What if we want to allow other actions? This involves overriding the whole collection template, which is messy and adds friction when upgrading to new versions of Administrate. We have some examples of this need in our issue tracker. For example: thoughtbot#1676 More selfishly, I want to add one such custom action in the changes I'm making to how the example app handles authorization :-P This PR splits the collection partial into new partials that can be more easily overridden by users.
Our authorization specs are based on arbitrary rules such as "you can only update orders from Arizona". I understand this made sense in early development but, as Administrate grows, these rules become more confusing and unexpected. On this commit, I add a simple authentication mechanism that I think will help us write clearer authorization specs and examples. Customers are now the users. By default you are an admin and have access to everything. At any point, you can go to the list of customers and click "Become" to become that customer. Then you'll have some limited access, such as only seeing your own orders. You can go back to being an admin anytime by clicking a link at the top.
This is part of a larger effort to tidy the example app, removing inconsistencies and making it easier to test new functionality going forward. While fixing the specs, I have removed `line_item_controller_spec.rb` and moved the example therein to `features/search_spec.rb`, where I think it belongs. I have also removed related examples under `features/index_page_spec.rb` as I felt they were redundant.
Currently, the route `payments#index` exists but is disallowed to all users. This is probably in order to provide a context to the authorization spec, but I think it's confusing. Now that we have an authentication mechanism, we can use this to provide a clearer (I hope) authorization spec and a less confusing app.
pablobm
force-pushed
the
reformulate-authorization
branch
from
5a2d09f to
82bb4d4
Compare
There was a problem hiding this comment.
I think this looks great. It's a much better example and provides a use case which is definitely needed.
I appreciate you splitting it out into a set of commits that are easy to understand in isolation — I think it'd be best to not squash these so that we maintain them.
Do you think we should document (this could be separate from this) how this example of authorization should work? A bit like a "here's something you probably will need to do" guide?
|
Agreed on avoiding the squash. I'm doing a rebase-merge, as this repo doesn't allow merge commits. I'm not sure what you mean by the following:
Do you mean documenting how authentication+authorization work specifically on the example app, or more generally how these should be implemented in apps? Who do you have in mind as an audience for that document? |
|
I was thinking of something in Guides, I think. Specifically around the addition of the "become" mode. But perhaps we've got enough coverage in the combination of the tests and what we have already? My intended audience would be someone who wanted to get this setup, but without digging deep into the project (assuming it's a common thing people would like to do). |
|
OK, so about adding authentication+authorization in general. I agree that we should document it. I'm adding it to the list at #1778 |
Working on #1941, I'm having issues with the way authorization is currently tested. I'm trying to solve this problem first before I proceed with the linked PR.
Sorry, this is a big PR :-( To make reviews easier, I have put some effort into splitting it into commits with individual descriptions that make as much sense as possible in isolation. I haven't created individual PRs for each because I think that would leave things in a weird intermediate state.
Although there are some changes to Administrate proper, this PR is actually about the specs and the example app. The commits can be grouped as follows:
The only change to Administrate proper is in the first commit of point 2 above, where I extract some parts of the collection partial into smaller partials, allowing them to be overriden more easily.
The example app gets new a new feature where users can "become" a customer with a single click. This provides a simple form of authentication that we can use to test authorization. This feature is visible in the Customers collection: