Ready for review.

Thanks for starting on this @sporkmonger, I think I had a deeper refactor in mind for this issue, which is to remove all provider logic from options validation.

I would approach this starting off with removing the provider field from Options completely and initializing the the Provider in NewAuthenticator and using a functional option to assign the provider to the Authenticator, as that is something we are trying to move towards as well. (see https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis)

I hadn't ever seen that article before, thanks for that. That's super-cool.

@shrayolacrayon I started on making the changes, but given the order in which options are processed, I'm not sure how this would work in a NewOptions context.

I was thinking, e.g.

opts, err := auth.NewOptions(auth.GoogleProvider("admin@domain.com", "credentials.json"))

But the provider's not known at the point where you call NewOptions? You'd know it after envconfig.Process is called.

Hmm I had in mind doing something like this, where we validate the non-provider options in options.Validate and validate and create the provider in a separate step when we're initializing the Authenticator.

provider.go

func AssignProvider(opts *Option)func(*Authenticator)error{
return func(a *Authenticator)error{
  // initialize the provider and validate options in NewProvider
  provider, err := NewProvider(options)
  if err != nil {
    return err
  }
  a.Provider = provider
}}

main.go 

func main(){
...
// initialize all options, validate all options that are not provider specific 
options := NewOptions()
err := envconfig.options.Process()
err := options.Validate() // This does NOT validate any of the provider data 


// AssignProvider
authenticator, err := NewAuthenticator(options, authenticator.AssignProvider, ....)
}

authenticator.AssignProvider seems like it would be an always-required parameter in this scenario? That seems odd to me. It might be beneficial for testing purposes, but otherwise it strikes me as harder to understand and reason about than a simple helper function. I really like this pattern, but it seems mostly applicable when you have a larger number of optional bits of configuration you're applying, particularly where they may have their own context-sensitive parameters. In this case we're always choosing a provider (except for maybe in some test scenarios?). I'm not sure if there are other config functions to pass in to NewAuthenticator besides authenticator.AssignProvider, but it doesn't appear like there would be in that design?

The pattern made more sense to me as a way to minimize the complexity of the provider switch logic, since you could do something like:

switch opts.Provider {
case "azure":
  providerConfig = authenticator.AzureProvider(opts.AzureTenantID)
case "google":
  providerConfig = authenticator.GoogleProvider(opts.GoogleAdminEmail, opts.GoogleServiceAccountJSON)
}

authenticator, err := NewAuthenticator(options, providerConfig, ....)

Writing it as authenticator.AssignProvider doesn't feel quite right to me.

The switch logic definitely makes sense to me, and would make sense to be encapsulated in a NewProvider function ie:

internal/auth/options.go

var (
GoogleProvider = "google"
AzureProvider = "azure"
)

func NewProvider(opts) (providers.Provider, error) {
switch opts.Provider {
  case GoogleProvider: 
    return providers.NewGoogleProvider(opts)
  case AzureProvider:
    return providers.NewAzureProvider(opts)
  default:
    return nil, fmt.Errorf("unsupported provider in options: %s", opts.Provider)
}

}

I think assigning the provider in a functional option could be useful for some test cases and is a pattern that we have been using for mocking in tests, but I'm not attached to the idea of using it for the providers. FWIW there are already functional options being passed into NewAuthenticator for the cookie store and for the statsd client (the CookieStore logic has not yet been migrated over to sso-proxy because it was a more recent change before the open-sourcing) so it would not be a new pattern to use.

OK, cool, I think that gives me enough to work with.

It might also be a good pattern for configuring statsd vs prometheus & open tracing too, FWIW.

OK, I removed the provider variable from Options entirely, it now only lives in Authenticator, so a couple tests shifted over from options_test.go to authenticator_test.go.

This looks good to me! Can you squash and we can merge?

Should be all set!

Thanks for doing this @sporkmonger!

Sure thing!