From dcf8684b9626e5eb8edceec688429720385ffdae Mon Sep 17 00:00:00 2001 From: Daniel Vaz Gaspar Date: Mon, 9 Oct 2023 14:30:08 +0100 Subject: [PATCH] fix: azure user info claims and JWT decode (#2121) --- CHANGELOG.rst | 2 +- docs/security.rst | 13 +- examples/oauth/config.py | 6 +- flask_appbuilder/exceptions.py | 8 + flask_appbuilder/security/manager.py | 75 ++++---- tests/security/test_auth_oauth.py | 183 ++++++++++++++++++- tests/security/test_base_security_manager.py | 11 -- 7 files changed, 236 insertions(+), 62 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index db595d49d5..3120157549 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -4,7 +4,7 @@ Flask-AppBuilder ChangeLog Improvements and Bug fixes on 4.3.7 ----------------------------------- -- fix: fix: swagger missing nonce (#2116) [Daniel Vaz Gaspar] +- fix: swagger missing nonce (#2116) [Daniel Vaz Gaspar] Improvements and Bug fixes on 4.3.6 ----------------------------------- diff --git a/docs/security.rst b/docs/security.rst index 03c01175af..b030e30df5 100644 --- a/docs/security.rst +++ b/docs/security.rst @@ -305,6 +305,8 @@ Specify a list of OAUTH_PROVIDERS in **config.py** that you want to allow for yo "client_kwargs": { "scope": "User.read name preferred_username email profile upn", "resource": "AZURE_APPLICATION_ID", + # Optionally enforce signature JWT verification + "verify_signature": False }, "request_token_url": None, "access_token_url": "https://login.microsoftonline.com/AZURE_TENANT_ID/oauth2/token", @@ -347,10 +349,13 @@ You can give FlaskAppBuilder roles based on Oauth groups:: To customize the userinfo retrieval, you can create your own method like this:: @appbuilder.sm.oauth_user_info_getter - def my_user_info_getter(sm, provider, response=None): + def my_user_info_getter( + sm: SecurityManager, + provider: str, + response: Dict[str, Any] + ) -> Dict[str, Any]: if provider == "okta": me = sm.oauth_remotes[provider].get("userinfo") - log.debug("User info from Okta: {0}".format(me.data)) return { "username": "okta_" + me.data.get("sub", ""), "first_name": me.data.get("given_name", ""), @@ -365,11 +370,9 @@ To customize the userinfo retrieval, you can create your own method like this:: "email": me.json().get("email"), "first_name": me.json().get("given_name", ""), "last_name": me.json().get("family_name", ""), - "id": me.json().get("sub", ""), "role_keys": ["User"], # set AUTH_ROLES_SYNC_AT_LOGIN = False } - else: - return {} + return {} On Flask-AppBuilder 3.4.0 the login page has changed. diff --git a/examples/oauth/config.py b/examples/oauth/config.py index 427107382c..87ea00602e 100644 --- a/examples/oauth/config.py +++ b/examples/oauth/config.py @@ -72,17 +72,17 @@ "remote_app": { "client_id": os.environ.get("AZURE_APPLICATION_ID"), "client_secret": os.environ.get("AZURE_SECRET"), - "api_base_url": "https://login.microsoftonline.com/{AZURE_TENANT_ID}/oauth2", + "api_base_url": f"https://login.microsoftonline.com/{os.environ.get('AZURE_TENANT_ID')}/oauth2", "client_kwargs": { "scope": "User.read name preferred_username email profile upn", "resource": os.environ.get("AZURE_APPLICATION_ID"), }, "request_token_url": None, "access_token_url": f"https://login.microsoftonline.com/" - f"{os.environ.get('AZURE_APPLICATION_ID')}/" + f"{os.environ.get('AZURE_TENANT_ID')}/" "oauth2/token", "authorize_url": f"https://login.microsoftonline.com/" - f"{os.environ.get('AZURE_APPLICATION_ID')}/" + f"{os.environ.get('AZURE_TENANT_ID')}/" f"oauth2/authorize", }, }, diff --git a/flask_appbuilder/exceptions.py b/flask_appbuilder/exceptions.py index 9cfb3c25c3..92189f6b2f 100644 --- a/flask_appbuilder/exceptions.py +++ b/flask_appbuilder/exceptions.py @@ -50,3 +50,11 @@ class ApplyFilterException(FABException): """When executing an apply filter a SQLAlchemy exception happens""" ... + + +class OAuthProviderUnknown(FABException): + """ + When an OAuth provider is not supported/unknown + """ + + ... diff --git a/flask_appbuilder/security/manager.py b/flask_appbuilder/security/manager.py index b9172b99eb..869735b8db 100644 --- a/flask_appbuilder/security/manager.py +++ b/flask_appbuilder/security/manager.py @@ -1,18 +1,17 @@ import datetime -import json import logging import re -from typing import Any, Dict, List, Optional, Set, Tuple, Union +from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Union -from authlib.jose import JsonWebKey, jwt from flask import Flask, g, session, url_for +from flask_appbuilder.exceptions import OAuthProviderUnknown from flask_babel import lazy_gettext as _ from flask_jwt_extended import current_user as current_user_jwt from flask_jwt_extended import JWTManager from flask_limiter import Limiter from flask_limiter.util import get_remote_address from flask_login import current_user, LoginManager -import requests +import jwt from werkzeug.security import check_password_hash, generate_password_hash from .api import SecurityApi @@ -271,7 +270,7 @@ def __init__(self, appbuilder): from authlib.integrations.flask_client import OAuth self.oauth = OAuth(app) - self.oauth_remotes = dict() + self.oauth_remotes = {} for _provider in self.oauth_providers: provider_name = _provider["name"] log.debug("OAuth providers init %s", provider_name) @@ -519,7 +518,10 @@ def current_user(self): elif current_user_jwt: return current_user_jwt - def oauth_user_info_getter(self, f): + def oauth_user_info_getter( + self, + func: Callable[["BaseSecurityManager", str, Dict[str, Any]], Dict[str, Any]], + ): """ Decorator function to be the OAuth user info getter for all the providers, receives provider and response @@ -534,21 +536,11 @@ def my_oauth_user_info(sm, provider, response=None): if provider == 'github': me = sm.oauth_remotes[provider].get('user') return {'username': me.data.get('login')} - else: - return {} + return {} """ - def wraps(provider, response=None): - ret = f(self, provider, response=response) - # Checks if decorator is well behaved and returns a dict as supposed. - if not type(ret) == dict: - log.error( - "OAuth user info decorated function " - "did not returned a dict, but: %s", - type(ret), - ) - return {} - return ret + def wraps(provider: str, response: Dict[str, Any] = None) -> Dict[str, Any]: + return func(self, provider, response) self.oauth_user_info = wraps return wraps @@ -587,9 +579,11 @@ def set_oauth_session(self, provider, oauth_response): ) session["oauth_provider"] = provider - def get_oauth_user_info(self, provider, resp): + def get_oauth_user_info( + self, provider: str, resp: Dict[str, Any] + ) -> Dict[str, Any]: """ - Since there are different OAuth API's with different ways to + Since there are different OAuth APIs with different ways to retrieve user info """ # for GITHUB @@ -628,21 +622,14 @@ def get_oauth_user_info(self, provider, resp): "last_name": data.get("family_name", ""), "email": data.get("email", ""), } - # for Azure AD Tenant. Azure OAuth response contains - # JWT token which has user info. - # JWT token needs to be base64 decoded. - # https://docs.microsoft.com/en-us/azure/active-directory/develop/ - # active-directory-protocols-oauth-code if provider == "azure": - log.debug("Azure response received:\n%s", json.dumps(resp, indent=4)) me = self._decode_and_validate_azure_jwt(resp["id_token"]) - log.debug("Decoded JWT:\n%s", json.dumps(me, indent=4)) + log.debug("User info from Azure: %s", me) + # https://learn.microsoft.com/en-us/azure/active-directory/develop/id-token-claims-reference#payload-claims return { - "name": me.get("name", ""), - "email": me["upn"], + "email": me["email"], "first_name": me.get("given_name", ""), "last_name": me.get("family_name", ""), - "id": me["oid"], "username": me["oid"], "role_keys": me.get("roles", []), } @@ -680,16 +667,26 @@ def get_oauth_user_info(self, provider, resp): "last_name": data.get("family_name", ""), "email": data.get("email", ""), } - else: - return {} + raise OAuthProviderUnknown() + + def _get_microsoft_jwks(self) -> List[Dict[str, Any]]: + import requests + + return requests.get(MICROSOFT_KEY_SET_URL).json() + + def _decode_and_validate_azure_jwt(self, id_token: str) -> Dict[str, str]: + verify_signature = self.oauth_remotes["azure"].client_kwargs.get( + "verify_signature", False + ) + if verify_signature: + from authlib.jose import JsonWebKey, jwt as authlib_jwt - def _decode_and_validate_azure_jwt(self, id_token): - keyset = JsonWebKey.import_key_set(requests.get(MICROSOFT_KEY_SET_URL).json()) - claims = jwt.decode(id_token, keyset) - claims.validate() - log.debug("Decoded JWT:\n%s", json.dumps(claims, indent=4)) + keyset = JsonWebKey.import_key_set(self._get_microsoft_jwks()) + claims = authlib_jwt.decode(id_token, keyset) + claims.validate() + return claims - return claims + return jwt.decode(id_token, options={"verify_signature": False}) def register_views(self): if not self.appbuilder.app.config.get("FAB_ADD_SECURITY_VIEWS", True): diff --git a/tests/security/test_auth_oauth.py b/tests/security/test_auth_oauth.py index 5040dd4812..6ee18c57bb 100644 --- a/tests/security/test_auth_oauth.py +++ b/tests/security/test_auth_oauth.py @@ -5,7 +5,9 @@ from flask import Flask from flask_appbuilder import AppBuilder, SQLA from flask_appbuilder.const import AUTH_OAUTH +from flask_appbuilder.exceptions import OAuthProviderUnknown import jinja2 +import jwt from tests.const import USERNAME_ADMIN, USERNAME_READONLY from tests.fixtures.users import create_default_users @@ -24,9 +26,29 @@ def setUp(self): ) self.app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False self.app.config["AUTH_TYPE"] = AUTH_OAUTH - self.app.config[ - "OAUTH_PROVIDERS" - ] = [] # can be empty, because we dont use the external providers in tests + self.app.config["OAUTH_PROVIDERS"] = [ + { + "name": "azure", + "icon": "fa-windows", + "token_key": "access_token", + "remote_app": { + "client_id": "CLIENT_ID", + "client_secret": "SECRET", + "api_base_url": "https://login.microsoftonline.com/TENANT_ID/oauth2", + "client_kwargs": { + "scope": "User.Read name email profile", + "resource": "AZURE_APPLICATION_ID", + }, + "request_token_url": None, + "access_token_url": "https://login.microsoftonline.com/" + "AZURE_APPLICATION_ID/" + "oauth2/token", + "authorize_url": "https://login.microsoftonline.com/" + "AZURE_APPLICATION_ID/" + "oauth2/authorize", + }, + } + ] # start Database self.db = SQLA(self.app) @@ -437,3 +459,158 @@ def test__registered__jmespath_role__with_role_sync(self): # validate - user was given the correct roles self.assertListEqual(user.roles, [sm.find_role("User")]) + + def test_oauth_user_info_getter(self): + self.appbuilder = AppBuilder(self.app, self.db.session) + + @self.appbuilder.sm.oauth_user_info_getter + def user_info_getter(sm, provider, response): + return {"username": "test"} + + self.assertEqual(self.appbuilder.sm.oauth_user_info, user_info_getter) + self.assertEqual( + self.appbuilder.sm.oauth_user_info("azure", {"claim": 1}), + {"username": "test"}, + ) + + def test_oauth_user_info_unknown_provider(self): + self.appbuilder = AppBuilder(self.app, self.db.session) + with self.assertRaises(OAuthProviderUnknown): + self.appbuilder.sm.oauth_user_info("unknown", {}) + + def test_oauth_user_info_azure(self): + self.appbuilder = AppBuilder(self.app, self.db.session) + claims = { + "aud": "test-aud", + "iss": "https://sts.windows.net/test/", + "iat": 7282182129, + "nbf": 7282182129, + "exp": 1000000000, + "amr": ["pwd"], + "email": "test@gmail.com", + "family_name": "user", + "given_name": "test", + "idp": "live.com", + "name": "Test user", + "oid": "b1a54a40-8dfa-4a6d-a2b8-f90b84d4b1df", + "unique_name": "live.com#test@gmail.com", + "ver": "1.0", + } + + # Create an unsigned JWT + unsigned_jwt = jwt.encode(claims, key=None, algorithm="none") + user_info = self.appbuilder.sm.get_oauth_user_info( + "azure", {"access_token": "", "id_token": unsigned_jwt} + ) + self.assertEqual( + user_info, + { + "email": "test@gmail.com", + "first_name": "test", + "last_name": "user", + "role_keys": [], + "username": "b1a54a40-8dfa-4a6d-a2b8-f90b84d4b1df", + }, + ) + + def test_oauth_user_info_azure_with_jwt_validation(self): + self.app.config["OAUTH_PROVIDERS"] = [ + { + "name": "azure", + "icon": "fa-windows", + "token_key": "access_token", + "remote_app": { + "client_id": "CLIENT_ID", + "client_secret": "SECRET", + "api_base_url": "https://login.microsoftonline.com/TENANT_ID/oauth2", + "client_kwargs": { + "scope": "User.Read name email profile", + "resource": "AZURE_APPLICATION_ID", + "verify_signature": True, + }, + "request_token_url": None, + "access_token_url": "https://login.microsoftonline.com/" + "AZURE_APPLICATION_ID/" + "oauth2/token", + "authorize_url": "https://login.microsoftonline.com/" + "AZURE_APPLICATION_ID/" + "oauth2/authorize", + }, + } + ] + + self.appbuilder = AppBuilder(self.app, self.db.session) + claims = { + "aud": "test-aud", + "iss": "https://sts.windows.net/test/", + "iat": 1696601585, + "nbf": 1696601585, + "exp": 7282182129, # 100 years from now ;) + "amr": ["pwd"], + "email": "test@gmail.com", + "family_name": "user", + "given_name": "test", + "idp": "live.com", + "name": "Test user", + "oid": "b1a54a40-8dfa-4a6d-a2b8-f90b84d4b1df", + "unique_name": "live.com#test@gmail.com", + "ver": "1.0", + } + from unittest.mock import MagicMock + + private_key = """-----BEGIN PRIVATE KEY----- +MIICdgIBADANBgkqhkiG9w0BAQEFAASCAmAwggJcAgEAAoGBALeDojEka93XZ/J8 +bDGgn2MIHykafgCx2D6wTZgmmhzpRH7/k7J/WSsqG6eSFg38mGJukPCa4dcG8dCL +meajEf2g4IoaYiE55yXs0ou/tixBJI8wRY+NfCluxgIcHdKhZISVO6CkR5r7diN/ +SLHPsFnDd0UiMJ5c48UsJwk8T5T7AgMBAAECgYEAqalrVB+mEi1KDud1Z9RmRzqF +BI1XnPDPSfXZZyeZJ82J5BgJxubx23RMqPnopfm4MJikK64lyZTED9hg6tgskk1X +J9pc7iyU4PQf+tx4tvElyOL4OSqGss/tQHtHz76hNOR1kxeCcJsJG+WS8P0/Kmj1 +0IoYKLFlb5AHr6KqDGECQQDZ0qKIzxdmZj3gSsNldc4oOQOKJgd1QSDGCOqR9p7f +oj7nuOPRVgnztqXALhNhpZXYJq8dWmpGYFi+EC1piRUDAkEA162gPgGzUJAIyhUM +sA6Uy9v64nqBnlygVpofhdvyznSf/KUsmWQZv7gpMMXnIGAQP+rqM1gJvuRtodml +hUeSqQJAHJH4J6GiHBhE/WpQ/rnY9IWl5TTfvY1xUwhQXBzQ8dxCC/rARvDWFVVb +oD1q5V/mq5dHWL5HOjvg5+0PR8xnKQJAMOdBik3AZugB1jBnrBPiUUcT3/5/HXVL +NdfEhgmVSJLRI+wf7LfxzrLnRBPbkE+334ZYjEPOEeahpS1AhrPv4QJAHpap1I+v +8m+N5G/MppasppHLJmXhnFeQsnBX7XcdYiCqHikuBlIzoQ0Cj5xbkfgMMCVORO64 +r9+EFRsxA5GNYA== +-----END PRIVATE KEY-----""" + # Create an unsigned JWT + unsigned_jwt = jwt.encode( + claims, key=private_key, algorithm="RS256", headers={"kid": "1"} + ) + self.appbuilder.sm._get_microsoft_jwks = MagicMock( + return_value={ + "keys": [ + { + "alg": "RS256", + "e": "AQAB", + "kid": "1", + "kty": "RSA", + "n": "t4OiMSRr3ddn8nxsMaCfYwgfKRp-ALHYPrBNmCaaHOlEfv-" + "Tsn9ZKyobp5IWDfyYYm6Q8Jrh1wbx0IuZ5qMR_aDgihpiITnnJezSi7-" + "2LEEkjzBFj418KW7GAhwd0qFkhJU7oKRHmvt2I39Isc-wWcN3RSIwnlz" + "jxSwnCTxPlPs", + "use": "sig", + "x5c": [ + "MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQC3g6IxJGvd12fyfGwxoJ9" + "jCB8pGn4Asdg+sE2YJpoc6UR+/5Oyf1krKhunkhYN/JhibpDwmuHXBvHQi5n" + "moxH9oOCKGmIhOecl7NKLv7YsQSSPMEWPjXwpbsYCHB3SoWSElTugpEea+3Y" + "jf0ixz7BZw3dFIjCeXOPFLCcJPE+U+wIDAQAB" + ], + } + ] + } + ) + user_info = self.appbuilder.sm.get_oauth_user_info( + "azure", {"access_token": "", "id_token": unsigned_jwt} + ) + self.assertEqual( + user_info, + { + "email": "test@gmail.com", + "first_name": "test", + "last_name": "user", + "role_keys": [], + "username": "b1a54a40-8dfa-4a6d-a2b8-f90b84d4b1df", + }, + ) diff --git a/tests/security/test_base_security_manager.py b/tests/security/test_base_security_manager.py index 899506575e..dfb04ea1a8 100644 --- a/tests/security/test_base_security_manager.py +++ b/tests/security/test_base_security_manager.py @@ -1,10 +1,8 @@ import datetime -import json import unittest from unittest.mock import MagicMock, patch from flask_appbuilder.security.manager import BaseSecurityManager -from flask_appbuilder.security.manager import JsonWebKey, jwt JWTClaimsMock = MagicMock() @@ -71,12 +69,3 @@ def test_subsequent_unsuccessful_auth(self, mock1, mock2): self.assertEqual(user_mock.fail_login_count, 10) self.assertEqual(user_mock.last_login, None) self.assertTrue(bsm.update_user.called_once) - - @patch.object(JsonWebKey, "import_key_set", MagicMock()) - @patch.object(jwt, "decode", MagicMock(return_value=JWTClaimsMock)) - @patch.object(json, "dumps", MagicMock(return_value="DecodedExampleAzureJWT")) - def test_azure_jwt_validated(self, mock1, mock2): - bsm = BaseSecurityManager() - - bsm._decode_and_validate_azure_jwt("ExampleAzureJWT") - JWTClaimsMock.validate.assert_called()