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

Allow non-GUID client id with PublicClientApplicationBuilder #4686

Closed
eberlekhaufe opened this issue Mar 28, 2024 · 9 comments · Fixed by #4748
Closed

Allow non-GUID client id with PublicClientApplicationBuilder #4686

eberlekhaufe opened this issue Mar 28, 2024 · 9 comments · Fixed by #4748

Comments

@eberlekhaufe
Copy link

eberlekhaufe commented Mar 28, 2024

MSAL client type

Public

Problem statement

MSAL.NET: 4.60.0
.NET 8

I use the PublicClientApplicationBuilder to create a msal app to authenticate with a 3rd party identity provider which issues client ids in the form "Some GUID@somesuffix"

Unfortunately PublicClientApplicationBuilder.Build fails on such a client id, as it expects a proper GUID.

Can that be changed to support non-GUID client ids?
Even if validation was changed, would there be down-stream errors with a non-GUID client id?

Here is my code
var clientBuilder = PublicClientApplicationBuilder.Create("my-non-GUID-clientid")
.WithDefaultRedirectUri()
.WithExperimentalFeatures()
.WithOidcAuthority("some url");

var msalClient = clientBuilder.Build(); // throws here
var result = await msalClient.AcquireTokenInteractive(scopes: ["api", "api:concurrent_access", "email", "oidc", "profile"]).ExecuteAsync();

Calling Build throws a Microsoft.Identity.Client.MsalClientException
Message: Error: ClientId is not a GUID.
Stack Trace:
at Microsoft.Identity.Client.PublicClientApplicationBuilder.Validate()
at Microsoft.Identity.Client.AbstractApplicationBuilder`1.BuildConfiguration()
at Microsoft.Identity.Client.PublicClientApplicationBuilder.BuildConcrete()
at Microsoft.Identity.Client.PublicClientApplicationBuilder.Build()
...

Thanks a lot in advance,
Krischan

Proposed solution

PublicClientApplicationBuilder and PublicClientApplication should support non-GUID client ids, if possible.

Alternatives

Any workaround would also be appreciated, if exists :)

@eberlekhaufe eberlekhaufe added needs attention Delete label after triage untriaged Do not delete. Needed for Automation labels Mar 28, 2024
@neha-bhargava neha-bhargava added public-client and removed untriaged Do not delete. Needed for Automation needs attention Delete label after triage labels Mar 28, 2024
@localden
Copy link
Collaborator

localden commented Apr 2, 2024

What identity provider are you using @eberlekhaufe?

@eberlekhaufe
Copy link
Author

It is an identity Provider implemented in the acumatica ERP system. It is implemented based on https://identityserver4.readthedocs.io/en/latest/

It supports oauth2/oidc.

@rayluo
Copy link
Contributor

rayluo commented Apr 3, 2024

It is an identity Provider implemented in the acumatica ERP system. It is implemented based on https://identityserver4.readthedocs.io/en/latest/

It supports oauth2/oidc.

By the way, when attempting to use a generic oidc Identity Provider, it is better to use WithOidcAuthority(). (It may still miss the non-GUID support at this time, but it will eventually be implemented.)

@localden localden self-assigned this Apr 3, 2024
@localden
Copy link
Collaborator

localden commented Apr 3, 2024

I'll assign to myself for the time being - I think there is value in this being a generic client ID, but likely this will fall a bit lower in the priority stack of things we need to do in the short-term.

@jwikberg
Copy link

Non-guid client id:s are already supported for ADFS:

//ADFS does not require client id to be in the form of a GUID.
if (Config.Authority.AuthorityInfo?.AuthorityType != AuthorityType.Adfs && !Guid.TryParse(Config.ClientId, out _))
{
throw new MsalClientException(MsalError.ClientIdMustBeAGuid, MsalErrorMessage.ClientIdMustBeAGuid);
}

And if I change that to also allow "Generic" authorities, I was able to use Auth0 and a non-guid client id with PublicClientApplicationBuilder.
Is there anything else that needs to be changed?

@eberlekhaufe
Copy link
Author

Hi @jwikberg , in my setup neither ADFS nor Auth0 is used (see code snippet above in the issue). Using the Builder as above, causes the client id must be guid error.

@jwikberg
Copy link

jwikberg commented May 7, 2024

Hi @jwikberg , in my setup neither ADFS nor Auth0 is used (see code snippet above in the issue). Using the Builder as above, causes the client id must be guid error.

My question was directed at Microsoft.

@bgavrilMS
Copy link
Member

Non-guid client id:s are already supported for ADFS:

//ADFS does not require client id to be in the form of a GUID.
if (Config.Authority.AuthorityInfo?.AuthorityType != AuthorityType.Adfs && !Guid.TryParse(Config.ClientId, out _))
{
throw new MsalClientException(MsalError.ClientIdMustBeAGuid, MsalErrorMessage.ClientIdMustBeAGuid);
}

And if I change that to also allow "Generic" authorities, I was able to use Auth0 and a non-guid client id with PublicClientApplicationBuilder.
Is there anything else that needs to be changed?

Feel free to propose a change. There is an AuthorityType.Generic type.

@kurtanr
Copy link
Contributor

kurtanr commented May 7, 2024

Hi @bgavrilMS,
hope you and the team will consider the PR I've created which fixes the issue.
PR just modifies the section mentioned by @jwikberg and there is also a new test added (not sure if the test location is correct - it uses Selenium).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment