Add Irker service configuration options

Closes #1713
Closes #1714
Closes gitlab-com/support-forum#139

6 files changed, 85 insertions, 121 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 616b41a4269..762ee0bb549 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 7.13.0 (unreleased)
+  - Add Irker service configuration options (Stan Hu)
   - Fix order of issues imported form GitHub (Hiroyuki Sato)
   - Bump rugments to 1.0.0beta8 to fix C prototype function highlighting (Jonathon Reinhart)
   - Fix Merge Request webhook to properly fire "merge" action when accepted from the web UI
diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb
index dc18bbd8d5b..1e435be8275 100644
--- a/app/controllers/projects/services_controller.rb
+++ b/app/controllers/projects/services_controller.rb
@@ -7,7 +7,8 @@ class Projects::ServicesController < Projects::ApplicationController
                     :colorize_messages, :channels,
                     :push_events, :issues_events, :merge_requests_events, :tag_push_events,
                     :note_events, :send_from_committer_email, :disable_diffs, :external_wiki_url,
-                    :notify, :color]
+                    :notify, :color,
+                    :server_host, :server_port, :default_irc_uri]
   # Authorize
   before_action :authorize_admin_project!
   before_action :service, only: [:edit, :update, :test]
diff --git a/app/models/project_services/irker_service.rb b/app/models/project_services/irker_service.rb
index 89f312e8c98..a4d0914dbe7 100644
--- a/app/models/project_services/irker_service.rb
+++ b/app/models/project_services/irker_service.rb
@@ -21,26 +21,11 @@
 require 'uri'
 
 class IrkerService < Service
+  prop_accessor :server_host, :server_port, :default_irc_uri
   prop_accessor :colorize_messages, :recipients, :channels
   validates :recipients, presence: true, if: :activated?
-  validate :check_recipients_count, if: :activated?
 
   before_validation :get_channels
-  after_initialize :initialize_settings
-
-  # Writer for RSpec tests
-  attr_writer :settings
-
-  def initialize_settings
-    # See the documentation (doc/project_services/irker.md) for possible values
-    # here
-    @settings ||= {
-      server_ip: 'localhost',
-      server_port: 6659,
-      max_channels: 3,
-      default_irc_uri: nil
-    }
-  end
 
   def title
     'Irker (IRC gateway)'
@@ -51,20 +36,6 @@ class IrkerService < Service
     'gateway.'
   end
 
-  def help
-    msg = 'Recipients have to be specified with a full URI: '\
-    'irc[s]://irc.network.net[:port]/#channel. Special cases: if you want '\
-    'the channel to be a nickname instead, append ",isnick" to the channel '\
-    'name; if the channel is protected by a secret password, append '\
-    '"?key=secretpassword" to the URI.'
-
-    unless @settings[:default_irc].nil?
-      msg += ' Note that a default IRC URI is provided by this service\'s '\
-      "administrator: #{default_irc}. You can thus just give a channel name."
-    end
-    msg
-  end
-
   def to_param
     'irker'
   end
@@ -77,30 +48,45 @@ class IrkerService < Service
     return unless supported_events.include?(data[:object_kind])
 
     IrkerWorker.perform_async(project_id, channels,
-                              colorize_messages, data, @settings)
+                              colorize_messages, data, settings)
+  end
+
+  def settings
+    { server_host: server_host.present? ? server_host : 'localhost',
+      server_port: server_port.present? ? server_port : 6659
+    }
   end
 
   def fields
     [
+      { type: 'text', name: 'server_host', placeholder: 'localhost',
+        help: 'Irker daemon hostname (defaults to localhost)' },
+      { type: 'text', name: 'server_port', placeholder: 6659,
+        help: 'Irker daemon port (defaults to 6659)' },
+      { type: 'text', name: 'default_irc_uri',
+        help: 'A default IRC URI to prepend before each recipient (optional)',
+        placeholder: 'irc://irc.network.net:6697/' },
       { type: 'textarea', name: 'recipients',
-        placeholder: 'Recipients/channels separated by whitespaces' },
+        placeholder: 'Recipients/channels separated by whitespaces',
+        help: 'Recipients have to be specified with a full URI: '\
+        'irc[s]://irc.network.net[:port]/#channel. Special cases: if '\
+        'you want the channel to be a nickname instead, append ",isnick" to ' \
+        'the channel name; if the channel is protected by a secret password, ' \
+        ' append "?key=secretpassword" to the URI. Note that if you specify a ' \
+        ' default IRC URI to prepend before each recipient, you can just give ' \
+        ' a channel name.'  },
       { type: 'checkbox', name: 'colorize_messages' },
     ]
   end
 
-  private
-
-  def check_recipients_count
-    return true if recipients.nil? || recipients.empty?
-
-    if recipients.split(/\s+/).count > max_chans
-      errors.add(:recipients, "are limited to #{max_chans}")
-    end
+  def help
+    ' NOTE: Irker does NOT have built-in authentication, which makes it' \
+    ' vulnerable to spamming IRC channels if it is hosted outside of a ' \
+    ' firewall. Please make sure you run the daemon within a secured network ' \
+    ' to prevent abuse. For more details, read: http://www.catb.org/~esr/irker/security.html.'
   end
 
-  def max_chans
-    @settings[:max_channels]
-  end
+  private
 
   def get_channels
     return true unless :activated?
@@ -114,40 +100,35 @@ class IrkerService < Service
 
   def map_recipients
     self.channels = recipients.split(/\s+/).map do |recipient|
-      format_channel default_irc_uri, recipient
+      format_channel(recipient)
     end
     channels.reject! &:nil?
   end
 
-  def default_irc_uri
-    default_irc = @settings[:default_irc_uri]
-    if !(default_irc.nil? || default_irc[-1] == '/')
-      default_irc += '/'
-    end
-    default_irc
-  end
-
-  def format_channel(default_irc, recipient)
-    cnt = 0
-    url = nil
+  def format_channel(recipient)
+    uri = nil
 
     # Try to parse the chan as a full URI
     begin
-      uri = URI.parse(recipient)
-      raise URI::InvalidURIError if uri.scheme.nil? && cnt == 0
+      uri = consider_uri(URI.parse(recipient))
     rescue URI::InvalidURIError
-      unless default_irc.nil?
-        cnt += 1
-        recipient = "#{default_irc}#{recipient}"
-        retry if cnt == 1
+    end
+
+    unless uri.present? and default_irc_uri.nil?
+      begin
+        new_recipient = URI.join(default_irc_uri, '/', recipient).to_s
+        uri = consider_uri(URI.parse(new_recipient))
+      rescue
+        Rails.logger.error("Unable to create a valid URL from #{default_irc_uri} and #{recipient}")
       end
-    else
-      url = consider_uri uri
     end
-    url
+
+    uri
   end
 
   def consider_uri(uri)
+    return nil if uri.scheme.nil?
+
     # Authorize both irc://domain.com/#chan and irc://domain.com/chan
     if uri.is_a?(URI) && uri.scheme[/^ircs?\z/] && !uri.path.nil?
       # Do not authorize irc://domain.com/
diff --git a/app/workers/irker_worker.rb b/app/workers/irker_worker.rb
index 84a54656df2..2d44d8d4dc6 100644
--- a/app/workers/irker_worker.rb
+++ b/app/workers/irker_worker.rb
@@ -19,7 +19,7 @@ class IrkerWorker
       branch = "\x0305#{branch}\x0f"
     end
 
-    # Firsts messages are for branch creation/deletion
+    # First messages are for branch creation/deletion
     send_branch_updates push_data, project, repo_name, committer, branch
 
     # Next messages are for commits
@@ -34,7 +34,7 @@ class IrkerWorker
   def init_perform(set, chans, colors)
     @colors = colors
     @channels = chans
-    start_connection set['server_ip'], set['server_port']
+    start_connection set['server_host'], set['server_port']
   end
 
   def start_connection(irker_server, irker_port)
diff --git a/doc/project_services/irker.md b/doc/project_services/irker.md
index 9875bebf66b..1dbca20baf9 100644
--- a/doc/project_services/irker.md
+++ b/doc/project_services/irker.md
@@ -9,38 +9,43 @@ See the project homepage for further info: https://gitlab.com/esr/irker
 ## Needed setup
 
 You will first need an Irker daemon. You can download the Irker code from its
-gitorious repository on https://gitorious.org/irker: `git clone
-git@gitorious.org:irker/irker.git`. Once you have downloaded the code, you can
-run the python script named `irkerd`. This script is the gateway script, it acts
-both as an IRC client, for sending messages to an IRC server obviously, and as a
-TCP server, for receiving messages from the GitLab service.
+repository on https://gitlab.com/esr/irker:
 
-If the Irker server runs on the same machine, you are done. If not, you will
-need to follow the firsts steps of the next section.
+```
+git clone https://gitlab.com/esr/irker.git
+```
 
-## Optional setup
+Once you have downloaded the code, you can run the python script named `irkerd`.
+This script is the gateway script, it acts both as an IRC client, for sending
+messages to an IRC server obviously, and as a TCP server, for receiving messages
+from the GitLab service.
 
-In the `app/models/project_services/irker_service.rb` file, you can modify some
-options in the `initialize_settings` method:
-- **server_ip** (defaults to `localhost`): the server IP address where the
-`irkerd` daemon runs;
-- **server_port** (defaults to `6659`): the server port of the `irkerd` daemon;
-- **max_channels** (defaults to `3`): the maximum number of recipients the
-client is authorized to join, per project;
-- **default_irc_uri** (no default) : if this option is set, it has to be in the
-format `irc[s]://domain.name` and will be prepend to each and every channel
-provided by the user which is not a full URI.
+If the Irker server runs on the same machine, you are done. If not, you will
+need to follow the firsts steps of the next section.
 
-If the Irker server and the GitLab application do not run on the same host, you
-will **need** to setup at least the **server_ip** option.
+## Complete these steps in GitLab:
+
+1. Navigate to the project you want to configure for notifications.
+1. Select "Settings" in the top navigation.
+1. Select "Services" in the left navigation.
+1. Click "Irker".
+1. Select the "Active" checkbox.
+1. Enter the server host address where `irkerd` runs (defaults to `localhost`)
+in the `Server host` field on the Web page
+1. Enter the server port of `irkerd` (e.g. defaults to 6659) in the
+`Server port` field on the Web page.
+1. Optional: if `Default irc uri` is set, it has to be in the format
+`irc[s]://domain.name` and will be prepend to each and every channel provided
+by the user which is not a full URI.
+1. Specify the recipients (e.g. #channel1, user1, etc.)
+1. Save or optionally click "Test Settings".
 
 ## Note on Irker recipients
 
 Irker accepts channel names of the form `chan` and `#chan`, both for the
 `#chan` channel. If you want to send messages in query, you will need to add
-`,isnick` avec the channel name, in this form: `Aorimn,isnick`. In this latter
+`,isnick` after the channel name, in this form: `Aorimn,isnick`. In this latter
 case, `Aorimn` is treated as a nick and no more as a channel name.
 
 Irker can also join password-protected channels. Users need to append
 `?key=thesecretpassword` to the chan name.
-
diff --git a/spec/models/project_services/irker_service_spec.rb b/spec/models/project_services/irker_service_spec.rb
index 37690434ec8..7d483a44c53 100644
--- a/spec/models/project_services/irker_service_spec.rb
+++ b/spec/models/project_services/irker_service_spec.rb
@@ -38,22 +38,6 @@ describe IrkerService do
       let(:_recipients) { nil }
       it { should validate_presence_of :recipients }
     end
-
-    context 'too many recipients' do
-      let(:_recipients) { 'a b c d' }
-      it 'should add an error if there is too many recipients' do
-        subject.send :check_recipients_count
-        expect(subject.errors).not_to be_blank
-      end
-    end
-
-    context '3 recipients' do
-      let(:_recipients) { 'a b c' }
-      it 'should not add an error if there is 3 recipients' do
-        subject.send :check_recipients_count
-        expect(subject.errors).to be_blank
-      end
-    end
   end
 
   describe 'Execute' do
@@ -62,7 +46,7 @@ describe IrkerService do
     let(:project) { create(:project) }
     let(:sample_data) { Gitlab::PushDataBuilder.build_sample(project, user) }
 
-    let(:recipients) { '#commits' }
+    let(:recipients) { '#commits irc://test.net/#test ftp://bad' }
     let(:colorize_messages) { '1' }
 
     before do
@@ -71,17 +55,12 @@ describe IrkerService do
         project: project,
         project_id: project.id,
         service_hook: true,
-        properties: {
-          'recipients' => recipients,
-          'colorize_messages' => colorize_messages
-        }
-      )
-      irker.settings = {
-        server_ip: 'localhost',
+        server_host: 'localhost',
         server_port: 6659,
-        max_channels: 3,
-        default_irc_uri: 'irc://chat.freenode.net/'
-      }
+        default_irc_uri: 'irc://chat.freenode.net/',
+        recipients: recipients,
+        colorize_messages: colorize_messages)
+
       irker.valid?
       @irker_server = TCPServer.new 'localhost', 6659
     end
@@ -97,11 +76,8 @@ describe IrkerService do
       conn.readlines.each do |line|
         msg = JSON.load(line.chomp("\n"))
         expect(msg.keys).to match_array(['to', 'privmsg'])
-        if msg['to'].is_a?(String)
-          expect(msg['to']).to eq 'irc://chat.freenode.net/#commits'
-        else
-          expect(msg['to']).to match_array(['irc://chat.freenode.net/#commits'])
-        end
+        expect(msg['to']).to match_array(["irc://chat.freenode.net/#commits",
+                                          "irc://test.net/#test"])
       end
       conn.close
     end