Support Bugzilla API keys (pending support in python-bugzilla) #238

Merged
merged 1 commit into from Apr 1, 2017

Conversation

Projects
None yet
5 participants
@djmitche
Contributor

djmitche commented Feb 24, 2017

python-bugzilla refers to these as tokens. It would be nice to be able to create an API key and provide it as

[bugzilla_mozilla]
bugzilla.username = dustin@mozilla.com
bugzilla.token = e1af7160b3162d423fdfeb04e230067a75e9b584
@ralphbean

This comment has been minimized.

Show comment
Hide comment
@ralphbean

ralphbean Sep 8, 2015

Owner

Agreed. Any idea if python-bugzilla supports this yet?

Owner

ralphbean commented Sep 8, 2015

Agreed. Any idea if python-bugzilla supports this yet?

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Sep 8, 2015

Contributor

I think so - see the link. Assuming "token" and "API key" are the same (see python-bugzilla/python-bugzilla#5)

Contributor

djmitche commented Sep 8, 2015

I think so - see the link. Assuming "token" and "API key" are the same (see python-bugzilla/python-bugzilla#5)

@grenade

This comment has been minimized.

Show comment
Hide comment

grenade commented Feb 10, 2016

+1

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Jan 18, 2017

Contributor

@ralphbean I've put in a PR for python-bugzilla at python-bugzilla/python-bugzilla#5

As far as using that from bugwarrior, currently it'd have to be configured as

bugzilla.api_key = e1af7160b3162d423fdfeb04e230067a75e9b584

with bugwarrior configured to pass that along to self.bz.login. However, it seems that python-bugzilla prefers a model where it caches credentials elsewhere, so that API consumers don't need to handle them. That would mean setting up python-bugzilla with something like /usr/bin/bugzilla login, then not specifying any credentials in bugwarrior.

Which would you prefer that I implement?

Contributor

djmitche commented Jan 18, 2017

@ralphbean I've put in a PR for python-bugzilla at python-bugzilla/python-bugzilla#5

As far as using that from bugwarrior, currently it'd have to be configured as

bugzilla.api_key = e1af7160b3162d423fdfeb04e230067a75e9b584

with bugwarrior configured to pass that along to self.bz.login. However, it seems that python-bugzilla prefers a model where it caches credentials elsewhere, so that API consumers don't need to handle them. That would mean setting up python-bugzilla with something like /usr/bin/bugzilla login, then not specifying any credentials in bugwarrior.

Which would you prefer that I implement?

@ryneeverett

This comment has been minimized.

Show comment
Hide comment
@ryneeverett

ryneeverett Jan 19, 2017

Collaborator

Seems like each auth model has it's pros and cons. One only persists temporary keys (good for security) but presumably requires interactive authentication (bad for the server). The other uses long-term keys (ok if you have a secret management system). I would think we'd be happy to support either/or/both.

Collaborator

ryneeverett commented Jan 19, 2017

Seems like each auth model has it's pros and cons. One only persists temporary keys (good for security) but presumably requires interactive authentication (bad for the server). The other uses long-term keys (ok if you have a secret management system). I would think we'd be happy to support either/or/both.

@djmitche djmitche changed the base branch from master to develop Feb 24, 2017

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Feb 24, 2017

Contributor

Look, ma, now I'm a pull request!

I've added support for api keys in python-bugzilla/python-bugzilla#5, although it is not in a released version yet. Still, I tested this with a temporary install of the latest git master and it worked OK both with password and api key auth. Once a new release is made, I would propose requiring that version in setup.py, but I'm also OK leaving it unspecified in setup.py and keeping the exception when using a too-old version.

Contributor

djmitche commented Feb 24, 2017

Look, ma, now I'm a pull request!

I've added support for api keys in python-bugzilla/python-bugzilla#5, although it is not in a released version yet. Still, I tested this with a temporary install of the latest git master and it worked OK both with password and api key auth. Once a new release is made, I would propose requiring that version in setup.py, but I'm also OK leaving it unspecified in setup.py and keeping the exception when using a too-old version.

@djmitche djmitche changed the title from Support Bugzilla API keys to [DO NOT MERGE] Support Bugzilla API keys (pending support in python-bugzilla) Mar 1, 2017

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Mar 1, 2017

Contributor

I just got word from @crobinso that he'll cut a new release of python-bugzilla in a few weeks, at which point we can depend on that version.

Contributor

djmitche commented Mar 1, 2017

I just got word from @crobinso that he'll cut a new release of python-bugzilla in a few weeks, at which point we can depend on that version.

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Mar 13, 2017

Contributor

(rebased onto latest develop)

Contributor

djmitche commented Mar 13, 2017

(rebased onto latest develop)

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Mar 31, 2017

Contributor

(rebased again, and a requirement for the newly-released python-bugzilla>=2.1.0 added)

ready for review and merging, I think!

Contributor

djmitche commented Mar 31, 2017

(rebased again, and a requirement for the newly-released python-bugzilla>=2.1.0 added)

ready for review and merging, I think!

@djmitche djmitche changed the title from [DO NOT MERGE] Support Bugzilla API keys (pending support in python-bugzilla) to Support Bugzilla API keys (pending support in python-bugzilla) Mar 31, 2017

bugwarrior/services/bz.py
+ except TypeError:
+ raise Exception("This version of python-bugzilla does not support api keys")
+ else:
+ password = self.config.get('password', self.username)

This comment has been minimized.

@ryneeverett

ryneeverett Apr 1, 2017

Collaborator

Shouldn't this still be self.get_password?

@ryneeverett

ryneeverett Apr 1, 2017

Collaborator

Shouldn't this still be self.get_password?

bugwarrior/services/bz.py
+ if 'username' not in service_config:
+ die("[%s] has no 'bugzilla.username'" % (target,))
+ if not ('password' in service_config or 'api_key' in service_config):
+ die("[%s] has no 'bugzilla.password' or 'bugzilla.api_key'" % (target,))

This comment has been minimized.

@ryneeverett

ryneeverett Apr 1, 2017

Collaborator

The keys variable appears to be unused. My preference would be to stick with the loop where it works since that makes modifications to the required configuration values easy:

req = ['username', 'base_uri']
for option in req:
    if option not in service_config:
        die("[%s] has no 'bugzilla.%s'" % (target, option))

if not ('password' in service_config or 'api_key' in service_config):
    die("[%s] has no 'bugzilla.password' or 'bugzilla.api_key'" % (target,))
@ryneeverett

ryneeverett Apr 1, 2017

Collaborator

The keys variable appears to be unused. My preference would be to stick with the loop where it works since that makes modifications to the required configuration values easy:

req = ['username', 'base_uri']
for option in req:
    if option not in service_config:
        die("[%s] has no 'bugzilla.%s'" % (target, option))

if not ('password' in service_config or 'api_key' in service_config):
    die("[%s] has no 'bugzilla.password' or 'bugzilla.api_key'" % (target,))
setup.py
@@ -47,7 +47,7 @@
activecollab=["pypandoc", "pyac>=0.1.5"],
bts=["PySimpleSOAP","python-debianbts>=2.6.1"],
trac=["offtrac"],
- bugzilla=["python-bugzilla"],
+ bugzilla=["python-bugzilla>=2.1.0"],

This comment has been minimized.

@ryneeverett

ryneeverett Apr 1, 2017

Collaborator

Do we have to add this constraint? Couldn't one continue using password authentication with older versions?

@ryneeverett

ryneeverett Apr 1, 2017

Collaborator

Do we have to add this constraint? Couldn't one continue using password authentication with older versions?

@ryneeverett

This comment has been minimized.

Show comment
Hide comment
@ryneeverett

ryneeverett Apr 1, 2017

Collaborator

By the way, thanks for persisting with this so long. My comments are regarding diffs that look far different from when you originally wrote the patch.

Collaborator

ryneeverett commented Apr 1, 2017

By the way, thanks for persisting with this so long. My comments are regarding diffs that look far different from when you originally wrote the patch.

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Apr 1, 2017

Contributor

No worries - it was more than a trivial rebase, so it definitely deserved another look.

Contributor

djmitche commented Apr 1, 2017

No worries - it was more than a trivial rebase, so it definitely deserved another look.

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Apr 1, 2017

Contributor

Branch updated

Contributor

djmitche commented Apr 1, 2017

Branch updated

@ryneeverett ryneeverett merged commit 21f46ea into ralphbean:develop Apr 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment