feat: implemented native authentication with pwa using auth0 API #343

vickywane · 2021-03-30T19:15:09Z

Purpose

This PR implements a native authentication with a user's PWA, as a requirement for the premium user feature

Approach

Work done here fetches the user's access_token from auth0 after the user's consent from the PWA.

This design diagram shows how the notification feature is being implemented in this Pull Request and also how it integrates with the connected PWA.

Merge Checklist

The code change is tested and works locally.
The user and dev documentation is up to date.
There is no commented-out code in this PR.
No lint errors (use flake8)
100% test coverage

commit-lint · 2021-03-30T19:15:13Z

Features

added new auth endpoint (2199ca3)

Bug Fixes

undefined build errors (f1d0aa2)
fixed auth0 client (bccf6d8)

Contributors

vickywane

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

@Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
@Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
@Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
@Commit-Lint merge disable will desactivate merge dependabot PR
@Commit-Lint review will approve dependabot PR
@Commit-Lint stop review will stop approve dependabot PR

pep8speaks · 2021-03-30T19:15:19Z

Hello @vickywane! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file src/ambianic/notification.py:

Line 35:80: E501 line too long (93 > 79 characters)

In the file src/ambianic/pipeline/store.py:

Line 11:80: E501 line too long (90 > 79 characters)
Line 74:80: E501 line too long (81 > 79 characters)

In the file tests/test_flaskr.py:

Line 77:80: E501 line too long (103 > 79 characters)

Comment last updated at 2021-05-20 11:00:22 UTC

lgtm-com · 2021-03-30T19:22:43Z

This pull request introduces 8 alerts when merging 911e583 into 440cebc - view on LGTM.com

new alerts:

4 for Unused local variable
4 for Unused import

ivelin · 2021-03-30T20:50:43Z

@vickywane the CI checks are failing. Let me know when all checks pass.

I would also like to review simultaneously the UI PR that handles user authorization approval for this edge device.

lgtm-com · 2021-03-31T01:29:43Z

This pull request introduces 3 alerts when merging 18c263b into 440cebc - view on LGTM.com

new alerts:

2 for Unused import
1 for Unused local variable

lgtm-com · 2021-03-31T02:22:15Z

This pull request introduces 3 alerts when merging e5e770d into 440cebc - view on LGTM.com

new alerts:

2 for Unused import
1 for Unused local variable

codecov · 2021-03-31T02:39:03Z

Codecov Report

Merging #343 (baed1d8) into master (440cebc) will increase coverage by 0.04%.
The diff coverage is 96.36%.

❗ Current head baed1d8 differs from pull request most recent head bccf6d8. Consider uploading reports for the commit bccf6d8 to get more accurate results

@@            Coverage Diff             @@
##           master     #343      +/-   ##
==========================================
+ Coverage   94.56%   94.61%   +0.04%     
==========================================
  Files          23       23              
  Lines        2228     2264      +36     
==========================================
+ Hits         2107     2142      +35     
- Misses        121      122       +1

Impacted Files	Coverage Δ
src/ambianic/notification.py	`88.60% <93.10%> (+5.27%)`	⬆️
src/ambianic/__init__.py	`100.00% <100.00%> (ø)`
src/ambianic/pipeline/store.py	`97.00% <100.00%> (-1.98%)`	⬇️
src/ambianic/webapp/flaskr.py	`88.46% <100.00%> (+0.74%)`	⬆️

lgtm-com · 2021-03-31T02:40:46Z

This pull request introduces 4 alerts when merging 44b96e6 into 440cebc - view on LGTM.com

new alerts:

2 for Unused import
1 for Unused local variable
1 for Except block handles 'BaseException'

ivelin

@vickywane the codecov check does not pass. This PR doesn't have a single test to cover the new code.

See other comments inline.

src/ambianic/pipeline/store.py

src/ambianic/webapp/flaskr.py

lgtm-com · 2021-04-02T19:54:05Z

This pull request introduces 5 alerts when merging 6c4595d into 440cebc - view on LGTM.com

new alerts:

2 for Except block handles 'BaseException'
2 for Unused import
1 for Unused local variable

lgtm-com · 2021-04-02T20:28:05Z

This pull request introduces 5 alerts when merging 5163e0c into 440cebc - view on LGTM.com

new alerts:

2 for Except block handles 'BaseException'
2 for Unused import
1 for Unused local variable

lgtm-com · 2021-04-02T21:03:52Z

This pull request introduces 4 alerts when merging c1fd0cf into 440cebc - view on LGTM.com

new alerts:

2 for Unused import
1 for Unused local variable
1 for Except block handles 'BaseException'

lgtm-com · 2021-04-02T22:14:45Z

This pull request introduces 5 alerts when merging 091b2cb into 440cebc - view on LGTM.com

new alerts:

2 for Except block handles 'BaseException'
2 for Unused import
1 for Unused local variable

ivelin · 2021-04-02T22:57:53Z

This design diagram shows how the notification feature is being implemented in this Pull Request and also how it integrates with the connected PWA.

@vickywane
I am not able to open this diagram. The URL points to an error.

vickywane · 2021-04-02T23:07:39Z

This design diagram shows how the notification feature is being implemented in this Pull Request and also how it integrates with the connected PWA.

@vickywane
I am not able to open this diagram. The URL points to an error.

Seems to be an issue with Draw. I just duplicated the diagram in a new document here

ivelin · 2021-04-02T23:21:55Z

This design diagram shows how the notification feature is being implemented in this Pull Request and also how it integrates with the connected PWA.

@vickywane
I am not able to open this diagram. The URL points to an error.

Seems to be an issue with Draw. I just duplicated the diagram in a new document here

OK. Added google doc comments.

vickywane · 2021-04-03T16:47:20Z

I am currently writing tests for the new API endpoints introduced as part of the edge-PWA auth flow.

The first endpoint (/user-code)has been covered with tests.

However, there's seems to be a limit to what can be tested. The /verify-token route returns a URL to a page containing a button that has to be manually clicked by a user before the authentication is complete. This cannot be done within the test suite except using simulation tests.

This also affects the third endpoint (/save-token). The authentication has be complete before a valid token is returned by auth0.

lgtm-com · 2021-04-03T16:59:49Z

This pull request introduces 5 alerts when merging 4196bf8 into 440cebc - view on LGTM.com

new alerts:

2 for Except block handles 'BaseException'
2 for Unused import
1 for Unused local variable

lgtm-com · 2021-04-05T11:37:23Z

This pull request introduces 3 alerts when merging 33b5231 into 440cebc - view on LGTM.com

new alerts:

2 for Unused import
1 for Except block handles 'BaseException'

ivelin

I am currently writing tests for the new API endpoints introduced as part of the edge-PWA auth flow.

The first endpoint (/user-code)has been covered with tests.

However, there's seems to be a limit to what can be tested. The /verify-token route returns a URL to a page containing a button that has to be manually clicked by a user before the authentication is complete. This cannot be done within the test suite except using simulation tests.

This also affects the third endpoint (/save-token). The authentication has be complete before a valid token is returned by auth0.

Unit tests should not depend on third party services. Isolate and test your own code.
Worry about integration tests after you have full unit test coverage.

src/ambianic/webapp/flaskr.py

ivelin · 2021-04-05T13:47:23Z

src/ambianic/webapp/flaskr.py

+
+            return response
+
+    @app.route('/api/auth/save-token', methods=['POST'])


All this new code needs to be covered with unit tests.

ivelin · 2021-04-05T17:25:32Z

@vickywane read a bit more how to create proper activity design diagrams. You have options, but whichever one you pick must communicate clearly who and when does what exactly:
https://drawio-app.com/create-uml-sequence-diagrams-in-draw-io/
https://www.diagrams.net/blog/swimlane-diagrams
https://nathanhumphrey.ca/posts/2021-01-11-bpmn-with-diagrams-net/

vickywane · 2021-04-05T21:07:49Z

@vickywane read a bit more how to create proper activity design diagrams. You have options, but whichever one you pick must communicate clearly who and when does what exactly:
https://drawio-app.com/create-uml-sequence-diagrams-in-draw-io/
https://www.diagrams.net/blog/swimlane-diagrams
https://nathanhumphrey.ca/posts/2021-01-11-bpmn-with-diagrams-net/

Thank you.

I just expanded the document following guides from the articles above. The updated image is in the PR description above.

ivelin · 2021-04-05T21:36:59Z

@vickywane read a bit more how to create proper activity design diagrams. You have options, but whichever one you pick must communicate clearly who and when does what exactly:
https://drawio-app.com/create-uml-sequence-diagrams-in-draw-io/
https://www.diagrams.net/blog/swimlane-diagrams
https://nathanhumphrey.ca/posts/2021-01-11-bpmn-with-diagrams-net/

Thank you.

I just expanded the document following guides from the articles above. The updated image is in the PR description above.

The link in the PR description text points to an outdated document. Make sure the view and the link are in sync. Even better, make the PNG clickable and linking directly to the latest diagram source so it's easy to collaborate on it.

vickywane · 2021-04-05T21:54:14Z

The link in the PR description text points to an outdated document. Make sure the view and the link are in sync. Even better, make the PNG clickable and linking directly to the latest diagram source so it's easy to collaborate on it.

The link has been fixed. It was becoming large so I had to use another means to export.

lgtm-com · 2021-04-06T03:38:40Z

This pull request introduces 5 alerts when merging 2ff1b12 into 440cebc - view on LGTM.com

new alerts:

3 for Except block handles 'BaseException'
2 for Unused import

lgtm-com · 2021-04-06T11:19:21Z

This pull request introduces 4 alerts when merging fa2ad11 into 440cebc - view on LGTM.com

new alerts:

3 for Except block handles 'BaseException'
1 for Unused import

ivelin · 2021-04-06T15:20:20Z

The link in the PR description text points to an outdated document. Make sure the view and the link are in sync. Even better, make the PNG clickable and linking directly to the latest diagram source so it's easy to collaborate on it.

The link has been fixed. It was becoming large so I had to use another means to export.

The link now points to a google doc, that does not allow comments....

vickywane · 2021-04-06T15:29:32Z

The link now points to a google doc, that does not allow comments....

I have updated the file permission here.

ivelin · 2021-04-06T15:33:51Z

The link now points to a google doc, that does not allow comments....

I have updated the file permission here.

OK, I can comment now.

lgtm-com · 2021-05-10T15:34:58Z

This pull request introduces 1 alert when merging ec392a3 into 440cebc - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

ivelin

still a few open comment that are awaiting response

src/ambianic/pipeline/store.py

lgtm-com · 2021-05-12T08:40:11Z

This pull request introduces 4 alerts when merging 7c4ddf1 into 440cebc - view on LGTM.com

new alerts:

3 for Unused import
1 for Except block handles 'BaseException'

lgtm-com · 2021-05-12T09:41:39Z

This pull request introduces 4 alerts when merging aecd2f4 into 440cebc - view on LGTM.com

new alerts:

3 for Unused import
1 for Except block handles 'BaseException'

lgtm-com · 2021-05-12T14:42:06Z

This pull request introduces 4 alerts when merging 054679e into 440cebc - view on LGTM.com

new alerts:

3 for Unused import
1 for Except block handles 'BaseException'

lgtm-com · 2021-05-12T18:32:40Z

This pull request introduces 4 alerts when merging e344c43 into 440cebc - view on LGTM.com

new alerts:

3 for Unused import
1 for Except block handles 'BaseException'

lgtm-com · 2021-05-12T18:52:51Z

This pull request introduces 1 alert when merging 67174b3 into 440cebc - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

lgtm-com · 2021-05-12T19:30:52Z

This pull request introduces 1 alert when merging f6a0d0c into 440cebc - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

vickywane · 2021-05-13T07:19:10Z

still a few open comments that are awaiting response

I think I have responded and made adjustments to all raised comments.
Please mention me on any comment that has been skipped or not answered fully.

ivelin · 2021-05-16T17:17:29Z

Hello @vickywane! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file src/ambianic/notification.py:

Line 35:80: E501 line too long (93 > 79 characters)

In the file src/ambianic/pipeline/store.py:

Line 11:80: E501 line too long (90 > 79 characters)
Line 72:80: E501 line too long (81 > 79 characters)

In the file tests/test_flaskr.py:

Line 77:80: E501 line too long (103 > 79 characters)
Line 81:80: E501 line too long (82 > 79 characters)

Comment last updated at 2021-05-12 19:16:16 UTC

@vickywane please address these linting issues.

ivelin · 2021-05-16T17:26:32Z

I am currently writing tests for the new API endpoints introduced as part of the edge-PWA auth flow.

The first endpoint (/user-code)has been covered with tests.

However, there's seems to be a limit to what can be tested. The /verify-token route returns a URL to a page containing a button that has to be manually clicked by a user before the authentication is complete. This cannot be done within the test suite except using simulation tests.

This also affects the third endpoint (/save-token). The authentication has be complete before a valid token is returned by auth0.

@vickywane use mockups to emulate server side behavior.

https://changhsinlee.com/pytest-mock/
https://www.toptal.com/python/an-introduction-to-mocking-in-python

vickywane · 2021-05-16T17:34:52Z

This also affects the third endpoint (/save-token). The authentication has be complete before a valid token is returned by auth0.

@vickywane use mockups to emulate server side behavior.

https://changhsinlee.com/pytest-mock/

https://www.toptal.com/python/an-introduction-to-mocking-in-python

This is an old and outdated comment. It was part of the previous implementation which has been discarded.

We no longer have those routes / endpoints in the codebase.

lgtm-com · 2021-05-17T10:39:18Z

This pull request introduces 1 alert when merging 1e4b19f into e7e963d - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

lgtm-com · 2021-05-17T14:18:43Z

This pull request introduces 1 alert when merging 0425a66 into e7e963d - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

lgtm-com · 2021-05-17T14:47:39Z

This pull request introduces 1 alert when merging 9704d71 into e7e963d - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

lgtm-com · 2021-05-18T09:53:26Z

This pull request introduces 1 alert when merging a3c6b97 into 0291b2a - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

lgtm-com · 2021-05-18T10:38:40Z

This pull request introduces 1 alert when merging 704b1b1 into 0291b2a - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

lgtm-com · 2021-05-18T19:34:49Z

This pull request introduces 1 alert when merging 2fb55f8 into 0291b2a - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

lgtm-com · 2021-05-18T19:56:48Z

This pull request introduces 1 alert when merging baed1d8 into 0291b2a - view on LGTM.com

new alerts:

1 for Unused import

ivelin

Cleanup redundant notification code and address unresolved comments.

src/ambianic/notification.py

vickywane · 2021-05-20T10:19:55Z

As requested I have created the following issues to be immediately worked upon after this pull request is merged;

Splitting auth management code into a separate module Split auth management code into a smaller module #352
Rewrite NotificationHandler to accommodate sending premium notification [FEATURE] Rewrite NotificationHandlers to accommodate sending premium notifications #353

The following were created in the Cloud API repository as they would be implemented there;

ivelin

@vickywane looks good. merging.

# [1.15.0](v1.14.7...v1.15.0) (2021-06-08) ### Bug Fixes * fixed auth0 client ([bccf6d8](bccf6d8)) * undefined build errors ([f1d0aa2](f1d0aa2)) ### Features * added new auth endpoint ([2199ca3](2199ca3)) * implemented native authentication with pwa using auth0 API ([d82d998](d82d998)), closes [#343](#343)

github-actions · 2021-06-08T15:03:08Z

🎉 This PR is included in version 1.15.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

# [1.12.0](v1.11.1...v1.12.0) (2021-06-24) ### Bug Fixes * fixed auth0 client ([bccf6d8](bccf6d8)) * undefined build errors ([f1d0aa2](f1d0aa2)) ### Features * added new auth endpoint ([2199ca3](2199ca3)) * implemented native authentication with pwa using auth0 API ([d82d998](d82d998)), closes [ambianic#343](https://github.com/ivelin/ambianic-edge/issues/343)

pull-request-size bot added the size/L label Mar 30, 2021

ivelin requested changes Mar 31, 2021

View reviewed changes

ivelin requested changes Apr 5, 2021

View reviewed changes

ivelin requested changes May 10, 2021

View reviewed changes

src/ambianic/pipeline/store.py Outdated Show resolved Hide resolved

ivelin requested changes May 18, 2021

View reviewed changes

src/ambianic/notification.py Show resolved Hide resolved

This was referenced May 19, 2021

Implement retries of undelivered notification at set interval ambianic/ambianic-subscriptions#12

Open

Split auth management code into a smaller module #352

Closed

fix: fixed auth0 client

bccf6d8

ivelin approved these changes Jun 8, 2021

View reviewed changes

ivelin merged commit d82d998 into ambianic:master Jun 8, 2021

github-actions bot added the released label Jun 8, 2021


		return response

		@app.route('/api/auth/save-token', methods=['POST'])

feat: implemented native authentication with pwa using auth0 API #343

feat: implemented native authentication with pwa using auth0 API #343

Conversation

vickywane commented Mar 30, 2021 • edited Loading

Purpose

Approach

Merge Checklist

commit-lint bot commented Mar 30, 2021 • edited Loading

Features

Bug Fixes

Contributors

pep8speaks commented Mar 30, 2021 • edited Loading

Comment last updated at 2021-05-20 11:00:22 UTC

lgtm-com bot commented Mar 30, 2021

ivelin commented Mar 30, 2021

lgtm-com bot commented Mar 31, 2021

lgtm-com bot commented Mar 31, 2021

codecov bot commented Mar 31, 2021 • edited Loading

Codecov Report

lgtm-com bot commented Mar 31, 2021

ivelin left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Apr 2, 2021

lgtm-com bot commented Apr 2, 2021

lgtm-com bot commented Apr 2, 2021

lgtm-com bot commented Apr 2, 2021

ivelin commented Apr 2, 2021 • edited Loading

vickywane commented Apr 2, 2021

ivelin commented Apr 2, 2021

vickywane commented Apr 3, 2021

lgtm-com bot commented Apr 3, 2021

lgtm-com bot commented Apr 5, 2021

ivelin left a comment

Choose a reason for hiding this comment

ivelin Apr 5, 2021

Choose a reason for hiding this comment

ivelin commented Apr 5, 2021

vickywane commented Apr 5, 2021

ivelin commented Apr 5, 2021

vickywane commented Apr 5, 2021 • edited Loading

lgtm-com bot commented Apr 6, 2021

lgtm-com bot commented Apr 6, 2021

ivelin commented Apr 6, 2021

vickywane commented Apr 6, 2021

ivelin commented Apr 6, 2021

lgtm-com bot commented May 10, 2021

ivelin left a comment

Choose a reason for hiding this comment

lgtm-com bot commented May 12, 2021

lgtm-com bot commented May 12, 2021

lgtm-com bot commented May 12, 2021

lgtm-com bot commented May 12, 2021

lgtm-com bot commented May 12, 2021

lgtm-com bot commented May 12, 2021

vickywane commented May 13, 2021

ivelin commented May 16, 2021

Comment last updated at 2021-05-12 19:16:16 UTC

ivelin commented May 16, 2021

vickywane commented May 16, 2021 • edited Loading

lgtm-com bot commented May 17, 2021

lgtm-com bot commented May 17, 2021

lgtm-com bot commented May 17, 2021

lgtm-com bot commented May 18, 2021

lgtm-com bot commented May 18, 2021

lgtm-com bot commented May 18, 2021

lgtm-com bot commented May 18, 2021

ivelin left a comment

Choose a reason for hiding this comment

vickywane commented May 20, 2021

ivelin left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 8, 2021

vickywane commented Mar 30, 2021 •

edited

Loading

commit-lint bot commented Mar 30, 2021 •

edited

Loading

pep8speaks commented Mar 30, 2021 •

edited

Loading

codecov bot commented Mar 31, 2021 •

edited

Loading

ivelin commented Apr 2, 2021 •

edited

Loading

vickywane commented Apr 5, 2021 •

edited

Loading

vickywane commented May 16, 2021 •

edited

Loading