Skip to content

Conversation

@8snit
Copy link
Contributor

@8snit 8snit commented Jun 9, 2016

With OWIN middleware there is no "default" way to use/consume/provide IOC container functionality, the best guess is to make it simple for the app developer to plug-in the own container implementation.

The current implementation for these dependendies doesn't make me totally happy:

  • registration of IAuthorizationHandler as well as overriding the default AuthorizationPolicyProvider resp. AuthorizationService is not obvious
  • not possible to say when construction of dependencies takes place
  • no way to dispose them
  • hard to influence lifetime

The proposed changes put all the construction work in the ResourceAuthorizationMiddleware -> Invoke call, the construction can be customized via the AuthorizationDependenciesProvider.

The WebApi Custom Handler sample has been adapted for demonstration:

           app.UseAuthorization(options =>
            {
                options.AddPolicy(ExampleConstants.EmployeeNumber2Policy, policyBuilder =>
                {
                    policyBuilder.AddRequirements(new EmployeeNumber2Requirement());
                });
                options.Handlers = new IAuthorizationHandler[] {new EmployeeNumber2Handler()};
                options.PolicyProvider = new CustomAuthorizationPolicyProvider(options);
           });

Existing tests targeting old implementation were removed/adaped accordingly.

Please let me know if you need further samples/tests :)

OnDispose = (options, context) => { };
}

public Func<AuthorizationOptions, AuthorizationDependencies> OnCreate { get; set; }

Choose a reason for hiding this comment

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

I dont understand why there are separate injection points (utilizing property dependency injection) for create/dispose pair as opposed to construction DI. What happens if the callers method that sets these properties throws or prematurely exits between setting these two properties, but continues to execute? You now have an object that leaks memory/recourses, and potentially expensive memory/resources.

@DavidParks8
Copy link
Owner

If you make a separate pull request for the WebApi Custom Handler sample, I'd be happy to merge it as is. You might be on the right track with your idea to influence lifetime, but I think we need to refine it a bit more before merging.

@8snit 8snit force-pushed the dependencyprovider branch from 4a51cfa to 43133f1 Compare June 11, 2016 14:53
@8snit
Copy link
Contributor Author

8snit commented Jun 11, 2016

Thanks for your input, I tried to make things more explicit now and added a sample using Autofac to demonstrate how it could work (using per request lifetime scope).

With the AuthorizationDependenciesProvider OnCreate/OnDispose approach, I tried to stay close what is e.g.done in Microsoft.AspNet.Identity.Owin -> CreatePerOwinContext but I think there is plenty of space for further refinement :) Afterwards I'm happy to split things up for pulling in...

public static class AppBuilderExtensions
{
public static IAppBuilder UseAuthorization(this IAppBuilder app, AuthorizationOptions options, AuthorizationDependenciesProvider dependenciesProvider = null)
public static IAppBuilder UseAuthorization(this IAppBuilder app, AuthorizationOptions options, IAuthorizationDependenciesProvider dependenciesProvider = null, IAuthorizationHandler[] handlers = null)
Copy link
Owner

Choose a reason for hiding this comment

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

The project engineering guidelines state that we should shy away from default parameters. For an explanation why, go here: https://msdn.microsoft.com/library/ms182135.aspx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, will provide corresponding overloads

…Funcs, removed Disposable stuff, adjusted samples
@8snit
Copy link
Contributor Author

8snit commented Jun 16, 2016

hi, thx for your input, I tried to incorporate your suggestions with my latest commit, I also removed the disposable stuff as it is not needed together with an IOC container...hope we can move forward :-)

@DavidParks8 DavidParks8 merged commit f44883b into DavidParks8:dev Jun 18, 2016
@DavidParks8
Copy link
Owner

Looks good enough to me!

@8snit 8snit deleted the dependencyprovider branch June 19, 2016 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants