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
Move to CanCanCan for authorization #2023
Changes from 28 commits
ffa65d4
2ab3d56
b16aa11
6da3ece
6b44a19
5232914
ac7c45b
060c686
2a44ff5
464c7f8
4d20a2c
91fc65a
25256a4
420a728
f8f7ab1
fb2c1f6
901c29a
dfb9e40
8360f27
b7baa2c
ce761b3
a50ad1c
71b21ec
0888f43
f11221f
149c07f
4161959
7a177cb
8c269ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
# frozen_string_literal: true | ||
|
||
class Ability | ||
include CanCan::Ability | ||
|
||
def initialize(user) | ||
can [:index, :permalink, :edit, :help, :fixthemap, :offline, :export, :about, :preview, :copyright, :key, :id], :site | ||
can [:index, :rss, :show, :comments], DiaryEntry | ||
can [:search, :search_latlon, :search_ca_postcode, :search_osm_nominatim, | ||
:search_geonames, :search_osm_nominatim_reverse, :search_geonames_reverse], :geocoder | ||
|
||
if user | ||
can :welcome, :site | ||
can [:create, :edit, :comment, :subscribe, :unsubscribe], DiaryEntry | ||
can [:new, :create], Report | ||
can [:read, :read_one, :update, :update_one, :delete_one], UserPreference | ||
|
||
if user.moderator? | ||
can [:index, :show, :resolve, :ignore, :reopen], Issue | ||
can :create, IssueComment | ||
end | ||
|
||
if user.administrator? | ||
can [:hide, :hidecomment], [DiaryEntry, DiaryComment] | ||
can [:index, :show, :resolve, :ignore, :reopen], Issue | ||
can :create, IssueComment | ||
end | ||
end | ||
|
||
# Define abilities for the passed in user here. For example: | ||
# | ||
# user ||= User.new # guest user (not logged in) | ||
# if user.admin? | ||
# can :manage, :all | ||
# else | ||
# can :read, :all | ||
# end | ||
# | ||
# The first argument to `can` is the action you are giving the user | ||
# permission to do. | ||
# If you pass :manage it will apply to every action. Other common actions | ||
# here are :read, :create, :update and :destroy. | ||
# | ||
# The second argument is the resource the user can perform the action on. | ||
# If you pass :all it will apply to every resource. Otherwise pass a Ruby | ||
# class of the resource. | ||
# | ||
# The third argument is an optional hash of conditions to further filter the | ||
# objects. | ||
# For example, here the user can only update published articles. | ||
# | ||
# can :update, Article, :published => true | ||
# | ||
# See the wiki for details: | ||
# https://github.com/CanCanCommunity/cancancan/wiki/Defining-Abilities | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
# frozen_string_literal: true | ||
|
||
class Capability | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Obviously similar comments apply here as with I wonder if this is even needed - given we are overriding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have any views on this, beyond what @cflipse wrote in the commit message when he split them out. To be blunt, I don't yet have a good understanding of how all the tokens stuff works, so I'm happy to take direction here. My experience elsewhere is just with straightforward approaches around having a current_user with particular roles, and our token handling here is more complex. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's been a while. IIRC, the capabilities reflect permissions granted to the application -- so, if it's making a request to access your GPS, and you say "yes", then you've granted the app that capability. Another common example is when you OAuth login and the system asks for permission to read your contact lists. This is, more or less, inverse of the CanCanCan's normal idea of an Ability, which is the app deciding if the user has permission to do something; Capability is the user deciding if the app has permission to do something. (Capability was an inherited name, I suspect that something better could be determined) The end result of the calculation is the same, but separating them helps to keep them from getting too crossed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name capability comes from us - it was something we built on top of OAuth 1 to associate a token with a set of things it was allowed to do. OAuth 2 has something similar built in but calls them scopes where when an application requests a token it indicates what scopes it wants access to. |
||
include CanCan::Ability | ||
|
||
def initialize(token) | ||
can [:read, :read_one], UserPreference if capability?(token, :allow_read_prefs) | ||
can [:update, :update_one, :delete_one], UserPreference if capability?(token, :allow_write_prefs) | ||
end | ||
|
||
private | ||
|
||
def capability?(token, cap) | ||
token&.read_attribute(cap) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
# frozen_string_literal: true | ||
|
||
require "test_helper" | ||
|
||
class AbilityTest < ActiveSupport::TestCase | ||
end | ||
|
||
class GuestAbilityTest < AbilityTest | ||
test "geocoder permission for a guest" do | ||
ability = Ability.new nil | ||
|
||
[:search, :search_latlon, :search_ca_postcode, :search_osm_nominatim, | ||
:search_geonames, :search_osm_nominatim_reverse, :search_geonames_reverse].each do |action| | ||
assert ability.can?(action, :geocoder), "should be able to #{action} geocoder" | ||
end | ||
end | ||
|
||
test "diary permissions for a guest" do | ||
ability = Ability.new nil | ||
[:index, :rss, :show, :comments].each do |action| | ||
assert ability.can?(action, DiaryEntry), "should be able to #{action} DiaryEntries" | ||
end | ||
|
||
[:create, :edit, :comment, :subscribe, :unsubscribe, :hide, :hidecomment].each do |action| | ||
assert ability.cannot?(action, DiaryEntry), "should not be able to #{action} DiaryEntries" | ||
assert ability.cannot?(action, DiaryComment), "should not be able to #{action} DiaryEntries" | ||
end | ||
end | ||
end | ||
|
||
class UserAbilityTest < AbilityTest | ||
test "Diary permissions" do | ||
ability = Ability.new create(:user) | ||
|
||
[:index, :rss, :show, :comments, :create, :edit, :comment, :subscribe, :unsubscribe].each do |action| | ||
assert ability.can?(action, DiaryEntry), "should be able to #{action} DiaryEntries" | ||
end | ||
|
||
[:hide, :hidecomment].each do |action| | ||
assert ability.cannot?(action, DiaryEntry), "should not be able to #{action} DiaryEntries" | ||
assert ability.cannot?(action, DiaryComment), "should not be able to #{action} DiaryEntries" | ||
end | ||
|
||
[:index, :show, :resolve, :ignore, :reopen].each do |action| | ||
assert ability.cannot?(action, Issue), "should not be able to #{action} Issues" | ||
end | ||
end | ||
end | ||
|
||
class ModeratorAbilityTest < AbilityTest | ||
test "Issue permissions" do | ||
ability = Ability.new create(:moderator_user) | ||
|
||
[:index, :show, :resolve, :ignore, :reopen].each do |action| | ||
assert ability.can?(action, Issue), "should be able to #{action} Issues" | ||
end | ||
end | ||
end | ||
|
||
class AdministratorAbilityTest < AbilityTest | ||
test "Diary for an administrator" do | ||
ability = Ability.new create(:administrator_user) | ||
[:index, :rss, :show, :comments, :create, :edit, :comment, :subscribe, :unsubscribe, :hide, :hidecomment].each do |action| | ||
assert ability.can?(action, DiaryEntry), "should be able to #{action} DiaryEntries" | ||
end | ||
|
||
[:hide, :hidecomment].each do |action| | ||
assert ability.can?(action, DiaryComment), "should be able to #{action} DiaryComment" | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
app/model
the right place for this? It doesn't look like a model and the CanCanCan documentation just refers to is as a "class" so I was expecting it to be inlib
but I guess this is where the generator put it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is where the generator puts it. I'm happy to put it where ever suits - perhaps in config? I'm not sure about lib, I consider that somewhere where stuff goes that (in theory) could be extracted into a gem, but I can see the logic there since we put various classes into lib already.
From https://github.com/CanCanCommunity/cancancan/blob/develop/lib/generators/cancan/ability/USAGE:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's tricky... I'm not sure rails even creates a
lib
directory these days, and I know it was removed from the default load path but we put it back inconfig/application.rb
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just weighing in, FWIW, Ive seen a folder created for these and seen them put in
app/abilities/
before. It would also make splitting the ability file (which are likely to want to do later) much easier.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lib
is almost certainly the wrong place, as this isn't external / library code. I could see putting it in config, but it's active code that gets checked, and config isn't the first place I'd look. Ben's thought about app/abilities appeals, if the plan is to deconstruct the large object at some point ...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did do some googling before and there seems to be approximately no consensus on where to put things that aren't one of the "standard" types of class.
I'd always seen
lib
as being for random bits of non-classifiable code and not just for external things (which wouldn't normally be in the repo at all) and in the early days whenlib
was on the autoload path I think that was a more common view but then it got removed.I didn't even know could just create random directories under
app
to be honest - how does that work? Does rails add every directory there to the autoload path?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does.
there are some additional app directories that are something approaching a standard:
app/presenters
,app/forms
,app/services
(especially the last one) but it's pretty much open season for organizing things under the top layer of app. Rails will loop through those directories and add them to autoload at boot time (so, if you add a newapp/foo
class, you'd need to reboot your dev server, but otherwise, classes underneath are subject to Rails' loading magicks)