Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] .WithTenantId started to throw ArgumentException #4280

Closed
rabenshm opened this issue Jul 31, 2023 · 5 comments · Fixed by #4347
Closed

[Bug] .WithTenantId started to throw ArgumentException #4280

rabenshm opened this issue Jul 31, 2023 · 5 comments · Fixed by #4347

Comments

@rabenshm
Copy link

Library version used

4.55.0

.NET version

4.8

MSAL client type

Confidential

Application type

Web API

Authentication flow type

Client credentials (service-to-service calls)

Is this a new or an existing app?

The app is in production, and I have upgraded to a new version of MSAL

Issue description and reproduction steps

Well calling on the ConfidentialClientApplication builder .WithTenantId with invalid tenant, a new argument exception regarding authorityurl is being throw, although we do not explictly set authorityurl by ourselves, and this is not declared as a thrown exception.

Until now we handled MsalException for those scenario. As discussed offline with Bogdan suggest to change to MsalClientException not to break flows.

For now we have rolled back to previous MSAL version.

Relevant code snippets

No response

Expected behavior

MsalClientException should be thrown , better on the Execute and not in the builder (even if MSAL itself will not go our for STS)

Identity provider

Microsoft Entra ID (Work and School accounts and Personal Microsoft accounts)

Regression

4.50

Solution and workarounds

No response

@rabenshm rabenshm added needs attention Delete label after triage untriaged Do not delete. Needed for Automation labels Jul 31, 2023
@bgavrilMS bgavrilMS added this to the 4.56.0 milestone Jul 31, 2023
@bgavrilMS
Copy link
Member

Today we throw an ArgumentException if authority URl is malformed (e.g. has spaces in it). We should throw an MsalClientException

@gladjohn gladjohn added confidential-client and removed untriaged Do not delete. Needed for Automation needs attention Delete label after triage labels Aug 4, 2023
@pmaytak pmaytak self-assigned this Aug 24, 2023
@pmaytak pmaytak modified the milestones: 4.57.0, 4.56.0 Aug 24, 2023
@pmaytak pmaytak added the regression Behavior that worked in a previous release that no longer works in a newer release label Aug 26, 2023
@pmaytak pmaytak modified the milestones: 4.56.0, 4.57.0 Aug 30, 2023
@pmaytak pmaytak removed the regression Behavior that worked in a previous release that no longer works in a newer release label Sep 15, 2023
@pmaytak
Copy link
Contributor

pmaytak commented Sep 15, 2023

Between 4.50 and 4.55, the exceptions in calling only WithTenant at app level didn't change. Exceptions for calling WithTenant and WithAuthority changed from MsalClient to Argument - which is what I assume this issue is about. We simply didn't allow calling both, WithTenant and WithAuthority, which I think was incorrect. I think now it's correct - WithTenant just modifies user-provided or default authority. There are more details in the PR: #4347.

@pmaytak
Copy link
Contributor

pmaytak commented Sep 27, 2023

@rabenshm Can you clarify which method you called and what the exception you were expecting, I wasn;t able to find the exact scenario.
image

@rabenshm
Copy link
Author

rabenshm commented Sep 28, 2023 via email

@bgavrilMS
Copy link
Member

bgavrilMS commented Oct 4, 2023

@rabenshm - changes in this part of the code are quite complex and this problem has not been reported by others. Is it so complex to update your code to take into account the new behavior? MSAL is just detecting an invalid URI earlier, in the app builder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
4 participants