Expose DefaultViewServiceOption list props as IEnumerables.

[johnkors]

Fixes #2947. NB! Breaking change & needs a bump of major.

The DefaultViewServiceOption / DefaultViewServiceRegistration allows mutating state on a registred singleton, causing possible bad side effects. This PR changes the option API to use IEnumerable instead of IList - restricting access to modify the list directly.

Example of a bad side effect caused by modifying one of the lists in a class inheriting from the DefaultViewService: https://github.com/IdentityServer/IdentityServer3/issues/2089#issuecomment-222965936

[dnfclas]

@johnkors, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.
Thanks, DNFBOT;

[brockallen]

Ok, will visit once we have a v3 branch. Thx

[MrJerB]

Hi all, I am currently experiencing an issue where I modify the config on each request (dynamically adding stylesheets depending on an acr_values) in the constructor of my class (which inherits from DefaultViewService). This feels really messy and I'm not even sure if this is thread safe as my assumption was that the "config" passed to the constructor is factory generated. If anyone can suggest a better method than the following, please:

public class BrandedViewService : DefaultViewService
{
	private static readonly string BrandAcr = "brand";

	public BrandedViewService(DefaultViewServiceOptions config, IViewLoader viewLoader) : base(config, viewLoader)
	{
		// Disable views to re-render styles correctly
		config.CacheViews = false;

		// Remove any branding stylesheets from previous config
		config.Stylesheets = new DefaultViewServiceOptions().Stylesheets;

		// Logic for branding
		var req = HttpContext.Current.Request;
		var env = req.GetOwinContext().Environment;
		var signInId = req["signin"];

		if (signInId == null)
			return;

		var signInMessage = env.GetSignInMessage(signInId);
		var brand = signInMessage?.AcrValues?.FirstOrDefault(acr => acr.StartsWith(BrandAcr + ":", System.StringComparison.Ordinal))?.Substring(BrandAcr.Length + 1);

		if (brand != null)
		{
			config.Stylesheets.Add("~/IdentityServer/Brands/test.css");
		}
	}
}

I am registering the dependency as follows:

factory.ViewService = new DefaultViewServiceRegistration<IdentityServer.BrandedViewService>()
{
	Mode = RegistrationMode.InstancePerHttpRequest
};

I am commenting here because this pull request might solve the issue or at least allow me to do this in a cleaner fashion.

[johnkors]

@MrJerB , the work-around for now is to not mutate the singleton-scoped config's lists.

For example:

public class CustomViewService : DefaultViewService 
{
        public CustomViewService(DefaultViewServiceOptions options, IViewLoader viewLoader, OwinEnvironmentService owinservice) : base(options, viewLoader)
        {
            _owinservice = owinservice;
        }

        protected override Task<Stream> Render(CommonViewModel model, string page)
        {
            var acrs = _owinservice.Environment.GetSignInMessage().AcrValues;
            var styleSheets = config.Stylesheets.ToList();
            var isBrandReq = acrs.Any(s => s.Equals("BRAND"));
            if(isBrandReq)
                styleSheets.Add("~/IdentityServer/Brands/test.css");

            return base.Render(model, page, styleSheets, config.Scripts);
        }
}

[MrJerB]

Thanks @johnkors and another plus one for showing me how to obtain the OWIN context through dependency injection 👍