Store IPs as JSON and validate them

app/models/carto/dbdirect_ip.rb

@@ -1,6 +1,47 @@
+require 'ip_checker'
+
 module Carto
   class DbdirectIp< ActiveRecord::Base
     belongs_to :user, inverse_of: :dbdirect_ip, foreign_key: :user_id
-    # TODO: validate ips
+
+    serialize :ips, JSON
+    validate :validate_ips
+
+    private
+
+    MAX_IP_MASK_HOST_BITS = 8
+
+    def validate_ips
+      support_legacy_ips_format
+      # Check type
+      unless ips.nil? || ips.kind_of?(Array) || ips.any? { |ip| !is.kind_of?(String) }
+        errors.add(:ips, "IPs must be either be nil or an array of strings ")
+        return false
+      end
+      ok = true
+      if ips.present?
+        ips.each do |ip|
+          error = IpChecker.validate(
+            ip,
+            max_host_bits: MAX_IP_MASK_HOST_BITS,
+            exclude_0: true,
+            exclude_private: true,
+            exclude_local: true,
+            exclude_loopback: true
+          )
+          if error.present?
+            ok = false
+            errors.add(:ips, error)
+          end
+        end
+      end
+      ok
+    end
+
+    def support_legacy_ips_format
+      if ips.present? && ips.kind_of?(String)
+        self.ips = ips.split(',')
+      end
+    end
   end
 end

app/models/carto/user.rb

@@ -323,7 +323,7 @@ class Carto::User < ActiveRecord::Base
     else
       dbdirect_ip&.destroy
       if ips.present?
-        create_dbdirect_ip(ips: ips)
+        create_dbdirect_ip!(ips: ips)
       end
     end
   end

db/migrate/20200330173736_create_dbdirect_ips.rb

@@ -7,7 +7,7 @@ migration(
     create_table :dbdirect_ips do
       Uuid        :id, primary_key: true, default: Sequel.lit('uuid_generate_v4()')
       foreign_key :user_id, :users, type: :uuid, null: false, index: true, on_delete: :cascade
-      String      :ips, null: true
+      String      :ips, null: true, type: 'json'
       DateTime    :created_at, null: false
     end
   end,

spec/requests/carto/api/dbdirect_ips_controller_spec.rb

@@ -34,7 +34,7 @@ describe Carto::Api::DbdirectIpsController do
 
     it 'needs authentication for ips creation' do
       params = {
-        ips: '10.20.30.40'
+        ips: ['100.20.30.40']
       }
       post_json api_v1_dbdirect_ips_update_url(params) do |response|
         expect(response.status).to eq(401)
@@ -43,7 +43,7 @@ describe Carto::Api::DbdirectIpsController do
 
     it 'needs the feature flag for ips creation' do
         params = {
-          ips: '10.20.30.40',
+          ips: ['100.20.30.40'],
           api_key: @user1.api_key
         }
         with_feature_flag @user1, 'dbdirect', false do
@@ -54,7 +54,7 @@ describe Carto::Api::DbdirectIpsController do
     end
 
     it 'creates ips with api_key authentication' do
-      ips = '10.20.30.40'
+      ips = ['100.20.30.40']
       params = {
         ips: ips,
         api_key: @user1.api_key
@@ -69,7 +69,7 @@ describe Carto::Api::DbdirectIpsController do
     end
 
     it 'creates ips with login authentication' do
-      ips = '10.20.30.40'
+      ips = ['100.20.30.40']
       params = {
         ips: ips
       }
@@ -84,8 +84,8 @@ describe Carto::Api::DbdirectIpsController do
     end
 
     it 'retains only latest ips assigned' do
-      ips1 = '10.20.30.40'
-      ips2 = '11.21.31.41'
+      ips1 = ['100.20.30.40', '200.20.30.40/24']
+      ips2 = ['11.21.31.41']
       with_feature_flag @user1, 'dbdirect', true do
         params = {
           ips: ips1,
@@ -108,12 +108,33 @@ describe Carto::Api::DbdirectIpsController do
         end
       end
     end
+
+    it 'rejects invalid IPs' do
+      invalid_ips = [
+        '0.0.0.0', '10.20.30.40', '127.0.0.1', '192.168.1.1', '120.120.120.120/20', '100.100.100.300', 'not-an-ip'
+      ]
+      invalid_ips.each do |ip|
+        params = {
+          ips: [ip],
+          api_key: @user1.api_key
+        }
+        with_feature_flag @user1, 'dbdirect', true do
+          post_json api_v1_dbdirect_ips_update_url(params) do |response|
+            expect(response.status).to eq(422)
+            expect(response.body[:errors]).not_to be_nil
+            expect(response.body[:errors][:ips]).not_to be_nil
+            expect(Carto::User.find(@user1.id).dbdirect_effective_ips).to be_nil
+          end
+        end
+
+      end
+    end
   end
 
   describe '#destroy' do
     before(:each) do
       @params = { api_key: @user1.api_key }
-      @existing_ips = '10.20.30.40'
+      @existing_ips = ['100.20.30.40']
       Carto::User.find_by_id(@user1.id).dbdirect_effective_ips = @existing_ips
     end
 
@@ -170,7 +191,7 @@ describe Carto::Api::DbdirectIpsController do
 
   describe '#show' do
     before(:each) do
-      @ips = '10.20.30.40'
+      @ips = ['100.20.30.40']
       Carto::User.find_by_id(@user1.id).dbdirect_effective_ips = @ips
     end