Screened ip address can be edited, deleted, and changed to allow or block.

app/assets/javascripts/admin/controllers/admin_logs_screened_ip_addresses_controller.js

@@ -9,6 +9,7 @@
 Discourse.AdminLogsScreenedIpAddressesController = Ember.ArrayController.extend(Discourse.Presence, {
   loading: false,
   content: [],
+  itemController: 'adminLogsScreenedIpAddress',
 
   show: function() {
     var self = this;
@@ -19,3 +20,72 @@ Discourse.AdminLogsScreenedIpAddressesController = Ember.ArrayController.extend(
     });
   }
 });
+
+Discourse.AdminLogsScreenedIpAddressController = Ember.ObjectController.extend({
+  editing: false,
+  savedIpAddress: null,
+
+  actions: {
+    allow: function(record) {
+      record.set('action', 'do_nothing');
+      this.send('save', record);
+    },
+
+    block: function(record) {
+      record.set('action', 'block');
+      this.send('save', record);
+    },
+
+    edit: function() {
+      if (!this.get('editing')) {
+        this.savedIpAddress = this.get('ip_address');
+      }
+      this.set('editing', true);
+    },
+
+    cancel: function() {
+      if (this.get('savedIpAddress') && this.get('editing')) {
+        this.set('ip_address', this.get('savedIpAddress'));
+      }
+      this.set('editing', false);
+    },
+
+    save: function(record) {
+      var self = this;
+      var wasEditing = this.get('editing');
+      this.set('editing', false);
+      record.save().then(function(saved){
+        if (saved.success) {
+          self.set('savedIpAddress', null);
+        } else {
+          bootbox.alert(saved.errors);
+          if (wasEditing) self.set('editing', true);
+        }
+      }, function(e){
+        if (e.responseJSON && e.responseJSON.errors) {
+          bootbox.alert(I18n.t("generic_error_with_reason", {error: e.responseJSON.errors.join('. ')}));
+        } else {
+          bootbox.alert(I18n.t("generic_error"));
+        }
+        if (wasEditing) self.set('editing', true);
+      });
+    },
+
+    destroy: function(record) {
+      var self = this;
+      return bootbox.confirm(I18n.t("admin.logs.screened_ips.delete_confirm", {ip_address: record.get('ip_address')}), I18n.t("no_value"), I18n.t("yes_value"), function(result) {
+        if (result) {
+          record.destroy().then(function(deleted) {
+            if (deleted) {
+              self.get("parentController.content").removeObject(record);
+            } else {
+              bootbox.alert(I18n.t("generic_error"));
+            }
+          }, function(e){
+            bootbox.alert(I18n.t("generic_error_with_reason", {error: "http: " + e.status + " - " + e.body}));
+          });
+        }
+      });
+    }
+  }
+});

app/assets/javascripts/admin/models/screened_ip_address.js

@@ -8,14 +8,40 @@
   @module Discourse
 **/
 Discourse.ScreenedIpAddress = Discourse.Model.extend({
-  // TODO: this is repeated in all 3 screened models. move it.
   actionName: function() {
-    if (this.get('action') === 'do_nothing') {
-      return I18n.t("admin.logs.screened_ips.allow");
+    return I18n.t("admin.logs.screened_ips.actions." + this.get('action'));
+  }.property('action'),
+
+  isBlocked: function() {
+    return (this.get('action') === 'block');
+  }.property('action'),
+
+  actionIcon: function() {
+    if (this.get('action') === 'block') {
+      return this.get('blockIcon');
     } else {
-      return I18n.t("admin.logs.screened_actions." + this.get('action'));
+      return this.get('doNothingIcon');
     }
-  }.property('action')
+  }.property('action'),
+
+  blockIcon: function() {
+    return 'icon-remove';
+  }.property(),
+
+  doNothingIcon: function() {
+    return 'icon-ok';
+  }.property(),
+
+  save: function() {
+    return Discourse.ajax("/admin/logs/screened_ip_addresses/" + this.get('id') + ".json", {
+      type: 'PUT',
+      data: {ip_address: this.get('ip_address'), action_name: this.get('action')}
+    });
+  },
+
+  destroy: function() {
+    return Discourse.ajax("/admin/logs/screened_ip_addresses/" + this.get('id') + ".json", {type: 'DELETE'});
+  }
 });
 
 Discourse.ScreenedIpAddress.reopenClass({

app/assets/javascripts/admin/templates/logs/screened_ip_addresses.js.handlebars

@@ -5,13 +5,14 @@
 {{else}}
   {{#if model.length}}
 
-    <div class='table screened-ip-addresses'>
+    <div class='table admin-logs-table screened-ip-addresses'>
       <div class="heading-container">
         <div class="col heading first ip_address">{{i18n admin.logs.ip_address}}</div>
         <div class="col heading action">{{i18n admin.logs.action}}</div>
         <div class="col heading match_count">{{i18n admin.logs.match_count}}</div>
         <div class="col heading last_match_at">{{i18n admin.logs.last_match_at}}</div>
         <div class="col heading created_at">{{i18n admin.logs.created_at}}</div>
+        <div class="col heading actions"></div>
         <div class="clearfix"></div>
       </div>
 

app/assets/javascripts/admin/templates/logs/screened_ip_addresses_list_item.js.handlebars

@@ -1,5 +1,14 @@
-<div class="col first ip_address">{{ip_address}}</div>
-<div class="col action">{{actionName}}</div>
+<div class="col first ip_address">
+  {{#if editing}}
+    {{textField value=ip_address autofocus="autofocus"}}
+  {{else}}
+    <span {{action edit this}}>{{ip_address}}</span>
+  {{/if}}
+</div>
+<div class="col action">
+  <i {{bindAttr class=":icon actionIcon"}}></i>
+  {{actionName}}
+</div>
 <div class="col match_count">{{match_count}}</div>
 <div class="col last_match_at">
   {{#if last_match_at}}
@@ -7,4 +16,18 @@
   {{/if}}
 </div>
 <div class="col created_at">{{unboundAgeWithTooltip created_at}}</div>
+<div class="col actions">
+  {{#unless editing}}
+    <button {{action destroy this}}>{{i18n admin.logs.delete}}</button>
+    <button {{action edit this}}>{{i18n admin.logs.edit}}</button>
+    {{#if isBlocked}}
+      <button {{action allow this}}><i {{bindAttr class=":icon doNothingIcon"}}></i> {{i18n admin.logs.screened_ips.actions.do_nothing}}</button>
+    {{else}}
+      <button {{action block this}}><i {{bindAttr class=":icon blockIcon"}}></i> {{i18n admin.logs.screened_ips.actions.block}}</button>
+    {{/if}}
+  {{else}}
+    <button {{action save this}}>{{i18n admin.logs.save}}</button>
+    <a {{action cancel this}}>{{i18n cancel}}</a>
+  {{/unless}}
+</div>
 <div class="clearfix"></div>

app/assets/stylesheets/common/admin/admin_base.scss

@@ -700,16 +700,44 @@ table {
 
 // Logs
 
+.admin-logs-table {
+  input.ember-text-field {
+    padding: 1px 4px;
+  }
+}
+
 .screened-emails, .screened-urls, .screened-ip-addresses {
   width: 900px;
   .email, .url {
     width: 300px;
   }
-  .action, .match_count, .last_match_at, .created_at, .ip_address {
+  .action, .match_count, .last_match_at, .created_at {
+    text-align: center;
+    width: 110px;
+  }
+}
+
+.screened-emails, .screened-urls {
+  .ip_address {
     width: 110px;
     text-align: center;
   }
 }
+.screened-ip-addresses {
+  .ip_address {
+    width: 150px;
+    text-align: left;
+    input {
+      width: 130px;
+    }
+  }
+  .actions {
+    width: 275px;
+    a {
+      text-decoration: underline;
+    }
+  }
+}
 
 .staff-actions {
   width: 100%;

app/controllers/admin/screened_ip_addresses_controller.rb

@@ -1,8 +1,34 @@
 class Admin::ScreenedIpAddressesController < Admin::AdminController
 
+  before_filter :fetch_screened_ip_address, only: [:update, :destroy]
+
   def index
-    screened_emails = ScreenedIpAddress.limit(200).order('last_match_at desc').to_a
-    render_serialized(screened_emails, ScreenedIpAddressSerializer)
+    screened_ip_addresses = ScreenedIpAddress.limit(200).order('last_match_at desc').to_a
+    render_serialized(screened_ip_addresses, ScreenedIpAddressSerializer)
   end
 
+  def update
+    if @screened_ip_address.update_attributes(allowed_params)
+      render json: success_json
+    else
+      render_json_error(@screened_ip_address)
+    end
+  end
+
+  def destroy
+    @screened_ip_address.destroy
+    render json: success_json
+  end
+
+  private
+
+    def allowed_params
+      params.require(:ip_address)
+      params.permit(:ip_address, :action_name)
+    end
+
+    def fetch_screened_ip_address
+      @screened_ip_address = ScreenedIpAddress.find(params[:id])
+    end
+
 end

app/models/screened_ip_address.rb

@@ -8,7 +8,7 @@ class ScreenedIpAddress < ActiveRecord::Base
 
   default_action :block
 
-  validates :ip_address, presence: true, uniqueness: true
+  validates :ip_address, ip_address_format: true, presence: true
 
   def self.watch(ip_address, opts={})
     match_for_ip_address(ip_address) || create(opts.slice(:action_type).merge(ip_address: ip_address))

app/serializers/screened_ip_address_serializer.rb

@@ -1,5 +1,6 @@
 class ScreenedIpAddressSerializer < ApplicationSerializer
-  attributes :ip_address,
+  attributes :id,
+             :ip_address,
              :action,
              :match_count,
              :last_match_at,

config/locales/client.en.yml

@@ -1223,6 +1223,9 @@ en:
         last_match_at: "Last Matched"
         match_count: "Matches"
         ip_address: "IP"
+        delete: 'Delete'
+        edit: 'Edit'
+        save: 'Save'
         screened_actions:
           block: "block"
           do_nothing: "do nothing"
@@ -1260,7 +1263,10 @@ en:
         screened_ips:
           title: "Screened IPs"
           description: "IP addresses that are being watched."
-          allow: "allow"
+          delete_confirm: "Are you sure you want to remove the rule for %{ip_address}?"
+          actions:
+            block: "Block"
+            do_nothing: "Allow"
 
       impersonate:
         title: "Impersonate User"

config/routes.rb

@@ -67,7 +67,7 @@ Discourse::Application.routes.draw do
     scope '/logs' do
       resources :staff_action_logs,     only: [:index]
       resources :screened_emails,       only: [:index]
-      resources :screened_ip_addresses, only: [:index]
+      resources :screened_ip_addresses, only: [:index, :update, :destroy]
       resources :screened_urls,         only: [:index]
     end
 

lib/screening_model.rb

@@ -23,6 +23,11 @@ module ScreeningModel
     self.action_type ||= self.class.actions[self.class.df_action]
   end
 
+  def action_name=(arg)
+    raise ArgumentError.new("Invalid action type #{arg}") if arg.nil? or !self.class.actions.has_key?(arg.to_sym)
+    self.action_type = self.class.actions[arg.to_sym]
+  end
+
   def record_match!
     self.match_count += 1
     self.last_match_at = Time.zone.now

lib/validators/ip_address_format_validator.rb

@@ -0,0 +1,11 @@
+# Allows unique IP address (10.0.1.20), and IP addresses with a mask (10.0.0.0/8).
+# Useful when storing in a Postgresql inet column.
+class IpAddressFormatValidator < ActiveModel::EachValidator
+
+  def validate_each(record, attribute, value)
+    unless record.ip_address.nil? or record.ip_address.split('/').first =~ Resolv::AddressRegex
+      record.errors.add(attribute, :invalid)
+    end
+  end
+
+end

spec/components/validators/allowed_ip_address_validator_spec.rb

@@ -30,4 +30,5 @@ describe AllowedIpAddressValidator do
       record.errors[:ip_address].should_not be_present
     end
   end
+
 end

spec/components/validators/ip_address_format_validator_spec.rb

@@ -0,0 +1,22 @@
+require 'spec_helper'
+
+describe IpAddressFormatValidator do
+
+  let(:record) { Fabricate.build(:screened_ip_address, ip_address: '99.232.23.123') }
+  let(:validator) { described_class.new({attributes: :ip_address}) }
+  subject(:validate) { validator.validate_each(record, :ip_address, record.ip_address) }
+
+  [nil, '99.232.23.123', '99.232.0.0/16', 'fd12:db8::ff00:42:8329', 'fc00::/7'].each do |arg|
+    it "should not add an error for #{arg}" do
+      record.ip_address = arg
+      validate
+      record.errors[:ip_address].should_not be_present
+    end
+  end
+
+  it 'should add an error for invalid IP address' do
+    record.ip_address = '99.99.99'
+    validate
+    record.errors[:ip_address].should be_present
+  end
+end

spec/controllers/admin/screened_ip_addresses_controller_spec.rb

@@ -7,7 +7,7 @@ describe Admin::ScreenedIpAddressesController do
 
   let!(:user) { log_in(:admin) }
 
-  context '.index' do
+  describe 'index' do
     before do
       xhr :get, :index
     end

spec/models/screened_ip_address_spec.rb

@@ -8,6 +8,29 @@ describe ScreenedIpAddress do
     it 'sets a default action_type' do
       described_class.create(valid_params).action_type.should == described_class.actions[:block]
     end
+
+    it 'sets an error when ip_address is invalid' do
+      described_class.create(valid_params.merge(ip_address: '99.99.99')).errors[:ip_address].should be_present
+    end
+
+    it 'can set action_type using the action_name virtual attribute' do
+      described_class.new(valid_params.merge(action_name: :do_nothing)).action_type.should == described_class.actions[:do_nothing]
+      described_class.new(valid_params.merge(action_name: :block)).action_type.should == described_class.actions[:block]
+      described_class.new(valid_params.merge(action_name: 'do_nothing')).action_type.should == described_class.actions[:do_nothing]
+      described_class.new(valid_params.merge(action_name: 'block')).action_type.should == described_class.actions[:block]
+    end
+
+    it 'raises a useful exception when action is invalid' do
+      expect {
+        described_class.new(valid_params.merge(action_name: 'dance'))
+      }.to raise_error(ArgumentError)
+    end
+
+    it 'raises a useful exception when action is nil' do
+      expect {
+        described_class.new(valid_params.merge(action_name: nil))
+      }.to raise_error(ArgumentError)
+    end
   end
 
   describe '#watch' do