From 05b0f1b08c63faaf673913676ff7f0aad701477e Mon Sep 17 00:00:00 2001 From: "COLA@Redminetest" Date: Tue, 8 Nov 2016 14:27:31 +0100 Subject: [PATCH 1/6] Changed MsOffice check to look for 'Microsoft Office' in the User Agent. --- lib/redmine_dmsf/webdav/controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/redmine_dmsf/webdav/controller.rb b/lib/redmine_dmsf/webdav/controller.rb index 19a33570..1fc1770c 100644 --- a/lib/redmine_dmsf/webdav/controller.rb +++ b/lib/redmine_dmsf/webdav/controller.rb @@ -33,7 +33,7 @@ module RedmineDmsf # Return NotFound if resource does not exist and the request is not anonymous. NotFound else - if request.env.has_key?('HTTP_X_OFFICE_MAJOR_VERSION') && User.current && User.current.anonymous? + if request.user_agent.downcase.include?('microsoft office') && User.current && User.current.anonymous? # Anonymous request from MsOffice, respond 405. # If responding with 401 then MsOffice will fail. # If responding with 200 then MsOffice will think that anonymous access is ok for everything. @@ -48,7 +48,7 @@ module RedmineDmsf # Return response to HEAD def head # exist? returns false if user is anonymous for ProjectResource and DmsfResource, but not for IndexResource. - unless(resource.exist? || (request.env.has_key?('HTTP_X_OFFICE_MAJOR_VERSION') && User.current && User.current.anonymous?)) + unless(resource.exist? || (request.user_agent.downcase.include?('microsoft office') && User.current && User.current.anonymous?)) # Return NotFound if resource does not exist and the request is not from an anonymous MsOffice product. NotFound else From f34c8ef4ecc6f10eedf7ab5813b9242934b38ab8 Mon Sep 17 00:00:00 2001 From: "COLA@Redminetest" Date: Tue, 8 Nov 2016 14:45:57 +0100 Subject: [PATCH 2/6] Anonymous OPTIONS and HEAD requests are only allowed from Microsoft Office clients. --- lib/redmine_dmsf/webdav/resource_proxy.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/redmine_dmsf/webdav/resource_proxy.rb b/lib/redmine_dmsf/webdav/resource_proxy.rb index 06999d0d..864d57f6 100644 --- a/lib/redmine_dmsf/webdav/resource_proxy.rb +++ b/lib/redmine_dmsf/webdav/resource_proxy.rb @@ -51,12 +51,12 @@ module RedmineDmsf # going to fork it to ensure compliance, checking the request method in the authentication # seems the next best step, if the request method is OPTIONS return true, controller will simply # call the options method within, which accesses nothing, just returns headers about dav env. - #return true if @request.request_method.downcase == 'options' && (path == '/' || path.empty?) + return true if @request.request_method.downcase == 'options' && (path == '/' || path.empty?) - # Allow anonymous OPTIONS requests. - return true if @request.request_method.downcase == 'options' - # Allow anonymous HEAD requests. - return true if @request.request_method.downcase == 'head' + # Allow anonymous OPTIONS requests from MsOffice + return true if @request.request_method.downcase == 'options' && @request.user_agent.downcase.include?('microsoft office') + # Allow anonymous HEAD requests from MsOffice + return true if @request.request_method.downcase == 'head' && request.user_agent.downcase.include?('microsoft office') return false unless username && password User.current = User.try_to_login(username, password) From 1ba6b3e7d5c9b962d6f7a9e8e2f93cfe9643a987 Mon Sep 17 00:00:00 2001 From: "COLA@Redminetest" Date: Tue, 8 Nov 2016 21:08:08 +0100 Subject: [PATCH 3/6] Verify that user_agent is not nil before using it. --- lib/redmine_dmsf/webdav/controller.rb | 4 ++-- lib/redmine_dmsf/webdav/resource_proxy.rb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/redmine_dmsf/webdav/controller.rb b/lib/redmine_dmsf/webdav/controller.rb index 1fc1770c..2ba4a3e3 100644 --- a/lib/redmine_dmsf/webdav/controller.rb +++ b/lib/redmine_dmsf/webdav/controller.rb @@ -33,7 +33,7 @@ module RedmineDmsf # Return NotFound if resource does not exist and the request is not anonymous. NotFound else - if request.user_agent.downcase.include?('microsoft office') && User.current && User.current.anonymous? + if !request.user_agent.nil? && request.user_agent.downcase.include?('microsoft office') && User.current && User.current.anonymous? # Anonymous request from MsOffice, respond 405. # If responding with 401 then MsOffice will fail. # If responding with 200 then MsOffice will think that anonymous access is ok for everything. @@ -48,7 +48,7 @@ module RedmineDmsf # Return response to HEAD def head # exist? returns false if user is anonymous for ProjectResource and DmsfResource, but not for IndexResource. - unless(resource.exist? || (request.user_agent.downcase.include?('microsoft office') && User.current && User.current.anonymous?)) + unless(resource.exist? || (!request.user_agent.nil? && request.user_agent.downcase.include?('microsoft office') && User.current && User.current.anonymous?)) # Return NotFound if resource does not exist and the request is not from an anonymous MsOffice product. NotFound else diff --git a/lib/redmine_dmsf/webdav/resource_proxy.rb b/lib/redmine_dmsf/webdav/resource_proxy.rb index 864d57f6..0e0c1260 100644 --- a/lib/redmine_dmsf/webdav/resource_proxy.rb +++ b/lib/redmine_dmsf/webdav/resource_proxy.rb @@ -54,9 +54,9 @@ module RedmineDmsf return true if @request.request_method.downcase == 'options' && (path == '/' || path.empty?) # Allow anonymous OPTIONS requests from MsOffice - return true if @request.request_method.downcase == 'options' && @request.user_agent.downcase.include?('microsoft office') + return true if @request.request_method.downcase == 'options' && !@request.user_agent.nil? && @request.user_agent.downcase.include?('microsoft office') # Allow anonymous HEAD requests from MsOffice - return true if @request.request_method.downcase == 'head' && request.user_agent.downcase.include?('microsoft office') + return true if @request.request_method.downcase == 'head' && !@request.user_agent.nil? && request.user_agent.downcase.include?('microsoft office') return false unless username && password User.current = User.try_to_login(username, password) From 311eeb86d60bd6cbffa6db7e75ebd12cb31cc011 Mon Sep 17 00:00:00 2001 From: "COLA@Redminetest" Date: Tue, 8 Nov 2016 22:11:37 +0100 Subject: [PATCH 4/6] Added tests for OPTIONS and HEAD requests for updated MsOffice support. --- test/integration/dmsf_webdav_head_test.rb | 50 +++++++++++++++++++- test/integration/dmsf_webdav_options_test.rb | 22 ++++++++- 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/test/integration/dmsf_webdav_head_test.rb b/test/integration/dmsf_webdav_head_test.rb index 676867ea..7e331d53 100644 --- a/test/integration/dmsf_webdav_head_test.rb +++ b/test/integration/dmsf_webdav_head_test.rb @@ -54,6 +54,18 @@ class DmsfWebdavHeadTest < RedmineDmsf::Test::IntegrationTest check_headers_exist end + def test_head_responds_anonymous_msoffice_user_agent + head "/dmsf/webdav/#{@project1.identifier}", nil, {:HTTP_USER_AGENT => "Microsoft Office Word 2014"} + assert_response :success + check_headers_exist + end + + def test_head_responds_anonymous_other_user_agent + head "/dmsf/webdav/#{@project1.identifier}", nil, {:HTTP_USER_AGENT => "Other"} + assert_response 401 + check_headers_dont_exist + end + # Note: # At present we use Rack to serve the file, this makes life easy however it removes the Etag # header and invalidates the test - where as a folder listing will always not include a last-modified @@ -65,18 +77,54 @@ class DmsfWebdavHeadTest < RedmineDmsf::Test::IntegrationTest check_headers_exist # Note it'll allow 1 out of the 3 expected to fail end + def test_head_responds_to_file_anonymous_msoffice_user_agent + head "/dmsf/webdav/#{@project1.identifier}/test.txt", nil, {:HTTP_USER_AGENT => "Microsoft Office Word 2014"} + assert_response :success + check_headers_exist # Note it'll allow 1 out of the 3 expected to fail + end + + def test_head_responds_to_file_anonymous_other_user_agent + head "/dmsf/webdav/#{@project1.identifier}/test.txt", nil, {:HTTP_USER_AGENT => "Other"} + assert_response 401 + check_headers_dont_exist + end + def test_head_fails_when_file_not_found head "/dmsf/webdav/#{@project1.identifier}/not_here.txt", nil, @admin assert_response :missing check_headers_dont_exist end + def test_head_fails_when_file_not_found_anonymous_msoffice_user_agent + head "/dmsf/webdav/#{@project1.identifier}/not_here.txt", nil, {:HTTP_USER_AGENT => "Microsoft Office Word 2014"} + assert_response :missing + check_headers_dont_exist + end + + def test_head_fails_when_file_not_found_anonymous_other_user_agent + head "/dmsf/webdav/#{@project1.identifier}/not_here.txt", nil, {:HTTP_USER_AGENT => "Other"} + assert_response 401 + check_headers_dont_exist + end + def test_head_fails_when_folder_not_found head '/dmsf/webdav/folder_not_here', nil, @admin assert_response :missing check_headers_dont_exist end + def test_head_fails_when_folder_not_found_anonymous_msoffice_user_agent + head '/dmsf/webdav/folder_not_here', nil, {:HTTP_USER_AGENT => "Microsoft Office Word 2014"} + assert_response :missing + check_headers_dont_exist + end + + def test_head_fails_when_folder_not_found_anonymous_other_user_agent + head '/dmsf/webdav/folder_not_here', nil, {:HTTP_USER_AGENT => "Other"} + assert_response 401 + check_headers_dont_exist + end + def test_head_fails_when_project_is_not_enabled_for_dmsf head "/dmsf/webdav/#{@project2.identifier}/test.txt", nil, @jsmith assert_response :missing @@ -116,4 +164,4 @@ class DmsfWebdavHeadTest < RedmineDmsf::Test::IntegrationTest end end -end \ No newline at end of file +end diff --git a/test/integration/dmsf_webdav_options_test.rb b/test/integration/dmsf_webdav_options_test.rb index 8e532280..0d0a140b 100644 --- a/test/integration/dmsf_webdav_options_test.rb +++ b/test/integration/dmsf_webdav_options_test.rb @@ -117,6 +117,26 @@ class DmsfWebdavOptionsTest < RedmineDmsf::Test::IntegrationTest assert response.headers['Ms-Author-Via'] == 'DAV', 'Ms-Author-Via header - expected: DAV' end + def test_un_authenticated_options_for_msoffice_user_agent + xml_http_request :options, "/dmsf/webdav/#{@project1.identifier}", nil, {:HTTP_USER_AGENT => "Microsoft Office Word 2014"} + assert_response 405 + end + + def test_authenticated_options_for_msoffice_user_agent + xml_http_request :options, "/dmsf/webdav/#{@project1.identifier}", nil, @admin.merge!({:HTTP_USER_AGENT => "Microsoft Office Word 2014"}) + assert_response :success + end + + def test_un_authenticated_options_for_other_user_agent + xml_http_request :options, "/dmsf/webdav/#{@project1.identifier}", nil, {:HTTP_USER_AGENT => "Other"} + assert_response 401 + end + + def test_authenticated_options_for_other_user_agent + xml_http_request :options, "/dmsf/webdav/#{@project1.identifier}", nil, @admin.merge!({:HTTP_USER_AGENT => "Other"}) + assert_response :success + end + # TODO: It doesn't work # def test_authenticated_options_returns_401_for_non_dmsf_enabled_items # @project2.disable_module! :dmsf @@ -129,4 +149,4 @@ class DmsfWebdavOptionsTest < RedmineDmsf::Test::IntegrationTest # assert_response 401 # refused # end -end \ No newline at end of file +end From 94369569f36ed9ca38f479431f364eaba4839bc9 Mon Sep 17 00:00:00 2001 From: "COLA@Redminetest" Date: Tue, 8 Nov 2016 22:47:04 +0100 Subject: [PATCH 5/6] Only allow anonymous OPTIONS and HEAD if request really is anonymous. --- lib/redmine_dmsf/webdav/resource_proxy.rb | 26 ++++++++++++----------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/lib/redmine_dmsf/webdav/resource_proxy.rb b/lib/redmine_dmsf/webdav/resource_proxy.rb index 0e0c1260..b433b78b 100644 --- a/lib/redmine_dmsf/webdav/resource_proxy.rb +++ b/lib/redmine_dmsf/webdav/resource_proxy.rb @@ -45,19 +45,21 @@ module RedmineDmsf end def authenticate(username, password) - # Bugfix: Current DAV4Rack (including production) authenticate against ALL requests - # Microsoft Web Client will not attempt any authentication (it'd seem) until it's acknowledged - # a completed OPTIONS request. Ideally this is a flaw with the controller, however as I'm not - # going to fork it to ensure compliance, checking the request method in the authentication - # seems the next best step, if the request method is OPTIONS return true, controller will simply - # call the options method within, which accesses nothing, just returns headers about dav env. - return true if @request.request_method.downcase == 'options' && (path == '/' || path.empty?) - - # Allow anonymous OPTIONS requests from MsOffice - return true if @request.request_method.downcase == 'options' && !@request.user_agent.nil? && @request.user_agent.downcase.include?('microsoft office') - # Allow anonymous HEAD requests from MsOffice - return true if @request.request_method.downcase == 'head' && !@request.user_agent.nil? && request.user_agent.downcase.include?('microsoft office') + unless username && password + # Bugfix: Current DAV4Rack (including production) authenticate against ALL requests + # Microsoft Web Client will not attempt any authentication (it'd seem) until it's acknowledged + # a completed OPTIONS request. Ideally this is a flaw with the controller, however as I'm not + # going to fork it to ensure compliance, checking the request method in the authentication + # seems the next best step, if the request method is OPTIONS return true, controller will simply + # call the options method within, which accesses nothing, just returns headers about dav env. + return true if @request.request_method.downcase == 'options' && (path == '/' || path.empty?) + # Allow anonymous OPTIONS requests from MsOffice + return true if @request.request_method.downcase == 'options' && !@request.user_agent.nil? && @request.user_agent.downcase.include?('microsoft office') + # Allow anonymous HEAD requests from MsOffice + return true if @request.request_method.downcase == 'head' && !@request.user_agent.nil? && request.user_agent.downcase.include?('microsoft office') + end + return false unless username && password User.current = User.try_to_login(username, password) return User.current && !User.current.anonymous? From 6bbe6fc3cc716c719c69a2998cfb5935ba9735c1 Mon Sep 17 00:00:00 2001 From: "COLA@Redminetest" Date: Tue, 8 Nov 2016 22:53:41 +0100 Subject: [PATCH 6/6] Instead of having one options/head method in each resource call the options/head method in the superclass. --- lib/redmine_dmsf/webdav/base_resource.rb | 4 +- lib/redmine_dmsf/webdav/controller.rb | 46 +++++++++++++-------- lib/redmine_dmsf/webdav/dmsf_resource.rb | 28 ++----------- lib/redmine_dmsf/webdav/index_resource.rb | 19 +++------ lib/redmine_dmsf/webdav/project_resource.rb | 24 +++-------- lib/redmine_dmsf/webdav/resource_proxy.rb | 12 ++---- 6 files changed, 50 insertions(+), 83 deletions(-) diff --git a/lib/redmine_dmsf/webdav/base_resource.rb b/lib/redmine_dmsf/webdav/base_resource.rb index 517ad763..5369f746 100644 --- a/lib/redmine_dmsf/webdav/base_resource.rb +++ b/lib/redmine_dmsf/webdav/base_resource.rb @@ -61,7 +61,7 @@ module RedmineDmsf end # Generate HTML for Get requests, or Head requests if no_body is true - def html_display(no_body = false) + def html_display @response.body = '' Confict unless collection? entities = children.map{|child| @@ -80,7 +80,7 @@ module RedmineDmsf '', '', ] + entities if parent - @response.body << index_page % [ path.empty? ? '/' : path, path.empty? ? '/' : path, entities ] unless no_body + @response.body << index_page % [ path.empty? ? '/' : path, path.empty? ? '/' : path, entities ] end # Run method through proxy class - ensuring always compatible child is generated diff --git a/lib/redmine_dmsf/webdav/controller.rb b/lib/redmine_dmsf/webdav/controller.rb index 2ba4a3e3..7804c693 100644 --- a/lib/redmine_dmsf/webdav/controller.rb +++ b/lib/redmine_dmsf/webdav/controller.rb @@ -29,31 +29,43 @@ module RedmineDmsf # Return response to OPTIONS def options # exist? returns false if user is anonymous for ProjectResource and DmsfResource, but not for IndexResource. - unless(resource.exist? || (User.current && User.current.anonymous?)) - # Return NotFound if resource does not exist and the request is not anonymous. - NotFound + if resource.exist? + # resource exists and user is not anonymous. + super + elsif resource.really_exist? && + !request.user_agent.nil? && request.user_agent.downcase.include?('microsoft office') && + User.current && User.current.anonymous? + # resource actually exist, but this was an anonymous request from MsOffice so respond with 405, + # hopefully the resource did actually exist but failed because of anon. + # If responding with 401 then MsOffice will fail. + # If responding with 200 then MsOffice will think that anonymous access is ok for everything. + # Responding with 405 is a workaround found in https://support.microsoft.com/en-us/kb/2019105 + MethodNotAllowed else - if !request.user_agent.nil? && request.user_agent.downcase.include?('microsoft office') && User.current && User.current.anonymous? - # Anonymous request from MsOffice, respond 405. - # If responding with 401 then MsOffice will fail. - # If responding with 200 then MsOffice will think that anonymous access is ok for everything. - # Responding with 405 is a workaround found in https://support.microsoft.com/en-us/kb/2019105 - MethodNotAllowed - else - resource.options - end + # Return NotFound if resource does not exist and the request is not anonymous from MsOffice + NotFound end end # Return response to HEAD def head # exist? returns false if user is anonymous for ProjectResource and DmsfResource, but not for IndexResource. - unless(resource.exist? || (!request.user_agent.nil? && request.user_agent.downcase.include?('microsoft office') && User.current && User.current.anonymous?)) - # Return NotFound if resource does not exist and the request is not from an anonymous MsOffice product. - NotFound + if resource.exist? + # resource exists and user is not anonymous. + super + elsif resource.really_exist? && + !request.user_agent.nil? && request.user_agent.downcase.include?('microsoft office') && + User.current && User.current.anonymous? + # resource said it don't exist, but this was an anonymous request from MsOffice so respond anyway + # Can not call super here since it calls resource.exist? which will fail + response['Etag'] = resource.etag + response['Content-Type'] = resource.content_type + response['Last-Modified'] = resource.last_modified.httpdate + OK else - resource.head(request, response) - end + # Return NotFound if resource does not exist and the request is not anonymous from MsOffice + NotFound + end end # Return response to PROPFIND diff --git a/lib/redmine_dmsf/webdav/dmsf_resource.rb b/lib/redmine_dmsf/webdav/dmsf_resource.rb index befbdbc0..5bd90295 100644 --- a/lib/redmine_dmsf/webdav/dmsf_resource.rb +++ b/lib/redmine_dmsf/webdav/dmsf_resource.rb @@ -75,6 +75,10 @@ module RedmineDmsf (User.current.admin? || User.current.allowed_to?(:view_dmsf_folders, project)) end + def really_exist? + return project && project.module_enabled?('dmsf') && (folder || file) + end + # Is this entity a folder? def collection? folder.present? # No need to check if entity exists, as false is returned if entity does not exist anyways @@ -194,23 +198,6 @@ module RedmineDmsf OK end - # Process incoming HEAD request - # - # MsOFfice uses anonymous HEAD requests, so always return a response. - # See https://support.microsoft.com/en-us/kb/2019105 - ## - def head(request, response) - raise NotFound unless project && project.module_enabled?('dmsf') && (folder || file) - - if collection? - html_display(true) - response['Content-Length'] = response.body.bytesize.to_s - else - response.body = '' - end - OK - end - # Process incoming MKCOL request # # Create a DmsfFolder at location requested, only if parent is a folder (or root) @@ -629,13 +616,6 @@ module RedmineDmsf end end - def options_req - response["Allow"] = 'OPTIONS,HEAD,GET,PUT,POST,DELETE,PROPFIND,PROPPATCH,MKCOL,COPY,MOVE,LOCK,UNLOCK' - response["Dav"] = '1, 2' - response["Ms-Author-Via"] = "DAV" - OK - end - private # Prepare file for download using Rack functionality: # Download (see RedmineDmsf::Webdav::Download) extends Rack::File to allow single-file diff --git a/lib/redmine_dmsf/webdav/index_resource.rb b/lib/redmine_dmsf/webdav/index_resource.rb index 7ad71a92..76c504bc 100644 --- a/lib/redmine_dmsf/webdav/index_resource.rb +++ b/lib/redmine_dmsf/webdav/index_resource.rb @@ -55,6 +55,11 @@ module RedmineDmsf def exist? true end + + # Index resource ALWAYS really exists + def really_exist? + true + end def etag sprintf('%x-%x-%x', children.count, 4096, Time.now.to_i) @@ -68,26 +73,12 @@ module RedmineDmsf 4096 end - def head(request, response) - html_display(true) - response['Content-Length'] = response.body.bytesize.to_s - OK - end - def get(request, response) html_display response['Content-Length'] = response.body.bytesize.to_s OK end - def options_req - response["Allow"] = 'OPTIONS,HEAD,GET,PUT,POST,DELETE,PROPFIND,PROPPATCH,MKCOL,COPY,MOVE,LOCK,UNLOCK' - #response["Allow"] = 'OPTIONS,PROPFIND' - response["Dav"] = '1, 2' - response["Ms-Author-Via"] = "DAV" - OK - end - # Bugfix: Ensure that this level never indicates a parent def parent nil diff --git a/lib/redmine_dmsf/webdav/project_resource.rb b/lib/redmine_dmsf/webdav/project_resource.rb index 47a79df0..5b5fb238 100644 --- a/lib/redmine_dmsf/webdav/project_resource.rb +++ b/lib/redmine_dmsf/webdav/project_resource.rb @@ -44,6 +44,12 @@ module RedmineDmsf User.current.admin? || User.current.allowed_to?(:view_dmsf_folders, project) end + def really_exist? + return false if project.nil? + return false unless project.module_enabled?('dmsf') + true + end + def collection? exist? end @@ -80,30 +86,12 @@ module RedmineDmsf 4096 end - def head(request, response) - # HEAD must be allowed even for anonymous users, so just verify that the project exists and that the dmsf module is enabled. - if project.nil? || !project.module_enabled?('dmsf') - NotFound - else - html_display(true) - response['Content-Length'] = response.body.bytesize.to_s - OK - end - end - def get(request, response) html_display response['Content-Length'] = response.body.bytesize.to_s OK end - def options_req - response["Allow"] = 'OPTIONS,HEAD,GET,PUT,POST,DELETE,PROPFIND,PROPPATCH,MKCOL,COPY,MOVE,LOCK,UNLOCK' - response["Dav"] = '1, 2' - response["Ms-Author-Via"] = "DAV" - OK - end - def folder nil end diff --git a/lib/redmine_dmsf/webdav/resource_proxy.rb b/lib/redmine_dmsf/webdav/resource_proxy.rb index b433b78b..6556281e 100644 --- a/lib/redmine_dmsf/webdav/resource_proxy.rb +++ b/lib/redmine_dmsf/webdav/resource_proxy.rb @@ -81,6 +81,10 @@ module RedmineDmsf @resource_c.exist? end + def really_exist? + @resource_c.really_exist? + end + def creation_date @resource_c.creation_date end @@ -105,10 +109,6 @@ module RedmineDmsf @resource_c.content_length end - def head(request,response) - @resource_c.head(request, response) - end - def get(request, response) @resource_c.get(request, response) end @@ -168,10 +168,6 @@ module RedmineDmsf def properties @resource_c.properties end - - def options - @resource_c.options_req - end end end end