Fix default scope settings for none oidc providers like GitHub

[tuunit]

Description

As described in detail in bug #1903 and related to #1724.
The current implementation overrides the default scope for all providers with the default oidc scopes when no scope is set in the configuration file. Which is obviously not the expected behaviour.

As the config file values are loaded before the default values of the providers:

oauth2-proxy/providers/providers.go

Lines 34 to 71 in fd2807c

	func NewProvider(providerConfig options.Provider) (Provider, error) {
	providerData, err := newProviderDataFromConfig(providerConfig)
	if err != nil {
	return nil, fmt.Errorf("could not create provider data: %v", err)
	}
	switch providerConfig.Type {
	case options.ADFSProvider:
	return NewADFSProvider(providerData, providerConfig.ADFSConfig), nil
	case options.AzureProvider:
	return NewAzureProvider(providerData, providerConfig.AzureConfig), nil
	case options.BitbucketProvider:
	return NewBitbucketProvider(providerData, providerConfig.BitbucketConfig), nil
	case options.DigitalOceanProvider:
	return NewDigitalOceanProvider(providerData), nil
	case options.FacebookProvider:
	return NewFacebookProvider(providerData), nil
	case options.GitHubProvider:
	return NewGitHubProvider(providerData, providerConfig.GitHubConfig), nil
	case options.GitLabProvider:
	return NewGitLabProvider(providerData, providerConfig.GitLabConfig)
	case options.GoogleProvider:
	return NewGoogleProvider(providerData, providerConfig.GoogleConfig)
	case options.KeycloakProvider:
	return NewKeycloakProvider(providerData, providerConfig.KeycloakConfig), nil
	case options.KeycloakOIDCProvider:
	return NewKeycloakOIDCProvider(providerData, providerConfig.KeycloakConfig), nil
	case options.LinkedInProvider:
	return NewLinkedInProvider(providerData), nil
	case options.LoginGovProvider:
	return NewLoginGovProvider(providerData, providerConfig.LoginGovConfig)
	case options.NextCloudProvider:
	return NewNextcloudProvider(providerData), nil
	case options.OIDCProvider:
	return NewOIDCProvider(providerData, providerConfig.OIDCConfig), nil
	default:
	return nil, fmt.Errorf("unknown provider type %q", providerConfig.Type)
	}
	}

oauth2-proxy/providers/providers.go

Lines 155 to 169 in fd2807c

	if p.Scope == "" {
	p.Scope = "openid email profile"
	if len(providerConfig.AllowedGroups) > 0 {
	p.Scope += " groups"
	}
	}
	if providerConfig.OIDCConfig.UserIDClaim == "" {
	providerConfig.OIDCConfig.UserIDClaim = "email"
	}
	p.setAllowedGroups(providerConfig.AllowedGroups)
	return p, nil
	}

The scope is overwritten with "openid email profile" which leads to issues later on in the setProviderDefaults method. As the p.Scope will never be empty, therefore all provider default scopes are ignored.

oauth2-proxy/providers/provider_data.go

Lines 197 to 199 in fd2807c

	if p.Scope == "" {
	p.Scope = defaults.scope
	}

I added a check for the providerName as an additional condition in the default setter while loading the data from config file.

How Has This Been Tested?

I tested it locally with GitHub and dex, to ensure both implementations still work and I extended the unit test cases.

Checklist:

• My change requires a change to the documentation or CHANGELOG.
• I have updated the documentation/CHANGELOG accordingly.
• I have created a feature (non-master) branch for my PR.

[tuunit]

@JoelSpeed as this seems to be blocking quite a few people and it has been a bug for nearly a year now. I would like to ask you directly to have a look at this PR, if anything is missing to get it merged.

[tuunit]

Resolves #1903, resolves #1724

[JoelSpeed]

Thanks for the fix, long term I intend to get this section rewritten and the provider settings will come before this defaulting code, but this certainly resolves the issue for now, so thanks!

[JoelSpeed]

Looks like the test updates aren't quite right as one of the scope tests failed, can you please look into that?

[tuunit]

Looks like the test updates aren't quite right as one of the scope tests failed, can you please look into that?

@JoelSpeed my mistake! I had another look at it and it should now work as expected.

[tuunit]

@JoelSpeed can you retrigger the build and tests? 😄

[JoelSpeed]

Lets see if the tests pass! LGTM, thanks

[bit-herder]

can we get a release of this? Theres no release since october of 2022. Is this project still maintained?

[tuunit]

@bit-herder unfortunately, @JoelSpeed has been looking for successors for a while now. There is an issue open for this very purpose. He moved to another job and doesn't have the time anymore to actively maintain this project. I'll be trying to get in contact with him as I've been interested in helping out this amazing project for a while now.

[tuunit]

@bit-herder we are currently working on getting a new release finalized.

[sstidl]

#2222 could this be related? how do i migrate my config?

[kvanzuijlen]

#2222 could this be related? how do i migrate my config?

That seems to be the case, see https://github.com/oauth2-proxy/oauth2-proxy/pull/1927/files#r1086869386

@tuunit can you link the PRs please? Are these still open or have these been used?
@JoelSpeed I think we'll need to add this as a known issue for this release and see if we can get it resolved asap.

[tuunit]

Hi @kvanzuijlen I'll check the issue and a possible fix tomorrow.

[kvanzuijlen]

@tuunit just saw #2197 that should fix this, @sstidl fyi