Add API Token authentication #20

micaelbergeron · 2018-09-27T17:05:50Z

This PR add the configuration options to either auth using OAuth (current behavior) or API Tokens as Zenpy supports both configurations.

new configuration options: 'email': the account's email 'api_token': the account's api token

timvisher

I like the overall addition here.

Can you provide some gists of runs with the different configurations and address some of the specific comments I left? Preferably with redacted cats of the config files for each run?

tap_zendesk/__init__.py

timvisher · 2018-09-28T18:38:09Z

tap_zendesk/__init__.py

+        creds.update({
+            "oauth_token": oauth_args.config['access_token'],
+        })
+    except:


Can we make this and the other except: handle a more specific error?

Sure will do.

micaelbergeron · 2018-10-01T14:29:49Z

Thanks for the comments, I'll fix the issues and provide a sample config/runs for you shortly.

- add INFO log for the used auth - fixed bare except: clauses - make sure the OAuth has precedence

…ndesk into add-api-token-auth

micaelbergeron · 2018-10-01T15:59:17Z

@timvisher I think I solved most issues, please do another review.

Here's a gist of runs: https://gist.github.com/micaelbergeron/50493d87ba711389143fcda3645ed88a

timvisher

Looking good.

I'd like to see a bit more documentation and slightly different structure to the auth calls. Also I really don't want to catch Exception.

README.md

timvisher · 2018-10-01T18:23:29Z

tap_zendesk/__init__.py

-    "access_token"
+]
+
+API_CONFIG_KEYS = REQUIRED_CONFIG_KEYS + [


Adding a similar comment here about the mutual exclusivity and preference ordering of these two config sections would be nice.

timvisher · 2018-10-01T18:24:50Z

tap_zendesk/__init__.py

+        return client
+    except ZenpyException:
+        LOGGER.error("OAuth authentication failed.")
+    except Exception:


Is this a ValueError or other kind of specific exception? Could we not catch Exception here by just checking if the args contain a particular key?

The parse_args will raise an Exception (see https://github.com/singer-io/singer-python/blob/dfad8fc467704900e0f268a05919bd34976aaa0e/singer/utils.py#L150) if the required_keys are not present.

So we can't use parse_args to validate that the config is there without catching an Exception when the config is not there.

I'll do the check manually then.

tap_zendesk/__init__.py

- add some documentation - wording - remove dead code, Zenpy doens't raise on failed authentication

micaelbergeron · 2018-10-01T20:32:17Z

@timvisher I think this is ready for another review. Thanks for your help :D

Here's the new output:

Using OAuth

(tap-zendesk.venv) ❯ tap-zendesk -c tap-zendesk/config.json
INFO Using OAuth authentication.

Using API Token

(tap-zendesk.venv) ❯ tap-zendesk -c tap-zendesk/config.json
INFO Using API Token authentication.

When something is wrong

(tap-zendesk.venv) ❯ tap-zendesk -c tap-zendesk/config.json
ERROR No suitable authentication keys provided.
  OAuth:		access_token
  API Token:		email, api_token

I've removed the ZenpyException because it seems it doesn't raise any of these when the client is constructed anyways.

nick-mccoy · 2018-10-02T15:37:09Z

👍Merging this -- thanks for contributing @micaelbergeron!

mbergeron and others added 4 commits September 27, 2018 10:18

add api_token authentication using Zenpy

bde0b59

new configuration options: 'email': the account's email 'api_token': the account's api token

update the README

6831ce5

fix a wrong logging

92aa979

Update README.md

d10c85f

timvisher suggested changes Sep 28, 2018

View reviewed changes

mbergeron added 2 commits October 1, 2018 11:56

apply feedback

27e63ea

- add INFO log for the used auth - fixed bare except: clauses - make sure the OAuth has precedence

Merge branch 'add-api-token-auth' of github.com:micaelbergeron/tap-ze…

0164930

…ndesk into add-api-token-auth

timvisher suggested changes Oct 1, 2018

View reviewed changes

apply feedback

ba26107

- add some documentation - wording - remove dead code, Zenpy doens't raise on failed authentication

micaelbergeron force-pushed the add-api-token-auth branch from 3301d9c to ba26107 Compare October 1, 2018 20:27

nick-mccoy merged commit f155e97 into singer-io:master Oct 2, 2018

luandy64 mentioned this pull request May 13, 2019

Feature/Implementation of OAuth Authorization option to generate bearer token singer-io/tap-pipedrive#54

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API Token authentication #20

Add API Token authentication #20

micaelbergeron commented Sep 27, 2018 •

edited

Loading

timvisher left a comment

timvisher Sep 28, 2018

micaelbergeron Oct 1, 2018

micaelbergeron commented Oct 1, 2018

micaelbergeron commented Oct 1, 2018

timvisher left a comment

timvisher Oct 1, 2018

timvisher Oct 1, 2018

micaelbergeron Oct 1, 2018

micaelbergeron commented Oct 1, 2018 •

edited

Loading

nick-mccoy commented Oct 2, 2018

Add API Token authentication #20

Add API Token authentication #20

Conversation

micaelbergeron commented Sep 27, 2018 • edited Loading

timvisher left a comment

Choose a reason for hiding this comment

timvisher Sep 28, 2018

Choose a reason for hiding this comment

micaelbergeron Oct 1, 2018

Choose a reason for hiding this comment

micaelbergeron commented Oct 1, 2018

micaelbergeron commented Oct 1, 2018

timvisher left a comment

Choose a reason for hiding this comment

timvisher Oct 1, 2018

Choose a reason for hiding this comment

timvisher Oct 1, 2018

Choose a reason for hiding this comment

micaelbergeron Oct 1, 2018

Choose a reason for hiding this comment

micaelbergeron commented Oct 1, 2018 • edited Loading

nick-mccoy commented Oct 2, 2018

micaelbergeron commented Sep 27, 2018 •

edited

Loading

micaelbergeron commented Oct 1, 2018 •

edited

Loading