Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix auth in wps #611

Merged
merged 5 commits into from
Oct 18, 2019
Merged

Conversation

cehbrecht
Copy link
Contributor

This PR fixes #609 in the wps module. It keeps the old interface with verify only for the WebProcessingService class. Internally only the self.auth parameter should be used.

I have made a quick test with birdy.

@cehbrecht
Copy link
Contributor Author

@eric-spitler @Tiranno Please give a comment.

@eric-spitler
Copy link
Contributor

The WPS* classes already had the username|password|cert|verify arguments at the time of addition of the Authentication class. They were left in there for backward compatibility with the attempt to allow them to override the values in an Authentication instance (if needed) when using readFromUrl and such. It is certainly more consistent to enforce the use of auth=, but the changes here will be breaking changes for anyone using the original authentication methods.

There is a comment in the class docs about deprecation, but deprecated functionality needs to remain functional and produce warnings to the developer so they can make the necessary updates prior to removing the functionality altogether.

@cehbrecht - Does owslib have a development timeline for how long to keep deprecated functionality? Number of releases?

owslib/wps.py Outdated
@@ -505,21 +489,17 @@ def __init__(self, version=WPS_DEFAULT_VERSION, verbose=False, timeout=None, aut
super(WPSCapabilitiesReader, self).__init__(
version=version, verbose=verbose, timeout=timeout, auth=auth)

def readFromUrl(self, url, username=None, password=None,
headers=None, verify=True, cert=None):
def readFromUrl(self, url, headers=None):
Copy link
Contributor

@eric-spitler eric-spitler Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should really issue a DeprecationWarning here for some time before removing existing functionality to avoid breaking code.

Note that this means a two-phase change - one to add the DeprecationWarning, another later to remove the kwargs.

def readFromUrl(self, url, username=None, password=None, headers=None, verify=True, cert=None):
    import warnings
    message = 'The use of "username", "password", "verify", and "cert" is deprecated. ' + \
        'Please use the "auth" keyword during class instantiation. ' + \
        'These keywords will be removed in a future release.'
    warnings.warn(message, DeprecationWarning)
    # rest of the method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add the deprecation warning in this PR.

I would have considered the WPSReader class as part of the internal interface. Though this is not obvious from the code.

@cehbrecht
Copy link
Contributor Author

@tomkralidis Do you have recommendations on how to handle interface deprecations?

@tomkralidis
Copy link
Member

@cehbrecht we have had such warnings in other parts of OWSLib (example: d4e26ea) for quite some time. Technically OWSLib is a pre 1.0 project so we have not gone the route of stable branch support (everything happens straight and away in master).

All to say I would leave deprecation handling (how long, etc.) up to the maintainer of the given code. We don't have an overall policy for OWSLib (yet) but perhaps this is a good time to make one).

@cehbrecht
Copy link
Contributor Author

@eric-spitler @Tiranno @tomkralidis I have added the method _fix_auth which handles the deprecation warning. The interface is converted to the master branch version ... except that I use verify=None instead of verify=True.

@cehbrecht
Copy link
Contributor Author

@tomkralidis Can I merge? It fixes at least the WPS for HTTPS ...

@cehbrecht cehbrecht added this to the 0.19.0 milestone Oct 17, 2019
@tomkralidis
Copy link
Member

@cehbrecht +1

@cehbrecht cehbrecht merged commit 5da48b2 into geopython:master Oct 18, 2019
@cehbrecht cehbrecht deleted the issue-609-wps-fix-auth branch October 18, 2019 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

openURL not respecting Authentication object's verify property
3 participants