Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added JumpCloud support #797

Merged
merged 10 commits into from
Aug 15, 2023
Merged

Conversation

AaronSadlerUK
Copy link
Contributor

Added support for the JumpCloud OIDC application.

https://jumpcloud.com/support/sso-with-oidc

Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

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

This looks good so far. Please add tests by following an example of one of the existing providers that has a custom domain, such as Okta.

@AaronSadlerUK
Copy link
Contributor Author

AaronSadlerUK commented Aug 15, 2023

I'm probably missing something simple but I'm getting the following error when I try to build:

NU1101 Unable to find package AspNet.Security.OAuth.JumpCloud. No packages exist with this id in source(s): dotnet-eng, dotnet-tools, nuget AspNet.Security.OAuth.JumpCloud

Of course I know it's not found as the package hasn't been published yet, how do I get past this?

@AaronSadlerUK
Copy link
Contributor Author

AaronSadlerUK commented Aug 15, 2023

I have removed the custom authentication as looking through the documentation with JumpCloud there is no mention of it.

I have added unit tests, also it looks like the domain is fixed and cannot be changed.

@martincostello
Copy link
Member

Before I merge this and prepare a release, can you confirm that you've tested this as working with the real JumpCloud service?

@AaronSadlerUK
Copy link
Contributor Author

Yes I have this working, happy to provide a URL privately

@martincostello martincostello merged commit 2fac680 into aspnet-contrib:dev Aug 15, 2023
8 checks passed
/// <summary>
/// Gets or sets the JumpCloud domain (Org URL) to use for authentication.
/// </summary>
public string? Domain { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Does JumpCloud support configuring your own domain?

  • If not, we shouldn't make the domain configurable and remove this property.
  • If so, it would likely make sense to use https://oauth.id.jumpcloud.com/ as the default domain when the user doesn't specify an explicit value.

CallbackPath = JumpCloudAuthenticationDefaults.CallbackPath;

Scope.Add("openid");
Scope.Add("profile");
Copy link
Member

Choose a reason for hiding this comment

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

In general, we try to limit the default scopes to the absolute required ones. For a standard-compliant OIDC provider, only openid should be required.

Copy link
Member

Choose a reason for hiding this comment

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

I just tested and it works fine with only openid.

@kevinchalet
Copy link
Member

@martincostello I guess I'm late to the party, but that would be nice to address my two remarks before shipping a public version of this provider.

@kevinchalet
Copy link
Member

Well... scratch that I guess 😅

image

@martincostello
Copy link
Member

Sorry, I was running the commands whilst also in a meeting so didn't see the notifications until just now.

@martincostello
Copy link
Member

We could always unlist 7.0.4 and then do a 7.0.5 that has the changes you want made?

@kevinchalet
Copy link
Member

Sorry, I was running the commands whilst also in a meeting so didn't see the notifications until just now.

Is that the secret sauce of being a x10 developer? Doing things in parallel? 🤣

We could always unlist 7.0.4 and then do a 7.0.5 that has the changes you want made?

Doesn't seem necessary to me: depending on @AaronSadlerUK's feedback, we'll still be able to give Domain a default value in a future minor version (or deprecate it if the domain is not configurable in JumpCloud). For the scopes, well, it would be a breaking change, so we'll have to live with it 😄

@martincostello
Copy link
Member

It would technically be a breaking change, but it's only been published for ~5 minutes and I think some one would need to be a 100x developer to have already integrated it and be broken by it.

@AaronSadlerUK
Copy link
Contributor Author

AaronSadlerUK commented Aug 15, 2023

I can't see anywhere that a custom domain is documented... However, that's not to say the enterprise subscribers can't do it

@kevinchalet
Copy link
Member

I can't see anywhere that a custom domain is documented... However, that's not to say the enterprise subscribers can't do it

I guess it would be more clearly documented if it was possible (and AFAIK, JumpCloud is cloud-only and not offered as an on-premises product, so you can't even self-host it).

@martincostello I'm not sure it's worth pushing a new release now. We can integrate these changes in a future version and document them, after all. As you said, folks are unlikely to massively start using this provider anyway 😄

@kevinchalet
Copy link
Member

Note: JumpCloud fully supports PKCE (for both public and confidential apps) so we'll also want to set UsePkce = true for increased security.

@martincostello martincostello added this to the 7.0.4 milestone Aug 15, 2023
@martincostello
Copy link
Member

I'll make these changes now as I've just rebased the new provider into the v8 branch anyway.

@AaronSadlerUK
Copy link
Contributor Author

@martincostello would it be possible to PR into the v6 version?

The site I plan on using this with uses .NET 6

@martincostello
Copy link
Member

We don't typically backport new providers to older versions because it's additional maintenance for us.

@AaronSadlerUK
Copy link
Contributor Author

Ah ok, this site runs on Umbraco 10 (LTS) which uses .NET 6 (LTS)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants