<p><b>@tomhughes</b> commented on this pull request.</p>

<p>I assume (given your question) that this can be used as is and the existing technology continues to work for unconverted methods?</p>
<p>If that is the case then I have no objection in principle to merging this and then proceeding with further refactoring separately.</p><hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2023#discussion_r224995996">app/models/ability.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+class Ability
</pre>
<p>Is <code>app/model</code> 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 in <code>lib</code> but I guess this is where the generator put it?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2023#discussion_r224996053">app/models/ability.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+class Ability
+  include CanCan::Ability
+
+  def initialize(user)
+    can :index, :site
+    can [:permalink, :edit, :help, :fixthemap, :offline, :export, :about, :preview, :copyright, :key, :id], :site
</pre>
<p>Can these not be merged into one?</p>
<p>Is the ability to use a symbol as the last argument to refer to actions in a controller that does correspond to a model documented anywhere? Everything I've seen just refers to using a class name there to authorize access to objects of that class?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2023#discussion_r224996280">app/controllers/application_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -466,6 +468,23 @@ def better_errors_allow_inline
     raise
   end
 
+  def current_ability
+    Ability.new(current_user).merge(granted_capability)
</pre>
<p>This has lost the memoization of the default method, which has <code>@current_ability ||=</code> as a prefix - is that deliberate?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2023#discussion_r224996687">app/controllers/application_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -466,6 +468,23 @@ def better_errors_allow_inline
     raise
   end
 
+  def current_ability
+    Ability.new(current_user).merge(granted_capability)
+  end
+
+  def granted_capability
+    Capability.new(current_user, current_token)
+  end
+
+  def deny_access(_exception)
+    if current_user
</pre>
<p>This should be <code>current_token</code> here I think? Otherwise we wind up talking about OAuth when it wasn't used - the current code is guarded by a token test at least.</p>
<p>If we did that and then had a suitably generic case after it for <code>current_user</code> then I think we could get rid of many of the special case overrides for <code>deny_access</code> in the other controllers - most of those are pretty much impossible to hit anyway so don't really need to give the precise detail they do now.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2023#discussion_r224996742">app/models/capability.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+class Capability
</pre>
<p>Obviously similar comments apply here as with <code>Ability</code> but this one I think is something of your invention as I don't see it mentioned in the documentation?</p>
<p>I wonder if this is even needed - given we are overriding <code>current_ability</code> could we not pass both the user and token to <code>Ability</code> and have it do everything?</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2023#pullrequestreview-164502055">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABWnLasBooK7MdFVDEBGV1JxTegBNiUjks5uk1hngaJpZM4XVheR">mute the thread</a>.<img src="https://github.com/notifications/beacon/ABWnLZu80QF_H9XvThtOk3dZVlpfnjJMks5uk1hngaJpZM4XVheR.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/openstreetmap/openstreetmap-website","title":"openstreetmap/openstreetmap-website","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/openstreetmap/openstreetmap-website"}},"updates":{"snippets":[{"icon":"PERSON","message":"@tomhughes commented on #2023"}],"action":{"name":"View Pull Request","url":"https://github.com/openstreetmap/openstreetmap-website/pull/2023#pullrequestreview-164502055"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/2023#pullrequestreview-164502055",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/2023#pullrequestreview-164502055",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
},
{
"@type": "MessageCard",
"@context": "http://schema.org/extensions",
"hideOriginalBody": "false",
"originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB",
"title": "@tomhughes commented on 2023",
"sections": [
{
"text": "I assume (given your question) that this can be used as is and the existing technology continues to work for unconverted methods?\r\n\r\nIf that is the case then I have no objection in principle to merging this and then proceeding with further refactoring separately.",
"activityTitle": "**Tom Hughes**",
"activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png",
"activitySubtitle": "@tomhughes",
"facts": [

]
}
],
"potentialAction": [
{
"targets": [
{
"os": "default",
"uri": "https://github.com/openstreetmap/openstreetmap-website/pull/2023#pullrequestreview-164502055"
}
],
"@type": "OpenUri",
"name": "View on GitHub"
},
{
"name": "Unsubscribe",
"@type": "HttpPOST",
"target": "https://api.github.com",
"body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 391518097\n}"
}
],
"themeColor": "26292E"
}
]</script>