Add optional params to control middleware

[bjornharrtell]

Closes #656

BUG FIX

• reproduce issue in tests
• fix issue
• bump package version

FEATURE

• write tests that address the requirements outlined in the issue
• fulfill the feature requirements
• bump package version

RELATED REPOSITORY UPDATES

• does this feature require documentation? if so, open an issue in the docs repo
• does this feature break an API that is implemented in the templates repository? if so, open an issue

[maurei]

Thank you for looking into this. I will review the tests tomorrow and see whats up with the CI build

[bjornharrtell]

@maurei CI build issue was just me missing to add the complete diff (missing using System).

[maurei]

OK it's been a while, my apologies for not being able to review this any earlier. It's too bad that netcore3 now requires us to wrap the authentication middleware calls with the internals of UseJsonApi().

I think there is minor a problem with the current approach. The insertExtraMiddleware parameter more generically implies the ability to register any extra middleware at this stage of the application startup, but in practice this functionality will be limited because a developer still has no control when this middleware is registered with respect to other internal middlewares. For example, if I'd want to register a middleware that relies on eg an IQueryParameterService implementation it wouldn't work because the CurrentRequestMiddleware is registered after the lambda executes. Indeed, this setup will suffice to activate authentication and authorization but that's only because the lambda happens to be invoked after the UseRouting() but before the UseEndpoints() call. It means the developer would need to know about the internals of AddJsonApi() to understand the limitations of that lambda parameter

In general, I don't think such lambda function will suffice to give the dev full control over the middleware registry. With respect to the authentication bug, I think I would be best to keep it simple with an overload with default parameters that take care of this, eg

public static void UseJsonApi(this IApplicationBuilder app, bool useAuthentication = false, bool useAuthorization = false)
{
	...

    if (useAuthentication) 
    {
    	app.UseAuthentication();
    }

    if (useAuthorization) 
    {
    	app.useAuthorization();
    }

    ... 
}

To give a developer full control over the middlewares being registered, we could add another parameter or overload. In this case the developer should be aware of the responsibility of calling UseRouting, UseMiddleware<CurrentRequestMiddleware>() and UseEndPoints( ... ). Eg.

public static void UseJsonApi(this IApplicationBuilder app, bool skipRegisterMiddleware = false, bool useAuthentication = false, bool useAuthorization = false)
{
	... 

    if (skipRegisterMiddleware) 
    {
    	...

	    if (useAuthentication) 
	    {
	    	app.UseAuthentication();
	    }

	    if (useAuthorization) 
	    {
	    	app.useAuthorization();
	    }

	    ...
    }
}

Then, after this call, the dev could register any middleware in any desired order. But maybe this PR should probably just address the authentication bug rather than introducing full configurability of middleware registration.

Let me know what you guys think @bjornharrtell @bart-degreed. Also let me know if this is workable or if you want me to finish it up. Either way I think we should be able to move forward with this quickly now.

[bjornharrtell]

Agreed @maurei, I'm open to make the adjustments to your suggested approach.

[maurei]

Agreed @maurei, I'm open to make the adjustments to your suggested approach.

Great! Looking forward to your updates.

[bart-degreed]

Sounds like good suggestions to me.

[bjornharrtell]

Quick rework done. Unable to test/verify at this time however, sorry.

[maurei]

Thanks. I added some more detailed comments, waiting for the CI build to go green. I don't think this fix comes with a high priority for tests as we would pretty much be testing a .netcore3 standard feature

[fdlane]

Thank you all! @bjornharrtell @maurei @bart-degreed

[bart-degreed]

Found some issues in the docs. Likely requires a new PR, as this one has already been merged.

[bart-degreed]

"is" twice

[bart-degreed]

half sentence.

[maurei]

This one I noticed too, did a hotfix in ebaa091. If you could squeeze the removal of the 2nd "is" in one of your upcoming PRs would be appreciated :)

[bart-degreed]

Fixed in #684.