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

Hard to diagnose exception thrown when AzureADOptions.Instance is not set #6022

Closed
DamianEdwards opened this issue Jul 10, 2018 · 5 comments
Closed
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed feature-AADIntegration This issue is related to Azure AD integration help wanted Up for grabs. We would accept a PR to help resolve this issue

Comments

@DamianEdwards
Copy link
Member

I was migrating an app that uses Azure AD auth from 1.x to 2.1, including using the new UI package. I had not set AzureADOptions.Instance (as this is new in 2.x and I didn't know it needed to be set). The exception that results does not have user-code on the stack, and thus has no line numbers. The exception is from the Uri constructor, which is being passed a null string (from this line).

We should ensure we validate options properties before using them to create other types where they might throw, as it's incredibly hard for app devs to diagnose.

@javiercn @blowdart

@aspnet-hello aspnet-hello transferred this issue from aspnet/AADIntegration Dec 19, 2018
@aspnet-hello aspnet-hello added area-identity Includes: Identity and providers bug This issue describes a behavior which is not expected - a bug. feature-AADIntegration This issue is related to Azure AD integration labels Dec 19, 2018
@blowdart blowdart added area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer and removed area-identity Includes: Identity and providers labels Jan 3, 2019
@Tratcher Tratcher added this to the 3.0.0-preview3 milestone Jan 17, 2019
@muratg
Copy link
Contributor

muratg commented Jan 31, 2019

@danroth27 @mkArtakMSFT is @javiercn the owner for this? Is is to be done in Preview3 or at a later preview?

@danroth27
Copy link
Member

@blowdart

@blowdart
Copy link
Contributor

blowdart commented Jan 31, 2019

When did that happen? It's not identity. When you're back then I need a break down of what the goals were for this feature.

@javiercn
Copy link
Member

javiercn commented Feb 1, 2019

We can validate the options here using "options validation" or just throw an exception, it shouldn't be very costly, but it's very low on priority IMO

@analogrelay
Copy link
Contributor

We should just check the Instance for null-or-empty and throw a useful exception.

@Eilon Eilon added the help wanted Up for grabs. We would accept a PR to help resolve this issue label May 2, 2019
@Eilon Eilon modified the milestones: 3.0.0-preview6, Backlog May 2, 2019
mderriey added a commit to mderriey/AspNetCore that referenced this issue May 9, 2019
@Tratcher Tratcher added the Done This issue has been fixed label May 16, 2019
@Tratcher Tratcher modified the milestones: Backlog, 3.0.0-preview6 May 16, 2019
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed feature-AADIntegration This issue is related to Azure AD integration help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
None yet
Development

No branches or pull requests

9 participants