Merge branch 'add-irker-options' into 'master'

Add Irker service configuration options

### What does this MR do?

This MR makes a number of hard-coded Irker parameters configurable in the service settings: Irker server host, port, and default IRC URI. It also removes the "max recipient" limit since the recipient list is configurable only by the project owner, and it makes no sense to update the limit when it is implied in the recipient list already.

### Why was this MR needed?

The existing service assumed that gitlab.com was running an Irker daemon on `localhost` when it was not. Using Irker on gitlab.com thus did not work at all. This MR allows users to provide their own Irker daemons.

### Are there points in the code the reviewer needs to double check?

My main concern is whether allowing a user to specify the server/port combination would have security implications for a host. Given that HipChat and Slack allow users to do this, I didn't think this was doing anything novel.

### What are the relevant issue numbers?

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

### Screenshots

### Before

![image](https://gitlab.com/stanhu/gitlab-ce/uploads/2eb3eb815e249e9fb669fc97ecd4f3c8/image.png)

### After

![image](https://gitlab.com/gitlab-org/gitlab-ce/uploads/cceaba951c05bd3df2c842cc68046b87/image.png)

See merge request !930

CHANGELOG

@@ -5,6 +5,7 @@ v 7.13.0 (unreleased)
   - Fix external issue tracker hook/test for HTTPS URLs (Daniel Gerhardt)
   - Remove link leading to a 404 error in Deploy Keys page (Stan Hu)
   - Add support for unlocking users in admin settings (Stan Hu)
+  - 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

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]

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/

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)

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.
-

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