Skip to content

Fix nullable warnings.#362

Merged
pmaytak merged 1 commit into
masterfrom
pmaytak/nullableWarnings
Aug 1, 2020
Merged

Fix nullable warnings.#362
pmaytak merged 1 commit into
masterfrom
pmaytak/nullableWarnings

Conversation

@pmaytak

@pmaytak pmaytak commented Jul 23, 2020

Copy link
Copy Markdown
Collaborator

No description provided.

@pmaytak
pmaytak requested review from jennyf19 and jmprieur July 23, 2020 22:24

string parameterString = string.Join(", ", parameters.Select(p => $"{p.Key}=\"{p.Value}\""));

if (CurrentHttpContext == null)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to do this, as for blazor, this will be null.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Suggestions? Without context, this method's kind of pointless. Will need a workaround for Blazor then, right?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's what we've been working on.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now have a work around for Blazor, @pmaytak
See Managing incremental consent and conditional access

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #380 to address this.

@jmprieur jmprieur left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of questions (and let's not throw if HttpContext is null in TokenAcquisition as it's a valid scenario now, and there is a work around for Blazor)

});
services.AddScoped<IMsalTokenCacheProvider, MsalSessionTokenCacheProvider>();
services.TryAddScoped(provider => provider.GetService<IHttpContextAccessor>().HttpContext.Session);
services.TryAddScoped(provider =>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work with Graph which is a singleton. Is there a possible mitigation?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because session token caches are added with scoped lifetime, they should not be used when TokenAcquisition is also used as a singleton (for example, when using Microsoft Graph SDK).

I added the above note to each method and to the Token cache serialization wiki.

In this method, I don't think we can reliably predict which services are added, since they can be modified in any order.

Maybe if ServiceProvider has some sort of validator or event that we can plug into, we could validate at that time.


string parameterString = string.Join(", ", parameters.Select(p => $"{p.Key}=\"{p.Value}\""));

if (CurrentHttpContext == null)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now have a work around for Blazor, @pmaytak
See Managing incremental consent and conditional access

@pmaytak
pmaytak force-pushed the pmaytak/nullableWarnings branch 2 times, most recently from db6c150 to 2d405a6 Compare July 31, 2020 05:24
@pmaytak

pmaytak commented Jul 31, 2020

Copy link
Copy Markdown
Collaborator Author

@jennyf19 @jmprieur FYI updated this PR.

@jmprieur jmprieur left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pmaytak
LGTM.

@pmaytak
pmaytak force-pushed the pmaytak/nullableWarnings branch from 2d405a6 to cbf790c Compare August 1, 2020 05:09
@pmaytak
pmaytak merged commit c8c0ff9 into master Aug 1, 2020
@pmaytak
pmaytak deleted the pmaytak/nullableWarnings branch August 1, 2020 05:12
jennyf19 added a commit that referenced this pull request Aug 3, 2020
* Fix nullable warnings. (#362)

* fix userflow claims in addAccounToCacheFromAuthCode (#388)

* make aadIssuerValidator public (#385)

* Fixes Azure DevOps issue 1097354 (#387)

Co-authored-by: pmaytak <34331512+pmaytak@users.noreply.github.com>
Co-authored-by: jennyf19 <jeferrie@microsoft.com>
jennyf19 added a commit that referenced this pull request Aug 4, 2020
* initial commit to improve obo

* add test & experiment w/renaming a few private methods

* Jennyf/merge from master (#390)

* Fix nullable warnings. (#362)

* fix userflow claims in addAccounToCacheFromAuthCode (#388)

* make aadIssuerValidator public (#385)

* Fixes Azure DevOps issue 1097354 (#387)

Co-authored-by: pmaytak <34331512+pmaytak@users.noreply.github.com>
Co-authored-by: jennyf19 <jeferrie@microsoft.com>

* pr feedback

Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com>
Co-authored-by: pmaytak <34331512+pmaytak@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants