REST API endpoint for tokens #1375
Conversation
|
Didn't test it locally at all, just basically Will see what CI says tests wise. |
|
Thanks for the PR! Reviewing the history of that commit in NetBox, I see that it was part of netbox-community/netbox#6592 - did you consider pulling in the entire PR? Also, as this is a new feature, I'm going to retarget this PR from |
|
I didn't notice it was part of a larger PR, good spot. I shall probably pull the rest in if it makes sense. |
imranh2
changed the title
|
Squashed all the commits from that linked PR in Netbox and quickly tried to apply it to Nautobot, expecting tests to fail in this first push. Also need to rebase on my end on to |
bryanculver
added
the
type: feature
|
Actually ran flake8 locally this time. Also think I got the only place where the codebases weren't the same. |
|
Went over the code with flake8 and black now. |
| @@ -66,6 +67,26 @@ class Meta: | |||
| fields = ("id", "url", "name", "user_count") | |||
|
|
|||
|
|
|||
| class TokenSerializer(ValidatedModelSerializer): | |||
| url = serializers.HyperlinkedIdentityField(view_name="users-api:token-detail") | |||
| key = serializers.CharField(min_length=40, max_length=40, allow_blank=True, required=False) | |||
There was a problem hiding this comment.
Outside the scope of this PR, but we'll need to keep this in mind when we get to addressing #941.
|
Testing all green on my end https://github.com/imranh2/nautobot/runs/5252733577 |
|
@glennmatthews Can I get another review on this please? |
| self.create_data = [ | ||
| { | ||
| "user": self.user.pk, | ||
| }, | ||
| { | ||
| "user": self.user.pk, | ||
| }, | ||
| { | ||
| "user": self.user.pk, | ||
| }, | ||
| ] |
There was a problem hiding this comment.
So this is interesting... reading this here made me realize that TokenViewSet is actually a full-featured API view set of its own, meaning that it can itself be used to create, bulk-create, update, and bulk-update Tokens, in addition to the specific Token creation flow enabled by TokenProvisionView. The overridden get_queryset method does mean that a non-superuser can at least only create/update tokens for their own user account, so I think this is OK, but just leaving a comment here as I'd missed this on initial review.
There was a problem hiding this comment.
On re-review I actually have some uncertainty here - would you mind adding a few more API test cases for safety:
- User A tries to
createa token belonging to User B (should fail with an appropriate error) - User B queries for a list of tokens (confirm that User A's tokens do not appear in the list)
- User A tries to
updatetheir own token so that it refers to User B as the owner (should fail with an appropriate error)
There was a problem hiding this comment.
Sorry for the crossed wires here - since addressing Bryan's comments about removing the user field from the serializer entirely, my first and third suggestions are no longer applicable. I'll offer a suggested implementation below.
|
@glennmatthews Thanks for the review, sorry for the delay, I saw you'd replied and set aside a chunk of time to go through it but you only wanted minor changes which took less time than waiting for my test to run :) Testing green on my end. |
|
Rebased on top of latest @glennmatthews assuming what @lampwins is asking for just doesn't work out what's the next steps to get this to land? |
|
Good news everyone! I figured out the issue. The timestamp being searched for is being considered an invalid timestamp. Changed the test to: ...
def test_expires(self):
params = {"expires": "3000-01-01T00:00:00"}
fs = self.filterset(params, self.queryset)
print(fs.errors)
self.assertEqual(fs.qs.count(), 2)Which returned: This is invalid because the auto-generation from BaseFilterSet is creating a the filter as a MultipleChoiceFilter of DateTime type: This means the filter set will expect these as an array instead: ...
def test_expires(self):
params = {"expires": ["3000-01-01 00:00:00"]}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2)
params = {"expires__gte": ["2021-01-01 00:00:00"]}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2)
params = {"expires__lte": ["2021-01-01 00:00:00"]}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 1)Additionally, it's expecting a date/time format of Result: Full Diff: diff --git a/nautobot/users/filters.py b/nautobot/users/filters.py
index 773262149..e066c50f9 100644
--- a/nautobot/users/filters.py
+++ b/nautobot/users/filters.py
@@ -85,16 +85,10 @@ class TokenFilterSet(BaseFilterSet):
to_field_name="username",
label="User (name)",
)
- created = django_filters.DateTimeFilter()
- created__gte = django_filters.DateTimeFilter(field_name="created", lookup_expr="gte")
- created__lte = django_filters.DateTimeFilter(field_name="created", lookup_expr="lte")
- expires = django_filters.DateTimeFilter()
- expires__gte = django_filters.DateTimeFilter(field_name="expires", lookup_expr="gte")
- expires__lte = django_filters.DateTimeFilter(field_name="expires", lookup_expr="lte")
class Meta:
model = Token
- fields = ["id", "key", "write_enabled"]
+ fields = ["id", "key", "write_enabled", "created", "expires"]
class ObjectPermissionFilterSet(BaseFilterSet):
diff --git a/nautobot/users/tests/test_filters.py b/nautobot/users/tests/test_filters.py
index f10c250f4..4ee14dfa4 100644
--- a/nautobot/users/tests/test_filters.py
+++ b/nautobot/users/tests/test_filters.py
@@ -228,11 +228,11 @@ class TokenTestCase(TestCase):
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2)
def test_expires(self):
- params = {"expires": "3000-01-01T00:00:00"}
+ params = {"expires": ["3000-01-01 00:00:00"]}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2)
- params = {"expires__gte": "2021-01-01T00:00:00"}
+ params = {"expires__gte": ["2021-01-01 00:00:00"]}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2)
- params = {"expires__lte": "2021-01-01T00:00:00"}
+ params = {"expires__lte": ["2021-01-01 00:00:00"]}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 1)
def test_key(self): |
|
@bryanculver great find! 🙂 I wonder what the correct way to document this is, not only for end users of this but also Devs in general as using the auto-generated for datetime related stuff gives you something that isn't a drop in replacement of Unless there's already a note somewhere I missed... |
|
@imranh2 I couldn't push to your fork to update this PR. If you don't mind implementing the changes and handling the merge conflict we can do a swift re-review and get this feature out soon 😄 Regarding updating developer documentation, I'll add it as a housekeeping item to address it. |
|
@bryanculver sweet, can apply your diff and sort out any conflicts :) Will aim to do so within the next 24 hours hopefully |
|
Applied the diff from @bryanculver (thanks again) Also rebased on top of the latest version of Testing green on my end. |
|
Latest changes are just accepting @bryanculver suggestion to remove super users from seeing peoples tokens. Also a rebase on top of the latest |
nautobot/users/filters.py
Outdated
| queryset=get_user_model().objects.all(), | ||
| label="User", | ||
| ) | ||
| user = django_filters.ModelMultipleChoiceFilter( |
There was a problem hiding this comment.
As I was reviewing the test criteria, since we are removing the ability for superusers to see all user API tokens, I don't know if this will do anything anymore. @glennmatthews Thoughts on leaving this is for potential future uses?
The TokenViewSet as implemented will already do user filtering for non-superusers.
There was a problem hiding this comment.
I think you're right - we could probably:
- Remove
user_idanduserfrom TokenFilterSet - Remove
userfromTokenSerializer - Update the example in
authentication.mdaccordingly.
|
|
Latest requested changes incorporated. Testing green on my end. |
| self.create_data = [ | ||
| { | ||
| "user": self.user.pk, | ||
| }, | ||
| { | ||
| "user": self.user.pk, | ||
| }, | ||
| { | ||
| "user": self.user.pk, | ||
| }, | ||
| ] |
There was a problem hiding this comment.
On re-review I actually have some uncertainty here - would you mind adding a few more API test cases for safety:
- User A tries to
createa token belonging to User B (should fail with an appropriate error) - User B queries for a list of tokens (confirm that User A's tokens do not appear in the list)
- User A tries to
updatetheir own token so that it refers to User B as the owner (should fail with an appropriate error)
|
Added @bryanculver suggestions. Can't figure out how to get tests working for @glennmatthews suggestions, I left a commented out test in |
|
|
||
| class Meta: | ||
| model = Token | ||
| fields = ("id", "url", "display", "user", "created", "expires", "key", "write_enabled", "description") |
There was a problem hiding this comment.
| fields = ("id", "url", "display", "user", "created", "expires", "key", "write_enabled", "description") | |
| fields = ("id", "url", "display", "created", "expires", "key", "write_enabled", "description") |
There was a problem hiding this comment.
Doing that causes:
======================================================================
FAIL: test_create_object (nautobot.users.tests.test_api.TokenTest)
POST a single object with permission.
----------------------------------------------------------------------
Traceback (most recent call last):
File "/source/nautobot/utilities/testing/api.py", line 340, in test_create_object
self.assertHttpStatus(response, status.HTTP_201_CREATED)
File "/source/nautobot/utilities/testing/views.py", line 150, in assertHttpStatus
self.assertEqual(response.status_code, expected_status, err_message)
AssertionError: 400 != 201 : Expected HTTP status 201; received 400: {'user': [ErrorDetail(string='This field cannot be null.', code='null')]}
----------------------------------------------------------------------
Trying to work out where this dependency for user is coming from... If you know, please shout!
| self.create_data = [ | ||
| { | ||
| "user": self.user.pk, | ||
| }, | ||
| { | ||
| "user": self.user.pk, | ||
| }, | ||
| { | ||
| "user": self.user.pk, | ||
| }, | ||
| ] |
There was a problem hiding this comment.
Sorry for the crossed wires here - since addressing Bryan's comments about removing the user field from the serializer entirely, my first and third suggestions are no longer applicable. I'll offer a suggested implementation below.
nautobot/users/tests/test_api.py
Outdated
| # def test_provision_token_diff_user(self): | ||
| # """ | ||
| # Test the behavior of the token provisioning view when a different user is supplied. | ||
| # """ | ||
| # # Create a user and token | ||
| # data = { | ||
| # "username": "user1", | ||
| # "password": "abc123", | ||
| # } | ||
| # user1 = User.objects.create_user(**data) | ||
| # Token(user=user1).save() | ||
| # token = Token.objects.get(user=user1) | ||
| # # create a 2nd user we'll try to make a token for | ||
| # data = { | ||
| # "username": "user2", | ||
| # "password": "password", | ||
| # } | ||
| # user2 = User.objects.create_user(**data) | ||
|
|
||
| # header = { | ||
| # "Authorization:": "Token " + token.key, | ||
| # } | ||
| # data = { | ||
| # "user": user2.id.__str__(), | ||
| # "expires": "2100-01-01T00:00:00.000Z", | ||
| # "key": "abcdefghijklmnopqrstubwxyzabcdefghijklmnopqrst", | ||
| # "write_enabled": "true", | ||
| # "description": "string" | ||
| # } | ||
| # print(type(user2.id)) | ||
| # print(data) | ||
| # url = reverse("users-api:tokens") | ||
|
|
||
| # response = self.client.post(url, header=header, data=data) | ||
| # self.assertEqual(response.status_code, 403) |
There was a problem hiding this comment.
| # def test_provision_token_diff_user(self): | |
| # """ | |
| # Test the behavior of the token provisioning view when a different user is supplied. | |
| # """ | |
| # # Create a user and token | |
| # data = { | |
| # "username": "user1", | |
| # "password": "abc123", | |
| # } | |
| # user1 = User.objects.create_user(**data) | |
| # Token(user=user1).save() | |
| # token = Token.objects.get(user=user1) | |
| # # create a 2nd user we'll try to make a token for | |
| # data = { | |
| # "username": "user2", | |
| # "password": "password", | |
| # } | |
| # user2 = User.objects.create_user(**data) | |
| # header = { | |
| # "Authorization:": "Token " + token.key, | |
| # } | |
| # data = { | |
| # "user": user2.id.__str__(), | |
| # "expires": "2100-01-01T00:00:00.000Z", | |
| # "key": "abcdefghijklmnopqrstubwxyzabcdefghijklmnopqrst", | |
| # "write_enabled": "true", | |
| # "description": "string" | |
| # } | |
| # print(type(user2.id)) | |
| # print(data) | |
| # url = reverse("users-api:tokens") | |
| # response = self.client.post(url, header=header, data=data) | |
| # self.assertEqual(response.status_code, 403) | |
| def test_tokens_are_restricted_by_user(self): | |
| """ | |
| Test that the tokens API can only access tokens belonging to the authenticated user. | |
| """ | |
| # Create a user and token | |
| data = { | |
| "username": "user1", | |
| "password": "abc123", | |
| } | |
| self.user = User.objects.create_user(**data) | |
| Token(user=self.user).save() | |
| token = Token.objects.get(user=self.user) | |
| self.header = { | |
| "HTTP_AUTHORIZATION:": "Token " + token.key, | |
| } | |
| # List all tokens available to user1 | |
| self.add_permissions("users.view_token") | |
| response = self.client.get(self._get_list_url(), **self.header) | |
| # Assert that only the user1_token appears in the results | |
| self.assertEqual(len(response.data["results"]), 1) | |
| self.assertEqual(response.data["results"][0]["id"], token.id) | |
| # Try to retrieve a token belonging to another user by ID | |
| response = self.client.get(self._get_detail_url(Token.objects.filter(user__n=self.user).first()), **self.header) | |
| self.assertHttpStatus(response, status.HTTP_404_NOT_FOUND) |
There was a problem hiding this comment.
@glennmatthews Might be missing something in your suggestion, but where do you create a token by another user? As I read the suggestion you might be getting the 404 because no other tokens exist.
There was a problem hiding this comment.
In the base APITestCase.setUp(), one token is created for the default testuser, and in this test-case's setUp() two more tokens are created for the default test user. In my suggested test case here we then switch to a different user account (overwriting self.user because self.add_permissions() is much easier to use that way).
There was a problem hiding this comment.
Added this, couldn't get the user__n thing working (django.core.exceptions.FieldError: Related Field got invalid lookup: n) so just created a 2nd user.
| { | ||
| "user": self.user.pk, | ||
| }, | ||
| { | ||
| "user": self.user.pk, | ||
| }, | ||
| { | ||
| "user": self.user.pk, | ||
| }, |
There was a problem hiding this comment.
Since user is no longer a serializer field, these inputs will IIRC just be discarded. Perhaps:
| { | |
| "user": self.user.pk, | |
| }, | |
| { | |
| "user": self.user.pk, | |
| }, | |
| { | |
| "user": self.user.pk, | |
| }, | |
| { | |
| "key": "a" * 40, | |
| }, | |
| { | |
| "key": "b" * 40, | |
| }, | |
| { | |
| "key": "c" * 40, | |
| }, |
Patch sets: * netbox-community/netbox#6592 * netbox-community/netbox@34aa231 author: jeremystretch <jstretch@ns1.com>
|
Added @jathanism suggested changes, thanks again for the good spot! Still trying (and failing) to remove |
Taken from: netbox-community/netbox#6592
author: jeremystretch jstretch@ns1.com
Fixes: #1374
Port over patches to allow API token creation.