Remove ConfigureGlobalOptions or SetApplicationName from DataProtectionConfigurationExtensions

[javiercn]

There are 3 ways right now to set the application name.

services.AddDataProtection(options => options.ApplicationDiscriminator = "name");
services.AddDataProtection().SetApplicationName("name");
services.AddDataProtection().ConfigureGlobalOptions(options => options.ApplicationDiscriminator = "name")

We could get rid of the two last ones or just keep one of the two for non DI enabled scenarios. (A non DI enabled scenario would look like this)

DataProtectionProvider.Create(directory, builder => { builder.SetApplicationName("name")}) or
DataProtectionProvider.Create(
    directory, 
    builder => { 
        builder.ConfigureGlobalOptions(options => options.ApplicationDiscriminator = "name")
    })

If we want to get rid of both methods, on a non DI scneario, the user only needs to write

DataProtectionProvider.Create(
    directory, 
    builder => { 
        builder.Services.Configure<DataProtectionGlobalOptions>(options => options.ApplicationDiscriminator = "name")
    })

builder.Services.Configure(options => options.ApplicationDiscriminator = "name")

In my opinion this is not crazy and we can just get rid of those two methods.

[muratg]

Seems reasonable to me. @Eilon @rynowak @lodejard Thoughts on this one?

[blowdart]

cough

I strongly suggest there is a way to set the application name globally that is easy to discover. Neither of the fixes look easy.

[lodejard]

@blowdart do you have a recommendation?

[blowdart]

Well right now, if you read the documentation you'll see there's a 4th way, we say

services.AddDataProtection();
services.ConfigureDataProtection(configure =>
{
    configure.SetApplicationName("my application");
});

That was easy, but I presume it doesn't stay in step with the new config madness and the equivalent is

services.AddDataProtection().SetApplicationName("name");

?

[javiercn]

@blowdart Yes, that's an option, but in that case, what would be the benefit of having an overload with options? The only thing you can configure there is the application name.

I consider more discoverable the options overload due to the fact that is the only thing you can configure in the global options object. SetApplicationName gets buried with the rest of configuration methods. That said, I don't see any reason for application name to hold that distinctive status (being part of options vs being a method on the builder like the rest). If we keep the method (SetApplicationName) would we consider removing the global options overload? If we add more things to the global options in the future, would we create an extension method for each detail we would like to configure there? Should other configurable elements that aren't part of the global options object be part of it?

I think that keeping things simple (in the options object) puts us in a better position moving forward. If it's a piece of data we want to configure, it should be part of the options object and configured in the initial lambda, '''options => options.Property = value''' if it requires adding extra services it should be a method on the builder. Services that require access to the configuration data should ask for the configuration object through DI. Thoughts? @lodejard @Eilon

[blowdart]

It's optional to configure. I presume the autogeneration pieces won't change?

(You will of course be submitting a PR to the docs right?)

[javiercn]

@blowdart Yes, I will fix the docs for everything and make you and @danroth27 happy 😄

[javiercn]

What we decided is to keep SetApplicationName and remove ConfigureGlobalServices

[Eilon]

@javiercn can you mark this issue as Done, plus post a announcement regarding this?

[guardrex]

@javiercn

[EDIT] This seems to work ...

services.AddDataProtection(options => options.ApplicationDiscriminator = "app_name");

Ping me if that's incorrect.

I can't seem to get SetApplicationName. This no longer seems to work ...

services.AddDataProtection(configure =>
{
    configure.SetApplicationName("app_name");
});

... and I've tried several other approaches, e.g. ...

services.AddDataProtection().SetApplicationName("app_name");

without luck. How are we doing this now?

I'm on CI nightly with latest dotnet cli ...

Microsoft.AspNetCore.DataProtection/1.0.0-rc2-20270

[javiercn]

Hi, sorry for the delay. Here is the announcement post that describes the changes. aspnet/Announcements#159

[javiercn]

Both options are valid for configuring the application discriminator. In general, functionality from DataProtectionConfiguration has moved to extension methods on IDataProtectionBuilder as we do elsewere in the framework. So the pattern now is

services
    .AddDataProtection()
    .SetApplicationName("...")
    .AddKeyManagementOptions(...)

Let me know if you have any further issue.

[guardrex]

@javiercn

I tried ...

services.AddDataProtection().SetApplicationName("my_app");

... and receive ...

c:\my_app\Program.cs(99,50): error CS1061: 'IDataProtectionBuilder' does not 
contain a definition for 'SetApplicationName' and no extension method 
'SetApplicationName' accepting a first argument of type 'IDataProtectionBuilder' 
could be found (are you missing a using directive or an assembly reference?)

It seems to be happy with ...

services.AddDataProtection(options => options.ApplicationDiscriminator = "my_app");

... so I'm on an old package?

"Microsoft.AspNetCore.DataProtection/1.0.0-rc2-20270"

[javiercn]

https://github.com/aspnet/DataProtection/blob/dev/src/Microsoft.AspNetCore.DataProtection/DataProtectionBuilderExtensions.cs#L35

Both changes went in at the same time. Maybe you are missing an using?

I've checked my nuget package cache and 1.0.0-rc2-20269 contains the changes. You can search for DataProtectionBuilderExtensions on the assembly to be sure.

Let me know if you still run into issues.

[guardrex]

@javiercn ok ... I'll see where I've gone wrong. Thanks.

[jonas-stjernquist]

@guardrex I'm facing same issue, did you found the reason why the following didn't work?
services.AddDataProtection().SetApplicationName("my_app");

[guardrex]

I did get it working, but I don't exactly recall what did the trick. As @javiercn said, I think I was missing a using ... maybe it was just using Microsoft.AspNetCore.DataProtection;.