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

Implements Public Access in netcore #10137

Merged
merged 30 commits into from
Apr 20, 2021

Conversation

Shazwazza
Copy link
Contributor

@Shazwazza Shazwazza commented Apr 15, 2021

This is a pretty hefty PR since I found a few things causing some issues.

  • The NotificationFactory for cache refreshers doesn't work for null parameters, this is because the underlying ActivatorUtilities doesn't want to work with null parameters so for this particular instance we use string.Empty. This isn't going to cause problems for our use case
  • Adds extensions for AddNotificationHandler to be for both UmbracoBuilder and ServiceCollection
  • Moves all Identity models and services into a single namespace/folders: Umbraco.Cms.Core.Security
  • Makes IPublishedRouter be re-routable for any IPublishedContent or none (null)
  • Implements IPublicAccessChecker
  • New PublicAccessMiddleware to deal with public access checking and rules - this actually fixes/enhances what we used to have
  • Migrates necessary methods from MembershipHelper to IMemberManager
  • Moves public access logic into separate controller PublicAccessController (out of ContentController)
  • Fixes up and cleans up a bunch of the umbraco builder and service collection extension methods, file names, locations of logic. Ensures that members identity is called in both front-end and back office scenarios.
  • Refactors how the startup works
    • No more IStartupFilter for calling UseUmbraco this will be problematic
    • We have one IStartupFilter purely for just capturing the service locator
    • New IUmbracoPipelineFilter with UmbracoPipelineOptions which allows for developers to register any custom IUmbracoPipelineFilter to execute during the umbraco boot phases: PrePipeline (before our core middlewares), PostPipeline (after our core middlewares) and Endpoints (after all middlewares and just before we start adding endpoints). These 3 phases are important and allows developers (and ourselves) to be able to insert middlewares where necessary since once endpoints are declared (and if they are matched), those are terminating middlewares. This new setup also allows a developer to be able to the list of registered IUmbracoPipelineFilter which will all have a name and/or an implementing type.
    • We have a new IUmbracoEndpointBuilder which is used specifically for our extension methods that add endpoint routes. This makes it impossible for developers to call those extension methods out of order. I've also named the extensions that add endpoints clear that they are adding endpoints since that is important to know about ordering. There's currently nothing smart about IUmbracoApplicationBuilder, it just exposes the services needed for most endpoint routing. It is instantiated and called from with a UseEndpoints call. It also still needs to expose the IApplicationBuilder because some endpoint routing still requires to be done via library ext methods on this.

There is a couple bits of magic which is worth discussing. For example, if you look at PublicAccessMiddleware, it implements IConfigureOptions<UmbracoPipelineOptions> and it is registered with ConfigureOptions. This takes advantage of the above IUmbracoPipelineFilter and adds itself to the umbraco pipeline. This is one of a very few front-end/back-office specific middlewares that is not added in the base UseUmbraco() call. The alternative is to explicit add these middlewares somewhere. The only issue with that is then our startup calls start getting larger and larger and will be slightly confusing because the user must ensure that the endpoint registrations come last which would be better if we can just guarantee that (which we can by using this approach).

The new Configure method looks like this:

/// <summary>
/// Configures the application
/// </summary>
public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
{
    if (env.IsDevelopment())
    {
        app.UseDeveloperExceptionPage();
    }

    app.UseUmbraco(u =>
    {
        u.UseInstallerEndpoints();
        u.UseBackOfficeEndpoints();
        u.UseWebsiteEndpoints();
    });            
}

Within the UseUmbraco callback, we will be inside of a UseEndpoints callback which guarantees that at that time we've passed the point of adding custom middlewares. So to add middlewares after umbraco core ones but before the endpoint routes, devs will need to use the UmbracoPipelineOptions. Else should we configure this startup in a different way? One proposal is:

app.UseUmbraco()
    .WithBackOffice()
    .WithFrontEnd()
    .WithEndpoints(u =>
    {
        u.UseInstallerEndpoints();
        u.UseBackOfficeEndpoints();
        u.UseWebsiteEndpoints();
    });

where WithBackOffice and WithFrontEnd return a new IUmbracoApplicationBuilder and these just register middlewares before endpoints. Then the WithEndpoints is a void and returns nothing since it must come last and uses our IUmbracoEndpointBuilder. And then we'd remove the 'magic' listed above. What do you think? Other ideas?

Public Access Testing

  • Create 2 member groups in the back office (i.e. called "employees" + "managers")
  • Creates content items somewhere in your tree:
    • Login (this is your login page). You will need a member login form here somehow (i.e. rte macro?)
    • No Access (this is your error page)
    • Account (this is the page to apply public access too)
  • Apply public access restrictions to your Account page as per above with role access for "employees"
  • Create a member (ensure they are approved) and assigned to "employees"
  • Ensure all cookies are cleared
  • Visit the Account page - you will be redirected to login
  • Login, you will still be on the Account page that you now can see (because we are not redirecting the URL)
  • Change the public access restrictions to be for the "managers" role instead
  • Visit the Account page - you will be shown the No Access page because your member is not part of "managers"

Public Access Testing 2

This is where things get a bit wild and scenarios that didn't work in any previous version. With the new implementation, since we re-route correctly to either the login page or error page, if those pages themselves have public access, we will adhere to those rules too. Similarly, if those pages specify any redirects (umbracoRedirect) or internal redirects (umbracoInternalRedirectId), those rules will be applied too.

This means you can have cascading rules applied. For example, if you have 2x Login pages. One of those pages is the login page for your account controller, but it's also protected, and it's own login page is your 2nd login page. If the member is not logged in, they will be internally redirected to your 2nd login page. This wasn't possible before in previous version. Same goes for error pages and redirects.

Once this is merged, then please merge this one #10138

@bergmania
Copy link
Member

There is a couple bits of magic which is worth discussing. For example, if you look at PublicAccessMiddleware, it implements IConfigureOptions and it is registered with ConfigureOptions. This takes advantage of the above IUmbracoPipelineFilter and adds itself to the umbraco pipeline. This is one of a very few front-end/back-office specific middlewares that is not added in the base UseUmbraco() call. The alternative is to explicit add these middlewares somewhere. The only issue with that is then our startup calls start getting larger and larger and will be slightly confusing because the user must ensure that the endpoint registrations come last which would be better if we can just guarantee that (which we can by using this approach).

I not sure I understand why PublicAccessMiddleware needs to implement IConfigureOptions<UmbracoPipelineOptions>?

Why not separate this into its own class like the following. That seems cleaner:

    public class PublicAccessMiddlewareOptions : IConfigureOptions<UmbracoPipelineOptions>
    {
        /// <summary>
        /// Adds ourselves to the Umbraco middleware pipeline before any endpoint routes are declared
        /// </summary>
        /// <param name="options"></param>
        public void Configure(UmbracoPipelineOptions options)
            => options.AddFilter(new UmbracoPipelineFilter(
                nameof(PublicAccessMiddleware),
                null,
                app => app.UseMiddleware<PublicAccessMiddleware>(),
                null));
    }

and then do

            builder.Services
                .AddSingleton<PublicAccessMiddleware>()
                .ConfigureOptions<PublicAccessMiddlewareOptions>();

@bergmania
Copy link
Member

bergmania commented Apr 16, 2021

Within the UseUmbraco callback, we will be inside of a UseEndpoints callback which guarantees that at that time we've passed the point of adding custom middlewares. So to add middlewares after umbraco core ones but before the endpoint routes, devs will need to use the UmbracoPipelineOptions. Else should we configure this startup in a different way? One proposal is:

app.UseUmbraco()
   .WithBackOffice()
  .WithFrontEnd()
  .WithEndpoints(u =>
   {
        u.UseInstallerEndpoints();
        u.UseBackOfficeEndpoints();
        u.UseWebsiteEndpoints();
    });
    ```

where WithBackOffice and WithFrontEnd return a new IUmbracoApplicationBuilder and these just register middlewares before endpoints. Then the WithEndpoints is a void and returns nothing since it must come last and uses our IUmbracoEndpointBuilder. And then we'd remove the 'magic' listed above. What do you think? Other ideas?

I like this pattern more. Is seems more clear. But maybe the name "AddFrontEnd" should be "AddWebsite" or "AddWebsiteRendering"

@bergmania
Copy link
Member

bergmania commented Apr 16, 2021

I see these errors in the log..

Successfully registered URL "http://localhost:9000/" for site "Umbraco.Web.UI.NetCore" application "/"
Successfully registered URL "https://localhost:44331/" for site "Umbraco.Web.UI.NetCore" application "/"
Registration completed for site "Umbraco.Web.UI.NetCore"
IIS Express is running.
Enter 'Q' to stop IIS Express
[11:32:29 WRN] Register called when MainDom has not been acquired
[11:32:29 INF] Creating the content store, localContentDbExists? False
[11:32:29 INF] Creating the media store, localMediaDbExists? False
[11:32:31 INF] Acquiring.
[11:32:31 INF] Acquired.
[11:32:31 INF] Initializing Umbraco internal event handlers for cache refreshing.
[11:32:31 INF] Adding examine event handlers for 3 index providers.
[11:32:31 INF] Starting initialize async background thread.
[11:32:32 INF] Application started. Press Ctrl+C to shut down.
[11:32:32 INF] Hosting environment: Development
[11:32:32 INF] Content root path: D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.UI.NetCore
[11:32:32 INF] Loading content from database [Timing 91a6b57]
[11:32:32 INF] Completed. (159ms) [Timing 91a6b57]
[11:32:32 INF] Loading media from database [Timing d23322b]
[11:32:32 INF] Completed. (29ms) [Timing d23322b]
[11:34:39 INF] Loading content from database [Timing 24a2636]
[11:34:39 INF] Completed. (8ms) [Timing 24a2636]
[11:34:39 INF] Loading media from database [Timing d0b4914]
[11:34:39 INF] Completed. (5ms) [Timing d0b4914]
[11:34:55 INF] Loading content from database [Timing 531984d]
[11:34:55 INF] Completed. (6ms) [Timing 531984d]
[11:34:55 INF] Loading media from database [Timing 955c37b]
[11:34:55 INF] Completed. (6ms) [Timing 955c37b]
[11:35:11 INF] Document Login (id=0) has been published.
[11:35:31 INF] Loading content from database [Timing 910fc7e]
[11:35:31 INF] Completed. (3ms) [Timing 910fc7e]
[11:35:31 INF] Loading media from database [Timing db093e2]
[11:35:31 INF] Completed. (2ms) [Timing db093e2]
[11:35:31 ERR] An unhandled exception has occurred while executing the request.
System.InvalidOperationException: There is no PublishedContent.
   at Umbraco.Cms.Web.Website.Middleware.PublicAccessMiddleware.EnsurePublishedContentAccess(HttpContext httpContext, UmbracoRouteValues routeValues) in D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.Website\Middleware\PublicAcc
essMiddleware.cs:line 92
   at Umbraco.Cms.Web.Website.Middleware.PublicAccessMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.Website\Middleware\PublicAccessMiddleware.cs:line 68
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Umbraco.Cms.Web.BackOffice.Middleware.BackOfficeExternalLoginProviderErrorMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.BackOffice\Middleware\BackOffice
ExternalLoginProviderErrorMiddleware.cs:line 61
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Localization.RequestLocalizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at SixLabors.ImageSharp.Web.Middleware.ImageSharpMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context)
   at StackExchange.Profiling.MiniProfilerMiddleware.Invoke(HttpContext context) in C:\projects\dotnet\src\MiniProfiler.AspNetCore\MiniProfilerMiddleware.cs:line 121
   at Umbraco.Cms.Web.Common.Middleware.UmbracoRequestMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.Common\Middleware\UmbracoRequestMiddleware.cs:line 135
   at Umbraco.Cms.Web.Common.Middleware.UmbracoRequestMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.Common\Middleware\UmbracoRequestMiddleware.cs:line 141
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Umbraco.Cms.Web.Common.Middleware.PreviewAuthenticationMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.Common\Middleware\PreviewAuthenticationMiddleware.c
s:line 74
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Umbraco.Cms.Web.Common.Middleware.UmbracoRequestLoggingMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.Common\Middleware\UmbracoRequestLoggingMiddleware.c
s:line 44
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
[11:35:38 INF] Loading content from database [Timing adafd58]
[11:35:38 INF] Completed. (3ms) [Timing adafd58]
[11:35:38 INF] Loading media from database [Timing baac885]
[11:35:38 INF] Completed. (1ms) [Timing baac885]
[11:36:39 WRN] No template exists with the specified alias: Login
[11:36:39 INF] Document No Access (id=0) has been published.
[11:40:31 ERR] An unhandled exception has occurred while executing the request.
System.InvalidOperationException: There is no PublishedContent.
   at Umbraco.Cms.Web.Website.Middleware.PublicAccessMiddleware.EnsurePublishedContentAccess(HttpContext httpContext, UmbracoRouteValues routeValues) in D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.Website\Middleware\PublicAcc
essMiddleware.cs:line 92
   at Umbraco.Cms.Web.Website.Middleware.PublicAccessMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.Website\Middleware\PublicAccessMiddleware.cs:line 68
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Umbraco.Cms.Web.BackOffice.Middleware.BackOfficeExternalLoginProviderErrorMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.BackOffice\Middleware\BackOffice
ExternalLoginProviderErrorMiddleware.cs:line 61
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Localization.RequestLocalizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at SixLabors.ImageSharp.Web.Middleware.ImageSharpMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context)
   at StackExchange.Profiling.MiniProfilerMiddleware.Invoke(HttpContext context) in C:\projects\dotnet\src\MiniProfiler.AspNetCore\MiniProfilerMiddleware.cs:line 121
   at Umbraco.Cms.Web.Common.Middleware.UmbracoRequestMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.Common\Middleware\UmbracoRequestMiddleware.cs:line 135
   at Umbraco.Cms.Web.Common.Middleware.UmbracoRequestMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.Common\Middleware\UmbracoRequestMiddleware.cs:line 141
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Umbraco.Cms.Web.Common.Middleware.PreviewAuthenticationMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.Common\Middleware\PreviewAuthenticationMiddleware.c
s:line 74
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Umbraco.Cms.Web.Common.Middleware.UmbracoRequestLoggingMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in D:\Projects\Umbraco-CMS-netcore\src\Umbraco.Web.Common\Middleware\UmbracoRequestLoggingMiddleware.c
s:line 44
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
Enter 'Q' to stop IIS Express


The error seems to happen for the KeepAlive request:

image

@bergmania
Copy link
Member

I get this error:
image

My setup is

  • One doc type that contains a grid.

I added Login, Account and NoAccess pages using this doc type, where the grid just is used to load macros for login and account.

My template is just like this:

@using Umbraco.Cms.Web.Common.PublishedModels;
@inherits Umbraco.Cms.Web.Common.Views.UmbracoViewPage<ContentModels.Login>
@using ContentModels = Umbraco.Cms.Web.Common.PublishedModels;
@{
	Layout = null;
}

@Html.GetGridHtml(Model, "grid")

It seems like the UmbracoRouteValues.TemplateName becomes null when the routing is reevaluated.. due to the public access restrictions.. If I don't have public access restrictions the page works.

@Shazwazza
Copy link
Contributor Author

I not sure I understand why PublicAccessMiddleware needs to implement IConfigureOptions?
Why not separate this into its own class like the following. That seems cleaner

It doesn't 'need' to at all was just a quick option to allow this. Separating classes is easy. But sounds like you'd rather go with this option so I'll update to that

app.UseUmbraco()
   .WithBackOffice()
  .WithWebsite()
  .WithEndpoints(u =>
   {
        u.UseInstallerEndpoints();
        u.UseBackOfficeEndpoints();
        u.UseWebsiteEndpoints();
    });

I see these errors in the log...

I had that too but seems strange since UmbracoRouteValues shouldn't exist for back office pages. The KeepAlive controller is a back office controller, yet it's URL is /api/keepalive/ping which seems strange. I'll investigate and fix.

It seems like the UmbracoRouteValues.TemplateName becomes null when the routing is reevaluated.. due to the public access restrictions.. If I don't have public access restrictions the page works.

I cannot replicate your error. I have done exactly like you say: Grid page with the template you've specified. 3 pages: Login (with login macro), Account (with Edit Profile macro) and No Access (with Register macro) and I've set public access on the Account page. It all seems to work. Do you have any other info on how to replicate?

@bergmania
Copy link
Member

I cannot replicate your error. I have done exactly like you say: Grid page with the template you've specified. 3 pages: Login (with login macro), Account (with Edit Profile macro) and No Access (with Register macro) and I've set public access on the Account page. It all seems to work. Do you have any other info on how to replicate?

Damn, I cannot replicate anymore.. But notices another thing.. Our 404s fail now

image

Copy link
Member

@bergmania bergmania left a comment

Choose a reason for hiding this comment

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

Should all the extensions have namespace Umbraco.Extensions?

@Shazwazza
Copy link
Contributor Author

@bergmania I've pushed some changes

  • Fixes KeepAlive, its now middleware, it is now routed to the correct URL, I have removed the localonly check as per a recent update to v8 Allow KeepAlive controller Ping method to be requested by non local requests #10126, the endpoint is added to the umbraco reserved paths so it actually executes. The KeepAliveSettings are fixed with a correct virtual path instead of the weird token.
  • Have removed the null check throw if there isn't content assigned in the public access middleware
  • Have updated the Startup.cs with the fluent syntax
  • WebPath.Combine is fixed and performs better. It does not implicitly attempt to 'root' the path, it just a utility to combine paths just like Path.Combine.
  • In my testing, 404s are working so maybe its something changed in what I've done above.

Should all the extensions have namespace Umbraco.Extensions?

Is this a rhetorical question and I've missed something? Or are you asking me a real question?

@bergmania
Copy link
Member

Is this a rhetorical question and I've missed something? Or are you asking me a real question?

You added a lot of new partial UmbracoApplicationBuilderExtensions, that is currently in namespace Umbraco.Cms.Web.Common.Extensions but we used to have public extensions in Umbraco.Extensions.. Is this on purpose, or a mistake..

@Shazwazza
Copy link
Contributor Author

It was a mistake, it's just in one place where UseUmbracoRuntimeMinificationEndpoints is defined

@bergmania bergmania merged commit a1624d2 into netcore/dev Apr 20, 2021
@bergmania bergmania deleted the netcore/task/public-access-10578 branch April 20, 2021 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants