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

Protected Browser Storage #23553

Merged
merged 14 commits into from
Jul 10, 2020
Merged

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Jul 1, 2020

Summary of the changes

  • Create new package AspNetCore.Components.Web.Extensions
  • Migrated Protected Browser Storage from AspLabs to aspnetcore
  • Wrote E2E tests
  • Added safeguard against using Protected Browser Storage in WebAssembly
  • Modify GetAsync to distinguish between nonexistent keys and keys that map to a default value.

Addresses #18755

public abstract class ProtectedBrowserStorage
{
   protected ProtectedBrowserStorage(string storeName, IJSRuntime jsRuntime, IDataProtectionProvider dataProtectionProvider);
   ValueTask SetAsync(string key, object value);
   ValueTask SetAsync(string purpose, string key, object value);
   ValueTask<ProtectedBrowserStorageResult<TValue>> GetAsync<TValue>(string key);
   ValueTask<ProtectedBrowserStorageResult<TValue>> GetAsync<TValue>(string purpose, string key);
   ValueTask DeleteAsync(string key);
}

public class ProtectedLocalStorage : ProtectedBrowserStorage
{
        public ProtectedLocalStorage(IJSRuntime jsRuntime, IDataProtectionProvider dataProtectionProvider)
}

public readonly struct ProtectedBrowserStorageResult<T>
{
        public bool Success { get; }

        [MaybeNull]
        [AllowNull]
        public T Value { get; }
}

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jul 1, 2020
@MackinnonBuck MackinnonBuck marked this pull request as ready for review July 6, 2020 15:53
@pranavkm pranavkm requested a review from a team July 7, 2020 02:18
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

This looks great to me. Nice one @MackinnonBuck!

I'm happy to approve now, but @pranavkm left a bunch of good comments to be addressed before merging. Also I left a couple of suggestions.

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Looks great!

The only piece of feedback that I have is the same as the one about not needing the JS file.

Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

This looks great. I'd like to get @javiercn's opinion on the data protection caching.

@pranavkm pranavkm added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jul 7, 2020
@ghost
Copy link

ghost commented Jul 7, 2020

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@@ -1,7 +1,7 @@

Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio 15
VisualStudioVersion = 15.0.26124.0
# Visual Studio Version 16
Copy link
Contributor

Choose a reason for hiding this comment

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

@Pilchie @MackinnonBuck tried to add a project using VS and it changed quite a few GUIDs and version numbers. I think he had to use VS (vs dotnet sln add) to create solution folders. Any idea why VS is producing so much diff?

Copy link
Member

Choose a reason for hiding this comment

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

Ah - one of those guids is "original C# project guid, so please autodetect whether to use the old project system, or the new SDK project system", and the other is "force use of the new project system".

I forget which is which, but I think the 9A19103F is the original guid, which VS seems to prefer.

It seems that dotnet sln add uses the FAE04EC0 one to force to the new project system though.

@davkean - does that match your understanding?

If so, we should probably just update them all, and file an issue on dotnet/sdk to change dotnet sln add to use the 9A19103F

@MackinnonBuck MackinnonBuck merged commit 3192da4 into master Jul 10, 2020
@MackinnonBuck MackinnonBuck deleted the t-mabuc/protected-browser-storage branch July 10, 2020 00:10
@pranavkm
Copy link
Contributor

pranavkm commented Jul 13, 2020

  • Make the derived types sealed. Also consider declaring the protected ProtectedBrowserStorage ctor as private protected
  • purpose -> protectionPurpose. Also move it to the end of the parameter list e.g:
ValueTask SetAsync(string key, object value);
ValueTask SetAsync(string key, object value, string protectionPurpose);

@SteveSandersonMS
Copy link
Member

Open question: could we drop the "purpose" parameter altogether and have the framework auto-create a purpose from "key"?

We were concerned that having any developer control over "purpose" risks confusing people. For example it might look as if each "purpose" is a separate key-value collection so you could use the same key multiple times with different purposes, when in fact they would overwrite each other.

@pranavkm
Copy link
Contributor

I was about to draft an email to @GrabYourPitchforks and @javiercn since they had previously commented on this PR. One of the scenarios where an additional purpose might be useful is if there's a server secret that you do not want to send to the browser (for instance something like the user id).

@GrabYourPitchforks
Copy link
Member

Feel free to shoot an email or set up a video chat. :)

@pranavkm pranavkm removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants