Added Entity Framework Core backed IXmlRepository with tests and sample #303

dansward · 2018-04-02T16:19:29Z

No description provided.

dnfclas · 2018-04-02T16:19:41Z

All CLA requirements met.

Eilon · 2018-04-02T16:25:48Z

@dansward is there an issue associated with this PR that describes the scenario? If not, could you open a new issue at https://github.com/aspnet/Home/issues and describe the details? Thanks!

dansward · 2018-04-02T16:32:24Z

@Eilon A related issue exists. Create SQL XmlRepository #2505, should I create one specifically for EF or will that one work?

Eilon · 2018-04-02T17:02:42Z

@dansward I think that's probably sufficient. I'll let the experts in this space evaluate the submission. Thanks!

Tagging @muratg @blowdart to evaluate.

blowdart · 2018-04-02T17:04:36Z

...ore.DataProtection.EntityFrameworkCore/EntityFrameworkCoreDataProtectionBuilderExtensions.cs

+namespace Microsoft.AspNetCore.DataProtection
+{
+    /// <summary>
+    /// Contains Redis-specific extension methods for modifying a <see cref="IDataProtectionBuilder"/>.


Change mention of Redis :)

blowdart · 2018-04-02T17:06:43Z

src/Microsoft.AspNetCore.DataProtection.EntityFrameworkCore/Key.cs

+    /// <summary>
+    /// Code first model used by <see cref="EntityFrameworkCoreXmlRepository"/>.
+    /// </summary>
+    public class Key


As this is never going to be typed in full by the user anywhere a more specific name would be better.

DataProtectionKey? Should the DbContext class be renamed to match as well, DataProtectionKeyStore?

blowdart · 2018-04-02T17:08:05Z

And adding @natemcmaster

Fixed comment on extension method: replaced Redis with EntityFrameworkCore

natemcmaster · 2018-04-02T23:18:24Z

Hey @dansward, thanks for taking an initial stab at this. We haven't yet nailed down a design for the API and expected usage of storing keys in a SQL database. We're going to take a look at dotnet/aspnetcore#2505 in our triage on Friday, and I'll get back to you then.

…tion more natural. Updated EFCore sample and tests to use new extension method.

natemcmaster · 2018-04-06T19:26:12Z

We discussed this in triage. First of all, thanks. We've been thinking of doing dotnet/aspnetcore#2505 for a while. This was a good nudge to start the conversation again.

We're going to leave this PR open for now for two reasons.

This feature isn't something we can commit to shipping the 2.1.0 timeframe. We want to leave enough time for due diligence on the design and testing, and 2.1.0 is locking down soon.
We want to make sure the design of the API satisfies a few requirements that I don't think this currently implementation has. Those requirements are a bit vague still -- we are going to have a conversation with the EF Core team first -- but when we have them, I'll list those requirements in Create SQL XmlRepository for storing Data Protection keys dotnet/aspnetcore#2505. If you're open to working with us on that design and willing to update this PR to meet those, we may be able to take this PR as the starting point for the feature.

Thanks,
Nate

dansward · 2018-04-06T20:13:35Z

@natemcmaster Sounds good. I'll wait for the update to aspnet/Home#2505.

ekarlso · 2018-06-24T23:13:40Z

Any way I can get migrations working with this?

I tried to do:

                .PersistKeysToEntityFrameworkCore(options =>
                {
                    options.KeyStoreOptionsBuilderAction = builder =>
                    {
                        var dpapiMigrationsAssembly = typeof(KeyStore).GetTypeInfo().Assembly.GetName().Name;
                        builder.UseNpgsql(idsrvKeysSqlConnectionString, b => b.MigrationsAssembly(dpapiMigrationsAssembly));
                    };
                })```

ekarlso · 2018-06-26T21:50:09Z

Anyone wanna play in on this? @dansward

dansward · 2018-06-27T12:03:22Z

Will put together an example or fix it if I can't as soon as I have a few minutes. But, I'd try putting migrations in your assembly first.

…

________________________________ From: Endre Karlson <[email protected]> Sent: Tuesday, June 26, 2018 5:50:11 PM To: aspnet/DataProtection Cc: dan.s.ward; Mention Subject: Re: [aspnet/DataProtection] Added Entity Framework Core backed IXmlRepository with tests and sample (#303) Anyone wanna play in on this? @dansward<https://github.com/dansward> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#303 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAKDpqvrcUCInqSLnRwbe5eE4ruMZGjhks5uAqyTgaJpZM4TDu7J>.

[auto-updated: dependencies]

… file (aspnet#308)

natemcmaster · 2018-07-06T21:06:06Z

Hey @dansward, just wanted to let you know I haven't forgotten about this. Thanks for being patient. Our master branch is open for 3.0 work now. DataProtection is one of the dozens of projects I have to find time to support, so I appreciate your help.

What are your thoughts on this comment? dotnet/aspnetcore#2505 (comment). I think @ekarlso's comment pointed out one of the things we should discuss more - how can we help users create/manage the database to back this repository?

natemcmaster · 2018-07-06T21:15:52Z

Just tossing out an idea. One way I thought we might implement is by providing an API like this:

    public static class EtityFrameworkDataProtectionExtensions
    {
        public static IDataProtectionBuilder PersistKeysToDbContext<TContext>(this IDataProtectionBuilder builder)
            where TContext: DbContext, IDataProtectionKeysDbSet
        {
            /// implementation ...
            return builder;
        }
    }

    public interface IDataProtectionKeysDbSet
    {
        DbSet<DataProtectionKeys> DataProtectionKeys { get; }
    }

    public class DataProtectionKeys
    {
        public long Id { get; set; }
        public string Content { get; set; }
    }

I would have to play with an implementation, but my thinking here is that this gives devs the option to use their existing DbContext type and just add a new table, but also gives them flexibility to keep keys in a separate DbContext if they wish.

Also, we can create the DbContext instance through dependency injection, so devs only have to configure their connection strings in EF Core once

[auto-updated: dependencies]

Updated test and sample projects

dansward · 2018-07-09T04:05:33Z

@natemcmaster, that worked really well. Let me know if this meets the requirements.

natemcmaster

@divega @bricelam can you also take a look at this? I want someone from EF to take a look, too. My memory is a bit rusty on some the EF Core workings, so want to make sure what I've said below is still accurate.

@dansward - this is a really good start. I like the API shape and I think this is headed in the right direction. My comments are mostly about implementation details now. Also, I've updated your PR to target release/2.2 since the master branch now targets 3.0, which won't ship for a while.

Thanks!
Nate

natemcmaster · 2018-07-11T17:55:45Z

src/Microsoft.AspNetCore.DataProtection.EntityFrameworkCore/DataProtectionKey.cs

+        /// <summary>
+        /// The friendly name of the <see cref="DataProtectionKey"/>.
+        /// </summary>
+        [Key]


The current contract for IXmlRepository specifies that friendly name is optional. If specified, it should be unique, but it may be null. We should probably add a property instead, such as public int Id { get; set; }.

@divega can you refresh my memory - do EF model conventions apply to internal properties?

natemcmaster · 2018-07-11T23:40:26Z

...AspNetCore.DataProtection.EntityFrameworkCore/EntityFrameworkCoreDataProtectionExtensions.cs

+        {
+            builder.Services.Configure<KeyManagementOptions>(options =>
+            {
+                options.XmlRepository = new EntityFrameworkCoreXmlRepository<TContext>(builder.Services.BuildServiceProvider().GetRequiredService<TContext>());


I think this may cause problems with scope. Typically, DbContext is a scoped service, but this would required a DbContext to be a singleton.

natemcmaster · 2018-07-11T23:41:19Z

src/Microsoft.AspNetCore.DataProtection.EntityFrameworkCore/EntityFrameworkCoreXmlRepository.cs

+
+        /// <inheritdoc />
+        public IReadOnlyCollection<XElement> GetAllElements()
+            => _context.DataProtectionKeys.Select(key => XElement.Parse(key.Xml)).ToList().AsReadOnly();


Let's be gentle and use TryParse instead. If the database has malformed xml, we should skip loading that key and log a warning.

Also, let's make this a notracking query. We don't need to make changes to the entities.

natemcmaster · 2018-07-11T23:44:12Z

src/Microsoft.AspNetCore.DataProtection.EntityFrameworkCore/EntityFrameworkCoreXmlRepository.cs

+        /// Creates a new instance of the <see cref="EntityFrameworkCoreXmlRepository{TContext}"/>.
+        /// </summary>
+        /// <param name="context">The context used to store instances of <see cref="DataProtectionKey"/></param>
+        public EntityFrameworkCoreXmlRepository(TContext context) => _context = context ?? throw new ArgumentNullException(nameof(context));


Instead of taking an instance of TContext, we should take a service which can construct a new instance of the DbContext. In earlier versions of EFCore, there was an IDbContextFactory<TContext> service you could use. I don't remember if that is still around in EF Core 2.0 (cc @divega).

Also, EntityFrameworkCoreXmlRepository is a singleton, but each time it invokes an interaction with the database, we should create a new instance of DbContext.

Don't use IDbContextFactory. It was only for design time. We renamed it in 2.0 to reflect that.

When are keys retrieved? Once during app start? During a request?

If it's just once, just creating a scope here might be best.

natemcmaster · 2018-07-11T23:45:26Z

src/Microsoft.AspNetCore.DataProtection.EntityFrameworkCore/EntityFrameworkCoreXmlRepository.cs

+            {
+                _context.DataProtectionKeys.Add(newKey);
+            }
+            _context.SaveChanges();


Can you add a logging statement to this method before you call SaveChanges? It only needs to be LogLevel.Debug that says something like "Saving key {friendlyName} to {dbcontextname}".

natemcmaster · 2018-07-11T23:48:29Z

src/Microsoft.AspNetCore.DataProtection.EntityFrameworkCore/IDataProtectionKeyContext.cs

@@ -0,0 +1,13 @@
+using Microsoft.EntityFrameworkCore;


Formatting nit: can you apply the standard file header to all .cs files?

natemcmaster · 2018-07-11T23:50:50Z

...otection.EntityFrameworkCore.Test/EntityFrameworkCoreDataProtectionBuilderExtensionsTests.cs

+                .AddDbContext<DataProtectionKeyContext>()
+                .AddDataProtection()
+                .PersistKeysToDbContext<DataProtectionKeyContext>();
+            var serviceProvider = serviceCollection.BuildServiceProvider();


Can you change this to .BuildServiceProvider(validateScopes: true)? This typically helps us find issues in the CI services.

bricelam · 2018-07-12T17:09:17Z

src/Microsoft.AspNetCore.DataProtection.EntityFrameworkCore/IDataProtectionKeyContext.cs

+    public interface IDataProtectionKeyContext
+    {
+        /// <inheritdoc />
+        DbSet<DataProtectionKey> DataProtectionKeys { get; }


Requiring a DbSet<DataProtectionKey> on the DbContext seems a bit rigid. Could just use DbContext.Set<DataProtectionKey>() instead.

bricelam · 2018-07-12T17:10:39Z

src/Microsoft.AspNetCore.DataProtection.EntityFrameworkCore/IDataProtectionKeyContext.cs

+    /// </summary>
+    public interface IDataProtectionKeyContext
+    {
+        /// <inheritdoc />


Inherit docs from where? 😉

Added exception handling and logging for malformed key xml Changed to NoTracking queries Added logging when saving new key Added standard file headers

dansward · 2018-08-27T15:30:43Z

@natemcmaster @bricelam
I made most of the changes requested. I did not remove the interface but am using DbContext.Set in the repository.

I'm not sure how to fix the conflicts. I hate to waste your time but any help would be appreciated.

natemcmaster · 2018-08-29T21:04:24Z

Thanks for doing the initial legwork to get this going @dansward. I've pushed a commit to your branch to resolve the conflicts. I'm going to merge this in to a staging branch while I do a few more tweaks. Assuming I don't run into other issues, this will be in the 2.2 Preview 2 release.

Added Entity Framework backed XmlRepository with tests and sample

f470d4a

blowdart reviewed Apr 2, 2018

View reviewed changes

Daniel Ward added 2 commits April 2, 2018 13:21

Fixed target frameworks in EFCore sample project

c6e216e

Fixed comment on extension method: replaced Redis with EntityFrameworkCore

Renamed test method: Replaced Redis with EntityFrameworkCore

8b39332

Added new EFCore extension method and options class to make configura…

7729eb8

…tion more natural. Updated EFCore sample and tests to use new extension method.

Nate McMaster added 2 commits June 28, 2018 16:48

Bumping version from 2.2.0 to 3.0.0

befdea1

Merge branch 'release/2.2' into dev

a3484f4

natemcmaster changed the base branch from dev to master July 2, 2018 17:57

Nate McMaster (automated) and others added 4 commits July 2, 2018 12:40

[automated] Change default branch to master

19aa1bf

Update dependencies.props

1962967

[auto-updated: dependencies]

Merge branch 'release/2.2'

3c55806

Update README.md - Added Google Cloud Storage provider link to readme…

80a2a62

… file (aspnet#308)

Daniel Ward and others added 4 commits July 8, 2018 14:47

Merge remote-tracking branch 'upstream/master' into dev

4437f71

Update dependencies.props

8037e75

[auto-updated: dependencies]

Merge branch 'master' into dev

80d5da7

Changed API to permit the DbContext to be injected

d782eca

Updated test and sample projects

Pulled updated dependencies

f1665b3

natemcmaster changed the base branch from master to release/2.2 July 11, 2018 23:51

natemcmaster reviewed Jul 11, 2018

View reviewed changes

bricelam reviewed Jul 12, 2018

View reviewed changes

natemcmaster mentioned this pull request Aug 18, 2018

Key material should be able to be stored in a database dotnet/aspnetcore#2528

Closed

Added Id to DataProtectionKey entity

0359f15

Added exception handling and logging for malformed key xml Changed to NoTracking queries Added logging when saving new key Added standard file headers

dansward and others added 2 commits August 27, 2018 11:53

Fixing conflicts

f53aa48

Merge branch 'release/2.2' into pr303

83c9648

natemcmaster changed the base branch from release/2.2 to feature/efcore-provider August 29, 2018 20:56

Revert unintentional changes to infrastructure

c9b4ead

natemcmaster merged commit 8f7d995 into aspnet:feature/efcore-provider Aug 29, 2018

natemcmaster mentioned this pull request Aug 30, 2018

Create SQL XmlRepository for storing Data Protection keys dotnet/aspnetcore#2505

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Entity Framework Core backed IXmlRepository with tests and sample #303

Added Entity Framework Core backed IXmlRepository with tests and sample #303

dansward commented Apr 2, 2018

dnfclas commented Apr 2, 2018 •

edited

Loading

Eilon commented Apr 2, 2018

dansward commented Apr 2, 2018 •

edited

Loading

Eilon commented Apr 2, 2018

blowdart Apr 2, 2018

blowdart Apr 2, 2018

dansward Apr 2, 2018

blowdart commented Apr 2, 2018

natemcmaster commented Apr 2, 2018

natemcmaster commented Apr 6, 2018

dansward commented Apr 6, 2018

ekarlso commented Jun 24, 2018 •

edited

Loading

ekarlso commented Jun 26, 2018

dansward commented Jun 27, 2018 via email

natemcmaster commented Jul 6, 2018

natemcmaster commented Jul 6, 2018

dansward commented Jul 9, 2018

natemcmaster left a comment

natemcmaster Jul 11, 2018

natemcmaster Jul 11, 2018

natemcmaster Jul 11, 2018

natemcmaster Jul 11, 2018

bricelam Jul 12, 2018 •

edited

Loading

bricelam Jul 12, 2018

natemcmaster Jul 11, 2018

natemcmaster Jul 11, 2018

natemcmaster Jul 11, 2018

bricelam Jul 12, 2018

bricelam Jul 12, 2018

dansward commented Aug 27, 2018

natemcmaster commented Aug 29, 2018

Added Entity Framework Core backed IXmlRepository with tests and sample #303

Added Entity Framework Core backed IXmlRepository with tests and sample #303

Conversation

dansward commented Apr 2, 2018

dnfclas commented Apr 2, 2018 • edited Loading

Eilon commented Apr 2, 2018

dansward commented Apr 2, 2018 • edited Loading

Eilon commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blowdart commented Apr 2, 2018

natemcmaster commented Apr 2, 2018

natemcmaster commented Apr 6, 2018

dansward commented Apr 6, 2018

ekarlso commented Jun 24, 2018 • edited Loading

ekarlso commented Jun 26, 2018

dansward commented Jun 27, 2018 via email

natemcmaster commented Jul 6, 2018

natemcmaster commented Jul 6, 2018

dansward commented Jul 9, 2018

natemcmaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bricelam Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dansward commented Aug 27, 2018

natemcmaster commented Aug 29, 2018

dnfclas commented Apr 2, 2018 •

edited

Loading

dansward commented Apr 2, 2018 •

edited

Loading

ekarlso commented Jun 24, 2018 •

edited

Loading

bricelam Jul 12, 2018 •

edited

Loading