Add BasicAuth to transfer API requests

[dkirkham]

This PR adds a Basic Auth capability to Wagtail-Transfer API requests. This is useful if a source site is protected with Basic Auth.

The rationale for this, in my case, is to use Basic Auth to protect a testing/staging site from accidental exposure to unsuspecting readers who may not recognise the difference from the production site. As the site advertises events, the likely fictitious test information on the testing/staging site is likely to mislead. Basic Auth also keeps unscrupulous bots out.

Basic Auth works fine with browsers, but is clearly mission over in the case of Wagtail-Transfer.

Adding the auth parameter to the various requests.get()/post() calls was quite straightforward except for the call that transfers image files in files.py. In that case I've carried the source site name (eg. "staging") via the ImportContext as I couldn't see another way without changing lots of other function type signatures. Alternative suggestions are welcome.

The new capability and its configuration are documented in the README.md.

[jacobtoppm]

This is a pretty broad try/except for just retrieving a value or None. Could you catch eg KeyError instead?

[dkirkham]

I guess so... presumably any other errors, if present, will pop up in the settings or requests modules.

Another option, if you prefer it, is to use dict.get() with a default:

return settings.WAGTAILTRANSFER_SOURCES[source].get('BASIC_AUTH', None)

[dkirkham]

Hi Jacob, it's been a busy week... finally made the above change.

[gasman]

Digging through old PRs... unfortunately this is now causing test failures with a KeyError on WAGTAILTRANSFER_SOURCES[source].

Also, this could really do with some tests to confirm that the auth credentials are indeed showing up on the HTTP requests where appropriate.

[dkirkham]

Hi @gasman, are you looking at merging this now? If so, I can look at rebasing and addressing the KeyError.

Regarding the test errors, is this with the wagtail-transfer test suite, or some other? I am intrigued why this has suddenly popped up since the PR hasn’t been merged.

[gasman]

Hi @dkirkham - aiming to get a new release out today to address some Wagtail 5.x compatibility issues, so I suspect there won't be time to get this in. (However, I now see that there are some further deprecations introduced by 5.2 that will need attention for the 6.0 release, so there will probably be a follow-up fairly soon...)

The test failures are happening on the wagtail-transfer test suite. Is it possible that this wasn't retested with the full suite after the fix in de47c00 was applied, maybe? Before that point it would have been caught by the try/except. Otherwise, it may be that some other commits in the meantime have changed the test config.

[dkirkham]

I haven’t any time to look at this for a few days, and the timezone here was against even having a quick look earlier. Since this hasn’t been merged anyway, it is probably safest to get your new release out (which it looks like you’ve done) and I’ll rebase to that.

[dkirkham]

Hi @gasman, it appears when I put this work aside in early 2021, I had left some incomplete updates to the tests. Most of the test_import tests required an update so that the source_site parameter was being passed to ImportPlanner, which was the fundamental reason the tests were failing.

I've now completed those changes, added a specific test to ensure the auth parameter is being passed to the (mocked) requests.get() method, and rebased.

During my own testing, I found that most of the test_import unit tests did not end up calling the new requests_auth() function or the surrounding requests.get() method. I also found that views.import_model() is never called. I haven't delved into the logic here, but it may indicate a lack of test coverage.