fix: Validate Azure JWTs using `authlib` #2112

wolfdn · 2023-09-01T06:48:38Z

Description

This PR introduces authlib to decode and validate JWTs from Azure.
Note: I tried to use joserfc instead of authlib - as recommended in the authlib docs - but then realized that joserfc is only compatible with Python >= 3.8. To keep compatibility with Python 3.7 we should use authlib.

Until now we were using a custom implementation to parse Azure JWTs. Only the content of JWTs was evaluated, but it was not checked if the signature of the JWT is even valid.

By using authlib we can get rid of the custom parsing implementation for Azure JWTs in manager.py and it also allows us to validate the claims of the JWT.
The public keys from Microsoft are automatically fetched and used to decode the JWT.
Some basic claims of the JWT are also validated (issued at, not before and expiry).

ADDITIONAL INFORMATION

dpgaspar

Looks good and simple, can you add tests please?

flask_appbuilder/security/manager.py

setup.py

codecov · 2023-10-04T11:35:23Z

Codecov Report

Merging #2112 (ec14163) into master (e4d613a) will increase coverage by 0.19%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #2112      +/-   ##
==========================================
+ Coverage   78.29%   78.49%   +0.19%     
==========================================
  Files          72       72              
  Lines        8699     8685      -14     
==========================================
+ Hits         6811     6817       +6     
+ Misses       1888     1868      -20

Flag	Coverage Δ
python	`78.49% <75.00%> (+0.19%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
flask_appbuilder/const.py	`100.00% <100.00%> (ø)`
flask_appbuilder/security/manager.py	`77.89% <72.72%> (+1.72%)`	⬆️

wolfdn · 2023-10-04T14:00:18Z

Looks good and simple, can you add tests please?

I just added a unittest for the new _decode_and_validate_azure_jwt method.
Since this method is relying on authlib for JWT decoding and validation, this test is just making sure that the validate function of the JWT claims is actually called.

Is this sufficient as a unit test?

dpgaspar

Looking good one last comment

dpgaspar · 2023-10-04T16:05:43Z

requirements.txt

@@ -6,6 +6,8 @@
 #
 apispec[yaml]==6.3.0
 # via Flask-AppBuilder (setup.py)
+authlib==1.2.1


can you move this to requirements extra?

I noticed that authlib is already in requirements-extra.txt - now I just upgraded the version there from 0.15.4 to the latest version 1.2.1

dpgaspar · 2023-10-04T16:08:50Z

Looks good and simple, can you add tests please?

I just added a unittest for the new _decode_and_validate_azure_jwt method. Since this method is relying on authlib for JWT decoding and validation, this test is just making sure that the validate function of the JWT claims is actually called.

Is this sufficient as a unit test?

It's ok, I'm going to revisit the Azure and claims mapping also, I'll add more tests

…zure-jwt

dpgaspar · 2023-10-06T08:30:36Z

flask_appbuilder/security/manager.py

-
- jwt_decoded_payload = json.loads(decoded_payload.decode("utf-8"))
+ def _decode_and_validate_azure_jwt(self, id_token):
+ keyset = JsonWebKey.import_key_set(requests.get(MICROSOFT_KEY_SET_URL).json())


this should be cached or set at boot time

dpgaspar · 2023-10-06T08:31:48Z

Thank you @wolfdn !

potiuk · 2023-10-06T22:44:24Z

Cool!

This PR brings all the necessary changes to upgrade to FAB 4.3.9 from 4.3.6. It incorporates those changes: * dpgaspar/Flask-AppBuilder#2112 * dpgaspar/Flask-AppBuilder#2121 It also removes the limitation of the WTForms after compatibility has been implemented: * dpgaspar/Flask-AppBuilder#2138

This PR brings all the necessary changes to upgrade to FAB 4.3.9 from 4.3.6. It incorporates those changes: * dpgaspar/Flask-AppBuilder#2112 * dpgaspar/Flask-AppBuilder#2121 It also removes the limitation of the WTForms after compatibility has been implemented: * dpgaspar/Flask-AppBuilder#2138 GitOrigin-RevId: 4198146f49b72d051d82fbd821c7105cf2f4a8bd

wolfdn added 6 commits September 1, 2023 08:36

Use joserfc to decode Azure jwt

5aa38b7

Apply black formatting

aa9ace7

Add joserfc to requirements.txt

8124fbf

Remove unnecessary import statement

e09b96b

Fix import order

243ff93

Switch to authlib to stay compatible with Python 3.7

88dd310

wolfdn changed the title ~~feat: Use joserfc to decode Azure jwt~~ feat: Use authlib to decode Azure jwt Sep 1, 2023

Fix import order

7afa57c

wolfdn changed the title ~~feat: Use authlib to decode Azure jwt~~ fix: Validate Azure JWTs using authlib Sep 1, 2023

potiuk approved these changes Sep 7, 2023

View reviewed changes

dpgaspar reviewed Sep 7, 2023

View reviewed changes

flask_appbuilder/security/manager.py Outdated Show resolved Hide resolved

flask_appbuilder/security/manager.py Outdated Show resolved Hide resolved

dpgaspar reviewed Sep 22, 2023

View reviewed changes

setup.py Outdated Show resolved Hide resolved

wolfdn added 4 commits October 4, 2023 13:23

Move Microsoft key set URL to constants

86fd8b7

Make logging less verbose

0f8af76

Add unittest

e57cab4

Remove authlib from install_requires

55a5ebe

wolfdn added 3 commits October 4, 2023 13:37

Fix formatting

63d1e4e

Update unittest

879edd0

Fix import order

a4e4fbb

dpgaspar approved these changes Oct 4, 2023

View reviewed changes

wolfdn added 2 commits October 5, 2023 07:27

Move authlib to requirements-extra.txt

3c17998

Merge branch 'dpgaspar:master' into feature/use-joserfc-to-evaluate-a…

ec14163

…zure-jwt

dpgaspar reviewed Oct 6, 2023

View reviewed changes

dpgaspar merged commit 57f4400 into dpgaspar:master Oct 6, 2023
11 of 12 checks passed

dpgaspar mentioned this pull request Oct 6, 2023

fix: azure user info claims and JWT decode #2121

Merged

6 tasks

dpgaspar mentioned this pull request Oct 19, 2023

fix: bump to FAB 4.3.9 remove CSP exception apache/superset#25712

Merged

9 tasks

potiuk mentioned this pull request Oct 20, 2023

Upgrade to Flask Application Builder 4.3.9 apache/airflow#35085

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Validate Azure JWTs using `authlib` #2112

fix: Validate Azure JWTs using `authlib` #2112

wolfdn commented Sep 1, 2023 •

edited

Loading

dpgaspar left a comment

codecov bot commented Oct 4, 2023 •

edited

Loading

wolfdn commented Oct 4, 2023

dpgaspar left a comment

dpgaspar Oct 4, 2023

wolfdn Oct 5, 2023

dpgaspar commented Oct 4, 2023

dpgaspar Oct 6, 2023

dpgaspar commented Oct 6, 2023

potiuk commented Oct 6, 2023

fix: Validate Azure JWTs using authlib #2112

fix: Validate Azure JWTs using authlib #2112

Conversation

wolfdn commented Sep 1, 2023 • edited Loading

Description

ADDITIONAL INFORMATION

dpgaspar left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

wolfdn commented Oct 4, 2023

dpgaspar left a comment

Choose a reason for hiding this comment

dpgaspar Oct 4, 2023

Choose a reason for hiding this comment

wolfdn Oct 5, 2023

Choose a reason for hiding this comment

dpgaspar commented Oct 4, 2023

dpgaspar Oct 6, 2023

Choose a reason for hiding this comment

dpgaspar commented Oct 6, 2023

potiuk commented Oct 6, 2023

fix: Validate Azure JWTs using `authlib` #2112

fix: Validate Azure JWTs using `authlib` #2112

wolfdn commented Sep 1, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading