Refactor provider options

[sporkmonger]

Introduces a simple switch to initialize the provider to allow for new providers to be subsequently added.

I did consider removing the TestGoogleGroupInvalidFile function and including it in TestNewOptions, since it no longer requires a panic recovery, but that seemed like maybe the wrong solution once multiple providers are introduced.

Fixes #6.

[sporkmonger]

Ready for review.

[shrayolacrayon]

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)

[sporkmonger]

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

[sporkmonger]

@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.

sso/cmd/sso-auth/main.go

Lines 21 to 33 in 17427ad

	opts := auth.NewOptions()
	err := envconfig.Process("", opts)
	if err != nil {
	logger.Error(err, "error loading in env vars")
	os.Exit(1)
	}
	err = opts.Validate()
	if err != nil {
	logger.Error(err, "error validating opts")
	os.Exit(1)
	}

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.

[shrayolacrayon]

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, ....)
}

[sporkmonger]

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.

[shrayolacrayon]

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.

[sporkmonger]

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

[sporkmonger]

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

[sporkmonger]

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.

[shrayolacrayon]

can we make the default return an error here rather than the google provider and add a separate switch case for google? We can for now set the provider in NewOptions() to be google.

[sporkmonger]

Sure. I actually had it that way initially but it broke half the test suite, but it's probably not a bad idea to make the tests more explicit.

[shrayolacrayon]

This is coming together, thanks for making those changes. In addition to the comments I left, I think we'd want to move the Google provider specific validation out of the options.Validate() and into probably providers.NewGoogleProvider() which should return an error if they aren't valid config vals.

This reasoning is because I think it ultimately makes sense to get rid of the Validate function in options completely and validate config values on initialization of the objects, which will make it easier to consolidate common logic between auth and proxy without having to rely on changing logic in the different options configs.

[shrayolacrayon]

this function does not really parse provider info anymore, I think it could be named to something like validateEndpoints

[shrayolacrayon]

When the file doesn't exist in NewGoogleProvider this panics right now, and this test will not catch that. We can either have NewGoogleProvider return an error instead (which might be easier to reason about behavior) or recover the panic in the test as we do below.

[sporkmonger]

I'd lean towards error rather than panic.

[shrayolacrayon]

I'm not sure if we need all of these constants yet, might be good to just keep Google for now and add them as the providers have been implemented as to not confuse anyone that they have been.

[sporkmonger]

Sure, that seems fine. I'm planning to do these next-ish, but they can go into another PR instead, that's fine.

[shrayolacrayon]

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

[sporkmonger]

Should be all set!

[shrayolacrayon]

Thanks for doing this @sporkmonger!

[sporkmonger]

Sure thing!