Merge remote-tracking branch 'upstream/pull/5666'

app/abilities/api_ability.rb

@@ -3,14 +3,12 @@
 class ApiAbility
   include CanCan::Ability
 
-  def initialize(token)
+  def initialize(user, scopes)
     can :read, [:version, :capability, :permission, :map]
 
     if Settings.status != "database_offline"
-      user = User.find(token.resource_owner_id) if token
-
       can [:read, :feed, :search], Note
-      can :create, Note unless token
+      can :create, Note unless user
 
       can [:read, :download], Changeset
       can :read, Tracepoint
@@ -19,31 +17,31 @@ class ApiAbility
       can :read, UserBlock
 
       if user&.active?
-        can [:create, :comment, :close, :reopen], Note if scope?(token, :write_notes)
+        can [:create, :comment, :close, :reopen], Note if scopes.include?("write_notes")
-        can [:create, :destroy], NoteSubscription if scope?(token, :write_notes)
+        can [:create, :destroy], NoteSubscription if scopes.include?("write_notes")
 
-        can :read, Trace if scope?(token, :read_gpx)
+        can :read, Trace if scopes.include?("read_gpx")
-        can [:create, :update, :destroy], Trace if scope?(token, :write_gpx)
+        can [:create, :update, :destroy], Trace if scopes.include?("write_gpx")
 
-        can :details, User if scope?(token, :read_prefs)
+        can :details, User if scopes.include?("read_prefs")
-        can :read, UserPreference if scope?(token, :read_prefs)
+        can :read, UserPreference if scopes.include?("read_prefs")
-        can [:update, :update_all, :destroy], UserPreference if scope?(token, :write_prefs)
+        can [:update, :update_all, :destroy], UserPreference if scopes.include?("write_prefs")
 
-        can [:read, :update, :destroy], Message if scope?(token, :consume_messages)
+        can [:read, :update, :destroy], Message if scopes.include?("consume_messages")
-        can :create, Message if scope?(token, :send_messages)
+        can :create, Message if scopes.include?("send_messages")
 
         if user.terms_agreed?
-          can [:create, :update, :upload, :close, :subscribe, :unsubscribe], Changeset if scope?(token, :write_api)
+          can [:create, :update, :upload, :close, :subscribe, :unsubscribe], Changeset if scopes.include?("write_map")
-          can :create, ChangesetComment if scope?(token, :write_api)
+          can :create, ChangesetComment if scopes.include?("write_changeset_comments")
-          can [:create, :update, :destroy], [Node, Way, Relation] if scope?(token, :write_api)
+          can [:create, :update, :destroy], [Node, Way, Relation] if scopes.include?("write_map")
         end
 
         if user.moderator?
-          can [:destroy, :restore], ChangesetComment if scope?(token, :write_api)
+          can [:destroy, :restore], ChangesetComment if scopes.include?("write_changeset_comments")
 
-          can :destroy, Note if scope?(token, :write_notes)
+          can :destroy, Note if scopes.include?("write_notes")
 
-          can :redact, [OldNode, OldWay, OldRelation] if user&.terms_agreed? && scope?(token, :write_redactions)
+          can :redact, [OldNode, OldWay, OldRelation] if user&.terms_agreed? && scopes.include?("write_redactions")
         end
       end
     end
@@ -75,10 +73,4 @@ class ApiAbility
     # See the wiki for details:
     # https://github.com/CanCanCommunity/cancancan/wiki/Defining-Abilities
   end
-
-  private
-
-  def scope?(token, scope)
-    token&.includes_scope?(scope)
-  end
 end

app/controllers/api_controller.rb

@@ -65,9 +65,16 @@ class ApiController < ApplicationController
   def current_ability
     # Use capabilities from the oauth token if it exists and is a valid access token
     if doorkeeper_token&.accessible?
-      ApiAbility.new(doorkeeper_token)
+      user = User.find(doorkeeper_token.resource_owner_id)
+      scopes = Set.new doorkeeper_token.scopes
+      if scopes.include?("write_api")
+        scopes.add("write_map")
+        scopes.add("write_changeset_comments")
+        scopes.delete("write_api")
+      end
+      ApiAbility.new(user, scopes)
     else
-      ApiAbility.new(nil)
+      ApiAbility.new(nil, Set.new)
     end
   end
 

config/locales/en.yml

@@ -87,12 +87,13 @@ en:
         url: Main Application URL (Required)
         callback_url: Callback URL
         support_url: Support URL
-        allow_read_prefs:  read their user preferences
+        allow_read_prefs: read their user preferences
         allow_write_prefs: modify their user preferences
         allow_write_diary: create diary entries and comments
-        allow_write_api:   modify the map
+        allow_write_api: modify the map
-        allow_read_gpx:    read their private GPS traces
+        allow_write_changeset_comments: comment on changesets
-        allow_write_gpx:   upload GPS traces
+        allow_read_gpx: read their private GPS traces
+        allow_write_gpx: upload GPS traces
         allow_write_notes: modify notes
       diary_comment:
         body: "Body"
@@ -2697,6 +2698,7 @@ en:
       write_prefs: Modify user preferences
       write_diary: Create diary entries and comments
       write_api: Modify the map
+      write_changeset_comments: Comment on changesets
       read_gpx: Read private GPS traces
       write_gpx: Upload GPS traces
       write_notes: Modify notes

lib/oauth.rb

@@ -1,7 +1,7 @@
 module Oauth
   SCOPES = %w[
     read_prefs write_prefs write_diary
-    write_api read_gpx write_gpx write_notes write_redactions
+    write_api write_changeset_comments read_gpx write_gpx write_notes write_redactions
     consume_messages send_messages openid
   ].freeze
   PRIVILEGED_SCOPES = %w[read_email skip_authorization].freeze

test/abilities/api_abilities_test.rb

@@ -7,7 +7,8 @@ end
 
 class GuestApiAbilityTest < ApiAbilityTest
   test "note permissions for a guest" do
-    ability = ApiAbility.new nil
+    scopes = Set.new
+    ability = ApiAbility.new nil, scopes
 
     [:index, :create, :feed, :show, :search].each do |action|
       assert ability.can?(action, Note), "should be able to #{action} Notes"
@@ -21,8 +22,9 @@ end
 
 class UserApiAbilityTest < ApiAbilityTest
   test "Note permissions" do
-    token = create(:oauth_access_token, :scopes => %w[write_notes])
+    user = create(:user)
-    ability = ApiAbility.new token
+    scopes = Set.new %w[write_notes]
+    ability = ApiAbility.new user, scopes
 
     [:index, :create, :comment, :feed, :show, :search, :close, :reopen].each do |action|
       assert ability.can?(action, Note), "should be able to #{action} Notes"
@@ -36,8 +38,9 @@ end
 
 class ModeratorApiAbilityTest < ApiAbilityTest
   test "Note permissions" do
-    token = create(:oauth_access_token, :scopes => %w[write_notes], :user => create(:moderator_user))
+    user = create(:moderator_user)
-    ability = ApiAbility.new token
+    scopes = Set.new %w[write_notes]
+    ability = ApiAbility.new user, scopes
 
     [:index, :create, :comment, :feed, :show, :search, :close, :reopen, :destroy].each do |action|
       assert ability.can?(action, Note), "should be able to #{action} Notes"

test/abilities/api_capability_test.rb

@@ -3,18 +3,20 @@
 require "test_helper"
 
 class ChangesetCommentApiCapabilityTest < ActiveSupport::TestCase
-  test "as a normal user with permissionless token" do
+  test "as a normal user without scopes" do
-    token = create(:oauth_access_token)
+    user = create(:user)
-    ability = ApiAbility.new token
+    scopes = Set.new
+    ability = ApiAbility.new user, scopes
 
     [:create, :destroy, :restore].each do |action|
       assert ability.cannot? action, ChangesetComment
     end
   end
 
-  test "as a normal user with write_api token" do
+  test "as a normal user with write_changeset_comments scope" do
-    token = create(:oauth_access_token, :scopes => %w[write_api])
+    user = create(:user)
-    ability = ApiAbility.new token
+    scopes = Set.new %w[write_changeset_comments]
+    ability = ApiAbility.new user, scopes
 
     [:destroy, :restore].each do |action|
       assert ability.cannot? action, ChangesetComment
@@ -25,18 +27,20 @@ class ChangesetCommentApiCapabilityTest < ActiveSupport::TestCase
     end
   end
 
-  test "as a moderator with permissionless token" do
+  test "as a moderator without scopes" do
-    token = create(:oauth_access_token, :user => create(:moderator_user))
+    user = create(:moderator_user)
-    ability = ApiAbility.new token
+    scopes = Set.new
+    ability = ApiAbility.new user, scopes
 
     [:create, :destroy, :restore].each do |action|
       assert ability.cannot? action, ChangesetComment
     end
   end
 
-  test "as a moderator with write_api token" do
+  test "as a moderator with write_changeset_comments scope" do
-    token = create(:oauth_access_token, :user => create(:moderator_user), :scopes => %w[write_api])
+    user = create(:moderator_user)
-    ability = ApiAbility.new token
+    scopes = Set.new %w[write_changeset_comments]
+    ability = ApiAbility.new user, scopes
 
     [:create, :destroy, :restore].each do |action|
       assert ability.can? action, ChangesetComment
@@ -45,18 +49,20 @@ class ChangesetCommentApiCapabilityTest < ActiveSupport::TestCase
 end
 
 class NoteApiCapabilityTest < ActiveSupport::TestCase
-  test "as a normal user with permissionless token" do
+  test "as a normal user without scopes" do
-    token = create(:oauth_access_token)
+    user = create(:user)
-    ability = ApiAbility.new token
+    scopes = Set.new
+    ability = ApiAbility.new user, scopes
 
     [:create, :comment, :close, :reopen, :destroy].each do |action|
       assert ability.cannot? action, Note
     end
   end
 
-  test "as a normal user with write_notes token" do
+  test "as a normal user with write_notes scope" do
-    token = create(:oauth_access_token, :scopes => %w[write_notes])
+    user = create(:user)
-    ability = ApiAbility.new token
+    scopes = Set.new %w[write_notes]
+    ability = ApiAbility.new user, scopes
 
     [:destroy].each do |action|
       assert ability.cannot? action, Note
@@ -67,18 +73,20 @@ class NoteApiCapabilityTest < ActiveSupport::TestCase
     end
   end
 
-  test "as a moderator with permissionless token" do
+  test "as a moderator without scopes" do
-    token = create(:oauth_access_token, :user => create(:moderator_user))
+    user = create(:moderator_user)
-    ability = ApiAbility.new token
+    scopes = Set.new
+    ability = ApiAbility.new user, scopes
 
     [:destroy].each do |action|
       assert ability.cannot? action, Note
     end
   end
 
-  test "as a moderator with write_notes token" do
+  test "as a moderator with write_notes scope" do
-    token = create(:oauth_access_token, :user => create(:moderator_user), :scopes => %w[write_notes])
+    user = create(:moderator_user)
-    ability = ApiAbility.new token
+    scopes = Set.new %w[write_notes]
+    ability = ApiAbility.new user, scopes
 
     [:destroy].each do |action|
       assert ability.can? action, Note
@@ -88,16 +96,16 @@ end
 
 class UserApiCapabilityTest < ActiveSupport::TestCase
   test "user preferences" do
-    # A user with empty tokens
+    user = create(:user)
-    token = create(:oauth_access_token)
+    scopes = Set.new
-    ability = ApiAbility.new token
+    ability = ApiAbility.new user, scopes
 
     [:index, :show, :update_all, :update, :destroy].each do |act|
       assert ability.cannot? act, UserPreference
     end
 
-    token = create(:oauth_access_token, :scopes => %w[read_prefs])
+    scopes = Set.new %w[read_prefs]
-    ability = ApiAbility.new token
+    ability = ApiAbility.new user, scopes
 
     [:update_all, :update, :destroy].each do |act|
       assert ability.cannot? act, UserPreference
@@ -107,8 +115,8 @@ class UserApiCapabilityTest < ActiveSupport::TestCase
       assert ability.can? act, UserPreference
     end
 
-    token = create(:oauth_access_token, :scopes => %w[write_prefs])
+    scopes = Set.new %w[write_prefs]
-    ability = ApiAbility.new token
+    ability = ApiAbility.new user, scopes
 
     [:index, :show].each do |act|
       assert ability.cannot? act, UserPreference

test/controllers/api/changeset_comments_controller_test.rb

@@ -31,110 +31,201 @@ module Api
       )
     end
 
-    ##
+    def test_create_by_unauthorized
-    # create comment success
+      assert_no_difference "ChangesetComment.count" do
-    def test_create_comment_success
+        post changeset_comment_path(create(:changeset, :closed), :text => "This is a comment")
-      user = create(:user)
+        assert_response :unauthorized
-      user2 = create(:user)
+      end
-      private_user = create(:user, :data_public => false)
+    end
-      suspended_user = create(:user, :suspended)
-      deleted_user = create(:user, :deleted)
-      private_user_closed_changeset = create(:changeset, :closed, :user => private_user)
 
+    def test_create_on_missing_changeset
+      assert_no_difference "ChangesetComment.count" do
+        post changeset_comment_path(999111, :text => "This is a comment"), :headers => bearer_authorization_header
+        assert_response :not_found
+      end
+    end
+
+    def test_create_on_open_changeset
+      assert_no_difference "ChangesetComment.count" do
+        post changeset_comment_path(create(:changeset), :text => "This is a comment"), :headers => bearer_authorization_header
+        assert_response :conflict
+      end
+    end
+
+    def test_create_without_text
+      assert_no_difference "ChangesetComment.count" do
+        post changeset_comment_path(create(:changeset, :closed)), :headers => bearer_authorization_header
+        assert_response :bad_request
+      end
+    end
+
+    def test_create_with_empty_text
+      assert_no_difference "ChangesetComment.count" do
+        post changeset_comment_path(create(:changeset, :closed), :text => ""), :headers => bearer_authorization_header
+        assert_response :bad_request
+      end
+    end
+
+    def test_create_when_not_agreed_to_terms
+      user = create(:user, :terms_agreed => nil)
+      auth_header = bearer_authorization_header user
+      changeset = create(:changeset, :closed)
+
+      assert_difference "ChangesetComment.count", 0 do
+        post changeset_comment_path(changeset), :params => { :text => "This is a comment" }, :headers => auth_header
+        assert_response :forbidden
+      end
+    end
+
+    def test_create_without_required_scope
+      user = create(:user)
+      auth_header = bearer_authorization_header user, :scopes => %w[read_prefs]
+      changeset = create(:changeset, :closed)
+
+      assert_difference "ChangesetComment.count", 0 do
+        post changeset_comment_path(changeset), :params => { :text => "This is a comment" }, :headers => auth_header
+        assert_response :forbidden
+      end
+    end
+
+    def test_create_with_write_changeset_comments_scope
+      user = create(:user)
+      auth_header = bearer_authorization_header user, :scopes => %w[write_changeset_comments]
+      changeset = create(:changeset, :closed)
+
+      assert_difference "ChangesetComment.count", 1 do
+        post changeset_comment_path(changeset), :params => { :text => "This is a comment" }, :headers => auth_header
+        assert_response :success
+      end
+
+      comment = ChangesetComment.last
+      assert_equal changeset.id, comment.changeset_id
+      assert_equal user.id, comment.author_id
+      assert_equal "This is a comment", comment.body
+      assert comment.visible
+    end
+
+    def test_create_with_write_api_scope
+      user = create(:user)
+      auth_header = bearer_authorization_header user, :scopes => %w[write_api]
+      changeset = create(:changeset, :closed)
+
+      assert_difference "ChangesetComment.count", 1 do
+        post changeset_comment_path(changeset), :params => { :text => "This is a comment" }, :headers => auth_header
+        assert_response :success
+      end
+
+      comment = ChangesetComment.last
+      assert_equal changeset.id, comment.changeset_id
+      assert_equal user.id, comment.author_id
+      assert_equal "This is a comment", comment.body
+      assert comment.visible
+    end
+
+    def test_create_on_changeset_with_no_subscribers
+      changeset = create(:changeset, :closed)
+      auth_header = bearer_authorization_header
+
+      assert_difference "ChangesetComment.count", 1 do
+        assert_no_difference "ActionMailer::Base.deliveries.size" do
+          perform_enqueued_jobs do
+            post changeset_comment_path(changeset, :text => "This is a comment"), :headers => auth_header
+            assert_response :success
+          end
+        end
+      end
+    end
+
+    def test_create_on_changeset_with_commenter_subscriber
+      user = create(:user)
+      changeset = create(:changeset, :closed, :user => user)
+      changeset.subscribers << user
       auth_header = bearer_authorization_header user
 
       assert_difference "ChangesetComment.count", 1 do
         assert_no_difference "ActionMailer::Base.deliveries.size" do
           perform_enqueued_jobs do
-            post changeset_comment_path(private_user_closed_changeset, :text => "This is a comment"), :headers => auth_header
+            post changeset_comment_path(changeset, :text => "This is a comment"), :headers => auth_header
+            assert_response :success
           end
         end
       end
-      assert_response :success
+    end
 
-      changeset = create(:changeset, :closed, :user => private_user)
+    def test_create_on_changeset_with_invisible_subscribers
-      changeset.subscribers.push(private_user)
+      changeset = create(:changeset, :closed)
-      changeset.subscribers.push(user)
+      changeset.subscribers << create(:user, :suspended)
-      changeset.subscribers.push(suspended_user)
+      changeset.subscribers << create(:user, :deleted)
-      changeset.subscribers.push(deleted_user)
+      auth_header = bearer_authorization_header
+
+      assert_difference "ChangesetComment.count", 1 do
+        assert_no_difference "ActionMailer::Base.deliveries.size" do
+          perform_enqueued_jobs do
+            post changeset_comment_path(changeset, :text => "This is a comment"), :headers => auth_header
+            assert_response :success
+          end
+        end
+      end
+    end
+
+    def test_create_on_changeset_with_changeset_creator_subscriber
+      creator_user = create(:user)
+      changeset = create(:changeset, :closed, :user => creator_user)
+      changeset.subscribers << creator_user
+      commenter_user = create(:user)
+      auth_header = bearer_authorization_header commenter_user
 
       assert_difference "ChangesetComment.count", 1 do
         assert_difference "ActionMailer::Base.deliveries.size", 1 do
           perform_enqueued_jobs do
             post changeset_comment_path(changeset, :text => "This is a comment"), :headers => auth_header
+            assert_response :success
           end
         end
       end
-      assert_response :success
 
       email = ActionMailer::Base.deliveries.first
       assert_equal 1, email.to.length
-      assert_equal "[OpenStreetMap] #{user.display_name} has commented on one of your changesets", email.subject
+      assert_equal "[OpenStreetMap] #{commenter_user.display_name} has commented on one of your changesets", email.subject
-      assert_equal private_user.email, email.to.first
+      assert_equal creator_user.email, email.to.first
 
       ActionMailer::Base.deliveries.clear
+    end
 
-      auth_header = bearer_authorization_header user2
+    def test_create_on_changeset_with_changeset_creator_and_other_user_subscribers
+      creator_user = create(:user)
+      changeset = create(:changeset, :closed, :user => creator_user)
+      changeset.subscribers << creator_user
+      other_user = create(:user)
+      changeset.subscribers << other_user
+      commenter_user = create(:user)
+      auth_header = bearer_authorization_header commenter_user
 
       assert_difference "ChangesetComment.count", 1 do
         assert_difference "ActionMailer::Base.deliveries.size", 2 do
           perform_enqueued_jobs do
             post changeset_comment_path(changeset, :text => "This is a comment"), :headers => auth_header
+            assert_response :success
           end
         end
       end
-      assert_response :success
 
-      email = ActionMailer::Base.deliveries.find { |e| e.to.first == private_user.email }
+      email = ActionMailer::Base.deliveries.find { |e| e.to.first == creator_user.email }
       assert_not_nil email
       assert_equal 1, email.to.length
-      assert_equal "[OpenStreetMap] #{user2.display_name} has commented on one of your changesets", email.subject
+      assert_equal "[OpenStreetMap] #{commenter_user.display_name} has commented on one of your changesets", email.subject
 
-      email = ActionMailer::Base.deliveries.find { |e| e.to.first == user.email }
+      email = ActionMailer::Base.deliveries.find { |e| e.to.first == other_user.email }
       assert_not_nil email
       assert_equal 1, email.to.length
-      assert_equal "[OpenStreetMap] #{user2.display_name} has commented on a changeset you are interested in", email.subject
+      assert_equal "[OpenStreetMap] #{commenter_user.display_name} has commented on a changeset you are interested in", email.subject
 
       ActionMailer::Base.deliveries.clear
     end
 
-    ##
-    # create comment fail
-    def test_create_comment_fail
-      # unauthorized
-      post changeset_comment_path(create(:changeset, :closed), :text => "This is a comment")
-      assert_response :unauthorized
-
-      auth_header = bearer_authorization_header
-
-      # bad changeset id
-      assert_no_difference "ChangesetComment.count" do
-        post changeset_comment_path(999111, :text => "This is a comment"), :headers => auth_header
-      end
-      assert_response :not_found
-
-      # not closed changeset
-      assert_no_difference "ChangesetComment.count" do
-        post changeset_comment_path(create(:changeset), :text => "This is a comment"), :headers => auth_header
-      end
-      assert_response :conflict
-
-      # no text
-      assert_no_difference "ChangesetComment.count" do
-        post changeset_comment_path(create(:changeset, :closed)), :headers => auth_header
-      end
-      assert_response :bad_request
-
-      # empty text
-      assert_no_difference "ChangesetComment.count" do
-        post changeset_comment_path(create(:changeset, :closed), :text => ""), :headers => auth_header
-      end
-      assert_response :bad_request
-    end
-
     ##
     # create comment rate limit for new users
-    def test_create_comment_new_user_rate_limit
+    def test_create_by_new_user_with_rate_limit
       changeset = create(:changeset, :closed)
       user = create(:user)
 
@@ -155,7 +246,7 @@ module Api
 
     ##
     # create comment rate limit for experienced users
-    def test_create_comment_experienced_user_rate_limit
+    def test_create_by_experienced_user_with_rate_limit
       changeset = create(:changeset, :closed)
       user = create(:user)
       create_list(:changeset_comment, Settings.comments_to_max_changeset_comments, :author_id => user.id, :created_at => Time.now.utc - 1.day)
@@ -177,7 +268,7 @@ module Api
 
     ##
     # create comment rate limit for reported users
-    def test_create_comment_reported_user_rate_limit
+    def test_create_by_reported_user_with_rate_limit
       changeset = create(:changeset, :closed)
       user = create(:user)
       create(:issue_with_reports, :reportable => user, :reported_user => user)
@@ -199,7 +290,7 @@ module Api
 
     ##
     # create comment rate limit for moderator users
-    def test_create_comment_moderator_user_rate_limit
+    def test_create_by_moderator_user_with_rate_limit
       changeset = create(:changeset, :closed)
       user = create(:moderator_user)
 
@@ -218,107 +309,118 @@ module Api
       end
     end
 
-    ##
+    def test_hide_by_unauthorized
-    # test hide comment fail
-    def test_destroy_comment_fail
-      # unauthorized
       comment = create(:changeset_comment)
-      assert comment.visible
 
       post changeset_comment_hide_path(comment)
+
       assert_response :unauthorized
       assert comment.reload.visible
-
-      auth_header = bearer_authorization_header
-
-      # not a moderator
-      post changeset_comment_hide_path(comment), :headers => auth_header
-      assert_response :forbidden
-      assert comment.reload.visible
-
-      auth_header = bearer_authorization_header create(:moderator_user)
-
-      # bad comment id
-      post changeset_comment_hide_path(999111), :headers => auth_header
-      assert_response :not_found
-      assert comment.reload.visible
     end
 
-    ##
+    def test_hide_by_normal_user
-    # test hide comment succes
-    def test_hide_comment_success
       comment = create(:changeset_comment)
-      assert comment.visible
+      auth_header = bearer_authorization_header
-
-      auth_header = bearer_authorization_header create(:moderator_user)
 
       post changeset_comment_hide_path(comment), :headers => auth_header
+
+      assert_response :forbidden
+      assert comment.reload.visible
+    end
+
+    def test_hide_missing_comment
+      auth_header = bearer_authorization_header create(:moderator_user)
+
+      post changeset_comment_hide_path(999111), :headers => auth_header
+
+      assert_response :not_found
+    end
+
+    def test_hide_without_required_scope
+      comment = create(:changeset_comment)
+      auth_header = bearer_authorization_header create(:moderator_user), :scopes => %w[read_prefs]
+
+      post changeset_comment_hide_path(comment), :headers => auth_header
+
+      assert_response :forbidden
+      assert comment.reload.visible
+    end
+
+    def test_hide_with_write_changeset_comments_scope
+      comment = create(:changeset_comment)
+      auth_header = bearer_authorization_header create(:moderator_user), :scopes => %w[write_changeset_comments]
+
+      post changeset_comment_hide_path(comment), :headers => auth_header
+
       assert_response :success
       assert_not comment.reload.visible
     end
 
-    ##
+    def test_hide_with_write_api_scope
-    # test unhide comment fail
+      comment = create(:changeset_comment)
-    def test_restore_comment_fail
+      auth_header = bearer_authorization_header create(:moderator_user), :scopes => %w[write_api]
-      # unauthorized
+
+      post changeset_comment_hide_path(comment), :headers => auth_header
+
+      assert_response :success
+      assert_not comment.reload.visible
+    end
+
+    def test_unhide_by_unauthorized
       comment = create(:changeset_comment, :visible => false)
-      assert_not comment.visible
 
       post changeset_comment_unhide_path(comment)
+
       assert_response :unauthorized
       assert_not comment.reload.visible
-
-      auth_header = bearer_authorization_header
-
-      # not a moderator
-      post changeset_comment_unhide_path(comment), :headers => auth_header
-      assert_response :forbidden
-      assert_not comment.reload.visible
-
-      auth_header = bearer_authorization_header create(:moderator_user)
-
-      # bad comment id
-      post changeset_comment_unhide_path(999111), :headers => auth_header
-      assert_response :not_found
-      assert_not comment.reload.visible
     end
 
-    ##
+    def test_unhide_by_normal_user
-    # test unhide comment succes
-    def test_unhide_comment_success
       comment = create(:changeset_comment, :visible => false)
-      assert_not comment.visible
+      auth_header = bearer_authorization_header
-
-      auth_header = bearer_authorization_header create(:moderator_user)
 
       post changeset_comment_unhide_path(comment), :headers => auth_header
+
+      assert_response :forbidden
+      assert_not comment.reload.visible
+    end
+
+    def test_unhide_missing_comment
+      auth_header = bearer_authorization_header create(:moderator_user)
+
+      post changeset_comment_unhide_path(999111), :headers => auth_header
+
+      assert_response :not_found
+    end
+
+    def test_unhide_without_required_scope
+      comment = create(:changeset_comment, :visible => false)
+      auth_header = bearer_authorization_header create(:moderator_user), :scopes => %w[read_prefs]
+
+      post changeset_comment_unhide_path(comment), :headers => auth_header
+
+      assert_response :forbidden
+      assert_not comment.reload.visible
+    end
+
+    def test_unhide_with_write_changeset_comments_scope
+      comment = create(:changeset_comment, :visible => false)
+      auth_header = bearer_authorization_header create(:moderator_user), :scopes => %w[write_changeset_comments]
+
+      post changeset_comment_unhide_path(comment), :headers => auth_header
+
       assert_response :success
       assert comment.reload.visible
     end
 
-    # This test ensures that token capabilities behave correctly for a method that
+    def test_unhide_with_write_api_scope
-    # requires the terms to have been agreed.
+      comment = create(:changeset_comment, :visible => false)
-    # (This would be better as an integration or system testcase, since the changeset_comment
+      auth_header = bearer_authorization_header create(:moderator_user), :scopes => %w[write_api]
-    # create method is simply a stand-in for any method that requires terms agreement.
-    # But writing oauth tests is hard, and so it's easier to put in a controller test.)
-    def test_api_write_and_terms_agreed_via_token
-      user = create(:user, :terms_agreed => nil)
-      auth_header = bearer_authorization_header(user, :scopes => %w[write_api])
-      changeset = create(:changeset, :closed)
 
-      assert_difference "ChangesetComment.count", 0 do
+      post changeset_comment_unhide_path(comment), :headers => auth_header
-        post changeset_comment_path(changeset), :params => { :text => "This is a comment" }, :headers => auth_header
-      end
-      assert_response :forbidden
 
-      # Try again, after agreement with the terms
-      user.terms_agreed = Time.now.utc
-      user.save!
-
-      assert_difference "ChangesetComment.count", 1 do
-        post changeset_comment_path(changeset), :params => { :text => "This is a comment" }, :headers => auth_header
-      end
       assert_response :success
+      assert comment.reload.visible
     end
   end
 end