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 https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/pull/797#discussion_r1294659063 - 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.

https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/7cf71deb0563f99390aac50e9e74220f582d3696/src/AspNet.Security.OAuth.JumpCloud/JumpCloudPostConfigureOptions.cs#L32-L45

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):

https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/7cf71deb0563f99390aac50e9e74220f582d3696/src/AspNet.Security.OAuth.JumpCloud/JumpCloudAuthenticationDefaults.cs#L36-L49

Calls to string.Format should also be removed here:

https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/7cf71deb0563f99390aac50e9e74220f582d3696/src/AspNet.Security.OAuth.JumpCloud/JumpCloudAuthenticationDefaults.cs#L51-L64

[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! 👍🏻