microsoft / o365-moodle

Office 365 and Azure Active Directory plugins for Moodle
GNU General Public License v3.0
180 stars 136 forks source link

attribute mail returns UPN #2182

Closed risticma closed 6 months ago

risticma commented 2 years ago

We are running Microsoft 365 Integration (4.0.1) and we have problems with our OPENID connect for moodle 4. In our maping we want to take mail attribute from our AD azure. If we put mail into email attribute in moodle settings we get UPN from our AD. If we put UPN instead of mail in settings is the same thing. We need to get the actual mail attribute back, because in our case mail and UPN are not the same. Thank you for your help.

weilai-irl commented 1 year ago

Hi @risticma,

When running user profile sync on email address profile field, there are a few possible places where the email profile can be retrieved, depending on the configuration and field mapping settings.

Assume you set Moodle email to be synced from OIDC email, firstly the sync will determine whether to get email from Graph API call or information from ID token. The logic is: If (local_o365 is installed and configure) { if (field mapping uses any remote field that are not on the basic remote fields) { USE GRAPH API. } else { USE ID token. } } else { USE ID token. } Basic remote fields include:

If Graph API is used, then Graph API at https://learn.microsoft.com/en-us/graph/api/user-get?view=graph-rest-1.0&tabs=http is called and email is retrieved from response.

If ID token is used, Moodle will try to find either "email" claim in the ID token and use it. If the "email" token is not received, or is empty, it will fall back to the "unique_name" claim, which is normally the UPN.

Note that the exact tokens included in the ID token claim is configurable in the IdP. The plugin only support the most common settings. If you find out that the email address uses a different token name, e.g. "mail", you can create a simple patch. The logic is at https://github.com/microsoft/o365-moodle/blob/3a12d241c81d4660c68af0d4911844b1c226509a/auth/oidc/classes/loginflow/base.php#L184-L196 and https://github.com/microsoft/o365-moodle/blob/3a12d241c81d4660c68af0d4911844b1c226509a/auth/oidc/classes/loginflow/base.php#L254-L265.

There is also plan to allow site admins to configure tokens from admin interface, but this may not be available for some time.

Regards, Lai

nbozhkov-ucl commented 1 year ago

Hi @weilai-irl,

We are also affected by this problem. Moodle 3.11.11 and latest auth_oidc for 311. The problem seem to have started after https://github.com/microsoft/moodle-auth_oidc/tree/70113f590bf22647b91c69962bc6cd97e4c27156.

We are sending Email in the optional claim both as ID and Access token types. The name in AAD is Email so exactly matching with the what the plugin is expecting. All was working correctly up to the above commit.

Any advise will be greatly appreciated.

Thanks. Nikola

mmulrthelp commented 1 year ago

@risticma & @nbozhkov-ucl , we saw this happening in August this year on 3.11 the solution was to press the Provide Admin Consent button again as the aadtenantid value was wiped out. Check out the blog I wrote about it here - It 'might' help you both out, if its the same issue.

Regards, Ray.

nbozhkov-ucl commented 1 year ago

Thanks @mmulrthelp. I saw https://github.com/microsoft/o365-moodle/issues/2066 - we are missing the aadtenantid in the plugin settings table as the admin consent button was not pressed within Moodle. The reason for that is the amount of unnecessary write permissions it configures in AAD. Seems like these write permissions are mostly needed if you need to integrate MS Teams and other o365 apps.

Our AAD authentication/integration was bringing the correct value for Email until we upgraded the auth_oidc and local_o365 plugins as per my above post. As @weilai-irl above there are two ways to get the email and we were getting it via the alternative approach by adding it as an optional claim. image

AAD admin has granted the required permissions within AAD as follows: image

The problem with getting upn instead of email is that some other integrations rely on the email as a unique field to match the user.

Some further thoughts:

Hope @weilai-irl will be able to provide advise on a temp fix.

Thanks. Nikola

mmulrthelp commented 1 year ago

@nbozhkov-ucl clicking the consent button asks you to login and takes you to your App Registration, it then passes the required tenancy value back to the database. It does not add any extra permissions for you which you have not already added, it just check that they are granted and grants them if not.

Click this button, then log in with an Azure administrator account to provide consent. This will need to be done whenever you change "Admin" permissions in Azure.

I know this, as I've had to get an Azure admin to do it after a upgrade to the office plugin suite wiped out the value.

Alternatively you can just put in your tenancy value direct to the database (if you have access) from here in the app registration: image

image

Regards, Ray.

nbozhkov-ucl commented 1 year ago

Thanks Ray.

Back in the days it was definitely adding a number of write permissions, not sure if this has changed. I have a screenshot when we were setting up the integration:

image

So one of our options to fix this is what you have suggested above. The other is understanding why the email is not picked up on login when the email value is passed in the optional claim and fixing this.

Thanks. Nikola

nbozhkov-ucl commented 1 year ago

Hi @weilai-irl,

We are also affected by this problem. Moodle 3.11.11 and latest auth_oidc for 311. The problem seem to have started after https://github.com/microsoft/moodle-auth_oidc/tree/70113f590bf22647b91c69962bc6cd97e4c27156.

We are sending Email in the optional claim both as ID and Access token types. The name in AAD is Email so exactly matching with the what the plugin is expecting. All was working correctly up to the above commit.

Any advise will be greatly appreciated.

Thanks. Nikola

Looking at our build the problem might have been introduced earlier than the commit I mentioned above as we have pinned to a local copy for some time in order to utilise some bug fixes that were not merged upstream.

I need some more time to pinpoint the point in time when the issue was introduced.

Thanks. Nikola

nbozhkov-ucl commented 1 year ago

Using a test instance, I have added the aadtenantid value manually in mdl_config_plugins table but still getting upn instead of email.

mmulrthelp commented 1 year ago

@nbozhkov-ucl did you then try deleting the delta token here /local/o365/acp.php?mode=maintenance_cleandeltatoken Then run the schedule task of 'Azure AD Sync' ?

nbozhkov-ucl commented 1 year ago

'Azure AD sync' task is returning the email not the UPN anyway as it does not rely on the aadtenantid based on the @weilai-irl explanation in #2066 . It is on login when the email gets reverted back to UPN.

aspark21 commented 1 year ago

I think I've cracked it, the problem is the expansion to using the access token for populating user fields https://github.com/microsoft/o365-moodle/blob/MOODLE_311_STABLE/auth/oidc/classes/loginflow/base.php#L211

however the access token does not receive mail https://learn.microsoft.com/en-us/azure/active-directory/develop/access-tokens#payload-claims but the idtoken does https://learn.microsoft.com/en-us/azure/active-directory/develop/id-tokens#payload-claims

This came from the change between v3.11.4 and v3.11.5 of the auth_oidc plugin -> https://github.com/microsoft/moodle-auth_oidc/compare/v3.11.4...v3.11.5#diff-fa93f6d0d967474acc7bb046526c9f9b805a51855f776e34a80edce9a291544dL186

aspark21 commented 1 year ago

btw I don't think the optional claims we've defined do anything.

aspark21 commented 1 year ago

the use of access token seems to be traced back to support for V2.0 but no logical explanation for their use - https://github.com/microsoft/o365-moodle/commit/dedbde7bae2df5bffa721cd033587afa6a556c60

think it's safe to say just removing 'token' from the arrays will resolve this

weilai-irl commented 9 months ago

Hi all,

I think I found the root of the issue. When trying to get email address from tokens, there's a bug which results in the "email" claim value retrieved from ID token mistakenly being overwritten by the fallback logic which gets "upn" claim value from access token. The linked PR fixes the issue.

@aspark21 Processing both ID token and access token is not a bug, but a feature. It turned out that Azure AD (i.e. Entra ID) and Microsoft Identity Platform tokens contain different set of claims. Please refer to the following table for the claims available by default in each IdP types.

  | Azure AD (Entra ID) | Microsoft Identity Platform -- | -- | -- upn | ID token / Access token | Access token oid | ID token / Access token | ID token / Access token unique_name | ID token / Access token | Access token family_name | ID token / Access token | Access token given_name | ID token / Access token | Access token email | N/A | ID token Processing only ID token would mean that the syncing of a few fields would be missing. The logic in the linked PR would be the best of both worlds. Regards, Lai
weilai-irl commented 6 months ago

Hi,

The bug in processing mail token claim has been fixed in the releases from today:

I'm going to close this issue now.

Regards, Lai