Creating providers with config #3334

jhrozek · 2024-05-15T13:11:21Z

Summary

This would create a provider with custom configuration.

The OAuth code is generic and can be used by any provider. The GH App config code is very GH App specific, but that whole flow is unlikely to be reusable, so I think that's fine.

With github you can enroll either with oauth:

minder provider enroll -c github -n my-little-oauth-github -o jakubtestorg --config /tmp/github-config.json --yes

or:

minder provider enroll -c github -n my-little-github -o jakubtestorg -t ghp_token --config /tmp/github-config.json --yes

Change Type

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

see above

Review Checklist:

[not at all] Reviewed my own code for quality and clarity.
[nope] Added comments to complex or tricky code sections.
Updated any affected documentation.
[lol no] Included tests that validate the fix or feature.
Checked that related changes are merged.

stacklokbot

✅ No Invisible Unicode Characters Detected.

proto/minder/v1/minder.proto

internal/providers/store.go

internal/providers/manager/manager.go

internal/providers/github/service/service.go

internal/controlplane/handlers_providers.go

internal/providers/manager/manager.go

internal/providers/github/manager/manager.go

cmd/cli/app/provider/provider_enroll.go

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

internal/controlplane/handlers_oauth.go

internal/providers/github/manager/manager.go

dmjb · 2024-05-16T10:08:49Z

internal/providers/manager/manager.go

+		return nil, fmt.Errorf("error getting provider config: %w", err)
+	}
+
+	// This method feels like it should be part of the class manager?


Yes, it probably should :) ... but I would not block on it.

internal/providers/manager/manager.go

cmd/cli/app/provider/provider_enroll.go

internal/controlplane/handlers_providers.go

stacklokbot

✅ No Invisible Unicode Characters Detected.

stacklokbot

✅ No Mixed Scripts Detected.

internal/providers/session/store.go

internal/providers/manager/manager.go

internal/providers/session/service.go

jhrozek · 2024-05-24T11:38:09Z

No longer a draft. Even after splitting some patches out, this is still a relatively large PR. I would welcome ideas on how to split it.

coveralls · 2024-05-24T12:07:58Z

coverage: 52.699% (+0.2%) from 52.48%
when pulling 377d3a3 on jhrozek:prov_cfg_create
into a896873 on stacklok:main.

dmjb · 2024-05-30T16:36:36Z

internal/providers/session/service.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package session contains the business logic for creating providers from session state.


Thought: I wonder if we should move other session state logic in here in future. (not a blocker for this PR)

dmjb · 2024-05-30T16:50:58Z

internal/providers/github/manager/manager.go

+	return g.ghService.GetConfig(ctx, class, userConfig)
+}
+
+func fallbackOAuthClientConfigValues(provider string, cfg *server.OAuthClientConfig) {


Could we do this config-reading logic once at application startup?

jhrozek · 2024-05-31T16:08:35Z

@dmjb ready for another review

…iderTracker In order to be able to add the providerAuthManager in the next step and avoid code duplication, let's make the providerClassManager a little more reusable by extracting the code that tracks registered classes.

Adds the providerAuthManager interface that exposes per-provider OAuth settings. This allows the provider creation in an OAuth callback to let the provider specific code sit in the provider codebase and let the creation be provider agnostic. ALso creates a sessionService that currently allows to create a provider from a session record that is created before the OAuth conversation. Fixes: mindersec#3263

Adds the CLI support for creating an OAuth provider with a config.

Two or more, use a for.

Allows to pass provider configuration when creating a GH app. This will be useful for creating providers with repository auto-enrollment. Fixes: mindersec#3263 Unlike the OAuth flow which is reusable for any provider, the GH App flow is quite GH-app specific and thus uses the original GH app handler.

We added a ProviderConfig-based structure but then in a subsequent patch we overwrite its values unconditionally. Let's only overwrite them when set.

Based on review feedback, to isolate the fallback code.

stacklokbot reviewed May 15, 2024

View reviewed changes