It doesn't look like we actually have any callers for clientassertion.New here in the cap library. And New is the only non-test code path that calls Validate. I do get that we pulled this out to a package for testability.

But from the perspective of an application consuming this API, I'd question why my application needs to call clientassertion.New instead of passing the appropriate configuration values to oidc.NewConfig. It seems like we're introducing more surface area here (and therefore more ways to get it wrong in the application).

My read of oidc.Config is to configure the Provider, which seems to include some minimum required features of OIDC. Other things (like PKCE, which I followed for my implementation) are more features of individual parts of the process. For client assertions (as with PKCE), it's part of a Request.

I could put more of this logic into the oidc.Option, which is passed in oidc.NewRequest -- currently oidc.WithClientAssertionJWT() just takes a string, but it could instead take all the things that this thing takes, and produce the string itself internally. I'm just not sure how much that buys us, and I figured leaving this open would allow applications to produce the JWT however they may want, making this package convenient but optional.

@johanbrandhorst suggests below that we could pass the ClientAssertion (perhaps renamed to JWT to avoid stuttery package.struct names) instead of a string, then the option or Request constructor could do the signing (if I'm understanding that suggestion right). That would make this extra package a lot less optional for this feature, but definitely harder to hold wrong.

Maybe an in-between would be to accept an interface, which ClientAssertion/JWT implements, so folks could build their own, if they wanted. Harder to hold wrong, but still flexible.

@johanbrandhorst suggests below that we could pass the ClientAssertion (perhaps renamed to JWT to avoid stuttery package.struct names) instead of a string, then the option or Request constructor could do the signing (if I'm understanding that suggestion right). That would make this extra package a lot less optional for this feature, but definitely harder to hold wrong.

Maybe an in-between would be to accept an interface, which ClientAssertion/JWT implements, so folks could build their own, if they wanted. Harder to hold wrong, but still flexible.

Yeah, either of those options sounds pretty solid.

What about a package doc string?

good point, I'll do this tomorrow when I make an example_test.go

Do we still need this function? I think if we can get this public API down to NewJWT and .Serialize(), that would be ideal. Can we move the checks that make sense from this function into NewJWT instead? If we're trying to help users using &JWT{} instead of NewJWT(), I don't think we can assume that they will call Validate if they already ignored the constructor, so I'm OK with that case just panicking or erroring in a bad way.

Seems reasonable enough to me -- Okay if I keep it as a separate function, just privatize it? I like small single-purpose functions.

Sure, but I think some of this error checking doesn't really belong here anymore. E.g. the checking if j.key and j.secret are both set aren't necessary if you move the logic into the option as I suggested. Calling Serialize in New also feel really wrong IMO, since it's the primary thing the user wants to do. If that's where we're going with this - why do we need a whole struct (or even package) encapsulating this? We could just have NewSerializedJWT as an exported function (maybe we should?). I think New() and Serialize() can be separate, but if this logic was moved into New, it would be clear from the context of that function what should and shouldn't be necessary to validate. Ideally nothing other than the inputs to the function, which should be all the required parameters to create a JWT, should need to be validated. On that note, since either Key or Secret must be set, should it be a parameter rather than an option?

I'm personally not a huge fan of more smaller functions, because the context in which they are used is not clear and indirection always leads to overhead for the reader. I almost left a comment saying that the structure in Serialize was unnecessarily abstracted into smaller functions, but it's such a minor comment that I didn't in the end feel that it was important enough to bring up, but since we're talking about it now, there you go 😁.

Thanks for the thoughtful reply!

I'll say up front that I'm down to remove the Serialize() call from New - it felt weird when I put it there, so easy to drop it. Also good to move validation into New.

the checking if j.key and j.secret are both set aren't necessary if you move the logic into the option

I like cross-field validation to happen in its own spot, but it amounts to the same result either way, so I'd be happy to put the checks in their respective options. except:

since either Key or Secret must be set, should it be a parameter rather than an option?

The current not-actually-optional-Option thing has been bugging me. Here's some (pardon the pun) other options:

1. I don't love funcs that take multiple params where any of them are expected to always be zero values like "" / nil
2. We could do similar to what jose does, and accept an any but assert supported types. Personally this kind of annoys me about jose
3. Or, what if we have 2 constructors?

func NewJWTWithRSAKey(clientID string, audience []string, alg RSAlgorithm, key *rsa.RSAKey, opts ...Option) (*JWT, error) {
}
func NewJWTWithHMAC(clientID string, audience []string, alg HSAlgorithm, secret string, opts ...Option) (*JWT, error) {
}

That would leave a lot less validation to do, but both could set up and return a JWT, which is mostly shared logic internally.

I like option 3 a lot!

I believe the algorithm will need to be a parameter too, and you could even make it RSAlgorithm and HSAlgorithm respectively :)