Optional API token-based authentication #8

rthomson · 2017-11-21T23:48:59Z

This pull request implements optional API token-based authentication when netbox is configured with LOGIN_REQUIRED = True in configuration.py.

netbox.yml example:

netbox:
    main:
        api_url: 'https://localhost/api/dcim/devices/'
        api_token: '1234567890987654321234567890987654321'

where the value of api_token is equivalent to any valid netbox user profile API token.

The api_token key can be omitted from netbox.yml and netbox-as-ansible-inventory behaves as expected (no change to previous behavior).

rthomson · 2017-11-21T23:53:50Z

Seems there are some tests to update for this PR to pass. Looking into it.

…mpty newline.

coveralls · 2017-11-22T00:22:30Z

Coverage decreased (-1.5%) to 97.692% when pulling d72878e on rthomson:master into f88b5f2 on AAbouZaid:master.

coveralls · 2017-11-22T00:22:30Z

Coverage decreased (-1.5%) to 97.692% when pulling d72878e on rthomson:master into f88b5f2 on AAbouZaid:master.

coveralls · 2017-11-22T00:22:30Z

Coverage decreased (-1.5%) to 97.692% when pulling d72878e on rthomson:master into f88b5f2 on AAbouZaid:master.

rthomson · 2017-11-22T00:34:25Z

Got the CI tests to pass by updating tests/test_netbox.py.

I'm not yet clear how on how update the tests so that coveralls passes for key_value = "" and api_url_headers = {}.

I'm new to tests to please bear with me.

coveralls · 2017-11-22T01:33:36Z

Coverage decreased (-0.8%) to 98.4% when pulling 13302ae on rthomson:master into f88b5f2 on AAbouZaid:master.

coveralls · 2017-11-22T01:33:36Z

Coverage decreased (-0.8%) to 98.4% when pulling 13302ae on rthomson:master into f88b5f2 on AAbouZaid:master.

coveralls · 2017-11-22T01:33:36Z

Coverage decreased (-0.8%) to 98.4% when pulling 13302ae on rthomson:master into f88b5f2 on AAbouZaid:master.

coveralls · 2017-11-22T01:42:38Z

Coverage decreased (-0.8%) to 98.4% when pulling f51e62f on rthomson:master into f88b5f2 on AAbouZaid:master.

rthomson · 2017-11-22T01:44:15Z

I'm admitting defeat on the final coveralls test. I'm not sure I understand how coveralls works nor how to write tests that will cover the case that coveralls is still complaining about.

coveralls · 2017-11-22T04:00:06Z

Coverage remained the same at 99.2% when pulling 62bc71e on rthomson:master into f88b5f2 on AAbouZaid:master.

rthomson · 2017-11-22T04:02:12Z

💯 Finally!

aabouzaid · 2017-11-22T21:38:44Z

@rthomson Thanks a lot, that looks pretty nice!
Could you please switch it develop branch instead master, then I will review and merge.

rthomson · 2017-11-22T21:40:31Z

@aabouzaid done!

aabouzaid · 2017-11-22T22:30:47Z

Cool, I'm doing a quick review now 👍

aabouzaid · 2017-11-22T22:36:38Z

netbox/netbox.py

@@ -79,6 +79,7 @@ def __init__(self, script_args, script_config_data):
        # Script configuration.
        self.script_config = script_config_data
        self.api_url = self._config(["main", "api_url"])
+        self.api_token = self._config(["main", "api_token"], ignore_key_error=True)


Why ignore_key_error? This should be default=None instead ignoring key error.

Letting the default value be assigned when api_token was missing from netbox.yml was resulting a sys.exit() in _config() due to the if value: conditional evaluation on line 144-148 (that I removed/changed).

Looking into it.

aabouzaid · 2017-11-22T22:37:09Z

netbox/netbox.py

        """Get value from config var.

        Args:
            key_path: A list has the path of the key.
            default: Default value if the key is not found.

        Returns:
-            The value of the key from config file or the default value.
+            The value of the key from config file or the default value


The dot? :-)

I don't even remember that! Typo.

aabouzaid · 2017-11-22T22:39:10Z

netbox/netbox.py

-            key_value = value
-        else:
-            sys.exit("The key '%s' is not found in config file." % ".".join(key_path))
+        key_value = self._get_value_by_path(config, key_path, ignore_key_error=ignore_key_error, default=default)


I kept that part like that because I like to have a different message than the default one in _get_value_by_path (I found it a bit misleading, that's why I use a different error message here).
So I'd suggest to edit _get_value_by_path to have the ability to change the error message.

This is the part that would result in sys.exit() if a default value of "" was assigned to api_token when missing from netbox.yml.

I'm reviewing the logic to see if there's a simpler/better way to have an optional key for api_token in netbox.yml.

aabouzaid · 2017-11-22T22:41:45Z

tests/test_netbox.py

@@ -34,6 +34,7 @@
 netbox:
    main:
        api_url: 'http://localhost/api/dcim/devices/'
+        api_token: '1234567890987654321234567890987654321'


Let's have a commented key inside netbox.yaml to make it part of conf file.
e.g.

netbox: main: api_url: 'http://localhost/api/dcim/devices/' #api_token: ''

Sounds good.

aabouzaid

Some parts need a second look?
Thanks a lot for your effort :-)

coveralls · 2017-11-22T23:48:14Z

Coverage remained the same at 99.2% when pulling f25cf5f on rthomson:master into 8607dd5 on AAbouZaid:devel.

rthomson · 2017-11-22T23:57:21Z

I made some changes to remove the stuff around ignore_key_error that I added. It now relies on the default value without going to sys.exit() if api_token isn't found. I did have to add a conditional to __init__ for NetboxAsInventory to sys.exit() if config was empty to satisfy a test (that I believe was satisfied by the previous logic that was sys.exit()ing when api_token wasn't found).

Not sure if this is any better or not. Also, I'm fumbling my way though pull requests, CI tests and code review as there are all new for me! I appreciate your pointing out when/how components of this PR require improvement.

coveralls · 2017-11-23T00:37:17Z

Coverage increased (+0.01%) to 99.213% when pulling df6c248 on rthomson:master into 8607dd5 on AAbouZaid:devel.

aabouzaid · 2017-11-23T17:26:20Z

You actually are doing good :-)
I'd suggest to have a boolean arg in _config, e.g. optional=Truel, so it passed that value to _get_value_by_path to ignore the error.
That should be fine.

And in all cases, since there a new behavior has been introduced, it's fine to change the tests to fit that case.

aabouzaid · 2017-11-23T17:27:50Z

Actually now I'm thinking about another way to deal with config in general.

aabouzaid · 2017-11-27T19:35:19Z

@rthomson I did the fix to make any conf key optional, later I will find a better way to handle the config.

Could you please pull the branch optional_key and rebase it on yours then push it to merge.

Thanks.

rthomson · 2017-11-29T20:49:00Z

Could you please pull the branch optional_key and rebase it on yours then push it to merge.

I will take a look this week!

Thank you.

…mpty newline.

coveralls · 2017-12-05T19:44:25Z

Coverage increased (+0.03%) to 99.231% when pulling f58fa3a on rthomson:master into 8607dd5 on AAbouZaid:devel.

rthomson · 2017-12-05T19:48:00Z

Pulling a new branch from upstream and rebasing against my changes, including manual merge conflict resolution, was an interesting learning experience!

I'm not sure but I think it might have worked...

aabouzaid · 2017-12-05T20:42:15Z

Well, now we have duplication of commits (the rebase should be on my last comment only).
So you need to do something like that to fix it.

On your branch.

git checkout master
git checkout c646552
git push origin master -f

I think it's a nice experience in all cases 😁

rthomson · 2017-12-05T21:05:01Z

Well, now we have duplication of commits (the rebase should be on my last comment only).

Oops.

I think it's a nice experience in all cases 😁

Absolutely! And thanks again for your patience.

I'll keep working at it.

aabouzaid · 2017-12-05T22:05:33Z

It has been merged 🎉
It had some extra commits but I did rebase it on my side to avoid more toing and froing :-)

Thanks a lot for your effort @rthomson, keep going you are doing well 🤘

aabouzaid and others added 4 commits October 8, 2017 03:25

version 1.0.6

ed3e51a

v1.0.7

bfb95b9

version 1.0.8

f88b5f2

Added optional API token authentication (main->api_token)

29955a1

rthomson added 2 commits November 21, 2017 16:09

Updated tests for new argument and configuration. Removed duplicate e…

33f29de

…mpty newline.

Fixed another whitespace issue

d72878e

Updated how optional/ignore_key_error is handled

13302ae

Added test for api_token not set, hopefully fixing coverage reduction

f51e62f

One more shot at my test coverage struggle!

62bc71e

rthomson changed the base branch from master to devel November 22, 2017 21:40

Implements names, slugs and labels for group_by and host_vars

e671ff1

aabouzaid reviewed Nov 22, 2017

View reviewed changes

aabouzaid requested changes Nov 22, 2017

View reviewed changes

Alternative to ignore_key_error hackery.

f25cf5f

Alternative to ignore_key_error hackery - take 2.

df6c248

ability to have optional config key

c646552

rthomson added 7 commits December 5, 2017 11:31

Added optional API token authentication (main->api_token)

007191b

Updated tests for new argument and configuration. Removed duplicate e…

095e2d6

…mpty newline.

Fixed another whitespace issue

d9dd551

Updated how optional/ignore_key_error is handled

7fd89a1

Added test for api_token not set, hopefully fixing coverage reduction

78c0a8d

One more shot at my test coverage struggle!

6b6913b

Alternative to ignore_key_error hackery - take 2.

f58fa3a

Merge branch 'names-slugs-labels'

7171516

aabouzaid approved these changes Dec 5, 2017

View reviewed changes

aabouzaid merged commit 9c97a1d into aabouzaid:devel Dec 5, 2017

Optional API token-based authentication #8

Optional API token-based authentication #8

Conversation

rthomson commented Nov 21, 2017

rthomson commented Nov 21, 2017

coveralls commented Nov 22, 2017

coveralls commented Nov 22, 2017

coveralls commented Nov 22, 2017

rthomson commented Nov 22, 2017

coveralls commented Nov 22, 2017

coveralls commented Nov 22, 2017

coveralls commented Nov 22, 2017

coveralls commented Nov 22, 2017 • edited Loading

rthomson commented Nov 22, 2017

coveralls commented Nov 22, 2017 • edited Loading

rthomson commented Nov 22, 2017

aabouzaid commented Nov 22, 2017 • edited Loading

rthomson commented Nov 22, 2017

aabouzaid commented Nov 22, 2017

aabouzaid Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

rthomson Nov 22, 2017

Choose a reason for hiding this comment

aabouzaid Nov 22, 2017

Choose a reason for hiding this comment

rthomson Nov 22, 2017

Choose a reason for hiding this comment

aabouzaid Nov 22, 2017

Choose a reason for hiding this comment

rthomson Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

aabouzaid Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

rthomson Nov 22, 2017

Choose a reason for hiding this comment

aabouzaid left a comment • edited Loading

Choose a reason for hiding this comment

coveralls commented Nov 22, 2017 • edited Loading

rthomson commented Nov 22, 2017 • edited Loading

coveralls commented Nov 23, 2017 • edited Loading

aabouzaid commented Nov 23, 2017 • edited Loading

aabouzaid commented Nov 23, 2017

aabouzaid commented Nov 27, 2017

rthomson commented Nov 29, 2017

coveralls commented Dec 5, 2017 • edited Loading

rthomson commented Dec 5, 2017

aabouzaid commented Dec 5, 2017

rthomson commented Dec 5, 2017 • edited Loading

aabouzaid commented Dec 5, 2017

coveralls commented Nov 22, 2017 •

edited

Loading

coveralls commented Nov 22, 2017 •

edited

Loading

aabouzaid commented Nov 22, 2017 •

edited

Loading

aabouzaid Nov 22, 2017 •

edited

Loading

rthomson Nov 22, 2017 •

edited

Loading

aabouzaid Nov 22, 2017 •

edited

Loading

aabouzaid left a comment •

edited

Loading

coveralls commented Nov 22, 2017 •

edited

Loading

rthomson commented Nov 22, 2017 •

edited

Loading

coveralls commented Nov 23, 2017 •

edited

Loading

aabouzaid commented Nov 23, 2017 •

edited

Loading

coveralls commented Dec 5, 2017 •

edited

Loading

rthomson commented Dec 5, 2017 •

edited

Loading