win_get_url: Fixed a few issues with using FTP and added tests (#39646)

* win_get_url: Fixed a few issues with using FTP and added tests

* Fixed typo in docs

changelogs/fragments/win_get_url-ftp-support.yaml

@@ -0,0 +1,2 @@
+bugfixes:
+- win_get_url - fixed a few bugs around authentication and force no when using an FTP URL

lib/ansible/modules/windows/win_get_url.ps1

@@ -5,8 +5,7 @@
 # Copyright: (c) 2017, Dag Wieers <dag@wieers.com>
 # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
 
-# WANT_JSON
-# POWERSHELL_COMMON
+#Requires -Module Ansible.ModuleUtils.Legacy
 
 $ErrorActionPreference = 'Stop'
 
@@ -35,8 +34,8 @@ Function CheckModified-File($url, $dest, $headers, $credentials, $timeout, $use_
     $fileLastMod = ([System.IO.FileInfo]$dest).LastWriteTimeUtc
     $webLastMod = $null
 
-    $webRequest = [System.Net.HttpWebRequest]::Create($url)
-
+    $webRequest = [System.Net.WebRequest]::Create($url)
+    
     foreach ($header in $headers.GetEnumerator()) {
         $webRequest.Headers.Add($header.Name, $header.Value)
     }
@@ -53,14 +52,23 @@ Function CheckModified-File($url, $dest, $headers, $credentials, $timeout, $use_
     }
 
     if ($credentials) {
-        $webRequest.Credentials = $credentials
+        if ($force_basic_auth) {
+            $extWebClient.Headers.Add("Authorization","Basic $credentials")
+        } else {
+            $extWebClient.Credentials = $credentials
+        }
     }
 
-    $webRequest.Method = "HEAD"
+    if ($webRequest -is [System.Net.FtpWebRequest]) {
+        $webRequest.Method = [System.Net.WebRequestMethods+Ftp]::GetDateTimestamp
+    } else {
+        $webRequest.Method = [System.Net.WebRequestMethods+Http]::Head
+    }
+    
     Try {
-        [System.Net.HttpWebResponse]$webResponse = $webRequest.GetResponse()
+        $webResponse = $webRequest.GetResponse()
 
-        $webLastMod = $webResponse.GetResponseHeader("Last-Modified")
+        $webLastMod = $webResponse.LastModified
     } Catch [System.Net.WebException] {
         $result.status_code = $_.Exception.Response.StatusCode
         Fail-Json -obj $result -message "Error requesting '$url'. $($_.Exception.Message)"
@@ -174,7 +182,7 @@ if ($proxy_url) {
 }
 
 $credentials = $null
-if ($url_username -and $url_password) {
+if ($url_username) {
     if ($force_basic_auth) {
         $credentials = [convert]::ToBase64String([System.Text.Encoding]::ASCII.GetBytes($url_username+":"+$url_password))
     } else {
@@ -241,3 +249,4 @@ if ($force -or -not (Test-Path -LiteralPath $dest)) {
 }
 
 Exit-Json -obj $result
+

lib/ansible/modules/windows/win_get_url.py

@@ -15,9 +15,10 @@ DOCUMENTATION = r'''
 ---
 module: win_get_url
 version_added: "1.7"
-short_description: Fetches a file from a given URL
+short_description: Downloads file from HTTP, HTTPS, or FTP to node
 description:
-- Fetches a file from a URL and saves it locally.
+- Downloads files from HTTP, HTTPS, or FTP to the remote server. The remote
+  server I(must) have direct access to the remote resource.
 - For non-Windows targets, use the M(get_url) module instead.
 author:
 - Paul Durivage (@angstwad)
@@ -101,8 +102,6 @@ options:
     - Timeout in seconds for URL request.
     default: 10
     version_added : '2.4'
-notes:
- - For non-Windows targets, use the M(get_url) module instead.
 '''
 
 EXAMPLES = r'''
@@ -124,6 +123,13 @@ EXAMPLES = r'''
     proxy_url: http://10.0.0.1:8080
     proxy_username: username
     proxy_password: password
+
+- name: Download file from FTP with authentication
+  win_get_url:
+    url: ftp://server/file.txt
+    dest: '%TEMP%\ftp-file.txt'
+    url_username: ftp-user
+    url_password: ftp-password
 '''
 
 RETURN = r'''

test/integration/targets/win_get_url/defaults/main.yml

@@ -1,8 +1,4 @@
 ---
-
+test_win_get_url_path: '{{win_output_dir}}\win_get_url'
 test_win_get_url_host: www.redhat.com
-test_win_get_url_link: "https://{{ test_win_get_url_host }}"
-test_win_get_url_invalid_link: https://www.redhat.com/skynet_module.html
-test_win_get_url_invalid_path: 'Q:\Filez\Cyberdyne.html'
-test_win_get_url_invalid_path_dir: 'Q:\Filez\'
-test_win_get_url_path: '%TEMP%\docs_index.html'
+test_win_get_url_env_var: WIN_GET_URL

test/integration/targets/win_get_url/files/ftp/anon/file.txt

@@ -0,0 +1 @@
+ftp/anon/file.txt

test/integration/targets/win_get_url/files/ftp/anon/file2.txt

@@ -0,0 +1 @@
+ftp/anon/file2.txt

test/integration/targets/win_get_url/files/ftp/user-pass/file.txt

@@ -0,0 +1 @@
+ftp/user-pass/file.txt

test/integration/targets/win_get_url/files/ftp/user/file.txt

@@ -0,0 +1 @@
+ftp/user/file.txt

test/integration/targets/win_get_url/tasks/main.yml

@@ -1,143 +1,59 @@
-# test code for the win_get_url module
-# (c) 2014, Chris Church <chris@ninemoreminutes.com>
-
-# This file is part of Ansible
-#
-# Ansible is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# Ansible is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
-
-- setup:
-
-- name: Remove test file if it exists
+---
+- name: ensure testing folder is present
   win_file:
-    path: '{{ test_win_get_url_path }}'
-    state: absent
-
-- name: Test win_get_url module
-  win_get_url:
-    url: '{{ test_win_get_url_link }}'
-    dest: '{{ test_win_get_url_path }}'
-  register: win_get_url_result
-
-- name: Check that url was downloaded
-  assert:
-    that:
-      - win_get_url_result is not failed
-      - win_get_url_result is changed
-      - win_get_url_result.url
-      - win_get_url_result.dest
-
-- name: Test win_get_url module again (force should be yes by default)
-  win_get_url:
-    url: '{{ test_win_get_url_link }}'
-    dest: '{{ test_win_get_url_path }}'
-  register: win_get_url_result_again
-
-- name: Check that url was downloaded again
-  assert:
-    that:
-      - win_get_url_result_again is not failed
-      - win_get_url_result_again is changed
-
-- name: Test win_get_url module again with force=no
-  win_get_url:
-    url: '{{ test_win_get_url_link }}'
-    dest: '{{ test_win_get_url_path }}'
-    force: no
-  register: win_get_url_result_noforce
-
-- name: Check that url was not downloaded again
-  assert:
-    that:
-      - win_get_url_result_noforce is not failed
-      - win_get_url_result_noforce is not changed
-
-- name: Test win_get_url module with url that returns a 404
-  win_get_url:
-    url: '{{ test_win_get_url_invalid_link }}'
-    dest: '{{ test_win_get_url_path }}'
-  register: win_get_url_result_invalid_link
-  ignore_errors: true
-
-- name: Check that the download failed for an invalid url
-  assert:
-    that:
-      - win_get_url_result_invalid_link is failed
-      - win_get_url_result_invalid_link.status_code == 404
-
-- name: Test win_get_url module with an invalid path
-  win_get_url:
-    url: '{{ test_win_get_url_link }}'
-    dest: '{{ test_win_get_url_invalid_path }}'
-  register: win_get_url_result_invalid_path
-  ignore_errors: true
-
-- name: Check that the download failed for an invalid path
-  assert:
-    that:
-      - win_get_url_result_invalid_path is failed
-
-- name: Test win_get_url module with a valid path that is a directory
-  win_get_url:
-    url: '{{ test_win_get_url_link }}'
-    dest: '%TEMP%'
-  register: win_get_url_result_dir_path
-  ignore_errors: true
-
-- name: Check that the download did NOT fail, even though dest was directory
-  assert:
-    that:
-      - win_get_url_result_dir_path is changed
-
-- name: Test win_get_url with a valid url path and a dest that is a directory (from 2.4 should use url path as filename)
-  win_get_url:
-    url: '{{ test_win_get_url_link }}'
-    dest: '%TEMP%'
-  register: win_get_url_result_dir_path_urlpath
-  ignore_errors: true
-
-- name: Set expected destination path fact
-  set_fact:
-    expected_dest_path: '{{ ansible_env.TEMP }}\{{ test_win_get_url_host }}'
-
-- name: Check that the download succeeded (changed) and dest is as expected
-  assert:
-    that:
-      - win_get_url_result_dir_path_urlpath is changed
-      - win_get_url_result_dir_path_urlpath.dest == expected_dest_path
-
-- name: Check you get a helpful message if the parent folder of the dest doesn't exist
-  win_get_url:
-    url: '{{ test_win_get_url_link }}'
-    dest: 'Q:\Filez\'
-  register: win_get_url_result_invalid_dest
-  ignore_errors: true
-
-- name: Check if dest parent dir does not exist, module fails and you get a specific error message
-  assert:
-    that:
-      - win_get_url_result_invalid_dest is failed
-      - win_get_url_result_invalid_dest.msg is search('does not exist, or is not visible to the current user')
-
-- name: Check you get a helpful message if the parent folder of the dest doesn't exist
-  win_get_url:
-    url: '{{ test_win_get_url_link }}'
-    dest: 'C:\Filez\'
-  register: win_get_url_result_invalid_dest2
-  ignore_errors: true
-
-- name: Check if dest parent dir does not exist, module fails and you get a specific error message
-  assert:
-    that:
-      - win_get_url_result_invalid_dest2 is failed
-      - win_get_url_result_invalid_dest2.msg is search('does not exist')
+    path: '{{test_win_get_url_path}}'
+    state: directory
+
+- name: copy across testing files
+  win_copy:
+    src: files/
+    dest: '{{test_win_get_url_path}}\'
+
+- name: download SlimFTPd binary
+  win_get_url:
+    url: https://s3.amazonaws.com/ansible-ci-files/test/integration/roles/test_win_get_url/SlimFTPd.exe
+    dest: '{{test_win_get_url_path}}\SlimFTPd.exe'
+
+- name: template SlimFTPd configuration file
+  win_template:
+    src: slimftpd.conf.tmpl
+    dest: '{{test_win_get_url_path}}\slimftpd.conf'
+
+- name: create SlimFTPd service
+  win_service:
+    name: SlimFTPd
+    path: '"{{test_win_get_url_path}}\SlimFTPd.exe" -service'
+    state: started
+    dependencies:
+    - tcpip
+
+- name: create env var for win_get_url tests
+  win_environment:
+    name: '{{test_win_get_url_env_var}}'
+    level: machine
+    value: '{{test_win_get_url_path}}'
+    state: present
+
+- block:
+  - name: run URL tests
+    include_tasks: tests_url.yml
+
+  - name: run FTP tests
+    include_tasks: tests_ftp.yml
+
+  always:
+  - name: remove SlimFTPd service
+    win_service:
+      name: SlimFTPd
+      state: absent
+
+  - name: remove test env var for tests
+    win_environment:
+      name: '{{test_win_get_url_env_var}}'
+      level: machine
+      state: absent
+
+  - name: remove testing folder
+    win_file:
+      path: '{{test_win_get_url_path}}'
+      state: absent

test/integration/targets/win_get_url/tasks/tests_ftp.yml

@@ -0,0 +1,157 @@
+---
+- name: download file from FTP source (check)
+  win_get_url:
+    url: ftp://localhost/anon/file.txt
+    dest: '{{test_win_get_url_path}}\ftp-anon.txt'
+  check_mode: yes
+  register: ftp_anon_check
+
+- name: get results of download file from FTP source (check)
+  win_stat:
+    path: '{{test_win_get_url_path}}\ftp-anon.txt'
+  register: ftp_anon_result_check
+
+- name: assert download file from FTP source (check)
+  assert:
+    that:
+    - ftp_anon_check is changed
+    - not ftp_anon_result_check.stat.exists
+
+- name: download file from FTP source
+  win_get_url:
+    url: ftp://localhost/anon/file.txt
+    dest: '{{test_win_get_url_path}}\ftp-anon.txt'
+  register: ftp_anon
+
+- name: get results of download file from FTP source
+  win_stat:
+    path: '{{test_win_get_url_path}}\ftp-anon.txt'
+  register: ftp_anon_result
+
+- name: assert download file from FTP source
+  assert:
+    that:
+    - ftp_anon is changed
+    - ftp_anon_result.stat.exists
+    - ftp_anon_result.stat.checksum == '67e0de92f29645cc30d8d147b767cceb81756651'
+
+# TODO: Add check for idempotent with force: yes once tmp download and checksum verify are in
+
+- name: download file from FTP source with force no (check)
+  win_get_url:
+    url: ftp://localhost/anon/file.txt
+    dest: '{{test_win_get_url_path}}\ftp-anon.txt'
+    force: no
+  check_mode: yes
+  register: ftp_anon_force_no_check
+
+- name: assert download file from FTP source with force no
+  assert:
+    that:
+    - ftp_anon_force_no_check is not changed
+
+- name: download file from FTP source with force no
+  win_get_url:
+    url: ftp://localhost/anon/file.txt
+    dest: '{{test_win_get_url_path}}\ftp-anon.txt'
+    force: no
+  register: ftp_anon_force_no
+
+- name: assert download file from FTP source with force no
+  assert:
+    that:
+    - ftp_anon_force_no is not changed
+
+- name: set last modified time on FTP source to newer datetime
+  win_shell: (Get-Item -Path '{{test_win_get_url_path}}\ftp\anon\file2.txt').LastWriteTime = (Get-Date).AddHours(24)
+
+- name: download newer file from FTP source to same dest (check)
+  win_get_url:
+    url: ftp://localhost/anon/file2.txt
+    dest: '{{test_win_get_url_path}}\ftp-anon.txt'
+    force: no
+  check_mode: yes
+  register: ftp_anon_force_no_different_check
+
+- name: get result of download newer file from FTP source to same dest (check)
+  win_stat:
+    path: '{{test_win_get_url_path}}\ftp-anon.txt'
+  register: ftp_anon_force_no_different_result_check
+
+- name: assert download newer file from FTP source to same dest (check)
+  assert:
+    that:
+    - ftp_anon_force_no_different_check is changed
+    - ftp_anon_force_no_different_result_check.stat.checksum == '67e0de92f29645cc30d8d147b767cceb81756651'
+
+- name: download newer file from FTP source to same dest
+  win_get_url:
+    url: ftp://localhost/anon/file2.txt
+    dest: '{{test_win_get_url_path}}\ftp-anon.txt'
+    force: no
+  register: ftp_anon_force_no_different
+
+- name: get result of download newer file from FTP source to same dest
+  win_stat:
+    path: '{{test_win_get_url_path}}\ftp-anon.txt'
+  register: ftp_anon_force_no_different_result
+
+- name: assert download newer file from FTP source to same dest (check)
+  assert:
+    that:
+    - ftp_anon_force_no_different is changed
+    - ftp_anon_force_no_different_result.stat.checksum == 'eac3baccd817f7137c00138559e2e62aca64aab0'
+
+- name: fail to download file from ftp protected by username
+  win_get_url:
+    url: ftp://localhost/user/file.txt
+    dest: '{{test_win_get_url_path}}\ftp-user.txt'
+  register: fail_ftp_no_user
+  ignore_errors: yes
+
+- name: assert fail to download file from ftp protected by username
+  assert:
+    that:
+    - fail_ftp_no_user is failed
+    - fail_ftp_no_user is not changed
+    - fail_ftp_no_user.status_code == 550
+    - '"File unavailable (e.g., file not found, no access)." in fail_ftp_no_user.msg'
+
+- name: download FTP file protected by username
+  win_get_url:
+    url: ftp://localhost/user/file.txt
+    dest: '{{test_win_get_url_path}}\ftp-user.txt'
+    url_username: username
+  register: ftp_user_file
+
+- name: get result of download FTP file protected by username
+  win_stat:
+    path: '{{test_win_get_url_path}}\ftp-user.txt'
+  register: ftp_user_file_result
+
+- name: assert download FTP file protected by username
+  assert:
+    that:
+    - ftp_user_file is changed
+    - ftp_user_file_result.stat.exists
+    - ftp_user_file_result.stat.checksum == '0efc2e97611cf74e25ec17a00d4b2cf65d0c28ba'
+
+- name: download FTP file protected by username and password
+  win_get_url:
+    url: ftp://localhost/user-pass/file.txt
+    dest: '{{test_win_get_url_path}}\ftp-user-pass.txt'
+    url_username: userpass
+    url_password: password
+  register: ftp_user_pass_file
+
+- name: get result of download FTP file protected by username and password
+  win_stat:
+    path: '{{test_win_get_url_path}}\ftp-user-pass.txt'
+  register: ftp_user_pass_file_result
+
+- name: assert download FTP file protected by username and password
+  assert:
+    that:
+    - ftp_user_pass_file is changed
+    - ftp_user_pass_file_result.stat.exists
+    - ftp_user_pass_file_result.stat.checksum == '7da5f1124d4a986cba2b4658d38d95eb55afe086'

test/integration/targets/win_get_url/tasks/tests_url.yml

@@ -0,0 +1,124 @@
+- name: download single file (check)
+  win_get_url:
+    url: https://{{test_win_get_url_host}}
+    dest: '{{test_win_get_url_path}}\web.html'
+  check_mode: yes
+  register: http_download_check
+
+- name: get result of download single file (check)
+  win_stat:
+    path: '{{test_win_get_url_path}}\web.html'
+  register: http_download_result_check
+
+- name: assert download single file (check)
+  assert:
+    that:
+    - http_download_check is not failed
+    - http_download_check is changed
+    - http_download_check.url
+    - http_download_check.dest
+    - not http_download_result_check.stat.exists
+
+- name: download single file
+  win_get_url:
+    url: https://{{test_win_get_url_host}}
+    dest: '{{test_win_get_url_path}}\web.html'
+  register: http_download
+
+- name: get result of download single file
+  win_stat:
+    path: '{{test_win_get_url_path}}\web.html'
+  register: http_download_result
+
+- name: assert download single file
+  assert:
+    that:
+    - http_download is not failed
+    - http_download is changed
+    - http_download.url
+    - http_download.dest
+    - http_download_result.stat.exists
+
+# TODO: add check for idempotent run once it is added with force: yes
+
+- name: download single file with force no
+  win_get_url:
+    url: https://{{test_win_get_url_host}}
+    dest: '{{test_win_get_url_path}}\web.html'
+    force: no
+  register: http_download_no_force
+
+- name: assert download single file with force no
+  assert:
+    that:
+    - http_download_no_force is not changed
+
+- name: manually change last modified time on FTP source to older datetime
+  win_shell: (Get-Item -Path '{{test_win_get_url_path}}\web.html').LastWriteTime = (Get-Date -Date "01/01/1970")
+
+- name: download newer file with force no
+  win_get_url:
+    url: https://{{test_win_get_url_host}}
+    dest: '{{test_win_get_url_path}}\web.html'
+    force: no
+  register: http_download_newer_no_force
+
+- name: assert download newer file with force no
+  assert:
+    that:
+    - http_download_newer_no_force is changed
+
+- name: download file to directory
+  win_get_url:
+    url: https://{{test_win_get_url_host}}
+    dest: '{{test_win_get_url_path}}'
+  register: http_download_to_directory
+
+- name: get result of download to directory
+  win_stat:
+    path: '{{test_win_get_url_path}}\{{test_win_get_url_host}}'
+  register: http_download_to_directory_result
+
+- name: assert download file to directory
+  assert:
+    that:
+    - http_download_to_directory is changed
+    - http_download_to_directory_result.stat.exists
+
+- name: download to path with env var
+  win_get_url:
+    url: https://{{test_win_get_url_host}}
+    dest: '%{{test_win_get_url_env_var}}%\http-env.html'
+  register: http_download_with_env
+
+- name: get result of download to path with env var
+  win_stat:
+    path: '{{test_win_get_url_path}}\http-env.html'
+  register: http_download_with_env_result
+
+- name: assert download to path with env var
+  assert:
+    that:
+    - http_download_with_env is changed
+    - http_download_with_env_result.stat.exists
+
+- name: fail when link returns 404
+  win_get_url:
+    url: https://{{test_win_get_url_host}}/skynet_module.html
+    dest: '{{test_win_get_url_path}}\skynet_module.html'
+  ignore_errors: yes
+  register: fail_download_404
+
+- name: assert fail when link returns 404
+  assert:
+    that:
+    - fail_download_404 is not changed
+    - fail_download_404 is failed
+    - fail_download_404.status_code == 404
+
+- name: fail when dest is an invalid path
+  win_get_url:
+    url: https://{{test_win_get_url_host}}
+    dest: Q:\Filez\Cyberdyne.html
+  register: fail_invalid_path
+  failed_when: '"The path ''Q:\Filez'' does not exist for destination ''Q:\Filez\Cyberdyne.html''" not in fail_invalid_path.msg'

test/integration/targets/win_get_url/templates/slimftpd.conf.tmpl

@@ -0,0 +1,28 @@
+# http://www.wiki.uniformserver.com/index.php/SlimFTPd:_Config_File
+BindInterface Local
+BindPort 21
+CommandTimeout 300
+ConnectTimeout 15
+MaxConnections 20
+LookupHosts On
+
+<User "anonymous">
+  Password ""
+  Mount / {{test_win_get_url_path}}\ftp
+  Allow /anon All
+  Deny /user All
+  Deny /user-pass All
+</User>
+
+<User "username">
+  Mount / {{test_win_get_url_path}}\ftp
+  Allow /anon All
+  Allow /user All
+  Deny /user-pass All
+</User>
+
+<User "userpass">
+  Password "password"
+  Mount / {{test_win_get_url_path}}\ftp
+  Allow / All
+</User>