dpgaspar / Flask-AppBuilder

Simple and rapid application development framework, built on top of Flask. includes detailed security, auto CRUD generation for your models, google charts and much more. Demo (login with guest/welcome) - http://flaskappbuilder.pythonanywhere.com/
BSD 3-Clause "New" or "Revised" License
4.7k stars 1.36k forks source link

Azure AD OAuth2 - flask_appbuilder.security.views:Error returning OAuth user info: 'upn' #1868

Closed haripraghash closed 12 months ago

haripraghash commented 2 years ago

If you'd like to report a bug in Flask-Appbuilder, fill out the template below. Provide any extra information that may be useful

Responsible disclosure: We want to keep Flask-AppBuilder safe for everyone. If you've discovered a security vulnerability please report to danielvazgaspar@gmail.com.

Environment

Flask-Appbuilder version: Flask-AppBuilder==4.0.0

pip freeze output: aiohttp==3.8.1 aiosignal==1.2.0 alembic==1.6.5 amqp==5.1.0

Editable install with no version control (apache-superset==0.0.0.dev0)

-e /app apispec==3.3.2 appnope==0.1.3 asttokens==2.0.5 async-timeout==4.0.2 attrs==21.2.0 Babel==2.9.1 backcall==0.2.0 backoff==1.11.1 billiard==3.6.4.0 bleach==3.3.1 boto3==1.18.19 botocore==1.21.19 Brotli==1.0.9 cached-property==1.5.2 cachelib==0.4.1 celery==5.2.2 certifi==2021.10.8 cffi==1.14.6 chardet==4.0.0 charset-normalizer==2.0.4 click==8.0.4 click-didyoumean==0.3.0 click-plugins==1.1.1 click-repl==0.2.0 colorama==0.4.4 convertdate==2.3.2 cron-descriptor==1.2.24 croniter==1.0.15 cryptography==3.4.7 decorator==5.1.1 deprecation==2.1.0 dnspython==2.1.0 email-validator==1.1.3 et-xmlfile==1.1.0 executing==0.8.3 Flask==2.0.3 Flask-AppBuilder==4.0.0 Flask-Babel==1.0.0 Flask-Caching==1.10.1 Flask-Compress==1.10.1 Flask-Cors==3.0.10 Flask-JWT-Extended==4.3.1 Flask-Login==0.4.1 Flask-Migrate==3.1.0 Flask-SQLAlchemy==2.5.1 flask-talisman==0.8.1 Flask-WTF==0.14.3 frozenlist==1.3.0 func-timeout==4.3.5 future==0.18.2 geographiclib==1.52 geopy==2.2.0 graphlib-backport==1.0.3 gunicorn==20.1.0 hashids==1.3.1 holidays==0.10.3 humanize==3.11.0 idna==3.2 ijson==3.1.4 ipython==8.3.0 isodate==0.6.0 itsdangerous==2.1.1 jedi==0.18.1 Jinja2==3.0.3 jmespath==0.10.0 jsonlines==2.0.0 jsonschema==3.2.0 kombu==5.2.4 korean-lunar-calendar==0.2.1 linear-tsv==1.1.0 Mako==1.1.4 Markdown==3.3.4 MarkupSafe==2.0.1 marshmallow==3.13.0 marshmallow-enum==1.5.1 marshmallow-sqlalchemy==0.23.1 matplotlib-inline==0.1.3 msgpack==1.0.2 multidict==5.1.0 mysqlclient==2.1.0 numpy==1.22.1 openpyxl==3.0.7 packaging==21.3 pandas==1.3.4 parsedatetime==2.6 parso==0.8.3 pexpect==4.8.0 pgsanity==0.2.9 pickleshare==0.7.5 Pillow==9.1.0 polyline==1.4.0 prison==0.2.1 progress==1.6 prompt-toolkit==3.0.28 psycopg2-binary==2.9.1 ptyprocess==0.7.0 pure-eval==0.2.2 pure-sasl==0.6.2 pyarrow==5.0.0 pycparser==2.20 pydruid==0.6.2 Pygments==2.12.0 PyHive==0.6.5 pyinstrument==4.0.2 PyJWT==2.4.0 PyMeeus==0.5.11 pyparsing==3.0.6 pyrsistent==0.16.1 python-dateutil==2.8.2 python-dotenv==0.19.0 python-editor==1.0.4 python-geohash==0.8.5 pytz==2021.3 PyYAML==5.4.1 redis==3.5.3 requests==2.26.0 rfc3986==1.5.0 s3transfer==0.5.0 sasl==0.3.1 selenium==3.141.0 simplejson==3.17.3 six==1.16.0 slackclient==2.5.0 SQLAlchemy==1.3.24 SQLAlchemy-Utils==0.37.8 sqloxide==0.1.17 sqlparse==0.3.0 stack-data==0.2.0 tableschema==1.20.2 tabulate==0.8.9 tabulator==1.53.5 thrift==0.13.0 thrift-sasl==0.4.3 traitlets==5.2.1.post0 typing-extensions==3.10.0.0 unicodecsv==0.14.1 urllib3==1.26.6 vine==5.0.0 wcwidth==0.2.5 webencodings==0.5.1 Werkzeug==2.0.3 WTForms==2.3.3 WTForms-JSON==0.3.3 xlrd==2.0.1 yarl==1.6.3

Describe the expected results

Tell us what should happen.

AUTH_TYPE = AUTH_OAUTH

      OAUTH_PROVIDERS = [
      {
        "name": "azure",
        "icon": "fa-windows",
        "token_key": "access_token",
        "remote_app": {
            "client_id": "id",
            "client_secret": "pass",
            "api_base_url": "https://login.microsoftonline.com/tenantid/v2.0",
            "client_kwargs": {
                "scope": "openid email profile offline_access",
                "resource": "clientid",
            },
            "request_token_url": None,
            "jwks_uri": "https://login.microsoftonline.com/common/discovery/v2.0/keys",
            "access_token_url": "https://login.microsoftonline.com/tenantid/oauth2/v2.0/token",
            "authorize_url": "https://login.microsoftonline.com/tenantid/oauth2/v2.0/authorize",
          }
          }

Describe the actual results

Tell us what happens instead.

ERROR:flask_appbuilder.security.views:Error returning OAuth user info: 'upn'

Steps to reproduce

evanerwee01 commented 2 years ago

I have the same issue

passren commented 2 years ago

I faced the same issue.

If add upn in scope like this: "scope": "openid email profile upn", the error message occur as below: The application 'xxxxxxxx' asked for scope 'upn' that doesn't exist on the resource.

I traced the code, and found the error happen in the method BaseSecurityManager.get_oauth_user_info:

if provider == "azure":
     log.debug("Azure response received : {0}".format(resp))`
     id_token = resp["id_token"]
     log.debug(str(id_token))
     me = self._azure_jwt_token_parse(id_token)
     log.debug("Parse JWT token : {0}".format(me))
     return {
          "name": me.get("name", ""),
          "email": me["upn"],    ### The error come out when 'upn' missed in id_token
          "first_name": me.get("given_name", ""),
          "last_name": me.get("family_name", ""),
          "id": me["oid"],
          "username": me["oid"],
          "role_keys": me.get("roles", []),
     }

The following code will use 'email' and 'username' to check if username is existing in DB.

The new feature I'd like to propose is creating a field mapping in config, which developer can determine what fields from id_token can map to email or username. Like, use 'email' parsed from id_token map to email, and 'preferred_username' parsed from id_token map to username.

krionbsd commented 2 years ago

@dpgaspar any chance to incorporate #1912 into next release?

georgewfisher commented 2 years ago

@passren Isn't upn simply never available ever in an id_token? ID token doc: https://learn.microsoft.com/en-us/azure/active-directory/develop/id-tokens

Is the right fix here to modify the field selection to use the openid claims, or to use the access_token which does have the upn claim?

georgewfisher commented 2 years ago

I have tested @passren 's change (https://github.com/dpgaspar/Flask-AppBuilder/pull/1912) - it works. Let's release it? Let me know if I can do anything to help.

My comment above that the access_token should be used intead of the id_token is orthogonal to allowing configurable claims.

Atheuz commented 2 years ago

You can make upn available in Azure AD access tokens and id tokens gotten in OAuth v2.0 if you set it as an optionalClaim for your app registration manifest:

"optionalClaims": {
    "accessToken": [
        {
            "name": "upn",
            "essential": true
        }
    ]
    "idToken": [
        {
            "name": "upn",
            "essential": true
        }
    ]
}

To get upn, you need to enable the profile scope, so you should also enable that in your app reg manifest, you do that by adding it to requiredResourceAccess in the manifest:

  "requiredResourceAccess": [
    {
      "resourceAppId": "00000003-0000-0000-c000-000000000000",
      "resourceAccess": [
        {
          "id": "14dad69e-099b-42c9-810b-d002981feec1",
          "type": "Scope"
        }
      ]
    }

This should probably be fixed in flask-appbuilder though, it shouldn't fail like this because upn is unavailable.

dpgaspar commented 12 months ago

This should have been fixed on 4.3.9 with #2121