api tokens - clarify terms: "monitors" are admins that authenticated via api token, "tmp" users are users that exist only in tmp db, "test" users are either tmp users or users named "test_user_x"

12 files changed, 66 insertions, 30 deletions

diff --git a/app/controllers/controller_extension/errors.rb b/app/controllers/controller_extension/errors.rb
index 8f8edde..2b68955 100644
--- a/app/controllers/controller_extension/errors.rb
+++ b/app/controllers/controller_extension/errors.rb
@@ -4,21 +4,22 @@ module ControllerExtension::Errors
   protected
 
   def access_denied
-    respond_to_error :not_authorized, :forbidden, home_url
+    render_error :not_authorized, :forbidden, home_url
   end
 
   def login_required
     # Warden will intercept the 401 response and call
     # SessionController#unauthenticated instead.
-    respond_to_error :not_authorized_login, :unauthorized, login_url
+    render_error :not_authorized_login, :unauthorized, login_url
   end
 
-  def not_found
-    respond_to_error :not_found, :not_found, home_url
+  def not_found(msg=nil, url=nil)
+    render_error(msg || :not_found, :not_found, url || home_url)
   end
 
+  private
 
-  def respond_to_error(message, status=nil, redirect=nil)
+  def render_error(message, status=nil, redirect=nil)
     error = message
     message = t(message) if message.is_a?(Symbol)
     respond_to do |format|
diff --git a/app/controllers/controller_extension/fetch_user.rb b/app/controllers/controller_extension/fetch_user.rb
index 695d723..97f92fa 100644
--- a/app/controllers/controller_extension/fetch_user.rb
+++ b/app/controllers/controller_extension/fetch_user.rb
@@ -8,11 +8,25 @@ module ControllerExtension::FetchUser
 
   protected
 
+  #
+  # fetch @user from params, but enforce permissions:
+  #
+  # * admins may fetch any user
+  # * monitors may fetch test users
+  # * users may fetch themselves
+  #
+  # these permissions matter, it is what protects
+  # users from being updated or deleted by other users.
+  #
   def fetch_user
     @user = User.find(params[:user_id] || params[:id])
-    if !@user && admin?
-      redirect_to users_url, :alert => t(:no_such_thing, :thing => 'user')
-    elsif !admin? && @user != current_user
+    if current_user.is_admin? || current_user.is_monitor?
+      if @user.nil?
+        not_found(t(:no_such_thing, :thing => 'user'), users_url)
+      elsif current_user.is_monitor?
+        access_denied unless @user.is_test?
+      end
+    elsif @user != current_user
       access_denied
     end
   end
diff --git a/app/models/account.rb b/app/models/account.rb
index abde89d..a85e56c 100644
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -21,7 +21,7 @@ class Account
     user = User.new(attrs)
     user.save
 
-    if !user.tmp? && user.persisted?
+    if !user.is_tmp? && user.persisted?
       identity = user.identity
       identity.user_id = user.id
       identity.save
@@ -59,7 +59,7 @@ class Account
 
   def destroy(destroy_identity=false)
     return unless @user
-    if !@user.tmp?
+    if !@user.is_tmp?
       if destroy_identity == false
         @user.identities.each do |id|
           id.orphan!
@@ -77,7 +77,7 @@ class Account
   # in place, but the user should not be able to send email or
   # create new authentication certificates.
   def disable
-    if @user && !@user.tmp?
+    if @user && !@user.is_tmp?
       @user.enabled = false
       @user.save
       @user.identities.each do |id|
@@ -119,7 +119,7 @@ class Account
 
   def self.creation_problem?(user, identity)
     return true if user.nil? || !user.persisted? || user.errors.any?
-    if !user.tmp?
+    if !user.is_tmp?
       return true if identity.nil? || !identity.persisted? || identity.errors.any?
     end
     return false
diff --git a/app/models/anonymous_user.rb b/app/models/anonymous_user.rb
index e1f7a0e..79a5f32 100644
--- a/app/models/anonymous_user.rb
+++ b/app/models/anonymous_user.rb
@@ -9,7 +9,7 @@ class AnonymousUser < Object
     false
   end
 
-  def is_test?
+  def is_monitor?
     false
   end
 
diff --git a/app/models/api_user.rb b/app/models/api_user.rb
index d80a4d1..2efe1cb 100644
--- a/app/models/api_user.rb
+++ b/app/models/api_user.rb
@@ -7,8 +7,8 @@ end
 # for running monitor tests against a live production
 # installation.
 #
-class ApiTestUser < ApiUser
-  def is_test?
+class ApiMonitorUser < ApiUser
+  def is_monitor?
     true
   end
 end
diff --git a/app/models/temporary_user.rb b/app/models/temporary_user.rb
index 87800ed..2afae15 100644
--- a/app/models/temporary_user.rb
+++ b/app/models/temporary_user.rb
@@ -16,7 +16,8 @@ module TemporaryUser
 
   USER_DB     = 'users'
   TMP_USER_DB = 'tmp_users'
-  TMP_LOGIN   = 'test_user'
+  TMP_LOGIN   = 'tmp_user'  # created and deleted frequently
+  TEST_LOGIN  = 'test_user' # created, rarely deleted
 
   included do
     use_database_method :db_name
@@ -26,7 +27,7 @@ module TemporaryUser
     # override it.
     instance_eval <<-EOS, __FILE__, __LINE__ + 1
       def find_by_login(*args)
-        if args.grep(/#{TMP_LOGIN}/).any?
+        if args.grep(/^#{TMP_LOGIN}/).any?
           by_login.database(tmp_database).key(*args).first()
         else
           by_login.key(*args).first()
@@ -60,6 +61,14 @@ module TemporaryUser
     def create_tmp_database!
       design_doc.sync!(tmp_database.tap{|db|db.create!})
     end
+
+    def is_tmp?(login)
+      !login.nil? && login =~ /^#{TMP_LOGIN}/
+    end
+
+    def is_test?(login)
+      !login.nil? && (login =~ /^#{TMP_LOGIN}/ || login =~ /^#{TEST_LOGIN}/)
+    end
   end
 
   #
@@ -71,8 +80,14 @@ module TemporaryUser
   end
 
   # returns true if this User instance is stored in tmp db.
-  def tmp?
-    !login.nil? && login.include?(TMP_LOGIN)
+  def is_tmp?
+    self.class.is_tmp?(self.login)
+  end
+
+  # returns true if this user is used for testing purposes
+  # (either a temporary or long lived)
+  def is_test?
+    self.class.is_test?(self.login)
   end
 
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index 1aeea1c..1c54f0c 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -107,7 +107,7 @@ class User < CouchRest::Model::Base
     false
   end
 
-  def is_test?
+  def is_monitor?
     false
   end
 
diff --git a/config/defaults.yml b/config/defaults.yml
index 84ee4c9..844adaa 100644
--- a/config/defaults.yml
+++ b/config/defaults.yml
@@ -118,7 +118,7 @@ development:
   <<: *service_levels
   admins: [blue, red, staff]
   api_tokens:
-    test: nil
+    monitor: nil
     admin: nil
   domain: example.org
   secret_token: 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx'
@@ -132,8 +132,10 @@ test:
   <<: *service_levels
   admins: [admin, admin2]
   api_tokens:
-    test: "212da28a59dcaca487365309dc93aa09"
+    monitor: "212da28a59dcaca487365309dc93aa09"
     admin: nil
+    allowed_ips:
+      - 0.0.0.0
   domain: test.me
   secret_token: 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx'
   reraise_errors: true
@@ -149,7 +151,7 @@ production:
   <<: *common
   admins: []
   api_tokens:
-    test: nil
+    monitor: nil
     admin: nil
   domain: example.net
   engines:
diff --git a/test/factories.rb b/test/factories.rb
index b6e1475..5d49729 100644
--- a/test/factories.rb
+++ b/test/factories.rb
@@ -26,6 +26,10 @@ FactoryGirl.define do
       end
     end
 
+    factory :test_user do
+      login {"test_user_" + Faker::Internet.user_name + '_' + SecureRandom.hex(4)}
+    end
+
     factory :premium_user do
       effective_service_level_code 2
     end
diff --git a/test/integration/api/tmp_user_test.rb b/test/integration/api/tmp_user_test.rb
index 4c1e659..bf5f99d 100644
--- a/test/integration/api/tmp_user_test.rb
+++ b/test/integration/api/tmp_user_test.rb
@@ -4,7 +4,7 @@ require_relative 'srp_test'
 class TmpUserTest < SrpTest
 
   setup do
-    register_user('test_user_'+SecureRandom.hex(5))
+    register_user('tmp_user_'+SecureRandom.hex(5))
   end
 
   test "login with srp" do
diff --git a/test/unit/api_token_test.rb b/test/unit/api_token_test.rb
index 55d7507..266a370 100644
--- a/test/unit/api_token_test.rb
+++ b/test/unit/api_token_test.rb
@@ -6,15 +6,15 @@ class ApiTokenTest < ActiveSupport::TestCase
   end
 
   test "api token only authenticates ApiUser" do
-    token_string = APP_CONFIG['api_tokens']['test']
-    assert !token_string.nil?
+    token_string = APP_CONFIG['api_tokens']['monitor']
+    assert !token_string.nil?, 'monitor token should be configured'
     assert !token_string.empty?
     token = ApiToken.find_by_token(token_string)
     user = token.authenticate
     assert user, 'api token should authenticate'
     assert user.is_a?(ApiUser), 'api token should return api user'
-    assert user.is_test?, 'api test token should return test user'
-    assert !user.is_admin?, 'api test token should not return admin user'
+    assert user.is_monitor?, 'api monitor token should return monitor user'
+    assert !user.is_admin?, 'api monitor token should not return admin user'
   end
 
   test "invalid api tokens can't authenticate" do
diff --git a/test/unit/tmp_user_test.rb b/test/unit/tmp_user_test.rb
index 9494377..1dea5f9 100644
--- a/test/unit/tmp_user_test.rb
+++ b/test/unit/tmp_user_test.rb
@@ -6,7 +6,7 @@ class TmpUserTest < ActiveSupport::TestCase
     InviteCodeValidator.any_instance.stubs(:validate)
   end
 
-  test "test_user saved to tmp_users" do
+  test "tmp_user saved to tmp_users" do
     begin
       assert User.ancestors.include?(TemporaryUser)
 
@@ -17,7 +17,7 @@ class TmpUserTest < ActiveSupport::TestCase
       end
 
       assert_difference('User.tmp_database.info["doc_count"]') do
-        tmp_user = User.create!(:login => 'test_user_'+SecureRandom.hex(5).downcase,
+        tmp_user = User.create!(:login => 'tmp_user_'+SecureRandom.hex(5).downcase,
           :password_verifier => 'ABCDEF0010101', :password_salt => 'ABCDEF')
         assert tmp_user.database.to_s.include?('tmp')
       end