JumpCloud provider improvements

[martincostello]

• Set the default domain for JumpCloud if a custom domain isn't used so that no extra configuration is required.
• Enable PKCE for the JumpCloud provider by default.
• Remove email and profile by default from JumpCloud.

I went with the second option on #797 (comment) - if it's not needed, then I can just mark the associated code as obsolete instead and delete it in the v8 branch.

/cc @AaronSadlerUK

[kevinchalet]

Looks good.

While you're at it, can you also update this helper? None of the calls to this method needs to set format arguments so it can be simplified.

AspNet.Security.OAuth.Providers/src/AspNet.Security.OAuth.JumpCloud/JumpCloudPostConfigureOptions.cs

Lines 32 to 45 in 7cf71de

	private static string CreateUrl(string domain, string pathFormat, params object[] args)
	{
	var path = string.Format(CultureInfo.InvariantCulture, pathFormat, args);
	// Enforce use of HTTPS
	var builder = new UriBuilder(domain)
	{
	Path = path,
	Port = -1,
	Scheme = Uri.UriSchemeHttps,
	};
	return builder.Uri.ToString();
	}

We should also mark the *Format fields as deprecated as these are not real format fields (there's no placeholder in any of the strings):

AspNet.Security.OAuth.Providers/src/AspNet.Security.OAuth.JumpCloud/JumpCloudAuthenticationDefaults.cs

Lines 36 to 49 in 7cf71de

	/// <summary>
	/// Default path format to use for <see cref="OAuthOptions.AuthorizationEndpoint"/>.
	/// </summary>
	public static readonly string AuthorizationEndpointPathFormat = "/oauth2/auth";
	/// <summary>
	/// Default path format to use for <see cref="OAuthOptions.TokenEndpoint"/>.
	/// </summary>
	public static readonly string TokenEndpointPathFormat = "/oauth2/token";
	/// <summary>
	/// Default path format to use for <see cref="OAuthOptions.UserInformationEndpoint"/>.
	/// </summary>
	public static readonly string UserInformationEndpointPathFormat = "/userinfo";

Calls to string.Format should also be removed here:

AspNet.Security.OAuth.Providers/src/AspNet.Security.OAuth.JumpCloud/JumpCloudAuthenticationDefaults.cs

Lines 51 to 64 in 7cf71de

	/// <summary>
	/// Default path to use for <see cref="OAuthOptions.AuthorizationEndpoint"/>.
	/// </summary>
	public static readonly string AuthorizationEndpointPath = string.Format(CultureInfo.InvariantCulture, AuthorizationEndpointPathFormat);
	/// <summary>
	/// Default path to use for <see cref="OAuthOptions.TokenEndpoint"/>.
	/// </summary>
	public static readonly string TokenEndpointPath = string.Format(CultureInfo.InvariantCulture, TokenEndpointPathFormat);
	/// <summary>
	/// Default path to use for <see cref="OAuthOptions.UserInformationEndpoint"/>.
	/// </summary>
	public static readonly string UserInformationEndpointPath = string.Format(CultureInfo.InvariantCulture, UserInformationEndpointPathFormat);

[martincostello]

Probably not today, but at some point, I'm going to make a GitHub Actions workflow that leaves a comment on PRs that add new providers with a checklist of common things to check for (PKCE etc).

I'll also leave PRs at least a day so you get a chance to look at them 😅

I just get cagey with new providers when they seem ready after that time we had someone name-squat the package before we could actually publish it to NuGet.org...

[kevinchalet]

Probably not today, but at some point, I'm going to make a GitHub Actions workflow that leaves a comment on PRs that add new providers with a checklist of common things to check for (PKCE etc).

Sounds like a great idea! Maybe we should also have a PR template that lists these things?

I'll also leave PRs at least a day so you get a chance to look at them 😅

My bad, I should have reacted earlier 🤣

I just get cagey with new providers when they seem ready after that time we had someone name-squat the package before we could actually publish it to NuGet.org...

Makes a lot of sense! To avoid that, I guess I should have chosen a more specific package prefix when starting this project (tho' prefix reservation was not a thing yet at the time 😅)

[martincostello]

Sounds like a great idea! Maybe we should also have a PR template that lists these things?

I considered that, but there's already a template and people often ignore them and/or delete them I've found through experience.

I thought a comment @'d at the user post-open might be a bit more noticable. Plus we could potentially run a bit of code to differentiate between new providers and edits based on the diff to make it a bit smarter.

[kevinchalet]

I thought we could have PR templates that would work the same way as issue templates (I.e with a screen showing the available templates) but apparently not: https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository

So yeah, an automated message may make a lot of sense 😄

[martincostello]

@kevinchalet Happy with the latest changes?

[kevinchalet]

Perfect, thanks! 👍🏻