[api] Rest API for PKI app #455

ManishShah120 · 2021-05-12T13:24:26Z

Closes #462

coveralls · 2021-05-12T13:32:52Z

Coverage increased (+0.03%) to 98.673% when pulling a020222 on rest-api-for-pki-app into 41badf5 on master.

atb00ker

Going in the correct direction, one question:

openwisp_controller/pki/api/views.py

openwisp_controller/pki/api/serializers.py

openwisp_controller/pki/tests/test_api.py

ManishShah120 · 2021-07-16T18:10:47Z

@ManishShah120 why in the cert detail endpoint the name of the CA is shown while in the list endpoint it's the ID?

I would always show the ID because it's the only way to be super sure someone is dealing with a specific object.

Let's be consistent.

@nemesisdesign, I tried to keep it similar to the admin panel, But, Fixed it now. 😊

nemesifier

@ManishShah120 I merged the latest master in and some query checks failed can you please double check?

ManishShah120 · 2021-07-17T06:17:59Z

@ManishShah120 I merged the latest master in and some query checks failed can you please double check?

Fixed it 👍 @nemesisdesign

atb00ker

LGTM! 😄

nemesifier

So here's some important actions that can be performed from the admin but seem missing from the API.

renew cert
renew CA
revoke certificate

To keep it simple, we can introduce these actions for singular objects (leaving the implementation for performing renew/revoke on multiple objects for a future phase if needed at all).

The models have method for each of these actions so the implementation should be pretty easy.

The API urls can be like:

POST /cert/{id}/renew/
POST /ca/{id}/renew/
POST /cert/{id}/revoke/

nemesifier · 2021-07-18T19:42:54Z

openwisp_controller/pki/api/serializers.py

+        if data.get('certificate') and data.get('private_key'):
+            data = get_import_data(instance)
+            data.update({'ca': instance.ca})
+        return data


this looks mostly the same as the Ca serializer

nemesifier · 2021-07-18T19:43:11Z

openwisp_controller/pki/api/serializers.py

+    def validate_validity_end(self, value):
+        if value is None:
+            value = default_cert_validity_end()
+        return value


these two method also are almost identical

can you please move these 3 methods above to a BaseListSerializer which inherits from BaseSerializer and then use it as a base for CertListSerializer and CatListSerializer?

The line which adds data.update({'ca': instance.ca}) can be added with super()

nemesifier

almost there

nemesifier · 2021-07-20T00:05:47Z

openwisp_controller/pki/api/views.py

+        instance.renew()
+        return Response(
+            {_("CA '{}' renewed successfully".format(instance.name))}, status=302,
+        )


return the serialized data with the updated data and 200 as status code, applies to the other endpoints as well

This is done. 👍

ManishShah120 · 2021-07-20T08:49:46Z

@nemesisdesign I have made the requested changes for the revoke and renew endpoints, but there's one minor issue, i.e., I have used the CaDetailSerializer & CertDetailSerializer for serializing the instance data for returning it, but since in this serializers some of the fields are set to write so these fields are also displayed in the response page.

However, this will not be a an issue, in production. Still I will see if I can fix it.

nemesifier · 2021-07-20T15:36:51Z

@nemesisdesign I have made the requested changes for the revoke and renew endpoints, but there's one minor issue, i.e., I have used the CaDetailSerializer & CertDetailSerializer for serializing the instance data for returning it, but since in this serializers some of the fields are set to write so these fields are also displayed in the response page.

However, this will not be a an issue, in production. Still I will see if I can fix it.

Sure, better fix this and ensure the serializers are correct, you simply need to create a new serializers inheriting the serializer you're using now and removing the write_only fields.

ManishShah120 · 2021-07-21T07:11:46Z

Sure, better fix this and ensure the serializers are correct, you simply need to create a new serializers inheriting the serializer you're using now and removing the write_only fields.

Done, Fixed it. 👍

nemesifier

Almost there, let's do some more code improvement and then we're done.

nemesifier · 2021-07-22T00:50:19Z

openwisp_controller/pki/api/serializers.py

+    return data
+
+
+def CertList_fields(fields=None):


two doubts here:

why the default argument is None? it looks like there should be no default argument

why this naming? Capital letter is used for classes, I think this should be named get_cert_list_fields

Eg:

def get_cert_list_fields(fields): pass

nemesifier · 2021-07-22T00:50:49Z

openwisp_controller/pki/api/serializers.py

+        return value
+
+
+def CaDetail_fields(fields=None):


same here as for get_cert_list_fields

nemesifier · 2021-07-22T00:52:33Z

openwisp_controller/pki/api/serializers.py

+        return value
+
+
+def CertDetail_fields(fields=None):


nemesifier · 2021-07-22T00:55:48Z

openwisp_controller/pki/api/serializers.py

+    def validate_validity_end(self, value):
+        if value is None:
+            value = default_cert_validity_end()
+        return value


can you please move these 3 methods above to a BaseListSerializer which inherits from BaseSerializer and then use it as a base for CertListSerializer and CatListSerializer?

The line which adds data.update({'ca': instance.ca}) can be added with super()

ManishShah120 · 2021-07-23T15:25:40Z

Hi @nemesisdesign have implemented the BaseListSerializer, Is the implementation correct.

nemesifier

I found another weird thing: why does the list endpoint provide the info about extensions but the details endpoint does not?

ManishShah120 · 2021-07-25T06:17:27Z

I found another weird thing: why does the list endpoint provide the info about extensions but the details endpoint does not?

@nemesisdesign currently I have also removed the passphrase field from the detail endpoint, Should I leave it as as it is or should I make it visible too in the detail endpoint.

nemesifier

I found another weird thing: why does the list endpoint provide the info about extensions but the details endpoint does not?

@nemesisdesign currently I have also removed the passphrase field from the detail endpoint, Should I leave it as as it is or should I make it visible too in the detail endpoint.

Passphrase shall be write_only: a020222.

With this change I think we're done here.

ManishShah120 requested review from nemesifier and atb00ker May 12, 2021 13:24

ManishShah120 self-assigned this May 12, 2021

atb00ker reviewed May 13, 2021

View reviewed changes

openwisp_controller/pki/api/views.py Outdated Show resolved Hide resolved

ManishShah120 force-pushed the rest-api-for-pki-app branch 2 times, most recently from ea2152d to 61de479 Compare May 20, 2021 12:51

ManishShah120 requested a review from okraits May 25, 2021 11:33

ManishShah120 force-pushed the rest-api-for-pki-app branch from 27348a4 to 7de743b Compare May 27, 2021 08:12

ManishShah120 marked this pull request as ready for review May 27, 2021 08:29

ManishShah120 changed the title ~~[WIP] [api] Rest API for PKI app~~ [api] Rest API for PKI app May 27, 2021

ManishShah120 added 9 commits June 3, 2021 20:21

[api] Rest API for PKI app

ec92a99

[tests] Added more test for Ca model endpoints

9f15ab7

[tests] Added tests for Cert model endpoints

443402e

[docs] Updated docs for the API endpoints of the PKI app

0f40be5

[change] Fixed endpoint in the docs and added slash to the url

84d02a5

[change] Removed api_ prefix from pki app endpoints

0bfe2d9

[api] Updated crl_download view with mixin

55e1e35

[test] Fix crl_view failing tests

7d958a1

[api] Added endpoint to download crl

3b7674b

ManishShah120 force-pushed the rest-api-for-pki-app branch from a199fe9 to 3b7674b Compare June 3, 2021 16:47

nemesifier reviewed Jun 3, 2021

View reviewed changes

openwisp_controller/pki/api/views.py Show resolved Hide resolved

[change] Undo changes in requirements

b8661e0

nemesifier reviewed Jun 6, 2021

View reviewed changes

openwisp_controller/pki/api/serializers.py Outdated Show resolved Hide resolved

ManishShah120 added 2 commits June 7, 2021 22:59

[api] Improved code quality

466da67

[change] Minor code improvements

a93805c

nemesifier requested changes Jun 8, 2021

View reviewed changes

openwisp_controller/pki/tests/test_api.py Show resolved Hide resolved

Merge branch 'master' into rest-api-for-pki-app

d67ae09

nemesifier requested changes Jul 17, 2021

View reviewed changes

[tests] Fixed failing query checks

ae42efc

atb00ker previously approved these changes Jul 17, 2021

View reviewed changes

nemesifier mentioned this pull request Jul 18, 2021

[bug] CA/Cert creation generates 2 SQL write queries instead of 1 openwisp/django-x509#120

Open

nemesifier requested changes Jul 18, 2021

View reviewed changes

[api] Added endpoints for revoke and renew for CA and Cert

44fed3c

ManishShah120 dismissed atb00ker’s stale review via 44fed3c July 19, 2021 21:05

nemesifier requested changes Jul 20, 2021

View reviewed changes

[change] Returned the updated insance data after renew & revoke

738aa63

ManishShah120 added 2 commits July 20, 2021 23:27

[api] Fixed response for revoke and renew endpoints

6130f5a

[change] Inherited respective base classes for renew & revoke

91cd457

nemesifier requested changes Jul 22, 2021

View reviewed changes

ManishShah120 added 2 commits July 23, 2021 12:33

[change] Minor code improvement

4bf3fa4

[change] Inroduced BaseListSerializer containing common methods

d386202

nemesifier requested changes Jul 24, 2021

View reviewed changes

ManishShah120 and others added 2 commits July 25, 2021 13:06

[change] Included extensions field in the detail endpoint

9abbb5c

[change] Passphrase shall be only write only

a020222

nemesifier approved these changes Jul 25, 2021

View reviewed changes

nemesifier merged commit 3f8e4c6 into master Jul 25, 2021

nemesifier deleted the rest-api-for-pki-app branch July 25, 2021 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api] Rest API for PKI app #455

[api] Rest API for PKI app #455

ManishShah120 commented May 12, 2021 •

edited

Loading

coveralls commented May 12, 2021 •

edited

Loading

atb00ker left a comment

ManishShah120 commented Jul 16, 2021

nemesifier left a comment

ManishShah120 commented Jul 17, 2021

atb00ker left a comment

nemesifier left a comment •

edited

Loading

nemesifier Jul 18, 2021

nemesifier Jul 18, 2021

nemesifier Jul 22, 2021

nemesifier left a comment

nemesifier Jul 20, 2021

ManishShah120 Jul 20, 2021

ManishShah120 commented Jul 20, 2021

nemesifier commented Jul 20, 2021

ManishShah120 commented Jul 21, 2021

nemesifier left a comment

nemesifier Jul 22, 2021

nemesifier Jul 22, 2021

nemesifier Jul 22, 2021

nemesifier Jul 22, 2021

ManishShah120 commented Jul 23, 2021

nemesifier left a comment

ManishShah120 commented Jul 25, 2021

nemesifier left a comment

[api] Rest API for PKI app #455

[api] Rest API for PKI app #455

Conversation

ManishShah120 commented May 12, 2021 • edited Loading

coveralls commented May 12, 2021 • edited Loading

atb00ker left a comment

Choose a reason for hiding this comment

ManishShah120 commented Jul 16, 2021

nemesifier left a comment

Choose a reason for hiding this comment

ManishShah120 commented Jul 17, 2021

atb00ker left a comment

Choose a reason for hiding this comment

nemesifier left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManishShah120 commented Jul 20, 2021

nemesifier commented Jul 20, 2021

ManishShah120 commented Jul 21, 2021

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManishShah120 commented Jul 23, 2021

nemesifier left a comment

Choose a reason for hiding this comment

ManishShah120 commented Jul 25, 2021

nemesifier left a comment

Choose a reason for hiding this comment

ManishShah120 commented May 12, 2021 •

edited

Loading

coveralls commented May 12, 2021 •

edited

Loading

nemesifier left a comment •

edited

Loading