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.

Sign up for GitHub

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

Jump to bottom

Upgraded SaasKit to netcoreapp2.0 #96

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Upgraded SaasKit to netcoreapp2.0 #96

wants to merge 5 commits into from

Conversation

ovidiaconescu
Copy link

Upgraded SaasKit to netcoreapp2.0

@ovidiaconescu
Copy link
Author

The main goal was to modify the way builder.UseCookieAuthentication works in v.2.0.
As of this version it is marked as Obsolete: [Obsolete("See https://go.microsoft.com/fwlink/?linkid=845470", true)]

My current mindset is to leverage a new set of Transient services:

services.AddTransient<IConfigureOptions<CookieAuthenticationOptions>, ConfigureMyCookie>();
services.AddTransient<IConfigureOptions<GoogleOptions>, ConfigureMyGoogleCookie>();

implemented in samples/AspNetMvcAuthDemo/Extensions.cs

    internal class ConfigureMyCookie : IConfigureNamedOptions<CookieAuthenticationOptions>
    {
        private readonly HttpContext _httpContext;

        public ConfigureMyCookie(IHttpContextAccessor contextAccessor)
        {
            _httpContext = contextAccessor.HttpContext;
        }

        public void Configure(string name, CookieAuthenticationOptions options)
        {
            var tenantContext = _httpContext.GetTenantContext<AppTenant>();

            if (tenantContext == null)
            {
                options.Cookie.Name = "AspNet.Cookies";
            }
            else
            {
                options.Cookie.Name = $"{tenantContext.Tenant.Id}.AspNet.Cookies";
            }
        }

        public void Configure(CookieAuthenticationOptions options)
            => Configure(Options.DefaultName, options);
    }

and

    internal class ConfigureMyGoogleCookie : IConfigureNamedOptions<GoogleOptions>
    {
        private readonly HttpContext _httpContext;
        private readonly IList<AppTenant> tenants;
        public GoogleOptions myCurrentOptions;

        public ConfigureMyGoogleCookie(
            IHttpContextAccessor contextAccessor, 
            IOptions<MultitenancyOptions> multitenancyOptions)
        {
            _httpContext = contextAccessor.HttpContext;
            tenants = multitenancyOptions.Value.Tenants;
        }

        public void Configure(string name, GoogleOptions options)
        {
            var tenantContext = _httpContext.GetTenantContext<AppTenant>();

            if (tenantContext == null)
                throw new Exception("Google Auth not set.");

            var currentTenant = tenants.Single(t => t.Id == tenantContext.Tenant.Id);

            options.ClientId = currentTenant.GoogleClientId;
            options.ClientSecret = currentTenant.GoogleClientSecret;

            myCurrentOptions = options;
        }

        public void Configure(GoogleOptions options)
            => Configure(Options.DefaultName, options);
    }

This way, every time the Cookie or Google configuration is required, the service overwrites them based on the current tenant from the context.
Let me know what you think @benfoster

This was referenced Mar 6, 2018
Open
Open
Closed
@RickBlouch RickBlouch mentioned this pull request Mar 8, 2018
Open
@ovidiaconescu ovidiaconescu mentioned this pull request Mar 9, 2018
Open
@jonas-stjernquist
Copy link

SO question about this topic: https://stackoverflow.com/q/49244634/294242

@quartilesoftware
Copy link

@ovidiaconescu
This is exactly what I need for a live project. Your changes look good to me. When do you think this could be merged into saaskit:master?

@benfoster
Copy link
Member

benfoster commented Mar 22, 2018 •
edited
Loading

@joeaudette would you be able to review this and give it the all clear? I can then push the packages

@joeaudette
Copy link
Contributor

@benfoster this pr looks good to me.

joeaudette
joeaudette approved these changes Mar 22, 2018
View reviewed changes
Copy link
Contributor

@joeaudette joeaudette left a comment

Choose a reason for hiding this comment

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

looks good

@quartilesoftware
Copy link

@ovidiaconescu - thanks very much for developing this.
@joeaudette and @benfoster - thanks very much for your prompt action on reviewing and merging the branch.

@jonas-stjernquist
Copy link

jonas-stjernquist commented Apr 9, 2018 •
edited
Loading

I'm looking forward to a merge of this PR :-)

@ovidiaconescu Would it be possible to target .NET Standard 2.0 instead of .NET Core 2.0? I have a ASP.NET Core web application using .NET Framework 4.61 which uses the SaasKit library.

Change SaasKit.Multitenancy.csproj file from:
<TargetFramework>netcoreapp2.0</TargetFramework>

To:
<TargetFramework>netstandard2.0</TargetFramework>

@mmillican
Copy link

@joeaudette @benfoster Any idea when this will be merged into master?

@markphillips100
Copy link

Thought I'd give the PR's AspNetMvcAuthDemo a run through to get my head around the options changes.

If I spin up the demo with a breakpoint set in the void Configure(string name, GoogleOptions options) method the debugger breaks the first time the app is launched, due to the initial request to http://localhost:60000.

If I open another tab to http://localhost:60001 the app renders tenant 2 content but the Configure method with the breakpoint doesn't get called. Even if I click the Google login links it's not called either.

When does the Configure method get called for other tenants?

@USPTraining
Copy link

USPTraining commented Apr 16, 2018 •
edited
Loading

@markphillips100 I am also facing the same. the cookie is not getting set per tenant. since the configure method called only once during the launch time. so the cookie got created with first tenant id and shared between the tenants. Did anyone get this working?

@markphillips100
Copy link

Glad it's not just me then ;-)

@markphillips100
Copy link

I haven't delved into how the IOptions are utilised within the AddCookie and AddGoogle but just reading the options doco I'm assuming for this to work the IOptionsSnapshot should be used to allow per-request config. I'm wondering if the aspnetcore security code supports this use case?

@markphillips100
Copy link

So looks like auth handlers are dependent on IOptionsMonitor<> as of 2.0.0 (preview versions were IOptionsSnapshot<>). Don't know if this impacts anything.

I can't find doco to indicate if we need to implement something to force the options to call Configure per-request.

@markphillips100
Copy link

This issue appears to shed some light. Looks like we need to implement an IOptionsMonitor for each auth handler used.

@markphillips100
Copy link

So based on the StackOverflow question referenced in the issue perhaps something like this:

Use a base provider so we don't have to repeat ourselves too much:

    public abstract class TenantOptionsProvider<TOptions> : IOptionsMonitor<TOptions> where TOptions : class
    {
        protected readonly ConcurrentDictionary<(string name, string tenant), Lazy<TOptions>> _cache;
        protected readonly IDataProtectionProvider _dataProtectionProvider;
        protected readonly IHttpContextAccessor _httpContextAccessor;
        protected readonly IEnumerable<IConfigureOptions<TOptions>> _setups;
        protected readonly IEnumerable<IPostConfigureOptions<TOptions>> _postConfigures;

        public TenantOptionsProvider(
            IDataProtectionProvider dataProtectionProvider,
            IHttpContextAccessor httpContextAccessor,
            IEnumerable<IConfigureOptions<TOptions>> setups,
            IEnumerable<IPostConfigureOptions<TOptions>> postConfigures)
        {
            _cache = new ConcurrentDictionary<(string name, string tenant), Lazy<TOptions>>();
            _dataProtectionProvider = dataProtectionProvider;
            _httpContextAccessor = httpContextAccessor;
            _setups = setups;
            _postConfigures = postConfigures;
        }

        public virtual TOptions CurrentValue => Get(Options.DefaultName);

        public virtual TOptions Get(string name)
        {
            // This sample uses the path base as the tenant.
            // You can replace that by your own logic.
            var tenantContext = _httpContextAccessor.HttpContext.GetTenantContext<AppTenant>();
            if (tenantContext == null)
                throw new Exception("No tenant context found");

            string tenant = tenantContext.Tenant.Id;

            return _cache.GetOrAdd((name, tenant), _ => new Lazy<TOptions>(() =>
            {
                var options = Activator.CreateInstance<TOptions>();
                return Create(options, name, tenant);
            })).Value;
        }

        protected virtual TOptions Create(TOptions options, string name, string tenant)
        {
            // Run the other initializers (which is normally done by OptionsFactory when
            // using the default OptionsMonitor implementation provided by ASP.NET Core).
            foreach (var setup in _setups)
            {
                if (setup is IConfigureNamedOptions<TOptions> namedSetup)
                {
                    namedSetup.Configure(name, options);
                }
                else if (name == Options.DefaultName)
                {
                    setup.Configure(options);
                }
            }
            foreach (var post in _postConfigures)
            {
                post.PostConfigure(name, options);
            }

            return options;
        }

        public IDisposable OnChange(Action<TOptions, string> listener) => null;
    }

and then create providers for the options we need. Firstly for CookieAuthenticationOptions:

    public class CookieTenantOptionsProvider : TenantOptionsProvider<CookieAuthenticationOptions>
    {
        private readonly IList<AppTenant> tenants;

        public CookieTenantOptionsProvider(
            IOptions<MultitenancyOptions> multitenancyOptions,
            IDataProtectionProvider dataProtectionProvider,
            IHttpContextAccessor httpContextAccessor,
            IEnumerable<IConfigureOptions<CookieAuthenticationOptions>> setups,
            IEnumerable<IPostConfigureOptions<CookieAuthenticationOptions>> postConfigures) : base(dataProtectionProvider, httpContextAccessor, setups, postConfigures)
        {
            tenants = multitenancyOptions.Value.Tenants;
        }

        protected override CookieAuthenticationOptions Create(CookieAuthenticationOptions options, string name, string tenant)
        {
            // Create a tenant-specific data protection provider to ensure cookie
            // can't be read/decrypted by the other tenants.
            options.DataProtectionProvider = _dataProtectionProvider.CreateProtector(tenant);

            // You can populate the "options" instance using data
            // associated with the tenant (e.g stored in a database).
            var currentTenant = tenants.Single(t => t.Id == tenant);

            options.Cookie.Name = $"{currentTenant.Id}.AspNet.Cookies";

            return base.Create(options, name, tenant);
        }
    }

and then for GoogleOptions:

    public class GoogleTenantOptionsProvider : TenantOptionsProvider<GoogleOptions>
    {
        private readonly IList<AppTenant> tenants;

        public GoogleTenantOptionsProvider(
            IOptions<MultitenancyOptions> multitenancyOptions,
            IDataProtectionProvider dataProtectionProvider,
            IHttpContextAccessor httpContextAccessor,
            IEnumerable<IConfigureOptions<GoogleOptions>> setups,
            IEnumerable<IPostConfigureOptions<GoogleOptions>> postConfigures) : base(dataProtectionProvider, httpContextAccessor, setups, postConfigures)
        {
            tenants = multitenancyOptions.Value.Tenants;
        }

        protected override GoogleOptions Create(GoogleOptions options, string name, string tenant)
        {
            // Create a tenant-specific data protection provider to ensure authorization codes,
            // access tokens and refresh tokens can't be read/decrypted by the other tenants.
            options.DataProtectionProvider = _dataProtectionProvider.CreateProtector(tenant);

            // You can populate the "options" instance using data
            // associated with the tenant (e.g stored in a database).
            var currentTenant = tenants.Single(t => t.Id == tenant);

            options.ClientId = currentTenant.GoogleClientId;
            options.ClientSecret = currentTenant.GoogleClientSecret;

            return base.Create(options, name, tenant);
        }
    }

and instead of registering IConfigureOptions in Startup, register IOptionsMonitor instances:

            services.AddSingleton<IOptionsMonitor<CookieAuthenticationOptions>, CookieTenantOptionsProvider>();
            services.AddSingleton<IOptionsMonitor<GoogleOptions>, GoogleTenantOptionsProvider>();

@quartilesoftware
Copy link

Mark,

Thanks very much for that great work. Like you and several others, I have a database per tenant. Net Core 2.0 project which resolves tenants by sub-domain name. But that also needs auth cookies per tenant to function.

My project is live, with deadlines, and I have been assuming for a few weeks that while I pressed ahead on the application functionality, that SaasKit was going to be upgraded with this pull request so that auth cookies could be handled per tenant/sub-domain.

@benfoster @joeaudette I know we all have multiple priorities, but so that we can plan our own projects and deadlines, are you please able to let us know roughly when you think SaasKit could be able to handle auth cookies per tenant/sub-domain?

@markphillips100
Copy link

markphillips100 commented Apr 16, 2018 via email

@benfoster
Copy link
Member

benfoster commented Apr 16, 2018 via email

@markphillips100
Copy link

Thanks Ben. Any chance it can include @devJ0n suggestion about making it NetStandard 2 please?

@quartilesoftware
Copy link

quartilesoftware commented Apr 16, 2018 via email

@kevinchalet
Copy link

@markphillips100 JSYK, I updated my SO answer to use IOptionsFactory, which should make the code nicer.

@rajapc
Copy link

rajapc commented Apr 17, 2018

@markphillips100 I have tried your solution and it is creating separate cookie for each tenant. Thanks for that solution. Still, if a user is authenticated in one tenant, he is automatically authenticated in to other tenant as well. How to restrict authentication based on tenant? Any idea? Thanks

@markphillips100
Copy link

@PinpointTownes thanks very much for the update, and for posting your original code.

@quartilesoftware
Copy link

@rajapc @markphillips100 @benfoster

Thanks.

For me, the first issue I raised, which is the only issue I've ever had, and which presently prevents me using Saaskit with .Net Core 2.0 is the same one @rajapc summarises:

"If a user is authenticated in one tenant, he is automatically authenticated in to other tenant as well. How to restrict authentication based on tenant? "

If an upgrade can provide that fundamental authentication per tenant, I think I'd have a perfect solution from Saaskit.

ben-foster-cko
ben-foster-cko suggested changes Apr 17, 2018
View reviewed changes
samples/AspNetMvcAuthDemo/Startup.cs Outdated

builder.UseAuthentication();

/////////////--------- to delete
Copy link

Choose a reason for hiding this comment

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

If this code is no longer required please remove it from the sample

@ben-foster-cko
Copy link

@quartilesoftware @rajapc if you are hosting these sites under the same domain and not setting the cookie domain (e.g. on localhost) then the user will be authenticated to both domains. This is standard browser behavior.

@ovidiaconescu please can you target the dev branch. Can you also confirm if there is anything else to go into this PR, based on the solution provided by @markphillips100 or can a separate PR be raised for that.

As I'm sure you're aware, I don't have much time for SaasKit these days so if you need a solution you're better off raising a PR.

Let me know and I'll get the necessary packages on Nuget.

ben-foster-cko
ben-foster-cko suggested changes Apr 17, 2018
View reviewed changes
Copy link

@ben-foster-cko ben-foster-cko left a comment

Choose a reason for hiding this comment

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

Please target netstandard2.0 not netcoreapp.20

@quartilesoftware
Copy link

@benfoster

Thanks Ben,
I was testing on a remote server with multiple sub-domains, such as fred.myapp.com; susan.myapp.com etc. Unless I’ve overlooked something in testing, or in startup.cs, the cookie per domain was not working then. If “Fred” logged in, he was also automatically authenticated to “Susan”.

@ben-foster-cko
Copy link

ben-foster-cko commented Apr 17, 2018 •
edited
Loading

@quartilesoftware I believe you'll still need to set the cookie domain as by default the cookie will be set for myapp.com not fred.myapp.com. If this is still an issue, and to avoid polluting this PR, please can you open a new issue.

@quartilesoftware
Copy link

@ben-foster-cko

OK. Will do.

@markphillips100
Copy link

@quartilesoftware is there an issue you want to continue the discussion under? I'd like to give some more feedback but as @ben-foster-cko rightly said this is probably not the place.

@quartilesoftware
Copy link

quartilesoftware commented Apr 18, 2018 via email

@rajapc
Copy link

rajapc commented Apr 18, 2018

@markphillips100 very big thanks to you. With your code we were able to create separate cookie per domain.

@quartilesoftware @rajapc if you are hosting these sites under the same domain and not setting the cookie domain (e.g. on localhost) then the user will be authenticated to both domains. This is standard browser behavior

@ben-foster-cko I was using different domain names only. Event though I was testing in localhost, I was using different domain names using Netsh ip redirect and hosts file dns names.
But the problem was, I was not setting the cookie domain name explicitly. That was causing the issue. Once I set the domain name for Cookie explicitly through the code, it is started working perfect.

Thanks to both @ben-foster-cko and @markphillips100

@ovidiaconescu
Copy link
Author

@markphillips100 Target changed to netstandard2.0, thanks for the info.
@ben-foster-cko I think the IOptionsMonitor solution can be a separate PR.

@ovidiaconescu ovidiaconescu changed the base branch from master to dev April 22, 2018 22:34
@ovidiaconescu
Copy link
Author

@ben-foster-cko Rebased into dev, but it seems there are some conflicts.

@AndrewTriesToCode
Copy link

@markphillips100 very nice solution.

I am working on my own side project for multitenancy and I found a good solution to be to implement just IOptionsCache and making it work within similarly to what you have there where you create and cache each tenant's version of the options.

@quartilesoftware
Copy link

@rajapc , @markphillips100

Hi - I'm returning to this after a break, and think I'm missing something somewhere. When you say: " Once I set the domain name for Cookie explicitly through the code, it is started working perfect. ", can you please share exactly how and where you are doing that? If it's better, I'll open a new issue.

@TomBeckett
Copy link

TomBeckett commented May 15, 2018 •
edited
Loading

Any update on when this may be merged (master or dev)? @benfoster @ovidiaconescu

EDIT : I've hopefully fixed any conflicts on a branch here. Happy to open a new PR or it can be merged into this one (whatever is easier) 👍

@ovidiaconescu
Copy link
Author

Any progress on the merge?

@grayniall
Copy link

Any chance this is going to happen?

@flauberjp
Copy link

@ovidiaconescu @ben-foster-cko what is missing here?

@chernihiv
Copy link

What is the status of this PR? Is it going to be merged?

@project-orcon
Copy link

Bit crazy to ask, but can this please be merged :D, please please please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ben-foster-cko ben-foster-cko ben-foster-cko requested changes

@joeaudette joeaudette joeaudette approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet