Move ProtectedBrowserStorage to the shared fx #25557

pranavkm · 2020-09-03T06:04:17Z

SteveSandersonMS · 2020-09-03T08:44:06Z

src/Components/Server/src/DependencyInjection/ComponentServiceCollectionExtensions.cs

            services.AddDataProtection();

+            services.TryAddScoped<ProtectedLocalStorage>();
+            services.TryAddScoped<ProtectedSessionStorage>();


This surprises me a bit. Do we definitely want to include the protected storage services by default?

For non-core functionality like this it seems more idiomatic for the developer to call services.AddProtectedBrowserStorage() (or similar) themselves to opt in.

I suppose that is a bit of an arbitrary hoop we're giving them to jump through, but it does mean they are making more of a conscious choice to plug it into their application.

Perhaps this is a question for API review.

but it does mean they are making more of a conscious choice to plug it into their application.

Users are making that choice by using these types. Having the service registration isn't a significant cost and we have precedence with MVC registering a whole bunch of optional features \ services in DI behind a single gesture.

Previously using these APIs required bringing in an optional dependency (DataProtection), which is no longer the case. I would have felt more strongly about a separate API if this required bringing in more optional services.

Generally our service registration chain in dependencies. In this case, AddProtectedBrowserStorage would only make sense if you had already called AddServerSideBlazor. That would require us to either call AddServerSideBlazor on your behalf or use a builder pattern, both of which seem unnecessary.

The difference is that libraries can start using this without the app developer knowing. I'm not sure if that's something we want, given that this will put data in the browser (even though is protected).

I can imagine it being hard to detect if an app brings in a library that starts making use of this "liberally".

OK, I see the arguments both ways and am fine with either. I think the developer experience is OK either way.

@pranavkm @javiercn If the two of you can come to an agreement on it let's go with that.

I'll let @pranavkm decide. I don't feel strongly either, but I favor having an explicit gesture. We can always change that in the future, but we can't do it the other way around.

SteveSandersonMS · 2020-09-03T08:45:18Z

...omponents/test/E2ETest/Infrastructure/WebDriverExtensions/BasicTestAppWebDriverExtensions.cs

+            var componentType = typeof(TComponent);
+            var componentTypeName = componentType.Assembly == typeof(BasicTestApp.Program).Assembly ?
+                componentType.FullName :
+                componentType.AssemblyQualifiedName;


Interesting. Why is this change needed?

Ah I see it's related to ProtectedBrowserStorageUsageComponent being in the server project, however I'm still unsure of the necessity as per question https://github.com/dotnet/aspnetcore/pull/25557/files#r482820977.

SteveSandersonMS · 2020-09-03T09:00:27Z

src/Components/test/testassets/BasicTestApp/Index.razor

+
+            <option value="Components.TestServer.ProtectedBrowserStorageUsageComponent, @serverAssembly.FullName">Protected browser storage usage</option>
+            <option value="Components.TestServer.ProtectedBrowserStorageInjectionComponent, @serverAssembly.FullName">Protected browser storage injection</option>
+        }


No doubt this is viable as-is, but if you feel like you have time to tweak it a bit more, I'd suggest it would be preferable not to have the @if statement, because then it would be really confusing if you were in the browser UI looking for something in the list that wasn't there because you accidentally loaded the wrong host environment and didn't realise some test cases are only available in some environments. It will also get pretty messy if we keep adding more conditional blocks like this in the future.

It would be much clearer if all the entries were present all the time, but if you picked a type that can't be found, we have an error message like "The type {name} could not be found. It might not be supported when running on this host environment.". This error could be triggered from Index.razor's SelectedComponentType getter, or just inline in the Razor UI logic when SelectedComponentTypeName is nonempty but SelectedComponentType is null.

Also rather than using assembly qualified names, what stops it from working just having the type name (independently of the assembly)? The logic in Index.razor's SelectedComponentType uses Type.GetType - doesn't that look at all the loaded assemblies?

The logic in Index.razor's SelectedComponentType uses Type.GetType - doesn't that look at all the loaded assemblies?

It only works if the type is in the executing assembly which requires the additional work around fully qualifying it. I have this tweaked a bit where we rely on the runtime to fail loudly if the type is unable to be found. Perhaps that would suffice here?

Ah yes, makes sense.

The improvement you've added looks good to me. It's test code - we can always try to streamline it later if we want.

pranavkm · 2020-09-03T15:38:44Z

src/Components/test/testassets/BasicTestApp/Index.razor

+
+            <option value="Components.TestServer.ProtectedBrowserStorageUsageComponent, @serverAssembly.FullName">Protected browser storage usage</option>
+            <option value="Components.TestServer.ProtectedBrowserStorageInjectionComponent, @serverAssembly.FullName">Protected browser storage injection</option>
+        }


The logic in Index.razor's SelectedComponentType uses Type.GetType - doesn't that look at all the loaded assemblies?

It only works if the type is in the executing assembly which requires the additional work around fully qualifying it. I have this tweaked a bit where we rely on the runtime to fail loudly if the type is unable to be found. Perhaps that would suffice here?

pranavkm · 2020-09-03T15:41:04Z

src/Components/test/testassets/TestServer/PrerenderedStartup.cs

            services.AddServerSideBlazor();
            services.AddAuthentication(CookieAuthenticationDefaults.AuthenticationScheme).AddCookie();
-            services.AddSingleton<LazyAssemblyLoader>();
+            services.AddScoped<LazyAssemblyLoader>();


@captainsafia I had trouble running the TestServer app without changing this. AddServerSideBlazor adds a scoped IJSRuntime instance. Our DI system will complain if a singleton service (in this case LazyAssemblyLoader) references a scoped service (IJSRuntime). Do you think there's a regression as a result of my changes?

pranavkm · 2020-09-03T15:51:34Z

src/Components/Server/src/DependencyInjection/ComponentServiceCollectionExtensions.cs

            services.AddDataProtection();

+            services.TryAddScoped<ProtectedLocalStorage>();
+            services.TryAddScoped<ProtectedSessionStorage>();


but it does mean they are making more of a conscious choice to plug it into their application.

Users are making that choice by using these types. Having the service registration isn't a significant cost and we have precedence with MVC registering a whole bunch of optional features \ services in DI behind a single gesture.

Previously using these APIs required bringing in an optional dependency (DataProtection), which is no longer the case. I would have felt more strongly about a separate API if this required bringing in more optional services.

Generally our service registration chain in dependencies. In this case, AddProtectedBrowserStorage would only make sense if you had already called AddServerSideBlazor. That would require us to either call AddServerSideBlazor on your behalf or use a builder pattern, both of which seem unnecessary.

SteveSandersonMS

I know there are still a couple of open questions but I'm fine with what's here and whatever decisions you take on them.

ghost · 2020-09-03T19:31:43Z

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

Pilchie · 2020-09-03T20:27:45Z

Does this have a breaking change impact for any customers that have started using it already?

pranavkm · 2020-09-04T00:44:55Z

@Pilchie the would have to remove the package reference and change namespaces when upgrading from rc1. It will be useful to cover some of these packaging changes (there's one more component that's affected) as an announcement.

Pilchie · 2020-09-04T17:06:36Z

Approved for .NET 5 RC2 pending CI checks, but we will need a breaking change announcement for this.

pranavkm · 2020-09-08T16:02:43Z

Announcement filed: aspnet/Announcements#436

Move ProtectedBrowserStorage to the shared fx

2e90965

SteveSandersonMS reviewed Sep 3, 2020

View reviewed changes

Fixups

ab102d0

pranavkm commented Sep 3, 2020

View reviewed changes

pranavkm marked this pull request as ready for review September 3, 2020 15:55

pranavkm requested a review from a team as a code owner September 3, 2020 15:55

pranavkm added this to the 5.0.0-rc2 milestone Sep 3, 2020

SteveSandersonMS approved these changes Sep 3, 2020

View reviewed changes

pranavkm added the area-blazor Includes: Blazor, Razor Components label Sep 3, 2020

mkArtakMSFT added the Servicing-consider Shiproom approval is required for the issue label Sep 3, 2020

Merge branch 'release/5.0-rc2' into prkrishn/protecteddata

d5f05a7

Pilchie added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Sep 4, 2020

Fixup

5395307

mkArtakMSFT merged commit 6469251 into release/5.0-rc2 Sep 9, 2020

mkArtakMSFT deleted the prkrishn/protecteddata branch September 9, 2020 16:54

Move ProtectedBrowserStorage to the shared fx #25557

Move ProtectedBrowserStorage to the shared fx #25557

Uh oh!

Conversation

pranavkm commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiercn Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 3, 2020

Uh oh!

Pilchie commented Sep 3, 2020

Uh oh!

pranavkm commented Sep 4, 2020

Uh oh!

Pilchie commented Sep 4, 2020

Uh oh!

pranavkm commented Sep 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pranavkm commented Sep 3, 2020 •

edited

Loading

javiercn Sep 3, 2020 •

edited

Loading

SteveSandersonMS Sep 3, 2020 •

edited

Loading