Flask-AppBuilder icon indicating copy to clipboard operation
Flask-AppBuilder copied to clipboard

feat: Flexible fields mapping for AAD user info and FAB user

Open passren opened this issue 3 years ago • 12 comments

Description

Currently in security module for AAD OAuth, if get user info from AAD, the module will return a fixed values which map user info from AAD to fab user as below:

return {
    "name": me.get("name", ""),
    "email": me["upn"],
    "first_name": me.get("given_name", ""),
    "last_name": me.get("family_name", ""),
    "id": me["oid"],
    "username": me["oid"],
    "role_keys": me.get("roles", []),
}

But I found some of fields from AAD may be missed like "upn" can't retrieve from AAD, and the fixed mapping doesn't meet my requirements, like "email" should map to me["email"] in my scenario. This PR is to enable the fields mapping can be set in config.py for more flexible configuration.

The new mapping structure is blow:

"user_info_mapping": {
    "name": ("name", ""),
    "email": ("email", ""),
    "first_name": ("given_name", ""),
    "last_name": ("family_name", ""),
    "id": ("oid", ""),
    "username": ("preferred_username", ""),
    "role_keys": ("roles", []),
}

The field mapping shall look like this "<FAB user info field>": ("<user info field from AAD>", <default value if no this field from AAD>).

The fields mapping will be put into this path: OAUTH_PROVIDERS > {provider: azure} > "remote_app" > "client_kwargs"

OAUTH_PROVIDERS = [
{
        "name": "azure",
        "icon": "fa-windows",
        "token_key": "access_token",
        "remote_app": {
            "client_id": "AZURE_APPLICATION_ID",
            "client_secret": "AZURE_SECRET",
            "api_base_url": "https://login.microsoftonline.com/AZURE_TENANT_ID/oauth2",
            "client_kwargs": {
                "scope": "User.read name preferred_username email profile upn",
                "resource": "AZURE_APPLICATION_ID",
                "user_info_mapping": {
                    "name": ("name", ""),
                    "email": ("email", ""),
                    "first_name": ("given_name", ""),
                    "last_name": ("family_name", ""),
                    "id": ("oid", ""),
                    "username": ("preferred_username", ""),
                    "role_keys": ("roles", []),
                }
            },
            "request_token_url": None,
            "access_token_url": "https://login.microsoftonline.com/AZURE_TENANT_ID/oauth2/token",
            "authorize_url": "https://login.microsoftonline.com/AZURE_TENANT_ID/oauth2/authorize",
        },
    },
]

ADDITIONAL INFORMATION

  • [x] Has associated issue: #1868
  • [ ] Is CRUD MVC related.
  • [x] Is Auth, RBAC security related.
  • [ ] Changes the security db schema.
  • [ ] Introduces new feature
  • [ ] Removes existing feature

passren avatar Aug 18 '22 05:08 passren

@passren Could you please fix black formatting to get it like that:

--- a/flask_appbuilder/security/manager.py
+++ b/flask_appbuilder/security/manager.py
@@ -615,12 +615,16 @@ class BaseSecurityManager(AbstractSecurityManager):
             log.debug(str(id_token))
             me = self._azure_jwt_token_parse(id_token)
             log.debug("Parse JWT token : {0}".format(me))
-            user_info_map = self.oauth_remotes[provider].client_kwargs.get("user_info_mapping")
+            user_info_map = self.oauth_remotes[provider].client_kwargs.get(
+                "user_info_mapping"
+            )

             user_info_data = {}
             if user_info_map:
                 for user_info_field, aad_user_info_field in user_info_map.items():
-                    user_info_data[user_info_field] = me.get(aad_user_info_field[0], aad_user_info_field[1])
+                    user_info_data[user_info_field] = me.get(
+                        aad_user_info_field[0], aad_user_info_field[1]
+                    )
             else:
                 user_info_data = {
                     "name": me.get("name", ""),

krionbsd avatar Aug 22 '22 14:08 krionbsd

@passren I tried above patch and new mapping with "email": ("email", "") but it doesn't seem to solve the issue, still getting ERROR - Error returning OAuth user info: 'upn' error

krionbsd avatar Aug 31 '22 14:08 krionbsd

@krionbsd just remove "upn" from client_kwargs > scope, like this:

"client_kwargs": { "scope": "User.read name preferred_username email profile",

passren avatar Sep 01 '22 01:09 passren

User.read name preferred_username email profile

I don't have upn in client_kwargs scope, but instead 'scope': 'openid profile email groups',

krionbsd avatar Sep 01 '22 14:09 krionbsd

User.read name preferred_username email profile

I don't have upn in client_kwargs scope, but instead 'scope': 'openid profile email groups',

That's strange. Please check if "user_info_mapping" was placed under client_kwargs. Only if the code can't get mapping from config file, it will try to get default "upn" from AAD user response.

passren avatar Sep 02 '22 00:09 passren

@passren I have tested your change with Superset and found that it works and addresses the claim mapping issue.

@dpgaspar can we get this change checked in?

@krionbsd It's possible your config was missing something, or you were missing the passren's change. Here's what I used:

OAUTH_PROVIDERS = [
        {
            "name": "azure",
            "icon": "fa-windows",
            "token_key": "access_token",
            "remote_app": {
               "client_id": "{{CLIENT_ID}}",
               "client_secret": os.environ.get("AZURE_SECRET"),
               "api_base_url": "https://login.microsoftonline.com/{{TENANT_ID}}/oauth2/v2.0/",
               "client_kwargs": {
                  "scope": "User.read email profile openid",
                  "resource": "{{CLIENT_ID}}",
                  "user_info_mapping":
                  {
                    "name": ("name", ""),
                    "email": ("email", ""),
                    "first_name": ("given_name", ""),
                    "last_name": ("family_name", ""),
                    "id": ("oid", ""),
                    "username": ("preferred_username", ""),
                    "role_keys": ("roles", []),
                  }
               },
               "request_token_url": None,
               "access_token_url": "https://login.microsoftonline.com/{{TENANT_ID}}/oauth2/v2.0/token",
               "authorize_url": "https://login.microsoftonline.com/{{TENANT_ID}}/oauth2/v2.0/authorize",
               "jwks_uri": "https://login.microsoftonline.com/common/discovery/v2.0/keys"
           }
       }
    ]

georgewfisher avatar Oct 17 '22 23:10 georgewfisher

@passren I have test this on AKS, it works for me too. Thank you for your proposed solution and we hope this PR could be merged

@dpgaspar This issue is a common issue for azure aad users, another team from my part also have a similar one. Could you please help to merge the pr?

ms-uhrs avatar Oct 19 '22 02:10 ms-uhrs

I've been using dex as OpenID provider which drives authentication to Azure, for whatever reasons I'm still getting the error with above mentioned config as well as with @passren patch:

ERROR - Error authorizing OAuth access token: invalid_scope: Unrecognized scope(s) ["User.read"]

krionbsd avatar Oct 20 '22 09:10 krionbsd

@georgewfisher If you are trying to config AAD in Superset, you could refer to this: Config Superset > Custom OAuth2 Configuration. Write a CustomSsoSecurityManager to resolve FAB AAD issue. Here is my code:

import logging
from superset.security import SupersetSecurityManager

class AADSecurityManager(SupersetSecurityManager):

    def oauth_user_info(self, provider, response=None):
        logging.debug("Oauth2 provider: {0}.".format(provider))
        if provider == 'azure':
            logging.debug("Azure response received : {0}".format(response))
            id_token = response["id_token"]
            logging.debug(str(id_token))
            me = self._azure_jwt_token_parse(id_token)
            logging.debug("Parse JWT token : {0}".format(me))
            return {
                "name": me.get("name", ""),
                "email": me["email"],
                "first_name": me.get("given_name", ""),
                "last_name": me.get("family_name", ""),
                "id": me["oid"],
                "username": me["preferred_username"],
                "role_keys": me.get("roles", []),
            }

Then set this manager class in config file, like this:

from custom_sso_security_manager import AADSecurityManager
CUSTOM_SECURITY_MANAGER = AADSecurityManager

passren avatar Oct 21 '22 02:10 passren

@georgewfisher If you are trying to config AAD in Superset, you could refer to this: Config Superset > Custom OAuth2 Configuration. Write a CustomSsoSecurityManager to resolve FAB AAD issue. Here is my code:

import logging
from superset.security import SupersetSecurityManager

class AADSecurityManager(SupersetSecurityManager):

    def oauth_user_info(self, provider, response=None):
        logging.debug("Oauth2 provider: {0}.".format(provider))
        if provider == 'azure':
            logging.debug("Azure response received : {0}".format(response))
            id_token = response["id_token"]
            logging.debug(str(id_token))
            me = self._azure_jwt_token_parse(id_token)
            logging.debug("Parse JWT token : {0}".format(me))
            return {
                "name": me.get("name", ""),
                "email": me["email"],
                "first_name": me.get("given_name", ""),
                "last_name": me.get("family_name", ""),
                "id": me["oid"],
                "username": me["preferred_username"],
                "role_keys": me.get("roles", []),
            }

Then set this manager class in config file, like this:

from custom_sso_security_manager import AADSecurityManager
CUSTOM_SECURITY_MANAGER = AADSecurityManager

but this is not correct procedure right because when we are deploying production how can we edit manager clients are not acceptable that way please give me better solution

Thanks..

Narender-007 avatar Aug 01 '23 10:08 Narender-007

May I know why this PR is still being pending? Thank u all your guys work.

Firxiao avatar Jun 26 '24 06:06 Firxiao