From 5aea0fbb196c20f32ec20afccaab5211d029c143 Mon Sep 17 00:00:00 2001 From: Jure Grabnar Date: Fri, 23 Sep 2016 11:42:12 +0200 Subject: [PATCH] Refactor the rest of WOPI logic from Nejc --- app/controllers/assets_controller.rb | 18 +++--- app/controllers/wopi_controller.rb | 20 +++---- app/models/asset.rb | 76 ++++++++++++------------ app/models/user.rb | 25 ++++---- app/models/wopi_action.rb | 17 +++--- app/models/wopi_app.rb | 16 +++--- app/models/wopi_discovery.rb | 76 +++++++++++++----------- app/utilities/wopi_util.rb | 83 +++++++++++++-------------- app/views/assets/edit.erb | 78 ++++++++++++------------- app/views/assets/view.erb | 78 ++++++++++++------------- config/application.rb | 11 ++-- db/migrate/20160728145000_add_wopi.rb | 10 ++-- 12 files changed, 247 insertions(+), 261 deletions(-) diff --git a/app/controllers/assets_controller.rb b/app/controllers/assets_controller.rb index 640d07ebe..9671bfa87 100644 --- a/app/controllers/assets_controller.rb +++ b/app/controllers/assets_controller.rb @@ -66,31 +66,27 @@ class AssetsController < ApplicationController end def edit - @action_url = @asset.get_action_url(current_user,"edit",false) + @action_url = @asset.get_action_url(current_user, 'edit', false) @token = current_user.get_wopi_token - @ttl = (current_user.wopi_token_ttl*1000).to_s + @ttl = (current_user.wopi_token_ttl * 1000).to_s end def view - @action_url = @asset.get_action_url(current_user,"view",false) + @action_url = @asset.get_action_url(current_user, 'view', false) @token = current_user.get_wopi_token - @ttl = (current_user.wopi_token_ttl*1000).to_s + @ttl = (current_user.wopi_token_ttl * 1000).to_s end private def load_vars @asset = Asset.find_by_id(params[:id]) - - unless @asset - render_404 - end + render_404 unless @asset step_assoc = @asset.step result_assoc = @asset.result - - @assoc = step_assoc if not step_assoc.nil? - @assoc = result_assoc if not result_assoc.nil? + @assoc = step_assoc unless step_assoc.nil? + @assoc = result_assoc unless result_assoc.nil? if @assoc.class == Step @protocol = @asset.step.protocol diff --git a/app/controllers/wopi_controller.rb b/app/controllers/wopi_controller.rb index b18e234ea..9c2a472f9 100644 --- a/app/controllers/wopi_controller.rb +++ b/app/controllers/wopi_controller.rb @@ -92,7 +92,7 @@ class WopiController < ActionController::Base logger.warn 'WOPI: lock; ' + lock.to_s render nothing: :true, status: 404 and return if lock.nil? || lock.blank? @asset.with_lock do - if @asset.is_locked + if @asset.locked? if @asset.lock == lock @asset.refresh_lock response.headers['X-WOPI-ItemVersion'] = @asset.version @@ -117,7 +117,7 @@ class WopiController < ActionController::Base render nothing: :true, status: 400 and return end @asset.with_lock do - if @asset.is_locked + if @asset.locked? if @asset.lock == old_lock @asset.unlock @asset.lock_asset(lock) @@ -138,9 +138,9 @@ class WopiController < ActionController::Base lock = request.headers['X-WOPI-Lock'] render nothing: :true, status: 400 and return if lock.nil? || lock.blank? @asset.with_lock do - if @asset.is_locked - logger.warn 'WOPI: current asset lock: #{@asset.lock}, - unlocking lock #{lock}' + if @asset.locked? + logger.warn "WOPI: current asset lock: #{@asset.lock}, + unlocking lock #{lock}" if @asset.lock == lock @asset.unlock response.headers['X-WOPI-ItemVersion'] = @asset.version @@ -161,7 +161,7 @@ class WopiController < ActionController::Base lock = request.headers['X-WOPI-Lock'] render nothing: :true, status: 400 and return if lock.nil? || lock.blank? @asset.with_lock do - if @asset.is_locked + if @asset.locked? if @asset.lock == lock @asset.refresh_lock response.headers['X-WOPI-ItemVersion'] = @asset.version @@ -180,7 +180,7 @@ class WopiController < ActionController::Base def get_lock @asset.with_lock do - if @asset.is_locked + if @asset.locked? response.headers['X-WOPI-Lock'] = @asset.lock else response.headers['X-WOPI-Lock'] = '' @@ -193,7 +193,7 @@ class WopiController < ActionController::Base def put_file @asset.with_lock do lock = request.headers['X-WOPI-Lock'] - if @asset.is_locked + if @asset.locked? if @asset.lock == lock logger.warn 'WOPI: replacing file' @asset.update_contents(request.body) @@ -263,8 +263,8 @@ class WopiController < ActionController::Base url = request.original_url.upcase.encode('utf-8') if convert_to_unix_timestamp(timestamp) + 20.minutes >= Time.now - if get_discovery.verify_proof(token, timestamp, signed_proof, - signed_proof_old, url) + if current_wopi_discovery.verify_proof(token, timestamp, signed_proof, + signed_proof_old, url) logger.warn 'WOPI: proof verification: successful' else logger.warn 'WOPI: proof verification: not verified' diff --git a/app/models/asset.rb b/app/models/asset.rb index 44eae8002..e82467a10 100644 --- a/app/models/asset.rb +++ b/app/models/asset.rb @@ -288,9 +288,12 @@ class Asset < ActiveRecord::Base end end - # Preserving attachments (on client-side) between failed validations (only usable for small/few files!) - # Needs to be called before save method and view needs to have :file_content and :file_info hidden field - # If file is an image, it can be viewed on front-end using @preview_cached with image_tag tag + # Preserving attachments (on client-side) between failed validations + # (only usable for small/few files!). + # Needs to be called before save method and view needs to have + # :file_content and :file_info hidden field. + # If file is an image, it can be viewed on front-end + # using @preview_cached with image_tag tag. def preserve(file_data) if file_data[:file_content].present? restore_cached(file_data[:file_content], file_data[:file_info]) @@ -299,28 +302,31 @@ class Asset < ActiveRecord::Base end def can_perform_action(action) - file_ext = file_file_name.split(".").last - action = get_action(file_ext,action) - if action.nil? - return false - end + file_ext = file_file_name.split('.').last + action = get_action(file_ext, action) + return false if action.nil? true end - - def get_action_url(user,action,with_tokens = true) - file_ext = file_file_name.split(".").last - action = get_action(file_ext,action) + def get_action_url(user, action, with_tokens = true) + file_ext = file_file_name.split('.').last + action = get_action(file_ext, action) if !action.nil? action_url = action.urlsrc - action_url = action_url.gsub(//, "IsLicensedUser=0&") - action_url = action_url.gsub(//, "IsLicensedUser=0") - action_url = action_url.gsub(/<.*?=.*?>/, "") + action_url = action_url.gsub(//, + 'IsLicensedUser=0&') + action_url = action_url.gsub(//, + 'IsLicensedUser=0') + action_url = action_url.gsub(/<.*?=.*?>/, '') - rest_url = Rails.application.routes.url_helpers.wopi_rest_endpoint_url(host: ENV["WOPI_ENDPOINT_URL"],id: id) - action_url = action_url + "WOPISrc=#{rest_url}" + rest_url = Rails.application.routes.url_helpers.wopi_rest_endpoint_url( + host: ENV['WOPI_ENDPOINT_URL'], + id: id + ) + action_url += "WOPISrc=#{rest_url}" if with_tokens - action_url = action_url + "&access_token=#{user.get_wopi_token}&access_token_ttl=#{(user.wopi_token_ttl*1000).to_s}" + action_url + "&access_token=#{user.get_wopi_token}"\ + "&access_token_ttl=#{(user.wopi_token_ttl * 1000)}" else action_url end @@ -329,57 +335,49 @@ class Asset < ActiveRecord::Base end end - #is_locked, lock_asset and refresh_lock rely on the asset being locked in the database to prevent race conditions - def is_locked - if lock.nil? - return false - else - return true - end + # locked?, lock_asset and refresh_lock rely on the asset + # being locked in the database to prevent race conditions + def locked? + !lock.nil? end def lock_asset(lock_string) self.lock = lock_string self.lock_ttl = Time.now.to_i + LOCK_DURATION delay(queue: :assets, run_at: LOCK_DURATION.seconds.from_now).unlock_expired - self.save! + save! end def refresh_lock self.lock_ttl = Time.now.to_i + LOCK_DURATION delay(queue: :assets, run_at: LOCK_DURATION.seconds.from_now).unlock_expired - self.save! + save! end def unlock self.lock = nil self.lock_ttl = nil - self.save! + save! end def unlock_expired - self.with_lock do - if !self.lock_ttl.nil? and self.lock_ttl>= Time.now.to_i + with_lock do + if !lock_ttl.nil? && lock_ttl >= Time.now.to_i self.lock = nil self.lock_ttl = nil - self.save! + save! end end end def update_contents(new_file) new_file.class.class_eval { attr_accessor :original_filename } - new_file.original_filename = self.file_file_name + new_file.original_filename = file_file_name self.file = new_file - if self.version.nil? - self.version = 1 - else - self.version = self.version + 1 - end - self.save + self.version = version.nil? ? 1 : version + 1 + save end - protected # Checks if attachments is an image (in post processing imagemagick will diff --git a/app/models/user.rb b/app/models/user.rb index b3d1dc20b..c34060677 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -253,33 +253,28 @@ class User < ActiveRecord::Base end def self.find_by_valid_wopi_token(token) - Rails.logger.warn "Searching by token #{token}" - user = User.where("wopi_token = ?", token).first - return user + Rails.logger.warn "WOPI: searching by token #{token}" + User.where('wopi_token = ?', token).first end def token_valid - if !self.wopi_token.nil? and (self.wopi_token_ttl==0 or self.wopi_token_ttl > Time.now.to_i) - return true - else - return false - end + !wopi_token.nil? && (wopi_token_ttl.zero? || wopi_token_ttl > Time.now.to_i) end def get_wopi_token unless token_valid - # if current token is not valid generate a new one with a one day TTL + # If current token is not valid generate a new one with a one day TTL self.wopi_token = Devise.friendly_token(20) # WOPI uses millisecond TTLs - self.wopi_token_ttl = Time.now.to_i + 60*60*24 - self.save - Rails.logger.warn("Generating new token #{self.wopi_token}") + self.wopi_token_ttl = Time.now.to_i + 1.days + save + Rails.logger.warn("Generating new token #{wopi_token}") end - Rails.logger.warn("Returning token #{self.wopi_token}") - self.wopi_token + Rails.logger.warn("Returning token #{wopi_token}") + wopi_token end -protected + protected def time_zone_check if time_zone.nil? or ActiveSupport::TimeZone.new(time_zone).nil? diff --git a/app/models/wopi_action.rb b/app/models/wopi_action.rb index 3c9efa432..aa58df170 100644 --- a/app/models/wopi_action.rb +++ b/app/models/wopi_action.rb @@ -1,12 +1,9 @@ class WopiAction < ActiveRecord::Base + belongs_to :wopi_app, foreign_key: 'wopi_app_id', class_name: 'WopiApp' + validates :action, :extension, :urlsrc, :wopi_app, presence: true - belongs_to :wopi_app, :foreign_key => 'wopi_app_id', :class_name => 'WopiApp' - validates :action,:extension,:urlsrc,:wopi_app, presence: true - - - def self.find_action(extension,activity) - WopiAction.distinct - .where("extension = ? and action = ?",extension,activity).first - end - -end \ No newline at end of file + def self.find_action(extension, activity) + WopiAction.distinct + .where('extension = ? and action = ?', extension, activity).first + end +end diff --git a/app/models/wopi_app.rb b/app/models/wopi_app.rb index 3150a03df..81d9a6116 100644 --- a/app/models/wopi_app.rb +++ b/app/models/wopi_app.rb @@ -1,8 +1,10 @@ class WopiApp < ActiveRecord::Base - - belongs_to :wopi_discovery, :foreign_key => 'wopi_discovery_id', :class_name => 'WopiDiscovery' - has_many :wopi_actions, class_name: 'WopiAction', foreign_key: 'wopi_app_id', :dependent => :destroy - - validates :name, :icon, :wopi_discovery, presence: true - -end \ No newline at end of file + belongs_to :wopi_discovery, + foreign_key: 'wopi_discovery_id', + class_name: 'WopiDiscovery' + has_many :wopi_actions, + class_name: 'WopiAction', + foreign_key: 'wopi_app_id', + dependent: :destroy + validates :name, :icon, :wopi_discovery, presence: true +end diff --git a/app/models/wopi_discovery.rb b/app/models/wopi_discovery.rb index 3ecea072e..3564ebe4a 100644 --- a/app/models/wopi_discovery.rb +++ b/app/models/wopi_discovery.rb @@ -1,45 +1,53 @@ class WopiDiscovery < ActiveRecord::Base - require 'base64' + require 'base64' - has_many :wopi_apps, class_name: 'WopiApp', foreign_key: 'wopi_discovery_id', :dependent => :destroy - validates :expires, :proof_key_mod, :proof_key_exp, :proof_key_old_mod, :proof_key_old_exp, presence: true + has_many :wopi_apps, + class_name: 'WopiApp', + foreign_key: 'wopi_discovery_id', + dependent: :destroy + validates :expires, + :proof_key_mod, + :proof_key_exp, + :proof_key_old_mod, + :proof_key_old_exp, + presence: true - # Verifies if proof from headers, X-WOPI-Proof/X-WOPI-OldProof was encrypted - # with this discovery public key (two key possible old/new) - def verify_proof(token, timestamp, signed_proof, signed_proof_old, url) - token_length = [token.length].pack('>N').bytes - timestamp_bytes = [timestamp.to_i].pack('>Q').bytes.reverse - timestamp_length = [timestamp_bytes.length].pack('>N').bytes - url_length = [url.length].pack('>N').bytes + # Verifies if proof from headers, X-WOPI-Proof/X-WOPI-OldProof was encrypted + # with this discovery public key (two key possible old/new) + def verify_proof(token, timestamp, signed_proof, signed_proof_old, url) + token_length = [token.length].pack('>N').bytes + timestamp_bytes = [timestamp.to_i].pack('>Q').bytes.reverse + timestamp_length = [timestamp_bytes.length].pack('>N').bytes + url_length = [url.length].pack('>N').bytes - expected_proof = token_length + token.bytes + - url_length + url.upcase.bytes + - timestamp_length + timestamp_bytes + expected_proof = token_length + token.bytes + + url_length + url.upcase.bytes + + timestamp_length + timestamp_bytes - key = generate_key(proof_key_mod, proof_key_exp) - old_key = generate_key(proof_key_old_mod, proof_key_old_exp) + key = generate_key(proof_key_mod, proof_key_exp) + old_key = generate_key(proof_key_old_mod, proof_key_old_exp) - # Try all possible combiniations - try_verification(expected_proof, signed_proof, key) || - try_verification(expected_proof, signed_proof_old, key) || - try_verification(expected_proof, signed_proof, old_key) - end + # Try all possible combiniations + try_verification(expected_proof, signed_proof, key) || + try_verification(expected_proof, signed_proof_old, key) || + try_verification(expected_proof, signed_proof, old_key) + end - # Generates a public key from given modulus and exponent - def generate_key(modulus, exponent) - mod = Base64.decode64(modulus).unpack('H*').first.to_i(16) - exp = Base64.decode64(exponent).unpack('H*').first.to_i(16) + # Generates a public key from given modulus and exponent + def generate_key(modulus, exponent) + mod = Base64.decode64(modulus).unpack('H*').first.to_i(16) + exp = Base64.decode64(exponent).unpack('H*').first.to_i(16) - seq = OpenSSL::ASN1::Sequence.new([OpenSSL::ASN1::Integer.new(mod), - OpenSSL::ASN1::Integer.new(exp)]) - OpenSSL::PKey::RSA.new(seq.to_der) - end + seq = OpenSSL::ASN1::Sequence.new([OpenSSL::ASN1::Integer.new(mod), + OpenSSL::ASN1::Integer.new(exp)]) + OpenSSL::PKey::RSA.new(seq.to_der) + end - # Verify if decrypting signed_proof with public_key equals to expected_proof - def try_verification(expected_proof, signed_proof_b64, public_key) - signed_proof = Base64.decode64(signed_proof_b64) - public_key.verify(OpenSSL::Digest::SHA256.new, signed_proof, - expected_proof.pack('c*')) - end + # Verify if decrypting signed_proof with public_key equals to expected_proof + def try_verification(expected_proof, signed_proof_b64, public_key) + signed_proof = Base64.decode64(signed_proof_b64) + public_key.verify(OpenSSL::Digest::SHA256.new, signed_proof, + expected_proof.pack('c*')) + end end diff --git a/app/utilities/wopi_util.rb b/app/utilities/wopi_util.rb index 328a7ea4c..01b4fa272 100644 --- a/app/utilities/wopi_util.rb +++ b/app/utilities/wopi_util.rb @@ -5,22 +5,22 @@ module WopiUtil UNIX_EPOCH_IN_CLR_TICKS = 621355968000000000 CLR_TICKS_PER_SECOND = 10000000 - DISCOVERY_TTL = 60*60*24 + DISCOVERY_TTL = 1.days DISCOVERY_TTL.freeze # For more explanation see this: # http://stackoverflow.com/questions/11888053/ # convert-net-datetime-ticks-property-to-date-in-objective-c def convert_to_unix_timestamp(timestamp) - Time.at((timestamp-UNIX_EPOCH_IN_CLR_TICKS)/CLR_TICKS_PER_SECOND) + Time.at((timestamp - UNIX_EPOCH_IN_CLR_TICKS) / CLR_TICKS_PER_SECOND) end def get_action(extension, activity) - get_discovery - action = WopiAction.find_action(extension, activity) + current_wopi_discovery + WopiAction.find_action(extension, activity) end - def get_discovery + def current_wopi_discovery discovery = WopiDiscovery.first return discovery if discovery && discovery.expires >= Time.now.to_i initialize_discovery(discovery) @@ -30,49 +30,44 @@ module WopiUtil # Currently only saves Excel, Word and PowerPoint view and edit actions def initialize_discovery(discovery) - begin - Rails.logger.warn "Initializing discovery" - discovery.destroy if discovery + Rails.logger.warn 'Initializing discovery' + discovery.destroy if discovery - @doc = Nokogiri::XML(open(ENV["WOPI_DISCOVERY_URL"])) + @doc = Nokogiri::XML(open(ENV['WOPI_DISCOVERY_URL'])) - discovery = WopiDiscovery.new - discovery.expires = Time.now.to_i + DISCOVERY_TTL - key = @doc.xpath("//proof-key") - discovery.proof_key_mod = key.xpath("@modulus").first.value - discovery.proof_key_exp = key.xpath("@exponent").first.value - discovery.proof_key_old_mod = key.xpath("@oldmodulus").first.value - discovery.proof_key_old_exp = key.xpath("@oldexponent").first.value - discovery.save! + discovery = WopiDiscovery.new + discovery.expires = Time.now.to_i + DISCOVERY_TTL + key = @doc.xpath('//proof-key') + discovery.proof_key_mod = key.xpath('@modulus').first.value + discovery.proof_key_exp = key.xpath('@exponent').first.value + discovery.proof_key_old_mod = key.xpath('@oldmodulus').first.value + discovery.proof_key_old_exp = key.xpath('@oldexponent').first.value + discovery.save! - @doc.xpath("//app").each do |app| - app_name = app.xpath("@name").first.value - if ["Excel","Word","PowerPoint","WopiTest"].include?(app_name) - wopi_app = WopiApp.new - wopi_app.name = app.xpath("@name").first.value - wopi_app.icon = app.xpath("@favIconUrl").first.value - wopi_app.wopi_discovery_id=discovery.id - wopi_app.save! - app.xpath("action").each do |action| - name = action.xpath("@name").first.value - if ["view","edit","wopitest"].include?(name) - wopi_action = WopiAction.new - wopi_action.action = name - wopi_action.extension = action.xpath("@ext").first.value - wopi_action.urlsrc = action.xpath("@urlsrc").first.value - wopi_action.wopi_app_id = wopi_app.id - wopi_action.save! - end - end - end + @doc.xpath('//app').each do |app| + app_name = app.xpath('@name').first.value + next unless %w(Excel Word PowerPoint WopiTest).include?(app_name) + + wopi_app = WopiApp.new + wopi_app.name = app.xpath('@name').first.value + wopi_app.icon = app.xpath('@favIconUrl').first.value + wopi_app.wopi_discovery_id = discovery.id + wopi_app.save! + app.xpath('action').each do |action| + name = action.xpath('@name').first.value + next unless %w(view edit wopitest).include?(name) + wopi_action = WopiAction.new + wopi_action.action = name + wopi_action.extension = action.xpath('@ext').first.value + wopi_action.urlsrc = action.xpath('@urlsrc').first.value + wopi_action.wopi_app_id = wopi_app.id + wopi_action.save! end - discovery - rescue - Rails.logger.warn "Initialization failed" - discovery = WopiDiscovery.first - discovery.destroy if discovery end - + discovery + rescue + Rails.logger.warn 'Initialization failed' + discovery = WopiDiscovery.first + discovery.destroy if discovery end - end diff --git a/app/views/assets/edit.erb b/app/views/assets/edit.erb index 41fa8eaf5..72f39b1f8 100644 --- a/app/views/assets/edit.erb +++ b/app/views/assets/edit.erb @@ -1,6 +1,6 @@ - + @@ -9,56 +9,56 @@ + content="width=device-width, initial-scale=1, maximum-scale=1, minimum-scale=1, user-scalable=no"> + href="" /> - - + + -
- method="post"> + method="post"> type="hidden"/> type="hidden"/> -
+ - + - - \ No newline at end of file + diff --git a/app/views/assets/view.erb b/app/views/assets/view.erb index 41fa8eaf5..72f39b1f8 100644 --- a/app/views/assets/view.erb +++ b/app/views/assets/view.erb @@ -1,6 +1,6 @@ - + @@ -9,56 +9,56 @@ + content="width=device-width, initial-scale=1, maximum-scale=1, minimum-scale=1, user-scalable=no"> + href="" /> - - + + -
- method="post"> + method="post"> type="hidden"/> type="hidden"/> -
+ - + - - \ No newline at end of file + diff --git a/config/application.rb b/config/application.rb index 06d0fd78d..d8d3e4693 100644 --- a/config/application.rb +++ b/config/application.rb @@ -30,13 +30,10 @@ module Scinote "[#{datetime}] #{severity}: #{msg}\n" end - #config.action_dispatch.default_headers = { - #'X-WOPI-Lock' => "", - #'Random-header' => "with value", - #'Random-non-special-header' => "a" - #} - # Paperclip spoof checking - Paperclip.options[:content_type_mappings] = {:csv => "text/plain", wopitest: ['text/plain', 'inode/x-empty'] } + Paperclip.options[:content_type_mappings] = { + csv: 'text/plain', + wopitest: ['text/plain', 'inode/x-empty'] + } end end diff --git a/db/migrate/20160728145000_add_wopi.rb b/db/migrate/20160728145000_add_wopi.rb index 01c7fc70b..f368bb254 100644 --- a/db/migrate/20160728145000_add_wopi.rb +++ b/db/migrate/20160728145000_add_wopi.rb @@ -1,10 +1,9 @@ -class AddWopi< ActiveRecord::Migration - +class AddWopi < ActiveRecord::Migration def up - add_column :users, :wopi_token, :string + add_column :users, :wopi_token, :string add_column :users, :wopi_token_ttl, :integer - add_column :assets, :lock, :string, :limit => 1024 + add_column :assets, :lock, :string, limit: 1024 add_column :assets, :lock_ttl, :integer add_column :assets, :version, :integer, default: 1 @@ -32,8 +31,7 @@ class AddWopi< ActiveRecord::Migration add_foreign_key :wopi_actions, :wopi_apps, column: :wopi_app_id add_foreign_key :wopi_apps, :wopi_discoveries, column: :wopi_discovery_id - add_index :wopi_actions, [:extension,:action] - + add_index :wopi_actions, [:extension, :action] end def down