OAuthSection

[prestoncraw]

Add public const SettingsSection

[StephenCWills]

Perhaps if an application needs to extend the claims principal using a third-party data service, we should consider using Microsoft's IClaimsTransformation interface.

https://learn.microsoft.com/en-us/aspnet/core/security/authentication/claims?view=aspnetcore-7.0#extend-or-add-custom-claims-using-iclaimstransformation

I don't see how shimming it into IAuthenticationSetup is easier or better.

---

Also, the word Claims is plural in ClaimsPrincipal.

Suggested change

	void ExtendClaimPrincipal(ClaimsPrincipal claimsPrincipal, string providerIdentity);
	void ExtendClaimsPrincipal(ClaimsPrincipal claimsPrincipal, string providerIdentity);

[StephenCWills]

Suggested change

	//Constants
	// Constants

[StephenCWills]

This comment is incorrect. There is no code in Gemstone.Security that actually uses the Settings API to populate the WindowsAuthenticationProviderOptions, and this property is unused as far as the WindowsAuthenticationProvider API is concerned. The code which initializes the Settings instance, calls DefineSettings(), and assigns the values to the WindowsAuthenticationProviderOptions instance is all in openHistorian.

[StephenCWills]

Suggested change

	/// Defines the settings used to configure the <see cref="OAuthAuthenticationProvider"/> in the Configuration File.
	/// Defines the settings used to configure the <see cref="WindowsAuthenticationProvider"/> in the Configuration File.

[StephenCWills]

I believe these descriptions, taken from the doc comments on the properties in the WindowsAuthenticationProviderOptions class, are better.

Suggested change

	section.LDAPPath = ("", "LDAP path to use for Windows Authentication");
	section.AllowLocalAccounts = (false, "Allow local accounts to authenticate with Windows Authentication");
	section.LDAPPath = ("", "Root path from which LDAP searches should be performed");
	section.AllowLocalAccounts = (false, "Indicates whether the UI will also search local Users and Groups");

[StephenCWills]

Frankly, I'm rather against including any hooks to the Settings API in these classes, since the application developer should be the one to decide what data sources should be used for configuration and, by extension, what API it should use to access configuration data.

To further solidify my point, refer to my previous comment about how there isn't actually any code in this API to read data from the Settings API and populate the WindowsAuthenticationProviderOptions class. If you were to add that code here, it would only serve to deepen the coupling between this API and the Settings API, eroding the application developer's freedom to choose a more appropriate configuration source for their application.

The knock-on effects of coupling the Settings API with other Gemstone APIs can already be seen in some of our command line tools where we were forced to place a superfluous call to the Settings class' constructor in order to avoid spurious NullReferenceExceptions at runtime!

[prestoncraw]

I understand the point you are making, but isn't this how it is done in many other Gemstone APIs?

See below:

• https://github.com/gemstone/security/blob/master/src/Gemstone.Security/AuthenticationProviders/OAuthAuthenticationProvider.cs#L162
• https://github.com/gemstone/diagnostics/blob/master/src/Gemstone.Diagnostics/DiagnosticsLogger.cs#L231
• https://github.com/gemstone/data/blob/master/src/Gemstone.Data/AdoDataConnection.cs#L1512
• https://github.com/gemstone/timeseries/blob/master/src/Gemstone.Timeseries/ServiceHostBase.cs#L4196