diff --git a/pkg/agent/handler/accessrequest_test.go b/pkg/agent/handler/accessrequest_test.go index 1041be369..93d3d5dad 100644 --- a/pkg/agent/handler/accessrequest_test.go +++ b/pkg/agent/handler/accessrequest_test.go @@ -418,6 +418,9 @@ type mockClient struct { isDeleting bool subError error deleteResCalled bool + getTeamResult []defs.PlatformTeam + getTeamErr error + gotTeamQuery map[string]string t *testing.T } @@ -459,6 +462,11 @@ func (m *mockClient) DeleteResourceInstance(ri v1.Interface) error { return nil } +func (m *mockClient) GetTeam(query map[string]string) ([]defs.PlatformTeam, error) { + m.gotTeamQuery = query + return m.getTeamResult, m.getTeamErr +} + type mockARProvision struct { expectedAccessDetails map[string]interface{} expectedAPIID string diff --git a/pkg/agent/handler/credential.go b/pkg/agent/handler/credential.go index 55772aed9..53951804c 100644 --- a/pkg/agent/handler/credential.go +++ b/pkg/agent/handler/credential.go @@ -218,43 +218,52 @@ func (h *credentials) onDeleting(ctx context.Context, cred *management.Credentia status := h.prov.CredentialDeprovision(provCreds) - h.deprovisionPostProcess(status, provCreds, logger, ctx, cred) + h.deprovisionPostProcess(status, provCreds, logger, ctx, cred, app) } -func (h *credentials) deprovisionPostProcess(status prov.RequestStatus, provCreds *provCreds, logger log.FieldLogger, ctx context.Context, cred *management.Credential) { - if status.GetStatus() == prov.Success { - if provCreds.IsIDPCredential() && !isExternalCredential(cred) { - err := provCreds.idpProvisioner.UnregisterClient() - if err != nil { - logger. - WithError(err). - WithField("client_id", provCreds.idpProvisioner.GetIDPCredentialData().GetClientID()). - WithField("provider", provCreds.GetIDPProvider().GetName()). - Warn("error deprovisioning credential request from IDP, please ask administrator to remove the client from IdP") - } - } - - ri, _ := cred.AsInstance() - h.client.UpdateResourceFinalizer(ri, crFinalizer, "", false) - - // update sub resources when expire - if cred.Metadata.State != v1.ResourceDeleting { - cred.State.Name = v1.Inactive - cred.Status.Level = prov.Success.String() - cred.Status.Reasons = []v1.ResourceStatusReason{} - h.client.CreateSubResource(cred.ResourceMeta, map[string]interface{}{ - "state": cred.State, - }) - h.client.CreateSubResource(cred.ResourceMeta, map[string]interface{}{ - "status": cred.Status, - }) - } - } else { +func (h *credentials) deprovisionPostProcess(status prov.RequestStatus, provCreds *provCreds, logger log.FieldLogger, + ctx context.Context, cred *management.Credential, app *management.ManagedApplication) { + if status.GetStatus() != prov.Success { err := errors.New(status.GetMessage()) logger.WithError(err).Error("request status was not Success, skipping") h.onError(ctx, cred, err) h.client.CreateSubResource(cred.ResourceMeta, cred.SubResources) + return + } + + if provCreds.IsIDPCredential() && !isExternalCredential(cred) { + clientID := provCreds.idpProvisioner.GetIDPCredentialData().GetClientID() + tokenURL := provCreds.GetIDPProvider().GetTokenEndpoint() + providerName := provCreds.GetIDPProvider().GetName() + logger = logger.WithField("client_id", clientID).WithField("provider", providerName).WithField("tokenURL", tokenURL) + + unregisterErr := provCreds.idpProvisioner.UnregisterClient() + if unregisterErr != nil { + logger.WithError(unregisterErr).Warn("error deprovisioning credential request from IDP, please ask administrator to remove the client from IdP") + } else { + if err := removeClientIDFromManagedApp(logger, h.client, app, clientID, tokenURL); err != nil { + logger.WithError(err).Warn("error removing clientID from Managed App clientIDs array") + } + } + } + + ri, _ := cred.AsInstance() + h.client.UpdateResourceFinalizer(ri, crFinalizer, "", false) + + // update sub resources when expire + if cred.Metadata.State == v1.ResourceDeleting { + return } + + cred.State.Name = v1.Inactive + cred.Status.Level = prov.Success.String() + cred.Status.Reasons = []v1.ResourceStatusReason{} + h.client.CreateSubResource(cred.ResourceMeta, map[string]interface{}{ + "state": cred.State, + }) + h.client.CreateSubResource(cred.ResourceMeta, map[string]interface{}{ + "status": cred.Status, + }) } func (h *credentials) onPending(ctx context.Context, cred *management.Credential) *management.Credential { @@ -272,12 +281,10 @@ func (h *credentials) onPending(ctx context.Context, cred *management.Credential return cred } - if provCreds.IsIDPCredential() && !isExternalCredential(cred) { - if err := provCreds.idpProvisioner.RegisterClient(); err != nil { - logger.WithError(err).Error("error provisioning credential request with IDP") - h.onError(ctx, cred, err) - return cred - } + if err := h.registerIDPClient(cred, provCreds, app, logger); err != nil { + logger.WithError(err).Error("error registering IDP client") + h.onError(ctx, cred, err) + return cred } status, credentialData := h.provision(provCreds) @@ -453,10 +460,9 @@ func (h *credentials) onUpdates(ctx context.Context, cred *management.Credential return cred } - // if IDP and rotate the agent will register a new client - if action == prov.Rotate && provCreds.IsIDPCredential() && !isExternalCredential(cred) { - if err := provCreds.idpProvisioner.RegisterClient(); err != nil { - logger.WithError(err).Error("error provisioning credential request with IDP") + if action == prov.Rotate { + if err := h.registerIDPClient(cred, provCreds, app, logger); err != nil { + logger.WithError(err).Error("error registering IDP client") h.onError(ctx, cred, err) return cred } @@ -598,6 +604,24 @@ func (h *credentials) newProvCreds(cr *management.Credential, app *management.Ma return provCred, nil } +// registerIDPClient registers a new IDP client and (for Okta) persists the reference +// on the managed app. It is a no-op when not an IDP credential or is external. +func (h *credentials) registerIDPClient(cred *management.Credential, provCreds *provCreds, app *management.ManagedApplication, logger log.FieldLogger) error { + if !provCreds.IsIDPCredential() || isExternalCredential(cred) { + return nil + } + if err := provCreds.idpProvisioner.RegisterClient(); err != nil { + return fmt.Errorf("error provisioning credential request with IDP: %w", err) + } + if provCreds.GetIDPProvider().GetConfig().GetIDPType() != oauth.TypeOkta { + return nil + } + return persistIDPClientOnManagedApplication(logger, h.client, app, + provCreds.GetIDPCredentialData().GetClientID(), + provCreds.GetIDPProvider().GetTokenEndpoint(), + ) +} + // GetApplicationName gets the name of the managed application func (c provCreds) GetApplicationName() string { return c.managedApp diff --git a/pkg/agent/handler/credential_test.go b/pkg/agent/handler/credential_test.go index 9af9e373e..f08cb0f6b 100644 --- a/pkg/agent/handler/credential_test.go +++ b/pkg/agent/handler/credential_test.go @@ -729,7 +729,7 @@ type mockCredProv struct { func (m *mockCredProv) CredentialProvision(cr prov.CredentialRequest) (status prov.RequestStatus, credentails prov.Credential) { m.expectedProvType = provision v := cr.(*provCreds) - assert.Equal(m.t, m.expectedAppDetails, v.appDetails) + assertMapContains(m.t, v.appDetails, m.expectedAppDetails) assert.Equal(m.t, m.expectedCredDetails, v.credDetails) assert.Equal(m.t, m.expectedManagedApp, v.managedApp) assert.Equal(m.t, m.expectedCredType, v.credType) @@ -739,7 +739,7 @@ func (m *mockCredProv) CredentialProvision(cr prov.CredentialRequest) (status pr func (m *mockCredProv) CredentialDeprovision(cr prov.CredentialRequest) (status prov.RequestStatus) { m.expectedProvType = deprovision v := cr.(*provCreds) - assert.Equal(m.t, m.expectedAppDetails, v.appDetails) + assertMapContains(m.t, v.appDetails, m.expectedAppDetails) assert.Equal(m.t, m.expectedCredDetails, v.credDetails) assert.Equal(m.t, m.expectedManagedApp, v.managedApp) assert.Equal(m.t, m.expectedCredType, v.credType) @@ -749,13 +749,20 @@ func (m *mockCredProv) CredentialDeprovision(cr prov.CredentialRequest) (status func (m *mockCredProv) CredentialUpdate(cr prov.CredentialRequest) (status prov.RequestStatus, credentails prov.Credential) { m.expectedProvType = update v := cr.(*provCreds) - assert.Equal(m.t, m.expectedAppDetails, v.appDetails) + assertMapContains(m.t, v.appDetails, m.expectedAppDetails) assert.Equal(m.t, m.expectedCredDetails, v.credDetails) assert.Equal(m.t, m.expectedManagedApp, v.managedApp) assert.Equal(m.t, m.expectedCredType, v.credType) return m.expectedStatus, &mockProvCredential{} } +func assertMapContains(t *testing.T, actual, expected map[string]interface{}) { + t.Helper() + for k, v := range expected { + assert.Equal(t, v, actual[k], "expected map to contain key %q", k) + } +} + func (m *mockCredProv) GetIgnoredCredentialTypes() []string { return m.ignoredCredTypeNames } @@ -1070,9 +1077,9 @@ func TestExternalCredentialOnPending(t *testing.T) { } p := &mockExternalCredProv{ - t: t, + t: t, expectedStatus: expectedStatus, - provisionErr: tc.provisionErr, + provisionErr: tc.provisionErr, } c := &credClient{ diff --git a/pkg/agent/handler/idpclientlifecycle.go b/pkg/agent/handler/idpclientlifecycle.go new file mode 100644 index 000000000..cdbf5f6a6 --- /dev/null +++ b/pkg/agent/handler/idpclientlifecycle.go @@ -0,0 +1,149 @@ +package handler + +import ( + "context" + "encoding/json" + "fmt" + + management "github.com/Axway/agent-sdk/pkg/apic/apiserver/models/management/v1alpha1" + defs "github.com/Axway/agent-sdk/pkg/apic/definitions" + "github.com/Axway/agent-sdk/pkg/authz/oauth" + "github.com/Axway/agent-sdk/pkg/util" + "github.com/Axway/agent-sdk/pkg/util/log" +) + +const ( + oktaClientIDsAgentDetail = "oktaClientIDs" + tokenURLAgentDetail = "tokenURL" +) + +// persistIDPClientOnManagedApplication stores IDP client IDs on the ManagedApplication x-agent-details +// so they can be cleaned up if the app is deleted before credential-level cleanup runs. +func persistIDPClientOnManagedApplication(logger log.FieldLogger, client client, app *management.ManagedApplication, clientID, tokenURL string) error { + if app == nil || clientID == "" { + return nil + } + + details := util.GetAgentDetails(app) + if details == nil { + details = make(map[string]interface{}) + } + + existing := extractClientIDs(details[oktaClientIDsAgentDetail]) + for _, id := range existing { + if id == clientID { + return nil + } + } + existing = append(existing, clientID) + + details[oktaClientIDsAgentDetail] = existing + if tokenURL != "" { + details[tokenURLAgentDetail] = tokenURL + } + + if err := client.CreateSubResource(app.ResourceMeta, map[string]interface{}{defs.XAgentDetails: details}); err != nil { + return fmt.Errorf("could not persist IDP client reference on managed application %s: %w", app.Name, err) + } + + if logger != nil { + logger.WithField("clientID", clientID).Trace("persisted IDP client reference on managed application") + } + + return nil +} + +func removeClientIDFromManagedApp(logger log.FieldLogger, client client, app *management.ManagedApplication, clientID, tokenURL string) error { + if app == nil || clientID == "" { + return nil + } + + details := util.GetAgentDetails(app) + if details == nil { + return nil + } + + clientIDs := extractClientIDs(details[oktaClientIDsAgentDetail]) + if len(clientIDs) == 0 { + logger.Trace("empty clientIDs agent detail") + return nil + } + + for i := len(clientIDs) - 1; i >= 0; i-- { + if clientIDs[i] != clientID { + continue + } + clientIDs = append(clientIDs[:i], clientIDs[i+1:]...) + details[oktaClientIDsAgentDetail] = clientIDs + if err := client.CreateSubResource(app.ResourceMeta, map[string]interface{}{defs.XAgentDetails: details}); err != nil { + return fmt.Errorf("could not persist IDP client reference on managed application %s: %w", app.Name, err) + } + return nil + } + + logger.WithField("clientID", clientID).Trace("could not find the clientID to remove from managedApp x-agent-details") + return nil +} + +// cleanupManagedApplicationIDPClients removes any tracked IDP clients for a ManagedApplication. +func cleanupManagedApplicationIDPClients(ctx context.Context, logger log.FieldLogger, registry oauth.IdPRegistry, app *management.ManagedApplication) error { + if registry == nil || app == nil { + return nil + } + + details := util.GetAgentDetails(app) + if details == nil { + return nil + } + + clientIDs := extractClientIDs(details[oktaClientIDsAgentDetail]) + if len(clientIDs) == 0 { + return nil + } + + tokenURL := util.ToString(details[tokenURLAgentDetail]) + if tokenURL == "" { + logger.Warn("no tokenURL in managed application x-agent-details, skipping IDP app cleanup") + return nil + } + + provider, err := registry.GetProviderByTokenEndpoint(ctx, tokenURL) + if err != nil || provider == nil { + logger.WithField("tokenURL", tokenURL).Warn("no IDP provider registered for tokenURL, skipping IDP app cleanup") + return nil + } + + for _, clientID := range clientIDs { + if err := provider.UnregisterClient(clientID, "", "", nil, ""); err != nil { + return fmt.Errorf("cleanupManagedApplicationIDPClients: failed for client %s: %w", clientID, err) + } + logger.WithField("clientID", clientID).Info("IDP client unregistered") + } + + return nil +} + +func extractClientIDs(raw interface{}) []string { + switch v := raw.(type) { + case []string: + return append([]string(nil), v...) + case []interface{}: + ids := make([]string, 0, len(v)) + for _, item := range v { + if s, ok := item.(string); ok && s != "" { + ids = append(ids, s) + } + } + return ids + case string: + if v == "" { + return nil + } + var ids []string + if err := json.Unmarshal([]byte(v), &ids); err == nil { + return ids + } + } + + return nil +} diff --git a/pkg/agent/handler/idpclientlifecycle_test.go b/pkg/agent/handler/idpclientlifecycle_test.go new file mode 100644 index 000000000..2ce4c785a --- /dev/null +++ b/pkg/agent/handler/idpclientlifecycle_test.go @@ -0,0 +1,355 @@ +package handler + +import ( + "context" + "errors" + "testing" + "time" + + apiv1 "github.com/Axway/agent-sdk/pkg/apic/apiserver/models/api/v1" + management "github.com/Axway/agent-sdk/pkg/apic/apiserver/models/management/v1alpha1" + defs "github.com/Axway/agent-sdk/pkg/apic/definitions" + "github.com/Axway/agent-sdk/pkg/authz/oauth" + corecfg "github.com/Axway/agent-sdk/pkg/config" + "github.com/Axway/agent-sdk/pkg/util" + "github.com/Axway/agent-sdk/pkg/util/log" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type lifecycleClientMock struct { + createSubCalled bool + createSubErr error + lastSubs map[string]interface{} +} + +func (m *lifecycleClientMock) GetResource(_ string) (*apiv1.ResourceInstance, error) { + return nil, nil +} + +func (m *lifecycleClientMock) UpdateResourceFinalizer(_ *apiv1.ResourceInstance, _, _ string, _ bool) (*apiv1.ResourceInstance, error) { + return nil, nil +} + +func (m *lifecycleClientMock) CreateSubResource(_ apiv1.ResourceMeta, subs map[string]interface{}) error { + m.createSubCalled = true + m.lastSubs = subs + return m.createSubErr +} + +func (m *lifecycleClientMock) UpdateResourceInstance(_ apiv1.Interface) (*apiv1.ResourceInstance, error) { + return nil, nil +} + +func (m *lifecycleClientMock) DeleteResourceInstance(_ apiv1.Interface) error { + return nil +} + +type idpProviderMock struct { + unregisterCalls []string + unregisterErr error +} + +func (m *idpProviderMock) GetName() string { return "mock" } + +func (m *idpProviderMock) GetTitle() string { return "mock" } + +func (m *idpProviderMock) GetIssuer() string { return "issuer" } + +func (m *idpProviderMock) GetTokenEndpoint() string { return "https://idp.example.com/token" } + +func (m *idpProviderMock) GetMTLSTokenEndpoint() string { return "" } + +func (m *idpProviderMock) GetAuthorizationEndpoint() string { return "https://idp.example.com/auth" } + +func (m *idpProviderMock) GetSupportedScopes() []string { return nil } + +func (m *idpProviderMock) GetSupportedGrantTypes() []string { return nil } + +func (m *idpProviderMock) GetSupportedTokenAuthMethods() []string { return nil } + +func (m *idpProviderMock) GetSupportedResponseMethod() []string { return nil } + +func (m *idpProviderMock) RegisterClient(_ oauth.ClientMetadata) (oauth.ClientMetadata, error) { + return nil, nil +} + +func (m *idpProviderMock) UnregisterClient(clientID, _, _ string, _ []string, _ string) error { + m.unregisterCalls = append(m.unregisterCalls, clientID) + return m.unregisterErr +} + +func (m *idpProviderMock) Validate() error { return nil } + +func (m *idpProviderMock) GetConfig() corecfg.IDPConfig { return nil } + +func (m *idpProviderMock) GetMetadata() *oauth.AuthorizationServerMetadata { return nil } + +func (m *idpProviderMock) GetIDPResourceName() string { return "" } + +type idpRegistryMock struct { + provider oauth.Provider + err error + called bool + gotTokenURL string +} + +func newManagedAppForLifecycle(name string) *management.ManagedApplication { + return management.NewManagedApplication(name, "env") +} + +func (m *idpRegistryMock) RegisterProvider(_ context.Context, _ corecfg.IDPConfig, _ corecfg.TLSConfig, _ string, _ time.Duration) error { + return nil +} + +func (m *idpRegistryMock) RegisterProviderWithMetadata(_ context.Context, _ corecfg.IDPConfig, _ *oauth.AuthorizationServerMetadata, _ corecfg.TLSConfig, _ string, _ time.Duration) error { + return nil +} + +func (m *idpRegistryMock) UnregisterProvider(_ context.Context, _ oauth.Provider) error { return nil } + +func (m *idpRegistryMock) GetProviderByName(_ context.Context, _ string, _ ...oauth.ConfigOption) (oauth.Provider, error) { + return nil, nil +} + +func (m *idpRegistryMock) GetProviderByIssuer(_ context.Context, _ string, _ ...oauth.ConfigOption) (oauth.Provider, error) { + return nil, nil +} + +func (m *idpRegistryMock) GetProviderByTokenEndpoint(_ context.Context, tokenEndpoint string, _ ...oauth.ConfigOption) (oauth.Provider, error) { + m.called = true + m.gotTokenURL = tokenEndpoint + return m.provider, m.err +} + +func (m *idpRegistryMock) GetProviderByAuthorizationEndpoint(_ context.Context, _ string, _ ...oauth.ConfigOption) (oauth.Provider, error) { + return nil, nil +} + +func (m *idpRegistryMock) GetProviderByMetadataURL(_ context.Context, _ string, _ ...oauth.ConfigOption) (oauth.Provider, error) { + return nil, nil +} + +func (m *idpRegistryMock) GetIDPResourceName(_ string) (string, bool) { return "", false } + +func TestExtractClientIDs(t *testing.T) { + tests := map[string]struct { + raw interface{} + expect []string + nilResp bool + }{ + "from []string": { + raw: []string{"a", "b"}, + expect: []string{"a", "b"}, + }, + "from []interface{}": { + raw: []interface{}{"a", 1, "b"}, + expect: []string{"a", "b"}, + }, + "from JSON string": { + raw: `["a","b"]`, + expect: []string{"a", "b"}, + }, + "invalid JSON string": { + raw: `not-json`, + nilResp: true, + }, + "nil input": { + raw: nil, + nilResp: true, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + got := extractClientIDs(tt.raw) + if tt.nilResp { + assert.Nil(t, got) + return + } + assert.Equal(t, tt.expect, got) + }) + } +} + +func TestPersistIDPClientOnManagedApplication(t *testing.T) { + tests := map[string]struct { + app *management.ManagedApplication + clientID string + tokenURL string + createSubErr error + wantErr bool + wantCall bool + wantIDs []string + wantTokenURL string + }{ + "nil app": { + clientID: "client-1", + tokenURL: "https://idp.example.com/token", + }, + "empty client ID": { + app: newManagedAppForLifecycle("app"), + tokenURL: "https://idp.example.com/token", + }, + "first client is stored": { + app: newManagedAppForLifecycle("app"), + clientID: "client-1", + tokenURL: "https://idp.example.com/token", + wantCall: true, + wantIDs: []string{"client-1"}, + wantTokenURL: "https://idp.example.com/token", + }, + "duplicate client is ignored": { + app: func() *management.ManagedApplication { + a := newManagedAppForLifecycle("app") + util.SetAgentDetails(a, map[string]interface{}{oktaClientIDsAgentDetail: []interface{}{"client-1"}}) + return a + }(), + clientID: "client-1", + tokenURL: "https://idp.example.com/token", + }, + "second client is appended": { + app: func() *management.ManagedApplication { + a := newManagedAppForLifecycle("app") + util.SetAgentDetails(a, map[string]interface{}{oktaClientIDsAgentDetail: []interface{}{"client-1"}}) + return a + }(), + clientID: "client-2", + tokenURL: "https://idp.example.com/token", + wantCall: true, + wantIDs: []string{"client-1", "client-2"}, + wantTokenURL: "https://idp.example.com/token", + }, + "create subresource error is returned": { + app: newManagedAppForLifecycle("app"), + clientID: "client-1", + tokenURL: "https://idp.example.com/token", + createSubErr: errors.New("write failed"), + wantErr: true, + wantCall: true, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + c := &lifecycleClientMock{createSubErr: tt.createSubErr} + err := persistIDPClientOnManagedApplication(log.NewFieldLogger(), c, tt.app, tt.clientID, tt.tokenURL) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + assert.Equal(t, tt.wantCall, c.createSubCalled) + + if tt.wantCall && !tt.wantErr { + details, ok := c.lastSubs[defs.XAgentDetails].(map[string]interface{}) + require.True(t, ok) + ids, ok := details[oktaClientIDsAgentDetail].([]string) + require.True(t, ok) + assert.Equal(t, tt.wantIDs, ids) + assert.Equal(t, tt.wantTokenURL, details[tokenURLAgentDetail]) + } + }) + } +} + +func TestCleanupManagedApplicationIDPClients(t *testing.T) { + tests := map[string]struct { + app *management.ManagedApplication + registry *idpRegistryMock + wantErr bool + wantRegistryLookup bool + wantUnregisterCalls []string + }{ + "nil registry": { + app: newManagedAppForLifecycle("app"), + }, + "nil app": { + registry: &idpRegistryMock{}, + }, + "no client IDs": { + app: newManagedAppForLifecycle("app"), + registry: &idpRegistryMock{}, + }, + "missing token URL skips cleanup": { + app: func() *management.ManagedApplication { + a := newManagedAppForLifecycle("app") + util.SetAgentDetails(a, map[string]interface{}{oktaClientIDsAgentDetail: []interface{}{"client-1"}}) + return a + }(), + registry: &idpRegistryMock{}, + }, + "provider lookup error is ignored": { + app: func() *management.ManagedApplication { + a := newManagedAppForLifecycle("app") + util.SetAgentDetails(a, map[string]interface{}{ + oktaClientIDsAgentDetail: []interface{}{"client-1"}, + tokenURLAgentDetail: "https://idp.example.com/token", + }) + return a + }(), + registry: &idpRegistryMock{err: errors.New("not found")}, + wantRegistryLookup: true, + }, + "provider is nil is ignored": { + app: func() *management.ManagedApplication { + a := newManagedAppForLifecycle("app") + util.SetAgentDetails(a, map[string]interface{}{ + oktaClientIDsAgentDetail: []interface{}{"client-1"}, + tokenURLAgentDetail: "https://idp.example.com/token", + }) + return a + }(), + registry: &idpRegistryMock{}, + wantRegistryLookup: true, + }, + "unregister error is returned": { + app: func() *management.ManagedApplication { + a := newManagedAppForLifecycle("app") + util.SetAgentDetails(a, map[string]interface{}{ + oktaClientIDsAgentDetail: []string{"client-1"}, + tokenURLAgentDetail: "https://idp.example.com/token", + }) + return a + }(), + registry: &idpRegistryMock{provider: &idpProviderMock{unregisterErr: errors.New("unregister failed")}}, + wantRegistryLookup: true, + wantErr: true, + wantUnregisterCalls: []string{"client-1"}, + }, + "success unregisters all IDs": { + app: func() *management.ManagedApplication { + a := newManagedAppForLifecycle("app") + util.SetAgentDetails(a, map[string]interface{}{ + oktaClientIDsAgentDetail: `["client-1","client-2"]`, + tokenURLAgentDetail: "https://idp.example.com/token", + }) + return a + }(), + registry: &idpRegistryMock{provider: &idpProviderMock{}}, + wantRegistryLookup: true, + wantUnregisterCalls: []string{"client-1", "client-2"}, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + err := cleanupManagedApplicationIDPClients(context.Background(), log.NewFieldLogger(), tt.registry, tt.app) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + + if tt.registry != nil { + assert.Equal(t, tt.wantRegistryLookup, tt.registry.called) + } + + if tt.registry != nil && tt.registry.provider != nil { + if p, ok := tt.registry.provider.(*idpProviderMock); ok { + assert.Equal(t, tt.wantUnregisterCalls, p.unregisterCalls) + } + } + }) + } +} diff --git a/pkg/agent/handler/managedapplication.go b/pkg/agent/handler/managedapplication.go index 50fcf9cef..a35c24ba9 100644 --- a/pkg/agent/handler/managedapplication.go +++ b/pkg/agent/handler/managedapplication.go @@ -3,6 +3,7 @@ package handler import ( "context" "errors" + "fmt" "time" agentcache "github.com/Axway/agent-sdk/pkg/agent/cache" @@ -10,6 +11,7 @@ import ( management "github.com/Axway/agent-sdk/pkg/apic/apiserver/models/management/v1alpha1" defs "github.com/Axway/agent-sdk/pkg/apic/definitions" prov "github.com/Axway/agent-sdk/pkg/apic/provisioning" + "github.com/Axway/agent-sdk/pkg/authz/oauth" "github.com/Axway/agent-sdk/pkg/util" "github.com/Axway/agent-sdk/pkg/watchmanager/proto" ) @@ -18,11 +20,17 @@ const ( maFinalizer = "agent.managedapplication.provisioned" ) +type teamFetcher interface { + GetTeam(query map[string]string) ([]defs.PlatformTeam, error) +} + type managedApplication struct { + idpRegistry oauth.IdPRegistry marketplaceHandler prov prov.ApplicationProvisioner cache agentcache.Manager client client + teamClient teamFetcher retryCount int } @@ -32,6 +40,12 @@ func WithManagedAppRetryCount(rc int) func(c *managedApplication) { } } +func WithManagedAppIDPRegistry(registry oauth.IdPRegistry) func(c *managedApplication) { + return func(c *managedApplication) { + c.idpRegistry = registry + } +} + // NewManagedApplicationHandler creates a Handler for Credentials func NewManagedApplicationHandler(prov prov.ApplicationProvisioner, cache agentcache.Manager, client client, opts ...func(c *managedApplication)) Handler { ma := &managedApplication{ @@ -39,6 +53,9 @@ func NewManagedApplicationHandler(prov prov.ApplicationProvisioner, cache agentc cache: cache, client: client, } + if tc, ok := client.(teamFetcher); ok { + ma.teamClient = tc + } for _, o := range opts { o(ma) } @@ -67,9 +84,13 @@ func (h *managedApplication) Handle(ctx context.Context, meta *proto.EventMeta, return nil } + owner := app.Owner + if owner == nil { + owner = app.Marketplace.Resource.Owner + } ma := provManagedApp{ managedAppName: app.Name, - teamName: getTeamName(h.cache, app.Owner), + teamName: h.resolveTeamName(owner), data: util.GetAgentDetails(app), consumerOrgID: getConsumerOrgID(app), id: app.Metadata.ID, @@ -93,6 +114,7 @@ func (h *managedApplication) onPending(ctx context.Context, app *management.Mana status := h.provision(pma) app.Status = prov.NewStatusReason(status) + util.SetAgentDetailsKey(app, prov.AgentDetailTeamName, pma.GetTeamName()) details := util.MergeMapStringString(util.GetAgentDetailStrings(app), status.GetProperties()) util.SetAgentDetails(app, util.MapStringStringToMapStringInterface(details)) @@ -146,8 +168,13 @@ func (h *managedApplication) provision(pma provManagedApp) prov.RequestStatus { func (h *managedApplication) onDeleting(ctx context.Context, app *management.ManagedApplication, pma provManagedApp) { log := getLoggerFromContext(ctx) + if err := cleanupManagedApplicationIDPClients(ctx, log, h.idpRegistry, app); err != nil { + log.WithError(err).Error("error cleaning up managed application IDP clients") + h.onError(app, err) + h.client.CreateSubResource(app.ResourceMeta, app.SubResources) + return + } status := h.prov.ApplicationRequestDeprovision(pma) - if status.GetStatus() == prov.Success { ri, _ := app.AsInstance() h.client.UpdateResourceFinalizer(ri, maFinalizer, "", false) @@ -206,6 +233,21 @@ func (a provManagedApp) GetConsumerOrgID() string { return a.consumerOrgID } +func (h *managedApplication) resolveTeamName(owner *apiv1.Owner) string { + if name := getTeamName(h.cache, owner); name != "" { + return name + } + if h.teamClient == nil || owner == nil || owner.ID == "" { + return "" + } + teams, err := h.teamClient.GetTeam(map[string]string{"query": fmt.Sprintf("guid==%q", owner.ID)}) + if err != nil || len(teams) == 0 { + return "" + } + h.cache.AddTeam(&teams[0]) + return teams[0].Name +} + func getTeamName(cache getTeamByID, owner *apiv1.Owner) string { teamName := "" if owner != nil && owner.ID != "" { diff --git a/pkg/agent/handler/managedapplication_test.go b/pkg/agent/handler/managedapplication_test.go index db8f004e1..0c0a83478 100644 --- a/pkg/agent/handler/managedapplication_test.go +++ b/pkg/agent/handler/managedapplication_test.go @@ -1,6 +1,7 @@ package handler import ( + "fmt" "testing" agentcache "github.com/Axway/agent-sdk/pkg/agent/cache" @@ -194,6 +195,110 @@ func TestManagedApplicationHandler_deleting(t *testing.T) { } } +func TestManagedApplicationHandlerCacheMiss(t *testing.T) { + cases := map[string]struct { + getTeamResult []defs.PlatformTeam + getTeamErr error + wantTeamName string + }{ + "API returns team on cache miss": { + getTeamResult: []defs.PlatformTeam{{ID: team.ID, Name: teamName}}, + wantTeamName: teamName, + }, + "API returns empty on cache miss": { + getTeamResult: []defs.PlatformTeam{}, + wantTeamName: "", + }, + "API errors on cache miss": { + getTeamErr: fmt.Errorf("network error"), + wantTeamName: "", + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + app := managedAppForTest + app.Status.Level = prov.Pending.String() + + p := &mockManagedAppProv{ + expectedManagedApp: app.Name, + expectedManagedAppData: util.GetAgentDetails(&app), + expectedTeamName: tc.wantTeamName, + status: mock.MockRequestStatus{ + Status: prov.Success, + Msg: "msg", + Properties: map[string]string{"status_key": "status_val"}, + }, + t: t, + } + c := &mockClient{ + expectedStatus: prov.Success.String(), + getTeamResult: tc.getTeamResult, + getTeamErr: tc.getTeamErr, + t: t, + } + // team intentionally NOT pre-loaded into cache to exercise the fallback path + cm := agentcache.NewAgentCacheManager(&config.CentralConfiguration{}, false) + h := NewManagedApplicationHandler(p, cm, c) + + ri, _ := app.AsInstance() + err := h.Handle(NewEventContext(proto.Event_CREATED, nil, ri.Kind, ri.Name), nil, ri) + assert.Nil(t, err) + assert.Equal(t, fmt.Sprintf("guid==%q", team.ID), c.gotTeamQuery["query"]) + }) + } +} + +func TestMPOwnerFallback(t *testing.T) { + cases := map[string]struct { + mpOwner *apiv1.Owner + wantTeamName string + }{ + "MP owner resolves from cache when top-level owner is nil": { + mpOwner: &apiv1.Owner{ID: team.ID}, + wantTeamName: teamName, + }, + "both owners nil yields empty team name": { + mpOwner: nil, + wantTeamName: "", + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + app := management.ManagedApplication{ + ResourceMeta: managedAppForTest.ResourceMeta, + Owner: nil, + Marketplace: management.ManagedApplicationMarketplace{ + Resource: management.ManagedApplicationMarketplaceResource{ + Owner: tc.mpOwner, + }, + }, + Spec: management.ManagedApplicationSpec{}, + Status: &apiv1.ResourceStatus{Level: prov.Pending.String()}, + } + p := &mockManagedAppProv{ + expectedManagedApp: app.Name, + expectedManagedAppData: util.GetAgentDetails(&app), + expectedTeamName: tc.wantTeamName, + status: mock.MockRequestStatus{ + Status: prov.Success, + Msg: "msg", + Properties: map[string]string{"status_key": "status_val"}, + }, + t: t, + } + c := &mockClient{expectedStatus: prov.Success.String(), t: t} + cm := agentcache.NewAgentCacheManager(&config.CentralConfiguration{}, false) + cm.AddTeam(team) + + h := NewManagedApplicationHandler(p, cm, c) + ri, _ := app.AsInstance() + err := h.Handle(NewEventContext(proto.Event_CREATED, nil, ri.Kind, ri.Name), nil, ri) + assert.Nil(t, err) + assert.Nil(t, c.gotTeamQuery, "team was in startup cache — no API call expected") + }) + } +} + func TestManagedApplicationHandler_wrong_kind(t *testing.T) { cm := agentcache.NewAgentCacheManager(&config.CentralConfiguration{}, false) c := &mockClient{ diff --git a/pkg/agent/provisioning.go b/pkg/agent/provisioning.go index d7b8b644c..6d93eee97 100644 --- a/pkg/agent/provisioning.go +++ b/pkg/agent/provisioning.go @@ -604,10 +604,12 @@ func CleanApplicationProfileDefinition(name string) error { // application provisioner func registerApplicationProvisioner(provisioner interface{}) { if appProv, ok := provisioner.(provisioning.ApplicationProvisioner); ok { + registry := oauth.NewIdpRegistry(oauth.WithProviderRegistry(GetAuthProviderRegistry())) agent.proxyResourceHandler.RegisterTargetHandler( "managedappHandler", handler.NewManagedApplicationHandler(appProv, agent.cacheManager, agent.apicClient, - handler.WithManagedAppRetryCount(agent.cfg.GetProvisioningRetryCount())), + handler.WithManagedAppRetryCount(agent.cfg.GetProvisioningRetryCount()), + handler.WithManagedAppIDPRegistry(registry)), ) } } diff --git a/pkg/apic/provisioning/definitions.go b/pkg/apic/provisioning/definitions.go index 6f556407a..a3a516789 100644 --- a/pkg/apic/provisioning/definitions.go +++ b/pkg/apic/provisioning/definitions.go @@ -29,7 +29,8 @@ const ( OauthTLSAuthSANURI = "tlsClientAuthSanURI" OauthRegistrationToken = "registration" - IDPTokenURL = "idpTokenURL" + IDPTokenURL = "idpTokenURL" + AgentDetailTeamName = "teamName" APIKey = "apiKey" diff --git a/pkg/apic/provisioning/idp/provisioner.go b/pkg/apic/provisioning/idp/provisioner.go index 3aedc6d35..4a80c69c0 100644 --- a/pkg/apic/provisioning/idp/provisioner.go +++ b/pkg/apic/provisioning/idp/provisioner.go @@ -2,6 +2,8 @@ package idp import ( "context" + "fmt" + "strings" management "github.com/Axway/agent-sdk/pkg/apic/apiserver/models/management/v1alpha1" "github.com/Axway/agent-sdk/pkg/apic/provisioning" @@ -11,7 +13,7 @@ import ( ) const ( - IDPTokenURL = "idpTokenURL" + registrationClientURIKey = "registrationClientURI" ) type Provisioner interface { @@ -46,7 +48,7 @@ func NewProvisioner(ctx context.Context, idpRegistry oauth.IdPRegistry, app *man p.provisioningMode = credential.Spec.Provision.Mode } - idpTokenURL, ok := p.credential.Spec.Data[IDPTokenURL].(string) + idpTokenURL, ok := p.credential.Spec.Data[provisioning.IDPTokenURL].(string) if ok && idpRegistry != nil { idpProvider, err := idpRegistry.GetProviderByTokenEndpoint(ctx, idpTokenURL, opts...) if err != nil { @@ -116,9 +118,13 @@ func (p *provisioner) RegisterClient() error { return nil } - // prepare external client metadata from CRD data + clientName, err := p.appClientName() + if err != nil { + return err + } + builder := oauth.NewClientMetadataBuilder(). - SetClientName(p.credential.GetName()). + SetClientName(clientName). SetScopes(p.credentialData.GetScopes()). SetGrantTypes(p.credentialData.GetGrantTypes()). SetTokenEndpointAuthMethod(p.credentialData.GetTokenEndpointAuthMethod()). @@ -144,7 +150,6 @@ func (p *provisioner) RegisterClient() error { return err } - // provision external client resClientMetadata, err := p.idpProvider.RegisterClient(clientMetadata) if err != nil { return err @@ -155,7 +160,7 @@ func (p *provisioner) RegisterClient() error { p.credentialData.clientSecret = resClientMetadata.GetClientSecret() if resClientMetadata.GetRegistrationClientURI() != "" { - util.SetAgentDetailsKey(p.credential, "registrationClientURI", resClientMetadata.GetRegistrationClientURI()) + util.SetAgentDetailsKey(p.credential, registrationClientURIKey, resClientMetadata.GetRegistrationClientURI()) } return nil @@ -166,9 +171,15 @@ func (p *provisioner) UnregisterClient() error { return nil } - registrationClientURI, _ := util.GetAgentDetailsValue(p.credential, "registrationClientURI") + registrationClientURI, _ := util.GetAgentDetailsValue(p.credential, registrationClientURIKey) + + scopes := p.credentialData.GetScopes() + grantType := "" + if gt := p.credentialData.GetGrantTypes(); len(gt) > 0 { + grantType = gt[0] + } - err := p.idpProvider.UnregisterClient(p.credentialData.GetClientID(), p.credentialData.registrationAccessToken, registrationClientURI) + err := p.idpProvider.UnregisterClient(p.credentialData.GetClientID(), p.credentialData.registrationAccessToken, registrationClientURI, scopes, grantType) if err != nil { return err } @@ -237,3 +248,28 @@ func (p *provisioner) getRegistrationTokenFromAgentDetails() string { registrationToken, _ := util.GetAgentDetailsValue(p.credential, provisioning.OauthRegistrationToken) return registrationToken } + +func (p *provisioner) appClientName() (string, error) { + idpCfg := p.idpProvider.GetConfig() + if idpCfg == nil || idpCfg.GetIDPType() != oauth.TypeOkta { + return p.credential.GetName(), nil + } + oktaCfg, ok := idpCfg.(interface{ GetAppNameTemplate() string }) + if !ok { + return p.credential.GetName(), nil + } + + template := oktaCfg.GetAppNameTemplate() + teamName, _ := util.GetAgentDetailsValue(p.app, provisioning.AgentDetailTeamName) + name := strings.NewReplacer( + config.OktaPlaceholderMPApplicationName, p.app.Name, + config.OktaPlaceholderOwningTeam, teamName, + config.OktaPlaceholderCredentialName, p.credential.GetName(), + ).Replace(template) + + name = util.NormalizeNameForCentral(name) + if len(name) > 100 { + return "", fmt.Errorf("Okta app name exceeds 100-character limit after normalization: %d chars", len(name)) + } + return name, nil +} diff --git a/pkg/apic/provisioning/idp/provisioner_test.go b/pkg/apic/provisioning/idp/provisioner_test.go index e304c6b5a..65ade16fc 100644 --- a/pkg/apic/provisioning/idp/provisioner_test.go +++ b/pkg/apic/provisioning/idp/provisioner_test.go @@ -4,6 +4,7 @@ import ( "context" "net/http" "os" + "strings" "testing" "time" @@ -15,6 +16,12 @@ import ( "github.com/stretchr/testify/assert" ) +const ( + testCredName = "my-cred" + testAppName = "my-app" + fullTemplate = config.OktaPlaceholderMPApplicationName + "-" + config.OktaPlaceholderOwningTeam + "-" + config.OktaPlaceholderCredentialName +) + func TestProvisioner(t *testing.T) { publicKey, err := os.ReadFile("../../../authz/oauth/testdata/publickey") assert.Nil(t, err) @@ -25,7 +32,7 @@ func TestProvisioner(t *testing.T) { defer s.Close() idpCfg := &config.IDPConfiguration{ Name: "test", - Type: "generic", + Type: oauth.TypeGeneric, AuthConfig: &config.IDPAuthConfiguration{ Type: config.Client, ClientID: "test", @@ -46,8 +53,7 @@ func TestProvisioner(t *testing.T) { err = idpReg.RegisterProvider(context.Background(), idpCfg, config.NewTLSConfig(), "", 30*time.Second) assert.Nil(t, err) - cases := []struct { - name string + cases := map[string]struct { idpType string appKey string credTokenURL string @@ -60,14 +66,12 @@ func TestProvisioner(t *testing.T) { expectRegistrationErr bool expectUnRegistrationErr bool }{ - { - name: "provisioner for non-IdP credential", + "provisioner for non-IdP credential": { credTokenURL: "", registrationResponseCode: http.StatusCreated, unRegistrationResponseCode: http.StatusNoContent, }, - { - name: "provisioner for IdP credential with client_credential", + "provisioner for IdP credential with client_credential": { credTokenURL: s.GetTokenURL(), tokenAuthMethod: config.ClientSecretPost, registrationResponseCode: http.StatusCreated, @@ -75,16 +79,14 @@ func TestProvisioner(t *testing.T) { appKey: "test-app-key", useRegistrationAccessToken: true, }, - { - name: "provisioner for IdP credential with private_key_jwt", + "provisioner for IdP credential with private_key_jwt": { credTokenURL: s.GetTokenURL(), tokenAuthMethod: config.PrivateKeyJWT, registrationResponseCode: http.StatusCreated, unRegistrationResponseCode: http.StatusNoContent, publicKey: string(publicKey), }, - { - name: "provisioner for IdP credential with tls_client_auth", + "provisioner for IdP credential with tls_client_auth": { credTokenURL: s.GetTokenURL(), tokenAuthMethod: config.TLSClientAuth, registrationResponseCode: http.StatusCreated, @@ -92,13 +94,13 @@ func TestProvisioner(t *testing.T) { certificate: string(certificate), }, } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { + for name, tc := range cases { + t.Run(name, func(t *testing.T) { app := management.NewManagedApplication("", "") app.Spec.Security.EncryptionKey = tc.appKey cred := management.NewCredential("", "") - cred.Spec.Data = map[string]interface{}{ - IDPTokenURL: tc.credTokenURL, + cred.Spec.Data = map[string]any{ + provisioning.IDPTokenURL: tc.credTokenURL, provisioning.OauthTokenAuthMethod: tc.tokenAuthMethod, provisioning.OauthJwks: tc.publicKey, provisioning.OauthCertificate: tc.certificate, @@ -134,7 +136,7 @@ func TestProvisioner(t *testing.T) { assert.NotEmpty(t, cr.registrationAccessToken) assert.NotEmpty(t, details) } - cred.Data = map[string]interface{}{ + cred.Data = map[string]any{ provisioning.OauthClientID: data.GetClientID(), provisioning.OauthRegistrationToken: cr.registrationAccessToken, } @@ -154,3 +156,144 @@ func TestProvisioner(t *testing.T) { }) } } + +type mockProvider struct { + cfg config.IDPConfig + capturedScopes []string + capturedGrant string + capturedName string +} + +func (m *mockProvider) GetName() string { return "" } +func (m *mockProvider) GetTitle() string { return "" } +func (m *mockProvider) GetIssuer() string { return "" } +func (m *mockProvider) GetTokenEndpoint() string { return "" } +func (m *mockProvider) GetMTLSTokenEndpoint() string { return "" } +func (m *mockProvider) GetAuthorizationEndpoint() string { return "" } +func (m *mockProvider) GetSupportedScopes() []string { return nil } +func (m *mockProvider) GetSupportedGrantTypes() []string { return nil } +func (m *mockProvider) GetSupportedTokenAuthMethods() []string { return nil } +func (m *mockProvider) GetSupportedResponseMethod() []string { return nil } +func (m *mockProvider) Validate() error { return nil } +func (m *mockProvider) GetConfig() config.IDPConfig { return m.cfg } +func (m *mockProvider) GetMetadata() *oauth.AuthorizationServerMetadata { return nil } +func (m *mockProvider) GetIDPResourceName() string { return "" } + +func (m *mockProvider) RegisterClient(meta oauth.ClientMetadata) (oauth.ClientMetadata, error) { + m.capturedName = meta.GetClientName() + result, err := oauth.NewClientMetadataBuilder().SetClientName(meta.GetClientName()).Build() + return result, err +} + +func (m *mockProvider) UnregisterClient(clientID, accessToken, registrationClientURI string, scopes []string, grantType string) error { + m.capturedScopes = scopes + m.capturedGrant = grantType + return nil +} + +func TestScopeGrantTypeThreading(t *testing.T) { + cases := map[string]struct { + scopes []string + grantType string + }{ + "multiple scopes with client credentials grant are passed to provider": { + scopes: []string{"scope1", "scope2"}, + grantType: oauth.GrantTypeClientCredentials, + }, + "single scope with authorization code grant is passed to provider": { + scopes: []string{"read:api"}, + grantType: oauth.GrantTypeAuthorizationCode, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + mock := &mockProvider{cfg: nil} + cred := management.NewCredential("", "") + p := &provisioner{ + app: management.NewManagedApplication("", ""), + credential: cred, + idpProvider: mock, + credentialData: &credData{ + scopes: tc.scopes, + grantTypes: []string{tc.grantType}, + }, + } + assert.NoError(t, p.UnregisterClient()) + assert.Equal(t, tc.scopes, mock.capturedScopes) + assert.Equal(t, tc.grantType, mock.capturedGrant) + }) + } +} + +func TestAppNameTemplate(t *testing.T) { + cases := map[string]struct { + cfg config.IDPConfig + appName string + teamName string + credName string + wantName string + wantErr bool + }{ + "nil config falls back to credential name": { + cfg: nil, + credName: testCredName, + wantName: testCredName, + }, + "non-okta IDP type falls back to credential name": { + cfg: &config.IDPConfiguration{Type: oauth.TypeGeneric}, + credName: testCredName, + wantName: testCredName, + }, + "template with all fields set": { + cfg: &config.IDPConfiguration{Type: oauth.TypeOkta, Okta: &config.OktaIDPConfiguration{AppNameTemplate: fullTemplate}}, + appName: testAppName, + teamName: "my-team", + credName: testCredName, + wantName: "my-app-my-team-my-cred", + }, + "empty team name collapses double dash after normalize": { + cfg: &config.IDPConfiguration{Type: oauth.TypeOkta, Okta: &config.OktaIDPConfiguration{AppNameTemplate: fullTemplate}}, + appName: testAppName, + teamName: "", + credName: testCredName, + wantName: "my-app-my-cred", + }, + "name exactly 100 chars passes": { + cfg: &config.IDPConfiguration{Type: oauth.TypeOkta, Okta: &config.OktaIDPConfiguration{AppNameTemplate: fullTemplate}}, + appName: strings.Repeat("a", 48), + teamName: strings.Repeat("b", 49), + credName: "c", + wantName: strings.Repeat("a", 48) + "-" + strings.Repeat("b", 49) + "-c", + }, + "name exceeds 100 chars returns error": { + cfg: &config.IDPConfiguration{Type: oauth.TypeOkta, Okta: &config.OktaIDPConfiguration{AppNameTemplate: fullTemplate}}, + appName: strings.Repeat("a", 50), + teamName: strings.Repeat("b", 49), + credName: strings.Repeat("c", 5), + wantErr: true, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + mock := &mockProvider{cfg: tc.cfg} + app := management.NewManagedApplication(tc.appName, "") + if tc.teamName != "" { + assert.NoError(t, util.SetAgentDetailsKey(app, provisioning.AgentDetailTeamName, tc.teamName)) + } + cred := management.NewCredential(tc.credName, "") + p := &provisioner{ + app: app, + credential: cred, + idpProvider: mock, + credentialData: &credData{}, + } + got, err := p.appClientName() + if tc.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, tc.wantName, got) + }) + } +} diff --git a/pkg/authz/oauth/clients/okta.go b/pkg/authz/oauth/clients/okta.go index 17a5a0231..e5d26bff8 100644 --- a/pkg/authz/oauth/clients/okta.go +++ b/pkg/authz/oauth/clients/okta.go @@ -13,13 +13,15 @@ import ( const OktaAuthHeaderPrefix = "SSWS" -var logger = log.NewFieldLogger().WithComponent("oktaClient").WithPackage("sdk.agent.authz.oauth.clients") - -// Okta wraps Okta Management API calls. type Okta struct { BaseURL string APIToken string Client coreapi.Client + logger log.FieldLogger +} + +type oktaGroupProfile struct { + Name string `json:"name"` } type oktaGroupSearchResult struct { @@ -27,15 +29,64 @@ type oktaGroupSearchResult struct { Profile oktaGroupProfile `json:"profile"` } -type oktaGroupProfile struct { - Name string `json:"name"` -} - type oktaPolicyListResult struct { ID string `json:"id"` Name string `json:"name"` } +type oktaPolicyConditionsClients struct { + Include []string `json:"include"` +} + +type oktaPolicyConditions struct { + Clients *oktaPolicyConditionsClients `json:"clients,omitempty"` +} + +type oktaCreatePolicyRequest struct { + Name string `json:"name"` + Type string `json:"type"` + Status string `json:"status"` + Priority int `json:"priority"` + Conditions oktaPolicyConditions `json:"conditions"` +} + +type oktaPolicyRuleConditionGroups struct { + Include []string `json:"include"` +} + +type oktaPolicyRuleConditionPeople struct { + Groups oktaPolicyRuleConditionGroups `json:"groups"` +} + +type oktaPolicyRuleConditionGrantTypes struct { + Include []string `json:"include"` +} + +type oktaPolicyRuleConditionScopes struct { + Include []string `json:"include"` +} + +type oktaPolicyRuleConditions struct { + People oktaPolicyRuleConditionPeople `json:"people"` + GrantTypes oktaPolicyRuleConditionGrantTypes `json:"grantTypes"` + Scopes oktaPolicyRuleConditionScopes `json:"scopes"` +} + +type oktaPolicyRuleActionToken struct { + AccessTokenLifetimeMinutes int `json:"accessTokenLifetimeMinutes"` +} + +type oktaPolicyRuleActions struct { + Token oktaPolicyRuleActionToken `json:"token"` +} + +type oktaCreatePolicyRuleRequest struct { + Name string `json:"name"` + Type string `json:"type"` + Conditions oktaPolicyRuleConditions `json:"conditions"` + Actions oktaPolicyRuleActions `json:"actions"` +} + func New(apiClient coreapi.Client, baseURL, apiToken string) *Okta { if apiClient == nil { apiClient = coreapi.NewClient(nil, "") @@ -44,6 +95,7 @@ func New(apiClient coreapi.Client, baseURL, apiToken string) *Okta { BaseURL: baseURL, APIToken: apiToken, Client: apiClient, + logger: log.NewFieldLogger().WithComponent("oktaClient").WithPackage("sdk.agent.authz.oauth.clients"), } } @@ -108,73 +160,7 @@ func isStatus(code int, allowed ...int) bool { return false } - - -func (o *Okta) FindGroupByName(groupName string) (string, error) { - endpoint := fmt.Sprintf("%s/api/v1/groups?q=%s", o.BaseURL, url.QueryEscape(groupName)) - resp, err := o.doRequest(coreapi.GET, endpoint, nil) - if err != nil { - return "", err - } - if !isStatus(resp.Code, http.StatusOK) { - return "", o.unexpectedStatusError(coreapi.GET, endpoint, resp) - } - - var groups []oktaGroupSearchResult - if err := json.Unmarshal(resp.Body, &groups); err != nil { - return "", err - } - for _, g := range groups { - if g.Profile.Name == groupName { - return g.ID, nil - } - } - return "", nil -} - -func (o *Okta) AssignGroupToApp(appID, groupID string) error { - endpoint := fmt.Sprintf("%s/api/v1/apps/%s/groups/%s", o.BaseURL, appID, groupID) - resp, err := o.doRequest(coreapi.PUT, endpoint, nil) - if err != nil { - return err - } - if resp.Code == http.StatusConflict { - logger. - WithField("appID", appID). - WithField("groupID", groupID). - Warn("group assignment already exists") - return nil - } - if !isStatus(resp.Code, http.StatusOK, http.StatusCreated, http.StatusNoContent) { - return o.unexpectedStatusError(coreapi.PUT, endpoint, resp) - } - return nil -} - -func (o *Okta) UnassignGroupFromApp(appID, groupID string) error { - endpoint := fmt.Sprintf("%s/api/v1/apps/%s/groups/%s", o.BaseURL, appID, groupID) - resp, err := o.doRequest(coreapi.DELETE, endpoint, nil) - if err != nil { - return err - } - if resp.Code == http.StatusNotFound { - logger. - WithField("appID", appID). - WithField("groupID", groupID). - Warn("group assignment not found during unassign. Assuming already unassigned") - return nil - } - if !isStatus(resp.Code, http.StatusOK, http.StatusNoContent) { - return o.unexpectedStatusError(coreapi.DELETE, endpoint, resp) - } - return nil -} - -// FindPolicyByName returns the policy object for the given policy name on the authorization server. -// Returns nil if not found. -// -// Note: This does a list call to locate the policy ID and then retrieves the policy by ID -// so callers can update it without needing an additional fetch. +// Two-step: list to find the policy ID, then fetch the full object so callers can update it without an extra round-trip. func (o *Okta) FindPolicyByName(authServerID, policyName string) (map[string]interface{}, error) { policyName = strings.TrimSpace(policyName) if authServerID == "" || policyName == "" { @@ -206,7 +192,6 @@ func (o *Okta) FindPolicyByName(authServerID, policyName string) (map[string]int return policy, nil } -// UpdatePolicy updates an existing authorization server policy. func (o *Okta) UpdatePolicy(authServerID, policyID string, policy map[string]interface{}) error { if authServerID == "" || policyID == "" { return nil @@ -222,23 +207,20 @@ func (o *Okta) UpdatePolicy(authServerID, policyID string, policy map[string]int return nil } -// AssignClientToPolicy updates the policy-level "Assigned to clients" list to include the given client. -// If the policy is already assigned to ALL_CLIENTS or already includes the client, it no-ops. -// -// The policy map is modified in-place and then persisted via UpdatePolicy. +// No-ops if the policy already includes the client or is assigned to ALL_CLIENTS. func (o *Okta) AssignClientToPolicy(authServerID string, policy map[string]interface{}, clientID string) error { clientID = strings.TrimSpace(clientID) if authServerID == "" || policy == nil || clientID == "" { return fmt.Errorf("invalid input for policy assignment") } - + policyID, _ := policy["id"].(string) policyID = strings.TrimSpace(policyID) if policyID == "" { return fmt.Errorf("invalid input for policy assignment") } - policyLogger := logger. + policyLogger := o.logger. WithField("authServerID", authServerID). WithField("policyID", policyID). WithField("clientID", clientID) @@ -258,13 +240,11 @@ func (o *Okta) AssignClientToPolicy(authServerID string, policy map[string]inter return nil } - // check if policy has all clients configured if includeHasAllClients(include) { policyLogger.Trace("policy assignment already includes ALL_CLIENTS. Skipping client-specific policy update") return nil } - // check if client is already included in policy assignment if includeHasClient(include, clientID) { policyLogger.Trace("policy assignment already includes client. Skipping client-specific policy update") return nil @@ -273,6 +253,275 @@ func (o *Okta) AssignClientToPolicy(authServerID string, policy map[string]inter return o.UpdatePolicy(authServerID, policyID, policy) } +func (o *Okta) CreatePolicy(authServerID, name string, clientID string) (map[string]interface{}, error) { + endpoint := o.authServerPoliciesEndpoint(authServerID) + o.logger. + WithField("authServerID", authServerID). + WithField("policyName", name). + WithField("endpoint", endpoint). + Trace("creating Okta authorization server policy") + req := oktaCreatePolicyRequest{ + Name: name, + Type: "OAUTH_AUTHORIZATION_POLICY", + Status: "ACTIVE", + Priority: 1, + Conditions: oktaPolicyConditions{ + Clients: &oktaPolicyConditionsClients{Include: []string{clientID}}, + }, + } + resp, err := o.doRequest(coreapi.POST, endpoint, req) + if err != nil { + return nil, err + } + if !isStatus(resp.Code, http.StatusCreated) { + return nil, o.unexpectedStatusError(coreapi.POST, endpoint, resp) + } + var policy map[string]interface{} + if err := json.Unmarshal(resp.Body, &policy); err != nil { + return nil, err + } + return policy, nil +} + +func (o *Okta) CreatePolicyRule(authServerID, policyID, name, grantType, scope string) error { + endpoint := fmt.Sprintf("%s/api/v1/authorizationServers/%s/policies/%s/rules", o.BaseURL, authServerID, policyID) + o.logger. + WithField("authServerID", authServerID). + WithField("policyID", policyID). + WithField("ruleName", name). + WithField("endpoint", endpoint). + Trace("creating Okta authorization server policy rule") + req := oktaCreatePolicyRuleRequest{ + Name: name, + Type: "RESOURCE_ACCESS", + Conditions: oktaPolicyRuleConditions{ + People: oktaPolicyRuleConditionPeople{Groups: oktaPolicyRuleConditionGroups{Include: []string{"EVERYONE"}}}, + GrantTypes: oktaPolicyRuleConditionGrantTypes{Include: []string{grantType}}, + Scopes: oktaPolicyRuleConditionScopes{Include: []string{scope}}, + }, + Actions: oktaPolicyRuleActions{ + Token: oktaPolicyRuleActionToken{AccessTokenLifetimeMinutes: 60}, + }, + } + resp, err := o.doRequest(coreapi.POST, endpoint, req) + if err != nil { + return err + } + if !isStatus(resp.Code, http.StatusCreated) { + return o.unexpectedStatusError(coreapi.POST, endpoint, resp) + } + return nil +} + +// RemoveClientFromPolicy removes clientID from the policy's conditions.clients.include list and updates +// the policy via the Okta API. If the client is not present the call is a no-op. The caller is +// responsible for deleting the policy when the include list becomes empty. +func (o *Okta) RemoveClientFromPolicy(authServerID string, policy map[string]interface{}, clientID string) error { + clientID = strings.TrimSpace(clientID) + policyID, _ := policy["id"].(string) + policyID = strings.TrimSpace(policyID) + if authServerID == "" || policyID == "" || clientID == "" { + return fmt.Errorf("invalid input for client removal from policy") + } + o.logger. + WithField("authServerID", authServerID). + WithField("policyID", policyID). + WithField("clientID", clientID). + Trace("removing client from Okta authorization server policy") + + conditions := ensureMap(policy, "conditions") + clients := ensureMap(conditions, "clients") + + includeRaw, _ := clients["include"] + include, ok := includeRaw.([]interface{}) + if !ok || !includeHasClient(include, clientID) { + return nil + } + + filtered := make([]interface{}, 0, len(include)) + for _, v := range include { + s, _ := v.(string) + if strings.TrimSpace(s) != clientID { + filtered = append(filtered, v) + } + } + clients["include"] = filtered + return o.UpdatePolicy(authServerID, policyID, policy) +} + +// DeactivateApp deactivates an Okta application. A 404 response is treated as success. +// DeactivateApp must be called before DeleteApp. +func (o *Okta) DeactivateApp(appID string) error { + endpoint := fmt.Sprintf("%s/api/v1/apps/%s/lifecycle/deactivate", o.BaseURL, appID) + o.logger.WithField("appID", appID).WithField("endpoint", endpoint).Trace("deactivating Okta app") + resp, err := o.doRequest(coreapi.POST, endpoint, nil) + if err != nil { + return err + } + if isStatus(resp.Code, http.StatusNotFound) { + return nil + } + if !isStatus(resp.Code, http.StatusOK, http.StatusNoContent) { + return o.unexpectedStatusError(coreapi.POST, endpoint, resp) + } + return nil +} + +// DeleteApp deletes an Okta application. A 404 response is treated as success. +// DeactivateApp must be called before this method. +func (o *Okta) DeleteApp(appID string) error { + endpoint := fmt.Sprintf("%s/api/v1/apps/%s", o.BaseURL, appID) + o.logger.WithField("appID", appID).WithField("endpoint", endpoint).Trace("deleting Okta app") + resp, err := o.doRequest(coreapi.DELETE, endpoint, nil) + if err != nil { + return err + } + if isStatus(resp.Code, http.StatusNotFound) { + return nil + } + if !isStatus(resp.Code, http.StatusNoContent) { + return o.unexpectedStatusError(coreapi.DELETE, endpoint, resp) + } + return nil +} + +// ActivatePolicy activates an Okta authorization server policy. A 404 is treated as success. +func (o *Okta) ActivatePolicy(authServerID, policyID string) error { + endpoint := fmt.Sprintf("%s/api/v1/authorizationServers/%s/policies/%s/lifecycle/activate", o.BaseURL, authServerID, policyID) + o.logger.WithField("authServerID", authServerID).WithField("policyID", policyID).WithField("endpoint", endpoint).Trace("activating Okta authorization server policy") + resp, err := o.doRequest(coreapi.POST, endpoint, nil) + if err != nil { + return err + } + if isStatus(resp.Code, http.StatusNotFound) { + return nil + } + if !isStatus(resp.Code, http.StatusOK, http.StatusNoContent) { + return o.unexpectedStatusError(coreapi.POST, endpoint, resp) + } + return nil +} + +// PolicyHasRuleForScope reports whether the given Okta authorization server policy has at least +// one rule that grants the specified scope. +func (o *Okta) PolicyHasRuleForScope(authServerID, policyID, scope string) (bool, error) { + endpoint := fmt.Sprintf("%s/api/v1/authorizationServers/%s/policies/%s/rules", o.BaseURL, authServerID, policyID) + o.logger. + WithField("authServerID", authServerID). + WithField("policyID", policyID). + WithField("scope", scope). + WithField("endpoint", endpoint). + Trace("checking Okta authorization server policy rules for scope") + + var rules []struct { + Conditions struct { + Scopes oktaPolicyRuleConditionScopes `json:"scopes"` + } `json:"conditions"` + } + if err := o.doGetJSON(endpoint, &rules); err != nil { + return false, err + } + for _, rule := range rules { + for _, s := range rule.Conditions.Scopes.Include { + if strings.TrimSpace(s) == scope { + return true, nil + } + } + } + return false, nil +} + +// DeactivatePolicy deactivates an Okta authorization server policy. A 404 is treated as success. +// DeactivatePolicy must be called before DeletePolicy. +func (o *Okta) DeactivatePolicy(authServerID, policyID string) error { + endpoint := fmt.Sprintf("%s/api/v1/authorizationServers/%s/policies/%s/lifecycle/deactivate", o.BaseURL, authServerID, policyID) + o.logger.WithField("authServerID", authServerID).WithField("policyID", policyID).WithField("endpoint", endpoint).Trace("deactivating Okta authorization server policy") + resp, err := o.doRequest(coreapi.POST, endpoint, nil) + if err != nil { + return err + } + if isStatus(resp.Code, http.StatusNotFound) { + return nil + } + if !isStatus(resp.Code, http.StatusOK, http.StatusNoContent) { + return o.unexpectedStatusError(coreapi.POST, endpoint, resp) + } + return nil +} + +// DeletePolicy deletes an Okta authorization server policy. A 404 is treated as success. +// DeactivatePolicy must be called before this method. +func (o *Okta) DeletePolicy(authServerID, policyID string) error { + endpoint := o.authServerPolicyEndpoint(authServerID, policyID) + o.logger.WithField("authServerID", authServerID).WithField("policyID", policyID).WithField("endpoint", endpoint).Trace("deleting Okta authorization server policy") + resp, err := o.doRequest(coreapi.DELETE, endpoint, nil) + if err != nil { + return err + } + if isStatus(resp.Code, http.StatusNotFound) { + return nil + } + if !isStatus(resp.Code, http.StatusNoContent) { + return o.unexpectedStatusError(coreapi.DELETE, endpoint, resp) + } + return nil +} + +// FindGroupByName searches Okta for a group with the given name and returns its ID. +// Returns ("", nil) when no matching group exists. +func (o *Okta) FindGroupByName(groupName string) (string, error) { + groupName = strings.TrimSpace(groupName) + if groupName == "" { + return "", nil + } + endpoint := fmt.Sprintf("%s/api/v1/groups?q=%s", o.BaseURL, url.QueryEscape(groupName)) + o.logger.WithField("groupName", groupName).WithField("endpoint", endpoint).Trace("searching for Okta group") + + var groups []oktaGroupSearchResult + if err := o.doGetJSON(endpoint, &groups); err != nil { + return "", err + } + + for _, g := range groups { + if g.Profile.Name == groupName { + return g.ID, nil + } + } + return "", nil +} + +// AssignGroupToApp adds an Okta application to the given group. +func (o *Okta) AssignGroupToApp(appID, groupID string) error { + endpoint := fmt.Sprintf("%s/api/v1/apps/%s/groups/%s", o.BaseURL, appID, groupID) + o.logger.WithField("appID", appID).WithField("groupID", groupID).WithField("endpoint", endpoint).Trace("assigning group to Okta app") + resp, err := o.doRequest(coreapi.PUT, endpoint, nil) + if err != nil { + return err + } + if !isStatus(resp.Code, http.StatusOK, http.StatusCreated) { + return o.unexpectedStatusError(coreapi.PUT, endpoint, resp) + } + return nil +} + +// UnassignGroupFromApp removes an Okta application from the given group. +// A 404 response is treated as success (app or group already gone). +func (o *Okta) UnassignGroupFromApp(appID, groupID string) error { + endpoint := fmt.Sprintf("%s/api/v1/apps/%s/groups/%s", o.BaseURL, appID, groupID) + o.logger.WithField("appID", appID).WithField("groupID", groupID).WithField("endpoint", endpoint).Trace("removing app from Okta group") + resp, err := o.doRequest(coreapi.DELETE, endpoint, nil) + if err != nil { + return err + } + if isStatus(resp.Code, http.StatusNotFound) { + return nil + } + if !isStatus(resp.Code, http.StatusNoContent, http.StatusOK) { + return o.unexpectedStatusError(coreapi.DELETE, endpoint, resp) + } + return nil +} + func ensureMap(parent map[string]interface{}, key string) map[string]interface{} { child, ok := parent[key].(map[string]interface{}) if ok && child != nil { diff --git a/pkg/authz/oauth/clients/okta_test.go b/pkg/authz/oauth/clients/okta_test.go index 93b48a5d7..0b34f1a18 100644 --- a/pkg/authz/oauth/clients/okta_test.go +++ b/pkg/authz/oauth/clients/okta_test.go @@ -1,86 +1,490 @@ package clients import ( + "encoding/json" + "fmt" + "io" "net/http" "net/http/httptest" "testing" coreapi "github.com/Axway/agent-sdk/pkg/api" + "github.com/stretchr/testify/assert" ) -func TestOktaAPIStatusHandling(t *testing.T) { +const ( + removeClientID = "remove-me" + errForbidden = "returns error on forbidden" + nilOn200 = "returns nil on 200" + nilOn204 = "returns nil on 204" + notFoundAsSuccess = "treats 404 as success" + testPolicyName = "my-policy" + testClientID = "client1" + testRuleName = "rule-1" + testGrantType = "client_credentials" + testRuleScope = "read:api" + testGroupID = "grp-123" + testGroupName = "Marketplace" + testAppID = "app-abc" + testAuthServerID = "as1" + testPolicyID = "pol1" +) + +func newTestOktaClient(t *testing.T, handler http.HandlerFunc) (*Okta, func()) { + t.Helper() + ts := httptest.NewServer(handler) + client := New(coreapi.NewClient(nil, ""), ts.URL, "token") + return client, ts.Close +} + +func assertOktaErr(t *testing.T, err error, wantErr bool) { + t.Helper() + if wantErr && err == nil { + t.Fatal("expected an error but got nil") + return + } + if !wantErr && err != nil { + t.Fatalf("expected no error but got: %v", err) + } +} + +func TestOktaUpdatePolicy(t *testing.T) { cases := map[string]struct { - handler http.HandlerFunc - call func(client *Okta) error - wantErr bool - expectedMethod string + code int + wantErr bool }{ - "AssignGroupToApp returns error on forbidden": { - expectedMethod: http.MethodPut, - handler: func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusForbidden) - w.Write([]byte("forbidden")) - }, - call: func(client *Okta) error { - return client.AssignGroupToApp("app123", "group456") + errForbidden: {code: http.StatusForbidden, wantErr: true}, + "returns nil on ok": {code: http.StatusOK, wantErr: false}, + } + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + client, close := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.code) + }) + defer close() + assertOktaErr(t, client.UpdatePolicy("as1", "pol1", map[string]interface{}{"id": "pol1"}), tc.wantErr) + }) + } +} + +func TestOktaCreatePolicy(t *testing.T) { + cases := map[string]struct { + code int + body string + wantErr bool + }{ + "returns created policy on 201": {code: http.StatusCreated, body: `{"id":"pol-new","name":"my-policy"}`, wantErr: false}, + errForbidden: {code: http.StatusForbidden, body: `forbidden`, wantErr: true}, + } + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + client, close := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.code) + _, _ = w.Write([]byte(tc.body)) + }) + defer close() + _, err := client.CreatePolicy("as1", testPolicyName, testClientID) + assertOktaErr(t, err, tc.wantErr) + }) + } +} + +func TestOktaCreatePolicyRequestBody(t *testing.T) { + var captured []byte + client, teardown := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + captured, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":"pol1"}`)) + }) + defer teardown() + _, err := client.CreatePolicy("as1", testPolicyName, testClientID) + assertOktaErr(t, err, false) + + var body map[string]interface{} + assert.NoError(t, json.Unmarshal(captured, &body)) + assert.Equal(t, "OAUTH_AUTHORIZATION_POLICY", body["type"]) + assert.Equal(t, "ACTIVE", body["status"]) + assert.EqualValues(t, 1, body["priority"]) + + conditions, _ := body["conditions"].(map[string]interface{}) + clients, _ := conditions["clients"].(map[string]interface{}) + include, _ := clients["include"].([]interface{}) + assert.Len(t, include, 1) + assert.Equal(t, testClientID, include[0]) + for _, v := range include { + assert.NotEqual(t, "ALL_CLIENTS", v) + } +} + +func TestOktaCreatePolicyRule(t *testing.T) { + cases := map[string]struct { + code int + wantErr bool + }{ + "returns nil on 201": {code: http.StatusCreated, wantErr: false}, + "returns error on bad req": {code: http.StatusBadRequest, wantErr: true}, + } + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + client, close := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.code) + }) + defer close() + err := client.CreatePolicyRule("as1", "pol1", "rule", testGrantType, testRuleScope) + assertOktaErr(t, err, tc.wantErr) + }) + } +} + +func TestOktaCreatePolicyRuleRequestBody(t *testing.T) { + var captured []byte + client, teardown := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + captured, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusCreated) + }) + defer teardown() + err := client.CreatePolicyRule("as1", "pol1", testRuleName, testGrantType, testRuleScope) + assertOktaErr(t, err, false) + + var body map[string]interface{} + assert.NoError(t, json.Unmarshal(captured, &body)) + assert.Equal(t, "RESOURCE_ACCESS", body["type"]) + + conds, _ := body["conditions"].(map[string]interface{}) + people, _ := conds["people"].(map[string]interface{}) + groups, _ := people["groups"].(map[string]interface{}) + groupIncludes, _ := groups["include"].([]interface{}) + assert.Equal(t, []interface{}{"EVERYONE"}, groupIncludes) + + grantTypes, _ := conds["grantTypes"].(map[string]interface{}) + grantIncludes, _ := grantTypes["include"].([]interface{}) + assert.Contains(t, grantIncludes, testGrantType) + + scopes, _ := conds["scopes"].(map[string]interface{}) + scopeIncludes, _ := scopes["include"].([]interface{}) + assert.Contains(t, scopeIncludes, testRuleScope) + + actions, _ := body["actions"].(map[string]interface{}) + token, _ := actions["token"].(map[string]interface{}) + assert.EqualValues(t, 60, token["accessTokenLifetimeMinutes"]) +} + +func TestOktaRemoveClientFromPolicy(t *testing.T) { + policyWith := func(include ...interface{}) map[string]interface{} { + return map[string]interface{}{ + "id": "pol1", + "conditions": map[string]interface{}{ + "clients": map[string]interface{}{ + "include": include, + }, }, + } + } + + cases := map[string]struct { + code int + policy map[string]interface{} + wantErr bool + wantPUT bool + }{ + "returns nil on ok": { + code: http.StatusOK, + policy: policyWith("keep-me", removeClientID), + wantErr: false, + wantPUT: true, + }, + errForbidden: { + code: http.StatusForbidden, + policy: policyWith("keep-me", removeClientID), wantErr: true, + wantPUT: true, }, - "AssignGroupToApp treats conflict as success": { - expectedMethod: http.MethodPut, - handler: func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusConflict) - w.Write([]byte("already assigned")) - }, - call: func(client *Okta) error { - return client.AssignGroupToApp("app123", "group456") - }, + "client not in list no PUT called": { + code: http.StatusOK, + policy: policyWith("other-client"), wantErr: false, + wantPUT: false, }, - "UnassignGroupFromApp treats not found as success": { - expectedMethod: http.MethodDelete, - handler: func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNotFound) - w.Write([]byte("not found")) - }, - call: func(client *Okta) error { - return client.UnassignGroupFromApp("app123", "group456") - }, + "empty include after removal policy not deleted": { + code: http.StatusOK, + policy: policyWith(removeClientID), wantErr: false, + wantPUT: true, }, - "UpdatePolicy returns error on forbidden": { - expectedMethod: http.MethodPut, - handler: func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusForbidden) - w.Write([]byte("forbidden")) - }, - call: func(client *Okta) error { - return client.UpdatePolicy("as1", "pol1", map[string]interface{}{"id": "pol1"}) - }, + } + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + putCalled := false + deleteCalled := false + client, teardown := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodPut: + putCalled = true + w.WriteHeader(tc.code) + case http.MethodDelete: + deleteCalled = true + w.WriteHeader(http.StatusNoContent) + default: + w.WriteHeader(http.StatusNotFound) + } + }) + defer teardown() + assertOktaErr(t, client.RemoveClientFromPolicy("as1", tc.policy, removeClientID), tc.wantErr) + assert.Equal(t, tc.wantPUT, putCalled, "PUT call expectation mismatch") + assert.False(t, deleteCalled, "policy must never be deleted") + }) + } +} + +func TestOktaActivatePolicy(t *testing.T) { + cases := map[string]struct { + code int + wantErr bool + }{ + nilOn200: {code: http.StatusOK, wantErr: false}, + nilOn204: {code: http.StatusNoContent, wantErr: false}, + notFoundAsSuccess: {code: http.StatusNotFound, wantErr: false}, + errForbidden: {code: http.StatusForbidden, wantErr: true}, + } + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + client, close := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.code) + }) + defer close() + assertOktaErr(t, client.ActivatePolicy(testAuthServerID, testPolicyID), tc.wantErr) + }) + } +} + +func TestOktaPolicyHasRuleForScope(t *testing.T) { + cases := map[string]struct { + code int + body string + scope string + wantHas bool + wantErr bool + }{ + "returns true when rule contains scope": { + code: http.StatusOK, + body: fmt.Sprintf(`[{"conditions":{"scopes":{"include":[%q,"write:api"]}}}]`, testRuleScope), + scope: testRuleScope, + wantHas: true, + }, + "returns false when rules list is empty": { + code: http.StatusOK, + body: `[]`, + scope: testRuleScope, + wantHas: false, + }, + "returns false when no rule contains scope": { + code: http.StatusOK, + body: `[{"conditions":{"scopes":{"include":["write:api"]}}}]`, + scope: testRuleScope, + wantHas: false, + }, + "trims whitespace when matching scope": { + code: http.StatusOK, + body: fmt.Sprintf(`[{"conditions":{"scopes":{"include":[" %s "]}}}]`, testRuleScope), + scope: testRuleScope, + wantHas: true, + }, + errForbidden: { + code: http.StatusForbidden, + body: `forbidden`, wantErr: true, }, } - for name, tc := range cases { tc := tc t.Run(name, func(t *testing.T) { - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if tc.expectedMethod != "" && r.Method != tc.expectedMethod { - w.WriteHeader(http.StatusMethodNotAllowed) - return - } - tc.handler(w, r) - })) - defer ts.Close() - - client := New(coreapi.NewClient(nil, ""), ts.URL, "token") - err := tc.call(client) - if tc.wantErr && err == nil { - t.Fatalf("expected error, got nil") + client, close := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.code) + _, _ = w.Write([]byte(tc.body)) + }) + defer close() + got, err := client.PolicyHasRuleForScope(testAuthServerID, testPolicyID, tc.scope) + assertOktaErr(t, err, tc.wantErr) + if !tc.wantErr { + assert.Equal(t, tc.wantHas, got) } - if !tc.wantErr && err != nil { - t.Fatalf("expected nil error, got %v", err) + }) + } +} + +func TestOktaDeactivatePolicy(t *testing.T) { + cases := map[string]struct { + code int + wantErr bool + }{ + nilOn200: {code: http.StatusOK, wantErr: false}, + nilOn204: {code: http.StatusNoContent, wantErr: false}, + notFoundAsSuccess: {code: http.StatusNotFound, wantErr: false}, + errForbidden: {code: http.StatusForbidden, wantErr: true}, + } + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + client, close := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.code) + }) + defer close() + assertOktaErr(t, client.DeactivatePolicy(testAuthServerID, testPolicyID), tc.wantErr) + }) + } +} + +func TestOktaDeletePolicy(t *testing.T) { + cases := map[string]struct { + code int + wantErr bool + }{ + nilOn204: {code: http.StatusNoContent, wantErr: false}, + notFoundAsSuccess: {code: http.StatusNotFound, wantErr: false}, + errForbidden: {code: http.StatusForbidden, wantErr: true}, + } + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + client, close := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.code) + }) + defer close() + assertOktaErr(t, client.DeletePolicy(testAuthServerID, testPolicyID), tc.wantErr) + }) + } +} + +func TestOktaDeactivateApp(t *testing.T) { + cases := map[string]struct { + code int + wantErr bool + }{ + nilOn200: {code: http.StatusOK, wantErr: false}, + nilOn204: {code: http.StatusNoContent, wantErr: false}, + notFoundAsSuccess: {code: http.StatusNotFound, wantErr: false}, + errForbidden: {code: http.StatusForbidden, wantErr: true}, + } + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + client, close := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.code) + }) + defer close() + assertOktaErr(t, client.DeactivateApp("app1"), tc.wantErr) + }) + } +} + +func TestOktaFindGroupByName(t *testing.T) { + cases := map[string]struct { + responseCode int + responseBody string + wantID string + wantErr bool + }{ + "returns group ID when found": { + responseCode: http.StatusOK, + responseBody: `[{"id":"grp-123","profile":{"name":"Marketplace"}},{"id":"grp-999","profile":{"name":"MarketplaceOther"}}]`, + wantID: testGroupID, + }, + "returns empty string when no exact match": { + responseCode: http.StatusOK, + responseBody: `[{"id":"grp-999","profile":{"name":"MarketplaceOther"}}]`, + wantID: "", + }, + "returns empty string on empty list": { + responseCode: http.StatusOK, + responseBody: `[]`, + wantID: "", + }, + errForbidden: { + responseCode: http.StatusForbidden, + responseBody: `forbidden`, + wantErr: true, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + client, close := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.responseCode) + _, _ = w.Write([]byte(tc.responseBody)) + }) + defer close() + got, err := client.FindGroupByName(testGroupName) + assertOktaErr(t, err, tc.wantErr) + if !tc.wantErr { + assert.Equal(t, tc.wantID, got) } }) } } + +func TestOktaAssignGroupToApp(t *testing.T) { + cases := map[string]struct { + code int + wantErr bool + }{ + nilOn200: {code: http.StatusOK, wantErr: false}, + "returns nil on 201": {code: http.StatusCreated, wantErr: false}, + errForbidden: {code: http.StatusForbidden, wantErr: true}, + "returns error on 404": {code: http.StatusNotFound, wantErr: true}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + client, close := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.code) + }) + defer close() + assertOktaErr(t, client.AssignGroupToApp(testAppID, testGroupID), tc.wantErr) + }) + } +} + +func TestOktaUnassignGroupFromApp(t *testing.T) { + cases := map[string]struct { + code int + wantErr bool + }{ + nilOn204: {code: http.StatusNoContent, wantErr: false}, + nilOn200: {code: http.StatusOK, wantErr: false}, + notFoundAsSuccess: {code: http.StatusNotFound, wantErr: false}, + errForbidden: {code: http.StatusForbidden, wantErr: true}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + client, close := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.code) + }) + defer close() + assertOktaErr(t, client.UnassignGroupFromApp(testAppID, testGroupID), tc.wantErr) + }) + } +} + +func TestOktaDeleteApp(t *testing.T) { + cases := map[string]struct { + code int + wantErr bool + }{ + nilOn204: {code: http.StatusNoContent, wantErr: false}, + notFoundAsSuccess: {code: http.StatusNotFound, wantErr: false}, + errForbidden: {code: http.StatusForbidden, wantErr: true}, + } + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + client, close := newTestOktaClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.code) + }) + defer close() + assertOktaErr(t, client.DeleteApp("app1"), tc.wantErr) + }) + } +} diff --git a/pkg/authz/oauth/genericprovider.go b/pkg/authz/oauth/genericprovider.go index 4f4064a69..887d90f3a 100644 --- a/pkg/authz/oauth/genericprovider.go +++ b/pkg/authz/oauth/genericprovider.go @@ -28,6 +28,6 @@ func (i *genericIDP) postProcessClientRegistration(clientRes ClientMetadata, idp return nil } -func (i *genericIDP) postProcessClientUnregister(clientID string, idp corecfg.IDPConfig, apiClient coreapi.Client) error { +func (i *genericIDP) postProcessClientUnregister(clientID string, idp corecfg.IDPConfig, apiClient coreapi.Client, scopes []string, grantType string) error { return nil } diff --git a/pkg/authz/oauth/mockidpserver.go b/pkg/authz/oauth/mockidpserver.go index 9db7b7a2b..9f2461218 100644 --- a/pkg/authz/oauth/mockidpserver.go +++ b/pkg/authz/oauth/mockidpserver.go @@ -58,7 +58,6 @@ func NewMockIDPServer() MockIDPServer { tokenResponseCode: http.StatusOK, registerResponseCode: http.StatusCreated, } - m.server = httptest.NewServer(http.HandlerFunc(m.handleRequest)) m.serverMetadata = &AuthorizationServerMetadata{ Issuer: m.GetIssuer(), @@ -115,9 +114,6 @@ func (m *mockIDPServer) handleRequest(resp http.ResponseWriter, req *http.Reques if strings.Contains(req.RequestURI, "/register") { m.reqHeaders = req.Header m.reqQueryParam = req.URL.Query() - defer func() { - m.registerResponseCode = http.StatusCreated - }() if req.Method == http.MethodPost { if m.registerResponseCode != http.StatusCreated { resp.WriteHeader(m.registerResponseCode) diff --git a/pkg/authz/oauth/oktaprovider.go b/pkg/authz/oauth/oktaprovider.go index ad723224c..d6fd873e4 100644 --- a/pkg/authz/oauth/oktaprovider.go +++ b/pkg/authz/oauth/oktaprovider.go @@ -9,6 +9,7 @@ import ( coreapi "github.com/Axway/agent-sdk/pkg/api" "github.com/Axway/agent-sdk/pkg/authz/oauth/clients" corecfg "github.com/Axway/agent-sdk/pkg/config" + "github.com/Axway/agent-sdk/pkg/util/log" ) const ( @@ -20,7 +21,9 @@ const ( oktaSpa = "okta-spa" ) -type okta struct{} +type okta struct { + logger log.FieldLogger +} func oktaBaseURLFromMetadataURL(metadataURL string) (string, error) { if metadataURL == "" { @@ -57,240 +60,308 @@ func oktaAuthServerIDFromMetadataURL(metadataURL string) (string, error) { return "", fmt.Errorf("unable to determine Okta authorization server from metadata URL") } -func validateOktaConfiguredResources(idp corecfg.IDPConfig, apiClient coreapi.Client) error { - groupName, policyName := oktaValidationNames(idp) - if groupName == "" && policyName == "" { - return nil - } - - apiToken := oktaManagementAPIToken(idp) - if apiToken == "" { - return fmt.Errorf("okta group/policy validation requires a management API access token. Set %q in the IDP auth configuration", "auth.accessToken") - } - - metadataURL := idp.GetMetadataURL() - oktaClient, err := oktaClientFromMetadataURL(apiClient, metadataURL, apiToken) - if err != nil { - return err - } - - authServerID := "" - if policyName != "" { - authServerID, err = oktaAuthServerIDFromMetadataURL(metadataURL) - if err != nil { - return err - } +func normalizeGrantType(grantType string) string { + switch grantType { + case GrantTypeClientCredentials: + return "clientcredentials" + case GrantTypeAuthorizationCode: + return "authorizationcode" + case GrantTypeImplicit: + return "implicitgrant" + default: + return strings.ReplaceAll(grantType, "_", "") } +} - if err := validateOktaGroupExists(oktaClient, groupName); err != nil { - return err +func policyNameTemplate(idp corecfg.IDPConfig) string { + cfg, ok := idp.(interface{ GetPolicyNameTemplate() string }) + if !ok { + return corecfg.OktaPlaceholderScope + "-" + corecfg.OktaPlaceholderOAuthFlow } - if err := validateOktaPolicyExists(oktaClient, authServerID, policyName); err != nil { - return err + if t := cfg.GetPolicyNameTemplate(); t != "" { + return t } - return nil + return corecfg.OktaPlaceholderScope + "-" + corecfg.OktaPlaceholderOAuthFlow } -func oktaValidationNames(idp corecfg.IDPConfig) (groupName, policyName string) { - return strings.TrimSpace(idp.GetOktaGroup()), strings.TrimSpace(idp.GetOktaPolicy()) -} +func (i *okta) postProcessClientRegistration(clientRes ClientMetadata, idp corecfg.IDPConfig, apiClient coreapi.Client) error { + i.logger.WithField("clientID", clientRes.GetClientID()).Debug("running Okta post-registration hook") -func oktaManagementAPIToken(idp corecfg.IDPConfig) string { - authCfg := idp.GetAuthConfig() - if authCfg == nil { - return "" + metadataURL := idp.GetMetadataURL() + var apiToken string + if authCfg := idp.GetAuthConfig(); authCfg != nil { + apiToken = authCfg.GetAccessToken() } - return strings.TrimSpace(authCfg.GetAccessToken()) -} -func oktaClientFromMetadataURL(apiClient coreapi.Client, metadataURL, apiToken string) (*clients.Okta, error) { baseURL, err := oktaBaseURLFromMetadataURL(metadataURL) if err != nil { - return nil, err - } - if baseURL == "" { - return nil, fmt.Errorf("invalid okta metadata URL: %q", metadataURL) + return err } - return clients.New(apiClient, baseURL, apiToken), nil -} - -func validateOktaGroupExists(oktaClient *clients.Okta, groupName string) error { - if strings.TrimSpace(groupName) == "" { + if baseURL == "" || apiToken == "" { return nil } - groupID, err := oktaClient.FindGroupByName(groupName) + + authServerID, err := oktaAuthServerIDFromMetadataURL(metadataURL) if err != nil { return err } - if groupID == "" { - return fmt.Errorf("configured okta group %q was not found", groupName) - } - return nil -} -func validateOktaPolicyExists(oktaClient *clients.Okta, authServerID, policyName string) error { - if strings.TrimSpace(policyName) == "" { - return nil - } - if authServerID == "" { - return fmt.Errorf("unable to determine Okta authorization server from metadata URL") + oktaClient := clients.New(apiClient, baseURL, apiToken) + + grantType := "" + if gt := clientRes.GetGrantTypes(); len(gt) > 0 { + grantType = gt[0] } - policy, err := oktaClient.FindPolicyByName(authServerID, policyName) - if err != nil { + + if err := i.handlePerScopePolicyAssign(oktaClient, clientRes, idp, authServerID, grantType); err != nil { return err } - if policy == nil { - return fmt.Errorf("configured okta policy %q was not found on authorization server %q", policyName, authServerID) + + if err := i.handleGroupAssignment(oktaClient, clientRes.GetClientID(), idp); err != nil { + return err } + + i.logger.WithField("clientID", clientRes.GetClientID()).Info("completed Okta post-registration hook") return nil } -// postProcessClientRegistration handles Okta provisioning after client registration -func (i *okta) postProcessClientRegistration(clientRes ClientMetadata, idp corecfg.IDPConfig, apiClient coreapi.Client) error { +func (i *okta) postProcessClientUnregister(clientID string, idp corecfg.IDPConfig, apiClient coreapi.Client, scopes []string, grantType string) error { + i.logger.WithField("clientID", clientID).Debug("running Okta post-unregister hook") + metadataURL := idp.GetMetadataURL() var apiToken string if authCfg := idp.GetAuthConfig(); authCfg != nil { apiToken = authCfg.GetAccessToken() } - groupName := idp.GetOktaGroup() - policyName := idp.GetOktaPolicy() baseURL, err := oktaBaseURLFromMetadataURL(metadataURL) if err != nil { return err } if baseURL == "" || apiToken == "" { - return nil // skip if not configured + return nil + } + + authServerID, err := oktaAuthServerIDFromMetadataURL(metadataURL) + if err != nil { + return err } + oktaClient := clients.New(apiClient, baseURL, apiToken) - if err := i.handleGroupAssignment(oktaClient, clientRes, groupName); err != nil { + if err := oktaClient.DeactivateApp(clientID); err != nil { + return err + } + if err := oktaClient.DeleteApp(clientID); err != nil { return err } - authServerID := "" - if strings.TrimSpace(policyName) != "" { - authServerID, err = oktaAuthServerIDFromMetadataURL(metadataURL) - if err != nil { - return err - } + if err := i.handleGroupRemoval(oktaClient, clientID, idp); err != nil { + return err } - if err := i.handlePolicyAssignment(oktaClient, clientRes.GetClientID(), authServerID, policyName); err != nil { + + if err := i.handlePerScopePolicyUnassign(oktaClient, clientID, scopes, idp, authServerID, grantType); err != nil { return err } + i.logger.WithField("clientID", clientID).Info("completed Okta post-unregister hook") return nil } -// postProcessClientUnregister removes the app→group assignment for the client. -// Explicit policy cleanup is not required: when the group assignment is removed, Okta -// automatically removes the client from any policy include lists that were scoped to that -// group, so no separate policy deprovision step is needed. -func (i *okta) postProcessClientUnregister(clientID string, idp corecfg.IDPConfig, apiClient coreapi.Client) error { - metadataURL := idp.GetMetadataURL() - var apiToken string - if authCfg := idp.GetAuthConfig(); authCfg != nil { - apiToken = authCfg.GetAccessToken() +func (i *okta) handlePerScopePolicyAssign( + oktaClient *clients.Okta, + clientRes ClientMetadata, + idp corecfg.IDPConfig, + authServerID string, + grantType string, +) error { + i.logger. + WithField("clientID", clientRes.GetClientID()). + WithField("authServerID", authServerID). + Trace("handling per-scope policy assignment") + + normalizedFlow := normalizeGrantType(grantType) + template := policyNameTemplate(idp) + scopes := clientRes.GetScopes() + + replacer := strings.NewReplacer(corecfg.OktaPlaceholderOAuthFlow, normalizedFlow) + for _, scope := range scopes { + if strings.TrimSpace(scope) == "" { + continue + } + policyName := replacer.Replace(strings.ReplaceAll(template, corecfg.OktaPlaceholderScope, scope)) + if err := i.assignScopePolicy(oktaClient, clientRes, idp, authServerID, grantType, policyName, scope); err != nil { + return err + } } - baseURL, err := oktaBaseURLFromMetadataURL(metadataURL) + return nil +} + +func (i *okta) assignScopePolicy( + oktaClient *clients.Okta, + clientRes ClientMetadata, + idp corecfg.IDPConfig, + authServerID, grantType, policyName, scope string, +) error { + if len(policyName) > 100 { + return fmt.Errorf("Okta policy name exceeds 100-character limit: %s (%d chars)", policyName, len(policyName)) + } + + i.logger.WithField("policyName", policyName).Trace("looking up policy") + + policy, err := oktaClient.FindPolicyByName(authServerID, policyName) if err != nil { return err } - if baseURL == "" || apiToken == "" { - return nil // skip if not configured - } - - oktaClient := clients.New(apiClient, baseURL, apiToken) - return i.handleGroupUnassignment(oktaClient, clientID, idp.GetOktaGroup()) -} -// handleGroupUnassignment unassigns the configured group from the app. -func (i *okta) handleGroupUnassignment(oktaClient *clients.Okta, clientID, groupName string) error { - groupName = strings.TrimSpace(groupName) - if groupName == "" { + if policy == nil { + i.logger.WithField("policyName", policyName).Trace("policy not found, creating") + createdPolicy, err := oktaClient.CreatePolicy(authServerID, policyName, clientRes.GetClientID()) + if err != nil { + return err + } + policyID, _ := createdPolicy["id"].(string) + if err := oktaClient.CreatePolicyRule(authServerID, policyID, policyName, grantType, scope); err != nil { + i.cleanupOrphanedPolicy(oktaClient, authServerID, policyID) + return err + } + i.logger.WithField("policyName", policyName).Trace("created policy and rule") return nil } - groupID, err := oktaClient.FindGroupByName(groupName) + i.logger.WithField("policyName", policyName).Trace("policy found, checking rule before assigning client") + + policyID, _ := policy["id"].(string) + hasRule, err := oktaClient.PolicyHasRuleForScope(authServerID, policyID, scope) if err != nil { return err } - if groupID == "" { - return fmt.Errorf("configured okta group %q was not found during client cleanup", groupName) + + if !hasRule { + i.logger.WithField("policyName", policyName).WithField("policyID", policyID).Debug("repairing policy, missing rule for scope") + status, _ := policy["status"].(string) + if err := i.ensurePolicyHasRule(oktaClient, authServerID, policyID, policyName, grantType, scope, status); err != nil { + return err + } } - if err := oktaClient.UnassignGroupFromApp(clientID, groupID); err != nil { + return oktaClient.AssignClientToPolicy(authServerID, policy, clientRes.GetClientID()) +} + +func (i *okta) ensurePolicyHasRule(oktaClient *clients.Okta, authServerID, policyID, policyName, grantType, scope, status string) error { + if err := oktaClient.CreatePolicyRule(authServerID, policyID, policyName, grantType, scope); err != nil { return err } + if status == "INACTIVE" { + return oktaClient.ActivatePolicy(authServerID, policyID) + } return nil } -func (i *okta) getAuthorizationHeaderPrefix() string { - return clients.OktaAuthHeaderPrefix -} - -// handleGroupAssignment looks up the named group and assigns it to the app. -// Returns an error if the group is not found. Even though we fail fast at the start, we check again to guard against the possibility that the group was deleted between validation and assignment. -func (i *okta) handleGroupAssignment(oktaClient *clients.Okta, clientRes ClientMetadata, groupName string) error { - groupName = strings.TrimSpace(groupName) - if groupName == "" { - return nil - } - groupId, err := oktaClient.FindGroupByName(groupName) - if err != nil { - return err +func (i *okta) cleanupOrphanedPolicy(oktaClient *clients.Okta, authServerID, policyID string) { + i.logger.WithField("policyID", policyID).Debug("removing orphaned policy after rule creation failure") + if err := oktaClient.DeactivatePolicy(authServerID, policyID); err != nil { + i.logger.WithField("policyID", policyID).WithError(err).Error("failed to deactivate orphaned policy, manual cleanup required in Okta") + return } - if groupId == "" { - return fmt.Errorf("configured okta group %q was not found", groupName) + if err := oktaClient.DeletePolicy(authServerID, policyID); err != nil { + i.logger.WithField("policyID", policyID).WithError(err).Error("failed to delete orphaned policy, manual cleanup required in Okta") } - if err := oktaClient.AssignGroupToApp(clientRes.GetClientID(), groupId); err != nil { - return err +} + +func (i *okta) handlePerScopePolicyUnassign( + oktaClient *clients.Okta, + clientID string, + scopes []string, + idp corecfg.IDPConfig, + authServerID string, + grantType string, +) error { + i.logger. + WithField("clientID", clientID). + WithField("authServerID", authServerID). + Trace("handling per-scope policy unassignment") + + normalizedFlow := normalizeGrantType(grantType) + template := policyNameTemplate(idp) + + for _, scope := range scopes { + policyName := strings.NewReplacer(corecfg.OktaPlaceholderScope, scope, corecfg.OktaPlaceholderOAuthFlow, normalizedFlow).Replace(template) + i.logger.WithField("policyName", policyName).Trace("looking up policy for removal") + + policy, err := oktaClient.FindPolicyByName(authServerID, policyName) + if err != nil { + return err + } + + if policy == nil { + i.logger. + WithField("scope", scope). + WithField("policyName", policyName). + Trace("policy not found for scope during unassign, skipping") + continue + } + + i.logger. + WithField("clientID", clientID). + WithField("policyName", policyName). + Trace("removing client from policy") + if err := oktaClient.RemoveClientFromPolicy(authServerID, policy, clientID); err != nil { + return err + } + + if err := i.deletePolicyIfEmpty(oktaClient, authServerID, policyName, policy); err != nil { + return err + } } + return nil } -// handlePolicyAssignment looks up the named policy on the authorization server and assigns the client to it. -// Returns an error if the policy is not found. Even though we fail fast at the start, we check again to guard against the possibility that the policy was deleted between validation and assignment. -func (i *okta) handlePolicyAssignment(oktaClient *clients.Okta, clientID, authServerID string, policyName string) error { - policyName = strings.TrimSpace(policyName) - if policyName == "" { +func (i *okta) deletePolicyIfEmpty(oktaClient *clients.Okta, authServerID, policyName string, policy map[string]interface{}) error { + conditions, _ := policy["conditions"].(map[string]any) + clientsCond, _ := conditions["clients"].(map[string]any) + include, ok := clientsCond["include"].([]any) + + if !ok || len(include) > 0 { return nil } - if authServerID == "" { - return fmt.Errorf("unable to determine Okta authorization server from metadata URL") + + policyID, _ := policy["id"].(string) + if policyID == "" { + return nil } - policy, err := oktaClient.FindPolicyByName(authServerID, policyName) - if err != nil { + + i.logger. + WithField("policyID", policyID). + WithField("policyName", policyName). + Debug("client list empty after removal, deleting policy") + + if err := oktaClient.DeactivatePolicy(authServerID, policyID); err != nil { return err } - if policy == nil { - return fmt.Errorf("configured okta policy %q was not found on authorization server %q", policyName, authServerID) - } - policyID, _ := policy["id"].(string) - policyID = strings.TrimSpace(policyID) - if policyID == "" { - return fmt.Errorf("okta policy %q has no id field", policyName) + + if err := oktaClient.DeletePolicy(authServerID, policyID); err != nil { + i.logger. + WithField("policyID", policyID). + WithField("policyName", policyName). + WithError(err). + Error("failed to delete deactivated empty Okta policy, manual cleanup required in Okta") + return err } - // Update policy-level client assignment (no rules). - return oktaClient.AssignClientToPolicy(authServerID, policy, clientID) + return nil } -// validateExtraProperties validates Okta-specific extra properties and sets defaults func (i *okta) validateExtraProperties(extraProps map[string]interface{}) error { pkceRequired, _ := extraProps[oktaPKCERequired].(bool) appType, _ := extraProps[oktaApplicationType].(string) - // Validate that if PKCE is required and application_type is explicitly set, it must be 'browser' if pkceRequired && appType != "" && appType != oktaAppTypeBrowser { return fmt.Errorf("when %s is true, %s must be '%s' or unset, got '%s'", oktaPKCERequired, oktaApplicationType, oktaAppTypeBrowser, appType) } - // Set default application_type only if not explicitly set by user - // Check appType == "" to preserve valid explicit user choices (e.g., "service") - // Default to 'web', override to 'browser' if PKCE is required - // Note: May be further overridden to 'service' in preProcessClientRequest for client credentials flows if appType == "" { extraProps[oktaApplicationType] = oktaAppTypeWeb if pkceRequired { @@ -308,8 +379,6 @@ func (i *okta) preProcessClientRequest(clientRequest *clientMetadata) { pkceRequired, _ := clientRequest.extraProperties[oktaPKCERequired].(bool) - // Override application_type to 'service' for client credentials flows - // (validateExtraProperties sets default to 'web' or 'browser') if slices.Contains(clientRequest.GrantTypes, GrantTypeClientCredentials) { clientRequest.extraProperties[oktaApplicationType] = oktaAppTypeService if len(clientRequest.ResponseTypes) == 0 { @@ -321,3 +390,74 @@ func (i *okta) preProcessClientRequest(clientRequest *clientMetadata) { clientRequest.TokenEndpointAuthMethod = "none" } } + +func (i *okta) getAuthorizationHeaderPrefix() string { + return clients.OktaAuthHeaderPrefix +} + +func (i *okta) handleGroupAssignment(oktaClient *clients.Okta, clientID string, idp corecfg.IDPConfig) error { + groupName := oktaGroupName(idp) + if groupName == "" { + return nil + } + i.logger.WithField("clientID", clientID).WithField("groupName", groupName).Trace("adding app to Okta group") + groupID, err := oktaClient.FindGroupByName(groupName) + if err != nil { + return fmt.Errorf("error finding Okta group %q: %w", groupName, err) + } + if groupID == "" { + return fmt.Errorf("configured Okta group %q not found", groupName) + } + return oktaClient.AssignGroupToApp(clientID, groupID) +} + +func (i *okta) handleGroupRemoval(oktaClient *clients.Okta, clientID string, idp corecfg.IDPConfig) error { + groupName := oktaGroupName(idp) + if groupName == "" { + return nil + } + i.logger.WithField("clientID", clientID).WithField("groupName", groupName).Trace("removing app from Okta group") + groupID, err := oktaClient.FindGroupByName(groupName) + if err != nil { + return fmt.Errorf("error finding Okta group %q: %w", groupName, err) + } + if groupID == "" { + return nil + } + return oktaClient.UnassignGroupFromApp(clientID, groupID) +} + +func oktaGroupName(idp corecfg.IDPConfig) string { + cfg, ok := idp.(interface{ GetOktaGroup() string }) + if !ok { + return "" + } + return cfg.GetOktaGroup() +} + +func validateOktaGroupExists(idp corecfg.IDPConfig, apiClient coreapi.Client) error { + groupName := oktaGroupName(idp) + if groupName == "" { + return nil + } + var apiToken string + if authCfg := idp.GetAuthConfig(); authCfg != nil { + apiToken = authCfg.GetAccessToken() + } + if apiToken == "" { + return nil + } + baseURL, err := oktaBaseURLFromMetadataURL(idp.GetMetadataURL()) + if err != nil { + return err + } + oktaClient := clients.New(apiClient, baseURL, apiToken) + groupID, err := oktaClient.FindGroupByName(groupName) + if err != nil { + return fmt.Errorf("error looking up Okta group %q: %w", groupName, err) + } + if groupID == "" { + return fmt.Errorf("configured Okta group %q not found", groupName) + } + return nil +} diff --git a/pkg/authz/oauth/oktaprovider_test.go b/pkg/authz/oauth/oktaprovider_test.go index ce4594d7b..bc854cc83 100644 --- a/pkg/authz/oauth/oktaprovider_test.go +++ b/pkg/authz/oauth/oktaprovider_test.go @@ -3,6 +3,7 @@ package oauth import ( "encoding/json" "fmt" + "io" "net/http" "net/http/httptest" "strings" @@ -10,39 +11,56 @@ import ( "testing" coreapi "github.com/Axway/agent-sdk/pkg/api" + "github.com/Axway/agent-sdk/pkg/authz/oauth/clients" corecfg "github.com/Axway/agent-sdk/pkg/config" + "github.com/Axway/agent-sdk/pkg/util/log" "github.com/stretchr/testify/assert" ) const ( - groupsEndpoint = "/api/v1/groups" - appGroupsEndpointBase = "/api/v1/apps/app123/groups/" - oktaGroupIDExisting = "00g-123" - oauthMetadataEndpoint = "/oauth2/authorizationID/.well-known/oauth-authorization-server" - oktaPoliciesEndpointByID = "/api/v1/authorizationServers/authorizationID/policies" - accessToken = "access-token" - oktaPolicyID = "pol-123" - oktaGroupID = "00g-123" - oktaPolicyEndpointByID = "/api/v1/authorizationServers/authorizationID/policies/pol-123" + testScope = "read:api" + testPolicyName = "read:api-clientcredentials" + testClientID = "app123" + accessToken = "access-token" + testAuthHeader = "SSWS " + accessToken + defaultAppTemplate = corecfg.OktaPlaceholderMPApplicationName + "-" + corecfg.OktaPlaceholderOwningTeam + "-" + corecfg.OktaPlaceholderCredentialName + defaultPolicyTemplate = corecfg.OktaPlaceholderScope + "-" + corecfg.OktaPlaceholderOAuthFlow + normalizedClientCredentials = "clientcredentials" + normalizedAuthorizationCode = "authorizationcode" + normalizedImplicitGrant = "implicitgrant" + testAuthMethodNone = "none" + oauthMetadataEndpoint = "/oauth2/authorizationID/.well-known/oauth-authorization-server" + oktaPoliciesEndpointByID = "/api/v1/authorizationServers/authorizationID/policies" + oktaPolicyID = "pol-123" + oktaPolicyEndpointByID = "/api/v1/authorizationServers/authorizationID/policies/pol-123" + oktaPolicyRulesEndpoint = "/api/v1/authorizationServers/authorizationID/policies/pol-123/rules" + oktaDeactivateEndpoint = "/api/v1/apps/app123/lifecycle/deactivate" + oktaDeleteEndpoint = "/api/v1/apps/app123" + oktaPolicyDeactivateEndpoint = "/api/v1/authorizationServers/authorizationID/policies/pol-123/lifecycle/deactivate" + oktaPolicyActivateEndpoint = "/api/v1/authorizationServers/authorizationID/policies/pol-123/lifecycle/activate" + callCreateRule = "create-rule" + callActivate = "activate" + testGroupName = "Marketplace" + testGroupID = "grp-456" + oktaGroupsEndpoint = "/api/v1/groups" + oktaAppGroupEndpoint = "/api/v1/apps/app123/groups/grp-456" ) +// oktaScriptedServer drives httptest with per-route handlers and call-count tracking. type oktaScriptedServer struct { - mu sync.Mutex - expectedAuth string - calls map[string]int - routes map[string]http.HandlerFunc - strictRouting bool + mu sync.Mutex + expectedAuth string + calls map[string]int + routes map[string]http.HandlerFunc } func newOktaScriptedServer(t *testing.T, expectedAuth string, routes map[string]http.HandlerFunc) (*httptest.Server, *oktaScriptedServer) { t.Helper() - s := &oktaScriptedServer{ expectedAuth: expectedAuth, calls: make(map[string]int), routes: routes, } - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { s.mu.Lock() s.calls[r.Method+" "+r.URL.Path]++ @@ -55,16 +73,13 @@ func newOktaScriptedServer(t *testing.T, expectedAuth string, routes map[string] return } } - - key := r.Method + " " + r.URL.Path - if h, ok := routes[key]; ok { + if h, ok := routes[r.Method+" "+r.URL.Path]; ok { h(w, r) return } w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte("not found")) })) - return ts, s } @@ -78,25 +93,13 @@ func (s *oktaScriptedServer) getCalls() map[string]int { return out } -type oktaGroupItem struct { - ID string - Name string -} - type oktaPolicyItem struct { ID string Name string } -func oktaGroupsSearchHandler(wantQ string, items []oktaGroupItem) http.HandlerFunc { +func oktaPoliciesListHandler(items []oktaPolicyItem) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - if wantQ != "" { - if q := r.URL.Query().Get("q"); q != wantQ { - w.WriteHeader(http.StatusBadRequest) - _, _ = w.Write([]byte("unexpected q")) - return - } - } w.WriteHeader(http.StatusOK) buf := make([]byte, 0, 256) buf = append(buf, '[') @@ -104,51 +107,48 @@ func oktaGroupsSearchHandler(wantQ string, items []oktaGroupItem) http.HandlerFu if i > 0 { buf = append(buf, ',') } - buf = append(buf, []byte(fmt.Sprintf(`{"id":%q,"profile":{"name":%q}}`, item.ID, item.Name))...) + buf = fmt.Appendf(buf, `{"id":%q,"name":%q}`, item.ID, item.Name) } buf = append(buf, ']') _, _ = w.Write(buf) } } -func oktaAssignGroupHandler() http.HandlerFunc { +func oktaPolicyGetHandler(policyID, name string, include []string) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`{}`)) + includeJSON, _ := json.Marshal(include) + _, _ = fmt.Fprintf(w, `{"id":%q,"name":%q,"conditions":{"clients":{"include":%s}}}`, policyID, name, string(includeJSON)) } } -func oktaPoliciesListHandler(items []oktaPolicyItem) http.HandlerFunc { +func oktaPolicyGetHandlerWithStatus(policyID, name, status string, include []string) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) - buf := make([]byte, 0, 256) - buf = append(buf, '[') - for i, item := range items { - if i > 0 { - buf = append(buf, ',') - } - buf = append(buf, []byte(fmt.Sprintf(`{"id":%q,"name":%q}`, item.ID, item.Name))...) - } - buf = append(buf, ']') - _, _ = w.Write(buf) + includeJSON, _ := json.Marshal(include) + _, _ = fmt.Fprintf(w, `{"id":%q,"name":%q,"status":%q,"conditions":{"clients":{"include":%s}}}`, policyID, name, status, string(includeJSON)) } } -func oktaPolicyGetHandler(policyID, policyName string, include []string) http.HandlerFunc { +func oktaPolicyRulesHandler(scopes ...string) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) - includeJSON, _ := json.Marshal(include) - _, _ = w.Write([]byte(fmt.Sprintf(`{"id":%q,"name":%q,"conditions":{"clients":{"include":%s}}}`, policyID, policyName, string(includeJSON)))) + if len(scopes) == 0 { + _, _ = w.Write([]byte(`[]`)) + return + } + scopeJSON, _ := json.Marshal(scopes) + _, _ = fmt.Fprintf(w, `[{"conditions":{"scopes":{"include":%s}}}]`, string(scopeJSON)) } } func oktaPolicyPutMustIncludeClientHandler(clientID string) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - var body map[string]interface{} + var body map[string]any _ = json.NewDecoder(r.Body).Decode(&body) - conds, _ := body["conditions"].(map[string]interface{}) - clients, _ := conds["clients"].(map[string]interface{}) - include, _ := clients["include"].([]interface{}) + conds, _ := body["conditions"].(map[string]any) + clients, _ := conds["clients"].(map[string]any) + include, _ := clients["include"].([]any) for _, v := range include { if s, ok := v.(string); ok && s == clientID { w.WriteHeader(http.StatusOK) @@ -161,15 +161,11 @@ func oktaPolicyPutMustIncludeClientHandler(clientID string) http.HandlerFunc { } } -func newIDPCredential(tsURL, group, policy string) *corecfg.IDPConfiguration { - credentialObj := &corecfg.IDPConfiguration{ +func newIDPCredential(tsURL string) *corecfg.IDPConfiguration { + return &corecfg.IDPConfiguration{ MetadataURL: tsURL + oauthMetadataEndpoint, AuthConfig: &corecfg.IDPAuthConfiguration{AccessToken: accessToken}, } - if strings.TrimSpace(group) != "" || strings.TrimSpace(policy) != "" { - credentialObj.Okta = &corecfg.OktaIDPConfiguration{Group: group, Policy: policy} - } - return credentialObj } func assertMinCalls(t *testing.T, calls map[string]int, wantMinCalls map[string]int) { @@ -181,188 +177,330 @@ func assertMinCalls(t *testing.T, calls map[string]int, wantMinCalls map[string] func TestOktaPostProcessClientRegistration(t *testing.T) { apiClient := coreapi.NewClient(nil, "") - oktaProvider := &okta{} + oktaProvider := &okta{logger: log.NewFieldLogger()} cases := map[string]struct { - oktaGroup string - oktaPolicy string + scopes []string + grantType string routes map[string]http.HandlerFunc wantMinCalls map[string]int wantErr bool }{ - "assigns existing group + records existing policy": { - oktaGroup: "MyAppUsers", - oktaPolicy: "MarketplacePolicy", + "creates new per-scope policy when none exists": { + scopes: []string{testScope}, + grantType: GrantTypeClientCredentials, + routes: map[string]http.HandlerFunc{ + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler(nil), + http.MethodPost + " " + oktaPoliciesEndpointByID: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + _, _ = fmt.Fprintf(w, `{"id":%q,"name":%q}`, oktaPolicyID, testPolicyName) + }, + http.MethodPost + " " + oktaPolicyRulesEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":"rule-1"}`)) + }, + }, + wantMinCalls: map[string]int{ + http.MethodGet + " " + oktaPoliciesEndpointByID: 1, + http.MethodPost + " " + oktaPoliciesEndpointByID: 1, + http.MethodPost + " " + oktaPolicyRulesEndpoint: 1, + }, + }, + "assigns client to existing per-scope policy": { + scopes: []string{testScope}, + grantType: GrantTypeClientCredentials, routes: map[string]http.HandlerFunc{ - http.MethodGet + " " + groupsEndpoint: oktaGroupsSearchHandler("MyAppUsers", []oktaGroupItem{{ID: "00g-other", Name: "Other"}, {ID: oktaGroupIDExisting, Name: "MyAppUsers"}}), - http.MethodPut + " " + appGroupsEndpointBase + oktaGroupIDExisting: oktaAssignGroupHandler(), - http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler([]oktaPolicyItem{{ID: oktaPolicyID, Name: "MarketplacePolicy"}}), - http.MethodGet + " " + oktaPolicyEndpointByID: oktaPolicyGetHandler(oktaPolicyID, "MarketplacePolicy", []string{}), - http.MethodPut + " " + oktaPolicyEndpointByID: oktaPolicyPutMustIncludeClientHandler("app123"), + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler([]oktaPolicyItem{{ID: oktaPolicyID, Name: testPolicyName}}), + http.MethodGet + " " + oktaPolicyEndpointByID: oktaPolicyGetHandler(oktaPolicyID, testPolicyName, []string{}), + http.MethodGet + " " + oktaPolicyRulesEndpoint: oktaPolicyRulesHandler(testScope), + http.MethodPut + " " + oktaPolicyEndpointByID: oktaPolicyPutMustIncludeClientHandler(testClientID), }, wantMinCalls: map[string]int{ - http.MethodGet + " " + groupsEndpoint: 1, - http.MethodPut + " " + appGroupsEndpointBase + oktaGroupIDExisting: 1, - http.MethodGet + " " + oktaPoliciesEndpointByID: 1, - http.MethodGet + " " + oktaPolicyEndpointByID: 1, - http.MethodPut + " " + oktaPolicyEndpointByID: 1, + http.MethodGet + " " + oktaPoliciesEndpointByID: 1, + http.MethodGet + " " + oktaPolicyEndpointByID: 1, + http.MethodGet + " " + oktaPolicyRulesEndpoint: 1, + http.MethodPut + " " + oktaPolicyEndpointByID: 1, }, }, - "infers auth server id for policy": { - oktaGroup: "MyAppUsers", - oktaPolicy: "MarketplacePolicy", + "repairs active policy missing rule before assigning client": { + scopes: []string{testScope}, + grantType: GrantTypeClientCredentials, routes: map[string]http.HandlerFunc{ - http.MethodGet + " " + groupsEndpoint: oktaGroupsSearchHandler("", []oktaGroupItem{{ID: oktaGroupIDExisting, Name: "MyAppUsers"}}), - http.MethodPut + " " + appGroupsEndpointBase + oktaGroupIDExisting: oktaAssignGroupHandler(), - http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler([]oktaPolicyItem{{ID: oktaPolicyID, Name: "MarketplacePolicy"}}), - http.MethodGet + " " + oktaPolicyEndpointByID: oktaPolicyGetHandler(oktaPolicyID, "MarketplacePolicy", []string{}), - http.MethodPut + " " + oktaPolicyEndpointByID: oktaPolicyPutMustIncludeClientHandler("app123"), + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler([]oktaPolicyItem{{ID: oktaPolicyID, Name: testPolicyName}}), + http.MethodGet + " " + oktaPolicyEndpointByID: oktaPolicyGetHandler(oktaPolicyID, testPolicyName, []string{}), + http.MethodGet + " " + oktaPolicyRulesEndpoint: oktaPolicyRulesHandler(), + http.MethodPost + " " + oktaPolicyRulesEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":"rule-1"}`)) + }, + http.MethodPut + " " + oktaPolicyEndpointByID: oktaPolicyPutMustIncludeClientHandler(testClientID), + }, + wantMinCalls: map[string]int{ + http.MethodGet + " " + oktaPoliciesEndpointByID: 1, + http.MethodGet + " " + oktaPolicyEndpointByID: 1, + http.MethodGet + " " + oktaPolicyRulesEndpoint: 1, + http.MethodPost + " " + oktaPolicyRulesEndpoint: 1, + http.MethodPut + " " + oktaPolicyEndpointByID: 1, + }, + }, + "reactivates and repairs deactivated policy missing rule before assigning client": { + scopes: []string{testScope}, + grantType: GrantTypeClientCredentials, + routes: map[string]http.HandlerFunc{ + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler([]oktaPolicyItem{{ID: oktaPolicyID, Name: testPolicyName}}), + http.MethodGet + " " + oktaPolicyEndpointByID: oktaPolicyGetHandlerWithStatus(oktaPolicyID, testPolicyName, "INACTIVE", []string{}), + http.MethodGet + " " + oktaPolicyRulesEndpoint: oktaPolicyRulesHandler(), + http.MethodPost + " " + oktaPolicyActivateEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }, + http.MethodPost + " " + oktaPolicyRulesEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":"rule-1"}`)) + }, + http.MethodPut + " " + oktaPolicyEndpointByID: oktaPolicyPutMustIncludeClientHandler(testClientID), }, wantMinCalls: map[string]int{ - http.MethodGet + " " + groupsEndpoint: 1, - http.MethodPut + " " + appGroupsEndpointBase + oktaGroupIDExisting: 1, - http.MethodGet + " " + oktaPoliciesEndpointByID: 1, - http.MethodGet + " " + oktaPolicyEndpointByID: 1, - http.MethodPut + " " + oktaPolicyEndpointByID: 1, + http.MethodGet + " " + oktaPoliciesEndpointByID: 1, + http.MethodGet + " " + oktaPolicyEndpointByID: 1, + http.MethodGet + " " + oktaPolicyRulesEndpoint: 1, + http.MethodPost + " " + oktaPolicyActivateEndpoint: 1, + http.MethodPost + " " + oktaPolicyRulesEndpoint: 1, + http.MethodPut + " " + oktaPolicyEndpointByID: 1, }, }, - "no actions when group/policy disabled": { + "no scopes results in no policy calls": { + scopes: []string{}, + grantType: GrantTypeClientCredentials, routes: map[string]http.HandlerFunc{}, wantMinCalls: map[string]int{}, }, - "error when group not found": { - oktaGroup: "NewGroup", + "policy name exactly 100 chars succeeds": { + scopes: []string{strings.Repeat("a", 82)}, + grantType: GrantTypeClientCredentials, routes: map[string]http.HandlerFunc{ - http.MethodGet + " " + groupsEndpoint: oktaGroupsSearchHandler("NewGroup", nil), + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler(nil), + http.MethodPost + " " + oktaPoliciesEndpointByID: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + _, _ = fmt.Fprintf(w, `{"id":%q}`, oktaPolicyID) + }, + http.MethodPost + " " + oktaPolicyRulesEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + }, }, - wantErr: true, - wantMinCalls: map[string]int{ - http.MethodGet + " " + groupsEndpoint: 1, + wantErr: false, + }, + "policy name length validation error": { + scopes: []string{"a-very-long-scope-name-that-when-combined-with-the-flow-name-exceeds-the-okta-100-char-limit-for-policies"}, + grantType: GrantTypeClientCredentials, + routes: map[string]http.HandlerFunc{}, + wantErr: true, + }, + "CreatePolicy error aborts registration": { + scopes: []string{testScope}, + grantType: GrantTypeClientCredentials, + routes: map[string]http.HandlerFunc{ + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler(nil), + http.MethodPost + " " + oktaPoliciesEndpointByID: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + }, }, + wantErr: true, }, - "error when policy not found": { - oktaPolicy: "MissingPolicy", + "CreatePolicyRule error aborts registration": { + scopes: []string{testScope}, + grantType: GrantTypeClientCredentials, routes: map[string]http.HandlerFunc{ - http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler([]oktaPolicyItem{{ID: "pol-other", Name: "OtherPolicy"}}), + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler(nil), + http.MethodPost + " " + oktaPoliciesEndpointByID: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + _, _ = fmt.Fprintf(w, `{"id":%q,"name":%q}`, oktaPolicyID, testPolicyName) + }, + http.MethodPost + " " + oktaPolicyRulesEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadRequest) + }, }, wantErr: true, + }, + "orphaned policy cleaned up when CreatePolicyRule fails": { + scopes: []string{testScope}, + grantType: GrantTypeClientCredentials, + routes: map[string]http.HandlerFunc{ + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler(nil), + http.MethodPost + " " + oktaPoliciesEndpointByID: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + _, _ = fmt.Fprintf(w, `{"id":%q,"name":%q}`, oktaPolicyID, testPolicyName) + }, + http.MethodPost + " " + oktaPolicyRulesEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadRequest) + }, + http.MethodPost + " " + oktaPolicyDeactivateEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }, + http.MethodDelete + " " + oktaPolicyEndpointByID: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }, + }, wantMinCalls: map[string]int{ - http.MethodGet + " " + oktaPoliciesEndpointByID: 1, + http.MethodPost + " " + oktaPolicyDeactivateEndpoint: 1, + http.MethodDelete + " " + oktaPolicyEndpointByID: 1, }, + wantErr: true, }, } for name, tc := range cases { - tc := tc t.Run(name, func(t *testing.T) { - expectedAuth := "SSWS access-token" - ts, scripted := newOktaScriptedServer(t, expectedAuth, tc.routes) + ts, scripted := newOktaScriptedServer(t, testAuthHeader, tc.routes) defer ts.Close() - credentialObj := newIDPCredential(ts.URL, tc.oktaGroup, tc.oktaPolicy) - clientRes := &clientMetadata{ClientID: "app123"} + credentialObj := newIDPCredential(ts.URL) + clientRes := &clientMetadata{ClientID: testClientID, Scope: tc.scopes, GrantTypes: []string{tc.grantType}} err := oktaProvider.postProcessClientRegistration(clientRes, credentialObj, apiClient) if tc.wantErr { assert.Error(t, err) - } else { - assert.NoError(t, err) + return } - + assert.NoError(t, err) assertMinCalls(t, scripted.getCalls(), tc.wantMinCalls) }) } } -func TestOktaPostProcessClientUnreg(t *testing.T) { +func TestOktaPostProcessClientUnregister(t *testing.T) { apiClient := coreapi.NewClient(nil, "") - oktaProvider := &okta{} + oktaProvider := &okta{logger: log.NewFieldLogger()} cases := map[string]struct { - oktaGroup string + scopes []string + grantType string routes map[string]http.HandlerFunc wantMinCalls map[string]int wantErr bool - errContains string }{ - "unassigns group when configured group exists": { - oktaGroup: "MyAppUsers", + "deactivates app, deletes app, removes client from shared policy": { + scopes: []string{testScope}, + grantType: GrantTypeClientCredentials, routes: map[string]http.HandlerFunc{ - http.MethodGet + " " + groupsEndpoint: oktaGroupsSearchHandler("MyAppUsers", []oktaGroupItem{{ID: oktaGroupID, Name: "MyAppUsers"}}), - http.MethodDelete + " " + appGroupsEndpointBase + oktaGroupID: func(w http.ResponseWriter, r *http.Request) { + http.MethodPost + " " + oktaDeactivateEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }, + http.MethodDelete + " " + oktaDeleteEndpoint: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) }, + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler([]oktaPolicyItem{{ID: oktaPolicyID, Name: testPolicyName}}), + http.MethodGet + " " + oktaPolicyEndpointByID: oktaPolicyGetHandler(oktaPolicyID, testPolicyName, []string{testClientID, "other-client"}), + http.MethodPut + " " + oktaPolicyEndpointByID: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }, }, wantMinCalls: map[string]int{ - http.MethodGet + " " + groupsEndpoint: 1, - http.MethodDelete + " " + appGroupsEndpointBase + oktaGroupID: 1, + http.MethodPost + " " + oktaDeactivateEndpoint: 1, + http.MethodDelete + " " + oktaDeleteEndpoint: 1, + http.MethodGet + " " + oktaPoliciesEndpointByID: 1, + http.MethodGet + " " + oktaPolicyEndpointByID: 1, + http.MethodPut + " " + oktaPolicyEndpointByID: 1, }, - wantErr: false, }, - "no cleanup when okta group is not configured": { - oktaGroup: "", - routes: map[string]http.HandlerFunc{}, - wantMinCalls: map[string]int{}, - wantErr: false, + "policy deactivated and deleted when last client removed": { + scopes: []string{testScope}, + grantType: GrantTypeClientCredentials, + routes: map[string]http.HandlerFunc{ + http.MethodPost + " " + oktaDeactivateEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }, + http.MethodDelete + " " + oktaDeleteEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }, + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler([]oktaPolicyItem{{ID: oktaPolicyID, Name: testPolicyName}}), + http.MethodGet + " " + oktaPolicyEndpointByID: oktaPolicyGetHandler(oktaPolicyID, testPolicyName, []string{testClientID}), + http.MethodPut + " " + oktaPolicyEndpointByID: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }, + http.MethodPost + " " + oktaPolicyDeactivateEndpoint: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }, + http.MethodDelete + " " + oktaPolicyEndpointByID: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) }, + }, + wantMinCalls: map[string]int{ + http.MethodGet + " " + oktaPoliciesEndpointByID: 1, + http.MethodGet + " " + oktaPolicyEndpointByID: 1, + http.MethodPut + " " + oktaPolicyEndpointByID: 1, + http.MethodPost + " " + oktaPolicyDeactivateEndpoint: 1, + http.MethodDelete + " " + oktaPolicyEndpointByID: 1, + }, }, - "returns error when configured group is not found": { - oktaGroup: "MissingGroup", + "missing policy during unassign is skipped": { + scopes: []string{testScope}, + grantType: GrantTypeClientCredentials, routes: map[string]http.HandlerFunc{ - http.MethodGet + " " + groupsEndpoint: oktaGroupsSearchHandler("MissingGroup", nil), + http.MethodPost + " " + oktaDeactivateEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }, + http.MethodDelete + " " + oktaDeleteEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }, + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler(nil), }, wantMinCalls: map[string]int{ - http.MethodGet + " " + groupsEndpoint: 1, + http.MethodPost + " " + oktaDeactivateEndpoint: 1, + http.MethodDelete + " " + oktaDeleteEndpoint: 1, + http.MethodGet + " " + oktaPoliciesEndpointByID: 1, + }, + }, + "deactivate 404 is treated as success": { + scopes: []string{}, + grantType: GrantTypeClientCredentials, + routes: map[string]http.HandlerFunc{ + http.MethodPost + " " + oktaDeactivateEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + }, + http.MethodDelete + " " + oktaDeleteEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }, + }, + wantMinCalls: map[string]int{ + http.MethodPost + " " + oktaDeactivateEndpoint: 1, + http.MethodDelete + " " + oktaDeleteEndpoint: 1, }, - wantErr: true, - errContains: "configured okta group", }, } for name, tc := range cases { - tc := tc t.Run(name, func(t *testing.T) { - expectedAuth := "SSWS access-token" - ts, scripted := newOktaScriptedServer(t, expectedAuth, tc.routes) + ts, scripted := newOktaScriptedServer(t, testAuthHeader, tc.routes) defer ts.Close() + credentialObj := newIDPCredential(ts.URL) - credentialObj := &corecfg.IDPConfiguration{ - MetadataURL: ts.URL + oauthMetadataEndpoint, - Okta: &corecfg.OktaIDPConfiguration{Group: tc.oktaGroup}, - AuthConfig: &corecfg.IDPAuthConfiguration{AccessToken: accessToken}, - } - err := oktaProvider.postProcessClientUnregister("app123", credentialObj, apiClient) + err := oktaProvider.postProcessClientUnregister(testClientID, credentialObj, apiClient, tc.scopes, tc.grantType) if tc.wantErr { assert.Error(t, err) - if tc.errContains != "" { - assert.Contains(t, err.Error(), tc.errContains) - } - } else { - assert.NoError(t, err) + return } + assert.NoError(t, err) + assertMinCalls(t, scripted.getCalls(), tc.wantMinCalls) + }) + } +} - calls := scripted.getCalls() - for key, minCount := range tc.wantMinCalls { - assert.GreaterOrEqual(t, calls[key], minCount, "expected at least %d calls to %s", minCount, key) - } +func TestNormalizeGrantType(t *testing.T) { + cases := map[string]struct { + input string + want string + }{ + "client_credentials": {input: GrantTypeClientCredentials, want: normalizedClientCredentials}, + "authorization_code": {input: GrantTypeAuthorizationCode, want: normalizedAuthorizationCode}, + "implicit": {input: GrantTypeImplicit, want: normalizedImplicitGrant}, + "unknown strips underscores": {input: "custom_grant_type", want: "customgranttype"}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + assert.Equal(t, tc.want, normalizeGrantType(tc.input)) }) } } func TestOktaPostProcessClientRegUsesIDPAccessToken(t *testing.T) { - // We only need to prove that auth.accessToken from the IDP config is used to - // enable Okta post-processing (i.e. the hook does not early-return). - // Avoid making any Okta API calls by disabling all optional actions. ts := httptest.NewServer(nil) defer ts.Close() - credentialObj := &corecfg.IDPConfiguration{ - MetadataURL: ts.URL + oauthMetadataEndpoint, - AuthConfig: &corecfg.IDPAuthConfiguration{AccessToken: accessToken}, - } - apiClient := coreapi.NewClient(nil, "") - oktaProvider := &okta{} - clientRes := &clientMetadata{ClientID: "app123"} - err := oktaProvider.postProcessClientRegistration(clientRes, credentialObj, apiClient) + oktaProvider := &okta{logger: log.NewFieldLogger()} + clientRes := &clientMetadata{ClientID: testClientID} + err := oktaProvider.postProcessClientRegistration(clientRes, newIDPCredential(ts.URL), apiClient) assert.NoError(t, err) } @@ -374,28 +512,22 @@ func TestOktaBaseURLFromMetadataURL(t *testing.T) { }{ "empty metadata url returns error": { metadataURL: "", - want: "", wantErr: true, }, "okta issuer url returns scheme+host": { metadataURL: "https://integrator-1663282.okta.com/oauth2/authorizationid/.well-known/oauth-authorization-server", want: "https://integrator-1663282.okta.com", - wantErr: false, }, "invalid url returns error": { metadataURL: "://bad-url", - want: "", wantErr: true, }, "missing scheme/host returns error": { metadataURL: "/relative/path", - want: "", wantErr: true, }, } - for name, tc := range cases { - tc := tc t.Run(name, func(t *testing.T) { got, err := oktaBaseURLFromMetadataURL(tc.metadataURL) if tc.wantErr { @@ -410,175 +542,605 @@ func TestOktaBaseURLFromMetadataURL(t *testing.T) { func TestOktaPKCERequired(t *testing.T) { cases := map[string]struct { - pkceRequired bool - expectedValue bool + pkceRequired bool + want bool }{ - "PKCE required true": { - pkceRequired: true, - expectedValue: true, - }, - "PKCE required false": { - pkceRequired: false, - expectedValue: false, - }, + "PKCE required true": {pkceRequired: true, want: true}, + "PKCE required false": {pkceRequired: false, want: false}, } - for name, tc := range cases { t.Run(name, func(t *testing.T) { - props := map[string]interface{}{ - oktaPKCERequired: tc.pkceRequired, - } + props := map[string]any{oktaPKCERequired: tc.pkceRequired} c, err := NewClientMetadataBuilder(). SetClientName(oktaSpa). SetExtraProperties(props). Build() - assert.Nil(t, err) + assert.NoError(t, err) cm := c.(*clientMetadata) buf, err := json.Marshal(cm) - assert.Nil(t, err) - assert.NotNil(t, buf) + assert.NoError(t, err) - var out map[string]interface{} - err = json.Unmarshal(buf, &out) - assert.Nil(t, err) + var out map[string]any + assert.NoError(t, json.Unmarshal(buf, &out)) - // Should be a boolean, not a string val, ok := out[oktaPKCERequired] assert.True(t, ok) - assert.IsType(t, tc.expectedValue, val) - assert.Equal(t, tc.expectedValue, val) + assert.Equal(t, tc.want, val) }) } } func TestValidateOktaExtraProperties(t *testing.T) { cases := map[string]struct { - extraProps map[string]interface{} - expectError bool + extraProps map[string]any + wantErr bool }{ "Valid: PKCE with browser type": { - extraProps: map[string]interface{}{ - oktaPKCERequired: true, - oktaApplicationType: oktaAppTypeBrowser, - }, - expectError: false, + extraProps: map[string]any{oktaPKCERequired: true, oktaApplicationType: oktaAppTypeBrowser}, }, "Valid: PKCE without app type": { - extraProps: map[string]interface{}{ - oktaPKCERequired: true, - }, - expectError: false, + extraProps: map[string]any{oktaPKCERequired: true}, }, "Invalid: PKCE with service type": { - extraProps: map[string]interface{}{ - oktaPKCERequired: true, - oktaApplicationType: oktaAppTypeService, - }, - expectError: true, + extraProps: map[string]any{oktaPKCERequired: true, oktaApplicationType: oktaAppTypeService}, + wantErr: true, }, "Invalid: PKCE with web type": { - extraProps: map[string]interface{}{ - oktaPKCERequired: true, - oktaApplicationType: oktaAppTypeWeb, - }, - expectError: true, + extraProps: map[string]any{oktaPKCERequired: true, oktaApplicationType: oktaAppTypeWeb}, + wantErr: true, }, "Valid: No PKCE with any type": { - extraProps: map[string]interface{}{ - oktaApplicationType: oktaAppTypeService, - }, - expectError: false, + extraProps: map[string]any{oktaApplicationType: oktaAppTypeService}, }, } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + err := (&okta{logger: log.NewFieldLogger()}).validateExtraProperties(tc.extraProps) + if tc.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + }) + } +} +func TestValidateOktaTemplates(t *testing.T) { + cases := map[string]struct { + appTemplate string + policyTemplate string + wantErr bool + errContains string + }{ + "default templates pass": { + appTemplate: defaultAppTemplate, + policyTemplate: defaultPolicyTemplate, + }, + "app template with only one recognized placeholder passes": { + appTemplate: corecfg.OktaPlaceholderMPApplicationName, + policyTemplate: defaultPolicyTemplate, + }, + "app template with two of three recognized placeholders passes": { + appTemplate: corecfg.OktaPlaceholderMPApplicationName + "-" + corecfg.OktaPlaceholderCredentialName, + policyTemplate: defaultPolicyTemplate, + }, + "unrecognized placeholder in app template fails": { + appTemplate: corecfg.OktaPlaceholderMPApplicationName + "-%TYPO%-" + corecfg.OktaPlaceholderCredentialName, + policyTemplate: defaultPolicyTemplate, + wantErr: true, + errContains: corecfg.OktaAppNameTemplateKey, + }, + "policy template missing %SCOPE% fails": { + appTemplate: defaultAppTemplate, + policyTemplate: corecfg.OktaPlaceholderOAuthFlow, + wantErr: true, + errContains: corecfg.OktaPlaceholderScope, + }, + "policy template missing %OAUTH_FLOW% fails": { + appTemplate: defaultAppTemplate, + policyTemplate: corecfg.OktaPlaceholderScope, + wantErr: true, + errContains: corecfg.OktaPlaceholderOAuthFlow, + }, + "unrecognized placeholder in policy template fails": { + appTemplate: defaultAppTemplate, + policyTemplate: corecfg.OktaPlaceholderScope + "-%CUSTOM_FLOW%", + wantErr: true, + errContains: corecfg.OktaPolicyNameTemplateKey, + }, + } for name, tc := range cases { t.Run(name, func(t *testing.T) { - oktaProvider := &okta{} - err := oktaProvider.validateExtraProperties(tc.extraProps) - if tc.expectError { + cfg := &corecfg.IDPConfiguration{ + Okta: &corecfg.OktaIDPConfiguration{ + AppNameTemplate: tc.appTemplate, + PolicyNameTemplate: tc.policyTemplate, + }, + } + err := validateOktaTemplates(cfg) + if tc.wantErr { assert.Error(t, err) - } else { - assert.NoError(t, err) + assert.Contains(t, err.Error(), tc.errContains) + return } + assert.NoError(t, err) }) } } func TestOktaPreProcessClientRequest(t *testing.T) { cases := map[string]struct { - grantTypes []string - responseTypes []string - extraProperties map[string]interface{} - expectedAppType string - expectedResponseTypes []string - expectedAuthMethod string + grantTypes []string + responseTypes []string + extraProperties map[string]any + wantAppType string + wantResponseTypes []string + wantAuthMethod string }{ "Authorization code with PKCE should use browser type": { - grantTypes: []string{GrantTypeAuthorizationCode}, - extraProperties: map[string]interface{}{ - oktaPKCERequired: true, - }, - expectedAppType: oktaAppTypeBrowser, - expectedAuthMethod: "none", + grantTypes: []string{GrantTypeAuthorizationCode}, + extraProperties: map[string]any{oktaPKCERequired: true}, + wantAppType: oktaAppTypeBrowser, + wantAuthMethod: testAuthMethodNone, }, "Authorization code without PKCE should use web type": { - grantTypes: []string{GrantTypeAuthorizationCode}, - extraProperties: map[string]interface{}{ - oktaPKCERequired: false, - }, - expectedAppType: oktaAppTypeWeb, + grantTypes: []string{GrantTypeAuthorizationCode}, + extraProperties: map[string]any{oktaPKCERequired: false}, + wantAppType: oktaAppTypeWeb, }, "Client credentials should remain service type": { - grantTypes: []string{GrantTypeClientCredentials}, - responseTypes: []string{}, - extraProperties: map[string]interface{}{}, - expectedAppType: oktaAppTypeService, - expectedResponseTypes: []string{AuthResponseToken}, + grantTypes: []string{GrantTypeClientCredentials}, + responseTypes: []string{}, + extraProperties: map[string]any{}, + wantAppType: oktaAppTypeService, + wantResponseTypes: []string{AuthResponseToken}, }, "Explicit browser type should be preserved with PKCE": { - grantTypes: []string{GrantTypeAuthorizationCode}, - extraProperties: map[string]interface{}{ - oktaApplicationType: oktaAppTypeBrowser, - oktaPKCERequired: true, - }, - expectedAppType: oktaAppTypeBrowser, - expectedAuthMethod: "none", + grantTypes: []string{GrantTypeAuthorizationCode}, + extraProperties: map[string]any{oktaApplicationType: oktaAppTypeBrowser, oktaPKCERequired: true}, + wantAppType: oktaAppTypeBrowser, + wantAuthMethod: testAuthMethodNone, }, "Implicit flow without PKCE should use web type": { - grantTypes: []string{GrantTypeImplicit}, - extraProperties: map[string]interface{}{ - oktaPKCERequired: false, - }, - expectedAppType: oktaAppTypeWeb, + grantTypes: []string{GrantTypeImplicit}, + extraProperties: map[string]any{oktaPKCERequired: false}, + wantAppType: oktaAppTypeWeb, }, } - for name, tc := range cases { t.Run(name, func(t *testing.T) { - oktaProvider := &okta{} + oktaProvider := &okta{logger: log.NewFieldLogger()} clientReq := &clientMetadata{ GrantTypes: tc.grantTypes, ResponseTypes: tc.responseTypes, extraProperties: tc.extraProperties, } - - // Simulate validation step which sets defaults (as happens in NewProvider) _ = oktaProvider.validateExtraProperties(clientReq.extraProperties) - oktaProvider.preProcessClientRequest(clientReq) appType, ok := clientReq.extraProperties[oktaApplicationType].(string) assert.True(t, ok) - assert.Equal(t, tc.expectedAppType, appType) + assert.Equal(t, tc.wantAppType, appType) + + if tc.wantResponseTypes != nil { + assert.Equal(t, tc.wantResponseTypes, clientReq.ResponseTypes) + } + if tc.wantAuthMethod != "" { + assert.Equal(t, tc.wantAuthMethod, clientReq.TokenEndpointAuthMethod) + } + }) + } +} + +func TestOktaCreatePolicyUsesDefaultPriority(t *testing.T) { + apiClient := coreapi.NewClient(nil, "") + oktaProvider := &okta{logger: log.NewFieldLogger()} + var captured []byte + routes := map[string]http.HandlerFunc{ + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler(nil), + http.MethodPost + " " + oktaPoliciesEndpointByID: func(w http.ResponseWriter, r *http.Request) { + captured, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusCreated) + _, _ = fmt.Fprintf(w, `{"id":%q}`, oktaPolicyID) + }, + http.MethodPost + " " + oktaPolicyRulesEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + }, + } + ts, _ := newOktaScriptedServer(t, testAuthHeader, routes) + defer ts.Close() + + idpCfg := &corecfg.IDPConfiguration{ + MetadataURL: ts.URL + oauthMetadataEndpoint, + AuthConfig: &corecfg.IDPAuthConfiguration{AccessToken: accessToken}, + } + clientRes := &clientMetadata{ClientID: testClientID, Scope: []string{testScope}, GrantTypes: []string{GrantTypeClientCredentials}} + assert.NoError(t, oktaProvider.postProcessClientRegistration(clientRes, idpCfg, apiClient)) + + var body map[string]any + assert.NoError(t, json.Unmarshal(captured, &body)) + assert.EqualValues(t, 1, body["priority"]) +} + +func TestOktaDeactivateThenDelete(t *testing.T) { + apiClient := coreapi.NewClient(nil, "") + oktaProvider := &okta{logger: log.NewFieldLogger()} + + var mu sync.Mutex + callOrder := make([]string, 0, 2) + + routes := map[string]http.HandlerFunc{ + http.MethodPost + " " + oktaDeactivateEndpoint: func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + callOrder = append(callOrder, "deactivate") + mu.Unlock() + w.WriteHeader(http.StatusOK) + }, + http.MethodDelete + " " + oktaDeleteEndpoint: func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + callOrder = append(callOrder, "delete") + mu.Unlock() + w.WriteHeader(http.StatusNoContent) + }, + } + ts, _ := newOktaScriptedServer(t, testAuthHeader, routes) + defer ts.Close() + + err := oktaProvider.postProcessClientUnregister(testClientID, newIDPCredential(ts.URL), apiClient, nil, GrantTypeClientCredentials) + assert.NoError(t, err) + assert.Equal(t, []string{"deactivate", "delete"}, callOrder) +} + +func TestOktaPolicyRepairOrder(t *testing.T) { + apiClient := coreapi.NewClient(nil, "") + oktaProvider := &okta{logger: log.NewFieldLogger()} + + cases := map[string]struct { + policyStatus string + wantCallOrder []string + }{ + "active policy: rule created, no activate": { + policyStatus: "ACTIVE", + wantCallOrder: []string{callCreateRule}, + }, + "inactive policy: rule created then activated": { + policyStatus: "INACTIVE", + wantCallOrder: []string{callCreateRule, callActivate}, + }, + } + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + var mu sync.Mutex + callOrder := make([]string, 0, 2) - if tc.expectedResponseTypes != nil { - assert.Equal(t, tc.expectedResponseTypes, clientReq.ResponseTypes) + routes := map[string]http.HandlerFunc{ + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler([]oktaPolicyItem{{ID: oktaPolicyID, Name: testPolicyName}}), + http.MethodGet + " " + oktaPolicyEndpointByID: oktaPolicyGetHandlerWithStatus(oktaPolicyID, testPolicyName, tc.policyStatus, []string{}), + http.MethodGet + " " + oktaPolicyRulesEndpoint: oktaPolicyRulesHandler(), + http.MethodPost + " " + oktaPolicyRulesEndpoint: func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + callOrder = append(callOrder, callCreateRule) + mu.Unlock() + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":"rule-1"}`)) + }, + http.MethodPost + " " + oktaPolicyActivateEndpoint: func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + callOrder = append(callOrder, callActivate) + mu.Unlock() + w.WriteHeader(http.StatusOK) + }, + http.MethodPut + " " + oktaPolicyEndpointByID: oktaPolicyPutMustIncludeClientHandler(testClientID), } + ts, _ := newOktaScriptedServer(t, testAuthHeader, routes) + defer ts.Close() - if tc.expectedAuthMethod != "" { - assert.Equal(t, tc.expectedAuthMethod, clientReq.TokenEndpointAuthMethod) + clientRes := &clientMetadata{ClientID: testClientID, Scope: []string{testScope}, GrantTypes: []string{GrantTypeClientCredentials}} + assert.NoError(t, oktaProvider.postProcessClientRegistration(clientRes, newIDPCredential(ts.URL), apiClient)) + assert.Equal(t, tc.wantCallOrder, callOrder) + }) + } +} + +func TestOktaCleanupOrphanedPolicy(t *testing.T) { + apiClient := coreapi.NewClient(nil, "") + oktaProvider := &okta{logger: log.NewFieldLogger()} + + cases := map[string]struct { + deactivateCode int + deleteCode int + wantDeleteCalled bool + }{ + "deactivate and delete both succeed": { + deactivateCode: http.StatusOK, + deleteCode: http.StatusNoContent, + wantDeleteCalled: true, + }, + "deactivate fails — delete is skipped": { + deactivateCode: http.StatusForbidden, + deleteCode: http.StatusNoContent, + wantDeleteCalled: false, + }, + "deactivate succeeds delete fails": { + deactivateCode: http.StatusOK, + deleteCode: http.StatusForbidden, + wantDeleteCalled: true, + }, + } + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + deleteCalled := false + routes := map[string]http.HandlerFunc{ + http.MethodPost + " " + oktaPolicyDeactivateEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.deactivateCode) + }, + http.MethodDelete + " " + oktaPolicyEndpointByID: func(w http.ResponseWriter, r *http.Request) { + deleteCalled = true + w.WriteHeader(tc.deleteCode) + }, } + ts, _ := newOktaScriptedServer(t, testAuthHeader, routes) + defer ts.Close() + + oktaClientObj := clients.New(apiClient, ts.URL, accessToken) + oktaProvider.cleanupOrphanedPolicy(oktaClientObj, "authorizationID", oktaPolicyID) + assert.Equal(t, tc.wantDeleteCalled, deleteCalled) + }) + } +} + +func TestOktaPostProcessClientUnregisterAbortOnRemoveError(t *testing.T) { + apiClient := coreapi.NewClient(nil, "") + oktaProvider := &okta{logger: log.NewFieldLogger()} + + routes := map[string]http.HandlerFunc{ + http.MethodPost + " " + oktaDeactivateEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }, + http.MethodDelete + " " + oktaDeleteEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }, + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler([]oktaPolicyItem{{ID: oktaPolicyID, Name: testPolicyName}}), + http.MethodGet + " " + oktaPolicyEndpointByID: oktaPolicyGetHandler(oktaPolicyID, testPolicyName, []string{testClientID}), + http.MethodPut + " " + oktaPolicyEndpointByID: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + }, + } + ts, scripted := newOktaScriptedServer(t, testAuthHeader, routes) + defer ts.Close() + + err := oktaProvider.postProcessClientUnregister(testClientID, newIDPCredential(ts.URL), apiClient, []string{testScope, "write:api"}, GrantTypeClientCredentials) + assert.Error(t, err) + + calls := scripted.getCalls() + assert.Equal(t, 1, calls[http.MethodGet+" "+oktaPoliciesEndpointByID], "policy list GET must stop after first scope fails") +} + +func groupListHandler(id, name string) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = fmt.Fprintf(w, `[{"id":%q,"profile":{"name":%q}}]`, id, name) + } +} + +func newIDPCredentialWithGroup(tsURL, group string) *corecfg.IDPConfiguration { + cfg := newIDPCredential(tsURL) + cfg.Okta = &corecfg.OktaIDPConfiguration{Group: group} + return cfg +} + +func TestValidateOktaGroupExists(t *testing.T) { + apiClient := coreapi.NewClient(nil, "") + + cases := map[string]struct { + group string + routes map[string]http.HandlerFunc + wantErr bool + }{ + "no group configured is a no-op": { + group: "", + routes: map[string]http.HandlerFunc{}, + wantErr: false, + }, + "configured group found returns nil": { + group: testGroupName, + routes: map[string]http.HandlerFunc{ + http.MethodGet + " " + oktaGroupsEndpoint: groupListHandler(testGroupID, testGroupName), + }, + wantErr: false, + }, + "configured group not found returns error": { + group: testGroupName, + routes: map[string]http.HandlerFunc{ + http.MethodGet + " " + oktaGroupsEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`[]`)) + }, + }, + wantErr: true, + }, + "API error returns error": { + group: testGroupName, + routes: map[string]http.HandlerFunc{ + http.MethodGet + " " + oktaGroupsEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + }, + }, + wantErr: true, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + ts, _ := newOktaScriptedServer(t, testAuthHeader, tc.routes) + defer ts.Close() + cfg := newIDPCredentialWithGroup(ts.URL, tc.group) + err := validateOktaGroupExists(cfg, apiClient) + if tc.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + }) + } +} + +func TestOktaPostProcessClientRegistrationWithGroup(t *testing.T) { + apiClient := coreapi.NewClient(nil, "") + oktaProvider := &okta{logger: log.NewFieldLogger()} + + cases := map[string]struct { + routes map[string]http.HandlerFunc + wantMinCalls map[string]int + wantErr bool + }{ + "group assigned after per-scope policy": { + routes: map[string]http.HandlerFunc{ + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler(nil), + http.MethodPost + " " + oktaPoliciesEndpointByID: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + _, _ = fmt.Fprintf(w, `{"id":%q}`, oktaPolicyID) + }, + http.MethodPost + " " + oktaPolicyRulesEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + }, + http.MethodGet + " " + oktaGroupsEndpoint: groupListHandler(testGroupID, testGroupName), + http.MethodPut + " " + oktaAppGroupEndpoint: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }, + }, + wantMinCalls: map[string]int{ + http.MethodGet + " " + oktaGroupsEndpoint: 1, + http.MethodPut + " " + oktaAppGroupEndpoint: 1, + }, + }, + "group not found returns error": { + routes: map[string]http.HandlerFunc{ + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler(nil), + http.MethodPost + " " + oktaPoliciesEndpointByID: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + _, _ = fmt.Fprintf(w, `{"id":%q}`, oktaPolicyID) + }, + http.MethodPost + " " + oktaPolicyRulesEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + }, + http.MethodGet + " " + oktaGroupsEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`[]`)) + }, + }, + wantErr: true, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + ts, scripted := newOktaScriptedServer(t, testAuthHeader, tc.routes) + defer ts.Close() + cfg := newIDPCredentialWithGroup(ts.URL, testGroupName) + clientRes := &clientMetadata{ClientID: testClientID, Scope: []string{testScope}, GrantTypes: []string{GrantTypeClientCredentials}} + err := oktaProvider.postProcessClientRegistration(clientRes, cfg, apiClient) + if tc.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assertMinCalls(t, scripted.getCalls(), tc.wantMinCalls) + }) + } +} + +func TestOktaPostProcessClientUnregisterWithGroup(t *testing.T) { + apiClient := coreapi.NewClient(nil, "") + oktaProvider := &okta{logger: log.NewFieldLogger()} + + cases := map[string]struct { + routes map[string]http.HandlerFunc + wantMinCalls map[string]int + wantErr bool + }{ + "group removed after app delete": { + routes: map[string]http.HandlerFunc{ + http.MethodPost + " " + oktaDeactivateEndpoint: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }, + http.MethodDelete + " " + oktaDeleteEndpoint: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) }, + http.MethodGet + " " + oktaGroupsEndpoint: groupListHandler(testGroupID, testGroupName), + http.MethodDelete + " " + oktaAppGroupEndpoint: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) }, + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler(nil), + }, + wantMinCalls: map[string]int{ + http.MethodGet + " " + oktaGroupsEndpoint: 1, + http.MethodDelete + " " + oktaAppGroupEndpoint: 1, + }, + }, + "group not in Okta during unregister is skipped": { + routes: map[string]http.HandlerFunc{ + http.MethodPost + " " + oktaDeactivateEndpoint: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }, + http.MethodDelete + " " + oktaDeleteEndpoint: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) }, + http.MethodGet + " " + oktaGroupsEndpoint: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`[]`)) + }, + http.MethodGet + " " + oktaPoliciesEndpointByID: oktaPoliciesListHandler(nil), + }, + wantMinCalls: map[string]int{ + http.MethodGet + " " + oktaGroupsEndpoint: 1, + }, + wantErr: false, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + ts, scripted := newOktaScriptedServer(t, testAuthHeader, tc.routes) + defer ts.Close() + cfg := newIDPCredentialWithGroup(ts.URL, testGroupName) + err := oktaProvider.postProcessClientUnregister(testClientID, cfg, apiClient, nil, GrantTypeClientCredentials) + if tc.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assertMinCalls(t, scripted.getCalls(), tc.wantMinCalls) + }) + } +} + +func TestPolicyNameTemplate(t *testing.T) { + cases := map[string]struct { + cfg corecfg.IDPConfig + grantType string + scope string + wantName string + }{ + "default template with client_credentials": { + cfg: &corecfg.IDPConfiguration{}, + grantType: GrantTypeClientCredentials, + scope: testScope, + wantName: testScope + "-" + normalizedClientCredentials, + }, + "default template with authorization_code": { + cfg: &corecfg.IDPConfiguration{}, + grantType: GrantTypeAuthorizationCode, + scope: testScope, + wantName: testScope + "-" + normalizedAuthorizationCode, + }, + "custom template overrides default": { + cfg: &corecfg.IDPConfiguration{Okta: &corecfg.OktaIDPConfiguration{PolicyNameTemplate: corecfg.OktaPlaceholderOAuthFlow + "-" + corecfg.OktaPlaceholderScope}}, + grantType: GrantTypeClientCredentials, + scope: testScope, + wantName: normalizedClientCredentials + "-" + testScope, + }, + "implicit flow normalizes correctly": { + cfg: &corecfg.IDPConfiguration{}, + grantType: GrantTypeImplicit, + scope: testScope, + wantName: testScope + "-" + normalizedImplicitGrant, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + template := policyNameTemplate(tc.cfg) + normalizedFlow := normalizeGrantType(tc.grantType) + got := strings.NewReplacer(corecfg.OktaPlaceholderScope, tc.scope, corecfg.OktaPlaceholderOAuthFlow, normalizedFlow).Replace(template) + assert.Equal(t, tc.wantName, got) }) } } diff --git a/pkg/authz/oauth/provider.go b/pkg/authz/oauth/provider.go index f83bbfe2f..14b715eba 100644 --- a/pkg/authz/oauth/provider.go +++ b/pkg/authz/oauth/provider.go @@ -36,7 +36,7 @@ type Provider interface { GetSupportedTokenAuthMethods() []string GetSupportedResponseMethod() []string RegisterClient(clientMetadata ClientMetadata) (ClientMetadata, error) - UnregisterClient(clientID, accessToken, registrationClientURI string) error + UnregisterClient(clientID, accessToken, registrationClientURI string, scopes []string, grantType string) error Validate() error GetConfig() corecfg.IDPConfig GetMetadata() *AuthorizationServerMetadata @@ -62,7 +62,7 @@ type typedIDP interface { preProcessClientRequest(clientRequest *clientMetadata) validateExtraProperties(extraProps map[string]interface{}) error postProcessClientRegistration(clientRes ClientMetadata, idp corecfg.IDPConfig, apiClient coreapi.Client) error - postProcessClientUnregister(clientID string, idp corecfg.IDPConfig, apiClient coreapi.Client) error + postProcessClientUnregister(clientID string, idp corecfg.IDPConfig, apiClient coreapi.Client, scopes []string, grantType string) error } type providerOptions struct { @@ -96,7 +96,9 @@ func NewProvider(idp corecfg.IDPConfig, tlsCfg corecfg.TLSConfig, proxyURL strin var idpType typedIDP switch idp.GetIDPType() { case TypeOkta: - idpType = &okta{} + idpType = &okta{ + logger: log.NewFieldLogger().WithComponent("oktaProvider").WithPackage("sdk.agent.authz.oauth"), + } default: // keycloak, generic idpType = &genericIDP{} } @@ -130,13 +132,6 @@ func NewProvider(idp corecfg.IDPConfig, tlsCfg corecfg.TLSConfig, proxyURL strin p.authServerMetadata = metadata } - // Fail-fast Okta validation: if Okta group/policy is configured, verify the resources exist. - if idp.GetIDPType() == TypeOkta { - if err := validateOktaConfiguredResources(idp, apiClient); err != nil { - return nil, fmt.Errorf("failed to validate Okta configuration for provider %q: %w", p.cfg.GetIDPName(), err) - } - } - // No OAuth client is needed to request token for access token based authentication to IdP if p.cfg.GetAuthConfig() != nil && p.cfg.GetAuthConfig().GetType() != corecfg.AccessToken { authClient, err := p.createAuthClient() @@ -151,9 +146,54 @@ func NewProvider(idp corecfg.IDPConfig, tlsCfg corecfg.TLSConfig, proxyURL strin return nil, fmt.Errorf("invalid extra properties for %s provider: %w", idp.GetIDPType(), err) } + if idp.GetIDPType() == TypeOkta { + if err := validateOktaTemplates(idp); err != nil { + return nil, err + } + if err := validateOktaGroupExists(idp, apiClient); err != nil { + return nil, err + } + } + return p, nil } +func validateOktaTemplates(idp corecfg.IDPConfig) error { + appTmplCfg, ok := idp.(interface{ GetAppNameTemplate() string }) + if !ok { + return nil + } + appTmpl := appTmplCfg.GetAppNameTemplate() + test := strings.NewReplacer( + corecfg.OktaPlaceholderMPApplicationName, "x", + corecfg.OktaPlaceholderOwningTeam, "x", + corecfg.OktaPlaceholderCredentialName, "x", + ).Replace(appTmpl) + if strings.Contains(test, "%") { + return fmt.Errorf("%s contains unrecognized placeholder: %q", corecfg.OktaAppNameTemplateKey, appTmpl) + } + + polTmplCfg, ok := idp.(interface{ GetPolicyNameTemplate() string }) + if !ok { + return nil + } + polTmpl := polTmplCfg.GetPolicyNameTemplate() + if !strings.Contains(polTmpl, corecfg.OktaPlaceholderScope) { + return fmt.Errorf("%s missing required placeholder %s: %q", corecfg.OktaPolicyNameTemplateKey, corecfg.OktaPlaceholderScope, polTmpl) + } + if !strings.Contains(polTmpl, corecfg.OktaPlaceholderOAuthFlow) { + return fmt.Errorf("%s missing required placeholder %s: %q", corecfg.OktaPolicyNameTemplateKey, corecfg.OktaPlaceholderOAuthFlow, polTmpl) + } + test = strings.NewReplacer( + corecfg.OktaPlaceholderScope, "x", + corecfg.OktaPlaceholderOAuthFlow, "x", + ).Replace(polTmpl) + if strings.Contains(test, "%") { + return fmt.Errorf("%s contains unrecognized placeholder: %q", corecfg.OktaPolicyNameTemplateKey, polTmpl) + } + return nil +} + func FetchMetadata(apiClient coreapi.Client, metadataURL string) (*AuthorizationServerMetadata, error) { if apiClient == nil || metadataURL == "" { return nil, errors.New("unexpected arguments") @@ -320,12 +360,44 @@ func (p *provider) GetAuthorizationEndpoint() string { return "" } -// GetSupportedScopes - returns the global scopes supported by provider +// GetSupportedScopes - returns the scopes supported by the provider, with any +// configured exclude list removed. Non-Okta providers receive the raw scope list +// unchanged. func (p *provider) GetSupportedScopes() []string { - if p.authServerMetadata != nil { - return p.authServerMetadata.ScopesSupported + if p.authServerMetadata == nil { + return []string{""} } - return []string{""} + scopes := p.authServerMetadata.ScopesSupported + if p.cfg.GetIDPType() != TypeOkta { + return scopes + } + ex, ok := p.cfg.(interface{ GetScopeExclude() string }) + if !ok || ex.GetScopeExclude() == "" { + return scopes + } + return filterScopeExclude(scopes, ex.GetScopeExclude()) +} + +// filterScopeExclude removes any scope that appears in the comma-separated +// exclude string. Order of the remaining scopes is preserved. +func filterScopeExclude(scopes []string, exclude string) []string { + if len(scopes) == 0 { + return scopes + } + denied := make(map[string]struct{}) + for _, s := range strings.Split(exclude, ",") { + s = strings.TrimSpace(s) + if s != "" { + denied[s] = struct{}{} + } + } + out := make([]string, 0, len(scopes)) + for _, s := range scopes { + if _, blocked := denied[s]; !blocked { + out = append(out, s) + } + } + return out } // GetSupportedGrantTypes - returns the grant type supported by provider @@ -409,6 +481,15 @@ func (p *provider) RegisterClient(clientReq ClientMetadata) (ClientMetadata, err if !p.cfg.GetAuthConfig().UseRegistrationAccessToken() { clientRes.RegistrationAccessToken = "" } + // Some IdPs (notably Okta for client_credentials) omit scope/grant_types + // from the registration response. Carry them from the request so the + // post-hook (per-scope policy assignment) has the data it needs. + if len(clientRes.Scope) == 0 { + clientRes.Scope = Scopes(clientReq.GetScopes()) + } + if len(clientRes.GrantTypes) == 0 { + clientRes.GrantTypes = clientReq.GetGrantTypes() + } if err := p.runPostRegistrationHook(clientRes, authPrefix, token); err != nil { return nil, err @@ -547,7 +628,7 @@ func addResponseType(clientRequest *clientMetadata, responseType string) { } // UnregisterClient - removes the OAuth client from IDP -func (p *provider) UnregisterClient(clientID, accessToken, registrationClientURI string) error { +func (p *provider) UnregisterClient(clientID, accessToken, registrationClientURI string, scopes []string, grantType string) error { logger := p.logger. WithField(logProvider, p.cfg.GetIDPName()). WithField(logClientID, clientID) @@ -563,38 +644,30 @@ func (p *provider) UnregisterClient(clientID, accessToken, registrationClientURI accessToken = token } - // Run provider-specific cleanup first (for example, Okta group/policy unassignment), but always continue to OAuth client deletion so transient cleanup failures do not - // leave active clients behind. Return whichever error(s) occurred after both steps. - cleanupErr := p.runPostUnregisterHook(clientID) - unregisterErr := p.attemptUnregisterAll(logger, clientID, registrationClientURI, authPrefix, accessToken) - - if unregisterErr == nil { - logger.Info("unregistered client") + // Continue to OAuth client deletion even on cleanup error; leave no active clients behind. + if p.cfg.GetIDPType() == TypeOkta { + cleanupErr := p.runPostUnregisterHook(clientID, scopes, grantType) + if cleanupErr != nil { + return fmt.Errorf("failed to complete provider cleanup after client unregistration. Manual cleanup in Okta may be required: %w", cleanupErr) + } + return nil } - switch { - case cleanupErr != nil && unregisterErr != nil: - return fmt.Errorf("failed to fully remove the Okta client. Provider cleanup failed and OAuth client deletion failed. cleanup error: %v; delete error: %w", cleanupErr, unregisterErr) - case unregisterErr != nil: + // if not Okta, attempt to unregister everything depending on the provided values + unregisterErr := p.attemptUnregisterAll(logger, clientID, registrationClientURI, authPrefix, accessToken) + if unregisterErr != nil { return fmt.Errorf("failed to delete the OAuth client from the identity provider: %w", unregisterErr) - case cleanupErr != nil: - return fmt.Errorf("failed to complete provider cleanup after client unregistration. Manual cleanup in Okta may be required: %w", cleanupErr) - default: - return nil } + logger.Info("successfully unregistered client") + return nil } -// runPostUnregisterHook calls provider-specific unregister hook when applicable. -func (p *provider) runPostUnregisterHook(clientID string) error { - if p.cfg.GetIDPType() != TypeOkta { - return nil - } - return p.idpType.postProcessClientUnregister(clientID, p.cfg, p.apiClient) +func (p *provider) runPostUnregisterHook(clientID string, scopes []string, grantType string) error { + return p.idpType.postProcessClientUnregister(clientID, p.cfg, p.apiClient, scopes, grantType) } // attemptUnregisterAll tries unregistering with the registration URI, the standard // registration endpoint (base + /clientID) and finally as a query-parameter. - func (p *provider) attemptUnregisterAll(logger log.FieldLogger, clientID, registrationClientURI, authPrefix, accessToken string) error { var err error @@ -649,7 +722,7 @@ func (p *provider) tryUnregister(unregisterURL, clientID, authPrefix, accessToke for k, v := range p.queryParameters { queryParams[k] = v } - queryParams["client_id"] = clientID + queryParams[metaClientID] = clientID } else { queryParams = p.queryParameters } @@ -666,7 +739,7 @@ func (p *provider) tryUnregister(unregisterURL, clientID, authPrefix, accessToke return err } - if response.Code == http.StatusNoContent { + if response.Code >= 200 && response.Code < 400 { return nil } diff --git a/pkg/authz/oauth/provider_test.go b/pkg/authz/oauth/provider_test.go index 3bf5e0a57..729a7a9f2 100644 --- a/pkg/authz/oauth/provider_test.go +++ b/pkg/authz/oauth/provider_test.go @@ -21,6 +21,13 @@ const ( testAuthServerMetadataURL = "/oauth2/authorizationID/.well-known/oauth-authorization-server" testMetadataURL = "/metadata" testRegisterURL = "/register/cid-1" + + scopeOpenID = "openid" + scopeProfile = "profile" + scopeEmail = "email" + scopeWriteAPI = "write:api" + + excludeOpenIDProfile = "openid,profile" ) type providerTestCase struct { @@ -42,7 +49,6 @@ type providerTestCase struct { } func TestProvider(t *testing.T) { - cases := map[string]providerTestCase{ "IDP metadata bad request": { idpType: "generic", @@ -59,7 +65,7 @@ func TestProvider(t *testing.T) { expectRegistrationErr: true, }, "unregistration bad request": { - idpType: "okta", + idpType: "generic", clientRequest: &clientMetadata{ ClientName: "test", RedirectURIs: []string{testLocalHost}, @@ -74,9 +80,8 @@ func TestProvider(t *testing.T) { TokenEndpointAuthMethod: config.ClientSecretBasic, ResponseTypes: []string{AuthResponseCode}, Scope: []string{"read", "write"}, - extraProperties: map[string]interface{}{ - "key": "value", - oktaApplicationType: oktaAppTypeWeb, + extraProperties: map[string]any{ + "key": "value", }, }, metadataResponseCode: http.StatusOK, @@ -100,7 +105,7 @@ func TestProvider(t *testing.T) { TokenEndpointAuthMethod: config.ClientSecretBasic, ResponseTypes: []string{AuthResponseToken}, Scope: []string{"read", "write"}, - extraProperties: map[string]interface{}{ + extraProperties: map[string]any{ "key": "value", }, }, @@ -128,7 +133,7 @@ func TestProvider(t *testing.T) { TokenEndpointAuthMethod: config.ClientSecretBasic, ResponseTypes: []string{}, Scope: []string{"read", "write"}, - extraProperties: map[string]interface{}{ + extraProperties: map[string]any{ "key": "value", }, }, @@ -152,7 +157,7 @@ func TestProvider(t *testing.T) { TokenEndpointAuthMethod: config.ClientSecretBasic, ResponseTypes: []string{}, Scope: []string{"read", "write"}, - extraProperties: map[string]interface{}{ + extraProperties: map[string]any{ "key": "value", }, }, @@ -163,7 +168,6 @@ func TestProvider(t *testing.T) { }, } for name, tc := range cases { - tc := tc t.Run(name, func(t *testing.T) { runProviderTestCase(t, tc) }) @@ -247,7 +251,7 @@ func runProviderTestCase(t *testing.T, tc providerTestCase) { assert.Equal(t, s.GetUnregisterEndpoint(), cr.GetRegistrationClientURI()) } s.SetRegistrationResponseCode(tc.unRegistrationResponseCode) - err = p.UnregisterClient(cr.GetClientID(), cr.GetRegistrationAccessToken(), s.GetUnregisterEndpoint()) + err = p.UnregisterClient(cr.GetClientID(), cr.GetRegistrationAccessToken(), s.GetUnregisterEndpoint(), cr.GetScopes(), "") if tc.expectUnRegistrationErr { assert.NotNil(t, err) return @@ -267,13 +271,13 @@ func TestNewProviderValidatesExtraProperties(t *testing.T) { tests := map[string]struct { idpType string - extraProperties map[string]interface{} + extraProperties map[string]any expectError bool errorContains string }{ "Valid Okta provider with PKCE and browser type": { idpType: TypeOkta, - extraProperties: map[string]interface{}{ + extraProperties: map[string]any{ oktaPKCERequired: true, oktaApplicationType: oktaAppTypeBrowser, }, @@ -281,7 +285,7 @@ func TestNewProviderValidatesExtraProperties(t *testing.T) { }, "Invalid Okta provider with PKCE and service type": { idpType: TypeOkta, - extraProperties: map[string]interface{}{ + extraProperties: map[string]any{ oktaPKCERequired: true, oktaApplicationType: oktaAppTypeService, }, @@ -290,7 +294,7 @@ func TestNewProviderValidatesExtraProperties(t *testing.T) { }, "Valid generic provider": { idpType: "generic", - extraProperties: map[string]interface{}{}, + extraProperties: map[string]any{}, expectError: false, }, } @@ -324,90 +328,6 @@ func TestNewProviderValidatesExtraProperties(t *testing.T) { } } -func TestNewProviderOktaValidatesConfiguredGroupAndPolicyExist(t *testing.T) { - token := testToken - var srv *httptest.Server - srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch { - case r.Method == http.MethodGet && r.URL.Path == testAuthServerMetadataURL: - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`{"issuer":"` + srv.URL + `","token_endpoint":"` + srv.URL + `/token","registration_endpoint":"` + srv.URL + `/register","authorization_endpoint":"` + srv.URL + `/auth"}`)) - return - case r.Method == http.MethodGet && r.URL.Path == "/api/v1/groups": - if r.URL.Query().Get("q") == "Marketplace" { - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`[{"id":"00g-123","profile":{"name":"Marketplace"}}]`)) - return - } - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`[]`)) - return - case r.Method == http.MethodGet && r.URL.Path == "/api/v1/authorizationServers/authorizationID/policies": - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`[{"id":"pol-123","name":"shane"}]`)) - return - case r.Method == http.MethodGet && r.URL.Path == "/api/v1/authorizationServers/authorizationID/policies/pol-123": - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`{"id":"pol-123","name":"shane","conditions":{"clients":{"include":[]}}}`)) - return - default: - w.WriteHeader(http.StatusNotFound) - return - } - })) - defer srv.Close() - - idpCfg := &config.IDPConfiguration{ - Name: "test", - Type: TypeOkta, - MetadataURL: srv.URL + testAuthServerMetadataURL, - Okta: &config.OktaIDPConfiguration{Group: "Marketplace", Policy: "shane"}, - AuthConfig: &config.IDPAuthConfiguration{ - Type: config.AccessToken, - AccessToken: token, - }, - } - - p, err := NewProvider(idpCfg, config.NewTLSConfig(), "", 10*time.Second) - assert.NoError(t, err) - assert.NotNil(t, p) -} - -func TestNewProviderOktaFailsFastWhenConfiguredGroupMissing(t *testing.T) { - token := testToken - var srv *httptest.Server - srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method == http.MethodGet && r.URL.Path == testAuthServerMetadataURL { - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`{"issuer":"` + srv.URL + `","token_endpoint":"` + srv.URL + `/token","registration_endpoint":"` + srv.URL + `/register","authorization_endpoint":"` + srv.URL + `/auth"}`)) - return - } - if r.Method == http.MethodGet && r.URL.Path == "/api/v1/groups" { - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`[]`)) - return - } - w.WriteHeader(http.StatusNotFound) - })) - defer srv.Close() - - idpCfg := &config.IDPConfiguration{ - Name: "test", - Type: TypeOkta, - MetadataURL: srv.URL + testAuthServerMetadataURL, - Okta: &config.OktaIDPConfiguration{Group: "Marketplace"}, - AuthConfig: &config.IDPAuthConfiguration{ - Type: config.AccessToken, - AccessToken: token, - }, - } - - p, err := NewProvider(idpCfg, config.NewTLSConfig(), "", 10*time.Second) - assert.Error(t, err) - assert.Nil(t, p) - assert.Contains(t, err.Error(), "configured okta group") -} - type failingHookIDP struct { authPrefix string regErr error @@ -425,7 +345,7 @@ func (f *failingHookIDP) preProcessClientRequest(clientRequest *clientMetadata) // No preprocessing needed for this mock IDP implementation } -func (f *failingHookIDP) validateExtraProperties(extraProps map[string]interface{}) error { +func (f *failingHookIDP) validateExtraProperties(extraProps map[string]any) error { return nil } @@ -433,7 +353,7 @@ func (f *failingHookIDP) postProcessClientRegistration(clientRes ClientMetadata, return f.regErr } -func (f *failingHookIDP) postProcessClientUnregister(clientID string, idp config.IDPConfig, apiClient coreapi.Client) error { +func (f *failingHookIDP) postProcessClientUnregister(clientID string, idp config.IDPConfig, apiClient coreapi.Client, scopes []string, grantType string) error { return f.unregErr } @@ -455,7 +375,6 @@ func TestRegisterClientRollBack(t *testing.T) { } for name, tc := range tests { - tc := tc t.Run(name, func(t *testing.T) { var deleteCalls atomic.Int32 var registerCalls atomic.Int32 @@ -546,11 +465,11 @@ func TestUnregisterClientDeleteHookFails(t *testing.T) { idpCfg.Type = TypeOkta p.idpType = &failingHookIDP{unregErr: errors.New("cleanup failed")} - err = p.UnregisterClient("cid-1", testToken, srv.URL+testRegisterURL) + err = p.UnregisterClient("cid-1", testToken, srv.URL+testRegisterURL, nil, "") assert.Error(t, err) assert.Contains(t, err.Error(), "failed to complete provider cleanup after client unregistration") - assert.Equal(t, int32(1), deleteCalls.Load()) + assert.Equal(t, int32(0), deleteCalls.Load()) } func TestUnregisterClientCleanupAndDeleteFail(t *testing.T) { @@ -589,10 +508,233 @@ func TestUnregisterClientCleanupAndDeleteFail(t *testing.T) { idpCfg.Type = TypeOkta p.idpType = &failingHookIDP{unregErr: errors.New("cleanup failed")} - err = p.UnregisterClient("cid-1", testToken, srv.URL+testRegisterURL) + err = p.UnregisterClient("cid-1", testToken, srv.URL+testRegisterURL, nil, "") assert.Error(t, err) - assert.Contains(t, err.Error(), "failed to fully remove the Okta client") - assert.Contains(t, err.Error(), "OAuth client deletion failed") - assert.Equal(t, int32(1), deleteCalls.Load()) + assert.Contains(t, err.Error(), "cleanup failed") + assert.Contains(t, err.Error(), "failed to complete provider cleanup after client unregistration. Manual cleanup in Okta may be required") + assert.Equal(t, int32(0), deleteCalls.Load()) +} + +func TestRegisterClientOktaScopesRequest(t *testing.T) { + cases := map[string]struct { + regResponseBody string + requestScopes Scopes + wantPolicyCalls int32 + }{ + "response omits scopes — policy creation runs from request scopes": { + regResponseBody: `{"client_id":"` + testClientID + `","client_secret":"sec-1"}`, + requestScopes: Scopes{testScope}, + wantPolicyCalls: 1, + }, + "response includes scopes — policy creation runs normally": { + regResponseBody: `{"client_id":"` + testClientID + `","client_secret":"sec-1","scope":"` + testScope + `","grant_types":["client_credentials"]}`, + requestScopes: Scopes{testScope}, + wantPolicyCalls: 1, + }, + "no scopes in request — policy creation skipped": { + regResponseBody: `{"client_id":"` + testClientID + `","client_secret":"sec-1"}`, + requestScopes: nil, + wantPolicyCalls: 0, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + var policyCalls atomic.Int32 + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.Contains(r.URL.Path, ".well-known"): + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"issuer":"` + srv.URL + `","token_endpoint":"` + srv.URL + `/token","registration_endpoint":"` + srv.URL + `/register","authorization_endpoint":"` + srv.URL + `/auth"}`)) + case r.Method == http.MethodPost && r.URL.Path == "/register": + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(tc.regResponseBody)) + case r.Method == http.MethodGet && r.URL.Path == oktaPoliciesEndpointByID: + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`[]`)) + case r.Method == http.MethodPost && r.URL.Path == oktaPoliciesEndpointByID: + policyCalls.Add(1) + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":"` + oktaPolicyID + `"}`)) + case r.Method == http.MethodPost && r.URL.Path == oktaPolicyRulesEndpoint: + w.WriteHeader(http.StatusCreated) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer srv.Close() + + idpCfg := &config.IDPConfiguration{ + Name: "test", + Type: TypeOkta, + MetadataURL: srv.URL + testAuthServerMetadataURL, + AuthConfig: &config.IDPAuthConfiguration{ + Type: config.AccessToken, + AccessToken: testToken, + }, + } + p, err := NewProvider(idpCfg, config.NewTLSConfig(), "", 10*time.Second) + assert.NoError(t, err) + + clientReq := &clientMetadata{ + ClientName: "test-app", + GrantTypes: []string{GrantTypeClientCredentials}, + Scope: tc.requestScopes, + } + cr, err := p.RegisterClient(clientReq) + assert.NoError(t, err) + assert.NotNil(t, cr) + assert.Equal(t, tc.wantPolicyCalls, policyCalls.Load()) + }) + } +} + +func TestNewProviderValidatesOktaGroup(t *testing.T) { + cases := map[string]struct { + groupsCode int + groupsBody string + wantErr bool + errContains string + }{ + "group found — NewProvider succeeds": { + groupsCode: http.StatusOK, + groupsBody: `[{"id":"grp-1","profile":{"name":"` + testGroupName + `"}}]`, + }, + "group not found — NewProvider fails": { + groupsCode: http.StatusOK, + groupsBody: `[]`, + wantErr: true, + errContains: "configured Okta group", + }, + "groups API error — NewProvider fails": { + groupsCode: http.StatusForbidden, + groupsBody: `forbidden`, + wantErr: true, + errContains: "Okta group", + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + var ts *httptest.Server + ts = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.Contains(r.URL.Path, ".well-known"): + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"issuer":"` + ts.URL + `","token_endpoint":"` + ts.URL + `/token","registration_endpoint":"` + ts.URL + `/register","authorization_endpoint":"` + ts.URL + `/auth"}`)) + case r.URL.Path == oktaGroupsEndpoint: + w.WriteHeader(tc.groupsCode) + _, _ = w.Write([]byte(tc.groupsBody)) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer ts.Close() + + idpCfg := &config.IDPConfiguration{ + Name: "test", + Type: TypeOkta, + MetadataURL: ts.URL + testAuthServerMetadataURL, + Okta: &config.OktaIDPConfiguration{Group: testGroupName}, + AuthConfig: &config.IDPAuthConfiguration{ + Type: config.AccessToken, + AccessToken: testToken, + }, + } + provider, err := NewProvider(idpCfg, config.NewTLSConfig(), "", 10*time.Second) + if tc.wantErr { + assert.Error(t, err) + assert.Nil(t, provider) + if tc.errContains != "" { + assert.Contains(t, err.Error(), tc.errContains) + } + return + } + assert.NoError(t, err) + assert.NotNil(t, provider) + }) + } +} + +func TestFilterScopeExclude(t *testing.T) { + cases := map[string]struct { + scopes []string + exclude string + want []string + }{ + "removes excluded scopes": { + scopes: []string{scopeOpenID, scopeProfile, testScope, scopeWriteAPI}, + exclude: excludeOpenIDProfile, + want: []string{testScope, scopeWriteAPI}, + }, + "empty exclude returns all scopes": { + scopes: []string{scopeOpenID, testScope}, + exclude: "", + want: []string{scopeOpenID, testScope}, + }, + "exclude with whitespace is trimmed": { + scopes: []string{scopeOpenID, testScope}, + exclude: " openid , profile ", + want: []string{testScope}, + }, + "no scopes match exclude returns all": { + scopes: []string{testScope, scopeWriteAPI}, + exclude: excludeOpenIDProfile, + want: []string{testScope, scopeWriteAPI}, + }, + "all scopes excludeed returns empty": { + scopes: []string{scopeOpenID, scopeProfile}, + exclude: excludeOpenIDProfile, + want: []string{}, + }, + "nil scopes returns nil": { + scopes: nil, + exclude: scopeOpenID, + want: nil, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := filterScopeExclude(tc.scopes, tc.exclude) + assert.Equal(t, tc.want, got) + }) + } +} + +func TestGetSupportedScopesAppliesExclude(t *testing.T) { + cases := map[string]struct { + cfg config.IDPConfig + rawScopes []string + wantScopes []string + }{ + "Okta config with exclude filters scopes": { + cfg: &config.IDPConfiguration{ + Type: TypeOkta, + Okta: &config.OktaIDPConfiguration{ScopeExclude: "openid,profile"}, + }, + rawScopes: []string{"openid", "profile", "read:api"}, + wantScopes: []string{"read:api"}, + }, + "Okta config with default exclude filters defaults": { + cfg: &config.IDPConfiguration{Type: TypeOkta, Okta: &config.OktaIDPConfiguration{}}, + rawScopes: []string{"openid", "profile", "email", "read:api"}, + wantScopes: []string{"read:api"}, + }, + "non-Okta config returns scopes unfiltered": { + cfg: &config.IDPConfiguration{Type: "generic"}, + rawScopes: []string{"openid", "profile", "read:api"}, + wantScopes: []string{"openid", "profile", "read:api"}, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + p := &provider{ + cfg: tc.cfg, + authServerMetadata: &AuthorizationServerMetadata{ + ScopesSupported: tc.rawScopes, + }, + } + assert.Equal(t, tc.wantScopes, p.GetSupportedScopes()) + }) + } } diff --git a/pkg/config/externalidpconfig.go b/pkg/config/externalidpconfig.go index 0f6b0fb04..4bcbd19ef 100644 --- a/pkg/config/externalidpconfig.go +++ b/pkg/config/externalidpconfig.go @@ -19,6 +19,14 @@ const ( TLSClientAuth = "tls_client_auth" SelfSignedTLSClientAuth = "self_signed_tls_client_auth" + OktaPlaceholderMPApplicationName = "%MP_APPLICATION_NAME%" + OktaPlaceholderOwningTeam = "%OWNING_TEAM%" + OktaPlaceholderCredentialName = "%CREDENTIAL_NAME%" + OktaPlaceholderScope = "%SCOPE%" + OktaPlaceholderOAuthFlow = "%OAUTH_FLOW%" + OktaAppNameTemplateKey = "AGENTFEATURES_IDP_OKTA_APPNAME_TEMPLATE" + OktaPolicyNameTemplateKey = "AGENTFEATURES_IDP_OKTA_POLICYNAME_TEMPLATE" + propInsecureSkipVerify = "insecureSkipVerify" propUseCachedToken = "useCachedToken" propUseRegistrationToken = "useRegistrationToken" @@ -28,7 +36,10 @@ const ( fldType = "type" fldMetadataURL = "metadataUrl" fldOktaGroup = "okta.group" - fldOktaPolicy = "okta.policy" + fldOktaAppNameTemplate = "okta.appname_template" + fldOktaPolicyNameTemplate = "okta.policyname_template" + fldOktaScopeSources = "okta.scope_sources" + fldOktaScopeExclude = "okta.scope_exclude" fldExtraProperties = "extraProperties" fldRequestHeaders = "requestHeaders" fldQueryParams = "queryParams" @@ -53,6 +64,11 @@ const ( fldSSLRootCACertPath = "ssl.rootCACertPath" fldSSLClientCertPath = "ssl.clientCertPath" fldSSLClientKeyPath = "ssl.clientKeyPath" + + defaultOktaAppNameTemplate = OktaPlaceholderMPApplicationName + "-" + OktaPlaceholderOwningTeam + "-" + OktaPlaceholderCredentialName + defaultOktaPolicyNameTemplate = OktaPlaceholderScope + "-" + OktaPlaceholderOAuthFlow + defaultOktaScopeSources = "swagger,gateway,okta" + defaultOktaScopeExclude = "openid,profile,email,address,phone,offline_access" ) var configProperties = []string{ @@ -61,7 +77,10 @@ var configProperties = []string{ fldType, fldMetadataURL, fldOktaGroup, - fldOktaPolicy, + fldOktaAppNameTemplate, + fldOktaPolicyNameTemplate, + fldOktaScopeSources, + fldOktaScopeExclude, fldExtraProperties, fldRequestHeaders, fldQueryParams, @@ -236,10 +255,6 @@ type IDPConfig interface { GetIDPTitle() string // GetAuthConfig - to be used for authentication with IDP GetAuthConfig() IDPAuthConfig - // GetOktaGroup - okta-specific group assignment configuration. - GetOktaGroup() string - // GetOktaPolicy - okta-specific authorization server policy name to look up. - GetOktaPolicy() string // GetClientScopes - default list of scopes that are included in the client metadata request to IDP GetClientScopes() string // GetGrantType - default grant type to be used when creating the client. (default : "client_credentials") @@ -254,6 +269,8 @@ type IDPConfig interface { GetRequestHeaders() map[string]string // GetQueryParams - set of additional query parameters to be applied when registering the client GetQueryParams() map[string]string + // GetOktaGroup - name of the Okta group that newly registered apps are added to + GetOktaGroup() string // GetTLSConfig - tls config for IDP connection GetTLSConfig() TLSConfig // validate - Validates the IDP configuration @@ -279,8 +296,11 @@ type IDPAuthConfiguration struct { // OktaIDPConfiguration - okta-specific configuration. type OktaIDPConfiguration struct { - Group string `json:"group,omitempty"` - Policy string `json:"policy,omitempty"` + Group string `json:"group,omitempty"` + AppNameTemplate string `json:"appname_template,omitempty"` + PolicyNameTemplate string `json:"policyname_template,omitempty"` + ScopeSources string `json:"scope_sources,omitempty"` + ScopeExclude string `json:"scope_exclude,omitempty"` } // IDPConfiguration - Structure to hold the IdP provider config @@ -306,7 +326,7 @@ func (i *IDPConfiguration) GetIDPName() string { return i.Name } -// GetIDPName - for the identity provider frinedly name +// GetIDPTitle - for the identity provider friendly name func (i *IDPConfiguration) GetIDPTitle() string { return i.Title } @@ -326,20 +346,44 @@ func (i *IDPConfiguration) GetMetadataURL() string { return i.MetadataURL } -// GetOktaGroup - returns Okta group name configured for this IDP (if any). -func (i *IDPConfiguration) GetOktaGroup() string { - if i.Okta == nil { - return "" +// GetAppNameTemplate - Okta application name template; defaults to MP_APPLICATION_NAME-OWNING_TEAM-CREDENTIAL_NAME pattern. +func (i *IDPConfiguration) GetAppNameTemplate() string { + if i.Okta == nil || i.Okta.AppNameTemplate == "" { + return defaultOktaAppNameTemplate } - return i.Okta.Group + return i.Okta.AppNameTemplate } -// GetOktaPolicy - returns Okta authorization server policy name for this IDP (if any). -func (i *IDPConfiguration) GetOktaPolicy() string { +// GetPolicyNameTemplate - Okta access policy name template; defaults to SCOPE-OAUTH_FLOW pattern. +func (i *IDPConfiguration) GetPolicyNameTemplate() string { + if i.Okta == nil || i.Okta.PolicyNameTemplate == "" { + return defaultOktaPolicyNameTemplate + } + return i.Okta.PolicyNameTemplate +} + +// GetScopeSources - comma-separated active scope sources (swagger, gateway, okta); consumed by the v7 agent. +func (i *IDPConfiguration) GetScopeSources() string { + if i.Okta == nil || i.Okta.ScopeSources == "" { + return defaultOktaScopeSources + } + return i.Okta.ScopeSources +} + +// GetScopeExclude - comma-separated scopes excluded from the Marketplace UI; consumed by the v7 agent. +func (i *IDPConfiguration) GetScopeExclude() string { + if i.Okta == nil || i.Okta.ScopeExclude == "" { + return defaultOktaScopeExclude + } + return i.Okta.ScopeExclude +} + +// GetOktaGroup - name of the Okta group that newly registered apps are added to. +func (i *IDPConfiguration) GetOktaGroup() string { if i.Okta == nil { return "" } - return i.Okta.Policy + return i.Okta.Group } // GetExtraProperties - set of additional properties to be applied when registering the client @@ -654,7 +698,7 @@ func ParseExternalIDPConfig(agentFeature AgentFeaturesConfig, props properties.P QueryParams: make(IDPQueryParams), ClientScopes: "resource.READ resource.WRITE", GrantType: "client_credentials", - AuthMethod: "client_secret_basic", + AuthMethod: ClientSecretBasic, AuthResponseType: "token", } diff --git a/pkg/config/externalidpconfig_test.go b/pkg/config/externalidpconfig_test.go index bd09d3514..5edd7b3fe 100644 --- a/pkg/config/externalidpconfig_test.go +++ b/pkg/config/externalidpconfig_test.go @@ -3,6 +3,7 @@ package config import ( "encoding/json" "os" + "reflect" "testing" "github.com/Axway/agent-sdk/pkg/cmd/properties" @@ -10,6 +11,29 @@ import ( "github.com/stretchr/testify/assert" ) +const ( + testIDPName = "test" + testMetaURL = "test" + testAccessTok = "accessToken" + testIDPTypeOkta = "okta" + + envKeyIDPName = "AGENTFEATURES_IDP_NAME_1" + envKeyIDPType = "AGENTFEATURES_IDP_TYPE_1" + envKeyIDPMetadataURL = "AGENTFEATURES_IDP_METADATAURL_1" + envKeyIDPRequestHeaders = "AGENTFEATURES_IDP_REQUESTHEADERS_1" + envKeyIDPQueryParams = "AGENTFEATURES_IDP_QUERYPARAMS_1" + envKeyIDPAuthType = "AGENTFEATURES_IDP_AUTH_TYPE_1" + envKeyIDPAuthAccessToken = "AGENTFEATURES_IDP_AUTH_ACCESSTOKEN_1" + envKeyIDPAuthClientID = "AGENTFEATURES_IDP_AUTH_CLIENTID_1" + envKeyIDPAuthClientSecret = "AGENTFEATURES_IDP_AUTH_CLIENTSECRET_1" + envKeyIDPAuthRequestHeaders = "AGENTFEATURES_IDP_AUTH_REQUESTHEADERS_1" + envKeyIDPAuthQueryParams = "AGENTFEATURES_IDP_AUTH_QUERYPARAMS_1" + envKeyIDPOktaAppNameTemplate = "AGENTFEATURES_IDP_OKTA_APPNAME_TEMPLATE_1" + envKeyIDPOktaPolicyNameTemplate = "AGENTFEATURES_IDP_OKTA_POLICYNAME_TEMPLATE_1" + envKeyIDPOktaScopeSources = "AGENTFEATURES_IDP_OKTA_SCOPE_SOURCES_1" + envKeyIDPOktaScopeExclude = "AGENTFEATURES_IDP_OKTA_SCOPE_EXCLUDE_1" +) + func setEnvVars(t *testing.T, env map[string]string) { t.Helper() for key, val := range env { @@ -32,7 +56,7 @@ func parseExternalIDP(t *testing.T) ExternalIDPConfig { return agentFeatures.ExternalIDPConfig } -func assertIDPRoundTrip(t *testing.T, idp IDPConfig, expectedOktaGroup string, expectedOktaPolicy string) { +func assertIDPRoundTrip(t *testing.T, idp IDPConfig) { t.Helper() buf, err := json.Marshal(idp) assert.NoError(t, err) @@ -52,23 +76,11 @@ func assertIDPRoundTrip(t *testing.T, idp IDPConfig, expectedOktaGroup string, e assert.Equal(t, idp.GetAuthConfig().GetClientSecret(), parsedIDP.GetAuthConfig().GetClientSecret()) assert.Equal(t, len(idp.GetAuthConfig().GetRequestHeaders()), len(parsedIDP.GetAuthConfig().GetRequestHeaders())) assert.Equal(t, len(idp.GetAuthConfig().GetQueryParams()), len(parsedIDP.GetAuthConfig().GetQueryParams())) - - if expectedOktaGroup != "" { - assert.Equal(t, expectedOktaGroup, idp.GetOktaGroup()) - assert.Equal(t, expectedOktaGroup, parsedIDP.GetOktaGroup()) - } - if expectedOktaPolicy != "" { - assert.Equal(t, expectedOktaPolicy, idp.GetOktaPolicy()) - assert.Equal(t, expectedOktaPolicy, parsedIDP.GetOktaPolicy()) - } } type externalIDPTestCase struct { - name string - envNames map[string]string - oktaGroup string - oktaPolicy string - hasError bool + envNames map[string]string + hasError bool } func runExternalIDPTestCase(t *testing.T, tc externalIDPTestCase) { @@ -83,132 +95,173 @@ func runExternalIDPTestCase(t *testing.T, tc externalIDPTestCase) { } assert.NoError(t, err) for _, idp := range idpCfgs.GetIDPList() { - assertIDPRoundTrip(t, idp, tc.oktaGroup, tc.oktaPolicy) + assertIDPRoundTrip(t, idp) + } +} + +func mergeEnv(base, extra map[string]string) map[string]string { + out := make(map[string]string, len(base)+len(extra)) + for k, v := range base { + out[k] = v } + for k, v := range extra { + out[k] = v + } + return out } func TestExternalIDPConfig(t *testing.T) { - testCases := []externalIDPTestCase{ - { - name: "no external IDP config", - envNames: map[string]string{}, - hasError: false, - }, - { - name: "no name in IDP config", - envNames: map[string]string{ - "AGENTFEATURES_IDP_METADATAURL_1": "test", - }, + baseOkta := map[string]string{ + envKeyIDPName: testIDPName, + envKeyIDPType: testIDPTypeOkta, + envKeyIDPMetadataURL: testMetaURL, + envKeyIDPAuthType: AccessToken, + envKeyIDPAuthAccessToken: testAccessTok, + } + + cases := map[string]externalIDPTestCase{ + "no external IDP config": {envNames: map[string]string{}}, + "no name in IDP config": { + envNames: map[string]string{envKeyIDPMetadataURL: testMetaURL}, hasError: true, }, - { - name: "no metadata URL in IDP config", - envNames: map[string]string{ - "AGENTFEATURES_IDP_NAME_1": "test", - }, + "no metadata URL in IDP config": { + envNames: map[string]string{envKeyIDPName: testIDPName}, hasError: true, }, - { - name: "no auth config in IDP config", + "no auth config in IDP config": { envNames: map[string]string{ - "AGENTFEATURES_IDP_NAME_1": "test", - "AGENTFEATURES_IDP_METADATAURL_1": "test", + envKeyIDPName: testIDPName, + envKeyIDPMetadataURL: testMetaURL, }, hasError: true, }, - { - name: "invalid IDP auth type config in IDP config", + "invalid IDP auth type config": { envNames: map[string]string{ - "AGENTFEATURES_IDP_NAME_1": "test", - "AGENTFEATURES_IDP_METADATAURL_1": "test", - "AGENTFEATURES_IDP_AUTH_TYPE_1": "invalid", + envKeyIDPName: testIDPName, + envKeyIDPMetadataURL: testMetaURL, + envKeyIDPAuthType: "invalid", }, hasError: true, }, - { - name: "accessToken auth config with no token in IDP config", + "accessToken auth config with no token": { envNames: map[string]string{ - "AGENTFEATURES_IDP_NAME_1": "test", - "AGENTFEATURES_IDP_METADATAURL_1": "test", - "AGENTFEATURES_IDP_AUTH_TYPE_1": "accessToken", + envKeyIDPName: testIDPName, + envKeyIDPMetadataURL: testMetaURL, + envKeyIDPAuthType: AccessToken, }, hasError: true, }, - { - name: "accessToken auth config with valid token in IDP config", + "accessToken auth config with valid token": { envNames: map[string]string{ - "AGENTFEATURES_IDP_NAME_1": "test", - "AGENTFEATURES_IDP_METADATAURL_1": "test", - "AGENTFEATURES_IDP_AUTH_TYPE_1": "accessToken", - "AGENTFEATURES_IDP_AUTH_ACCESSTOKEN_1": "accessToken", + envKeyIDPName: testIDPName, + envKeyIDPMetadataURL: testMetaURL, + envKeyIDPAuthType: AccessToken, + envKeyIDPAuthAccessToken: testAccessTok, }, - hasError: false, }, - { - name: "okta group config via env var", - envNames: map[string]string{ - "AGENTFEATURES_IDP_NAME_1": "test", - "AGENTFEATURES_IDP_TYPE_1": "okta", - "AGENTFEATURES_IDP_METADATAURL_1": "test", - "AGENTFEATURES_IDP_AUTH_TYPE_1": "accessToken", - "AGENTFEATURES_IDP_AUTH_ACCESSTOKEN_1": "accessToken", - "AGENTFEATURES_IDP_OKTA_GROUP_1": "MyAppUsers", - }, - oktaGroup: "MyAppUsers", - hasError: false, + "okta appname template config via env var": { + envNames: mergeEnv(baseOkta, map[string]string{envKeyIDPOktaAppNameTemplate: OktaPlaceholderMPApplicationName}), }, - { - name: "okta policy config via env var", - envNames: map[string]string{ - "AGENTFEATURES_IDP_NAME_1": "test", - "AGENTFEATURES_IDP_TYPE_1": "okta", - "AGENTFEATURES_IDP_METADATAURL_1": "test", - "AGENTFEATURES_IDP_AUTH_TYPE_1": "accessToken", - "AGENTFEATURES_IDP_AUTH_ACCESSTOKEN_1": "accessToken", - "AGENTFEATURES_IDP_OKTA_POLICY_1": "marketplacePolicy", - }, - oktaPolicy: "marketplacePolicy", - hasError: false, + "okta policy name template config via env var": { + envNames: mergeEnv(baseOkta, map[string]string{envKeyIDPOktaPolicyNameTemplate: OktaPlaceholderScope}), + }, + "okta scope sources config via env var": { + envNames: mergeEnv(baseOkta, map[string]string{envKeyIDPOktaScopeSources: "swagger,okta"}), + }, + "okta scope exclude config via env var": { + envNames: mergeEnv(baseOkta, map[string]string{envKeyIDPOktaScopeExclude: "openid,profile"}), }, - { - name: "client auth config with no clientid/secret in IDP config", + "client auth config with no clientid/secret": { envNames: map[string]string{ - "AGENTFEATURES_IDP_NAME_1": "test", - "AGENTFEATURES_IDP_METADATAURL_1": "test", - "AGENTFEATURES_IDP_AUTH_TYPE_1": "client", + envKeyIDPName: testIDPName, + envKeyIDPMetadataURL: testMetaURL, + envKeyIDPAuthType: Client, }, hasError: true, }, - { - name: "client auth config with no client secret in IDP config", + "client auth config with no client secret": { envNames: map[string]string{ - "AGENTFEATURES_IDP_NAME_1": "test", - "AGENTFEATURES_IDP_METADATAURL_1": "test", - "AGENTFEATURES_IDP_AUTH_TYPE_1": "client", - "AGENTFEATURES_IDP_AUTH_CLIENTID_1": "client-id", + envKeyIDPName: testIDPName, + envKeyIDPMetadataURL: testMetaURL, + envKeyIDPAuthType: Client, + envKeyIDPAuthClientID: "client-id", }, hasError: true, }, - { - name: "client auth config with valid client config in IDP config", + "client auth config with valid client config": { envNames: map[string]string{ - "AGENTFEATURES_IDP_NAME_1": "test", - "AGENTFEATURES_IDP_METADATAURL_1": "test", - "AGENTFEATURES_IDP_REQUESTHEADERS_1": "{\"hdr\":\"value\"}", - "AGENTFEATURES_IDP_QUERYPARAMS_1": "{\"param\":\"value\"}", - "AGENTFEATURES_IDP_AUTH_TYPE_1": "client", - "AGENTFEATURES_IDP_AUTH_CLIENTID_1": "client-id", - "AGENTFEATURES_IDP_AUTH_CLIENTSECRET_1": "client-secret", - "AGENTFEATURES_IDP_AUTH_REQUESTHEADERS_1": "{\"authhdr\":\"value\"}", - "AGENTFEATURES_IDP_AUTH_QUERYPARAMS_1": "{\"authparam\":\"value\"}", + envKeyIDPName: testIDPName, + envKeyIDPMetadataURL: testMetaURL, + envKeyIDPRequestHeaders: `{"hdr":"value"}`, + envKeyIDPQueryParams: `{"param":"value"}`, + envKeyIDPAuthType: Client, + envKeyIDPAuthClientID: "client-id", + envKeyIDPAuthClientSecret: "client-secret", + envKeyIDPAuthRequestHeaders: `{"authhdr":"value"}`, + envKeyIDPAuthQueryParams: `{"authparam":"value"}`, }, - hasError: false, }, } - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { + for name, tc := range cases { + t.Run(name, func(t *testing.T) { runExternalIDPTestCase(t, tc) }) } } + +func TestOktaIDPConfigGetters(t *testing.T) { + cases := map[string]struct { + cfg *IDPConfiguration + wantAppTemplate string + wantPolicyTemplate string + wantScopeSources string + wantScopeExclude string + wantGroup string + }{ + "nil okta config returns all defaults": { + cfg: &IDPConfiguration{}, + wantAppTemplate: defaultOktaAppNameTemplate, + wantPolicyTemplate: defaultOktaPolicyNameTemplate, + wantScopeSources: defaultOktaScopeSources, + wantScopeExclude: defaultOktaScopeExclude, + wantGroup: "", + }, + "configured values are returned": { + cfg: &IDPConfiguration{Okta: &OktaIDPConfiguration{ + Group: "Marketplace", + AppNameTemplate: "my-" + OktaPlaceholderCredentialName, + PolicyNameTemplate: OktaPlaceholderScope, + ScopeSources: "swagger", + ScopeExclude: "openid", + }}, + wantAppTemplate: "my-" + OktaPlaceholderCredentialName, + wantPolicyTemplate: OktaPlaceholderScope, + wantScopeSources: "swagger", + wantScopeExclude: "openid", + wantGroup: "Marketplace", + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + assert.Equal(t, tc.wantAppTemplate, tc.cfg.GetAppNameTemplate()) + assert.Equal(t, tc.wantPolicyTemplate, tc.cfg.GetPolicyNameTemplate()) + assert.Equal(t, tc.wantScopeSources, tc.cfg.GetScopeSources()) + assert.Equal(t, tc.wantScopeExclude, tc.cfg.GetScopeExclude()) + assert.Equal(t, tc.wantGroup, tc.cfg.GetOktaGroup()) + }) + } +} + +func TestRemovedSymbols(t *testing.T) { + typ := reflect.TypeFor[OktaIDPConfiguration]() + cases := map[string]struct{}{ + "Policy": {}, + } + for field := range cases { + t.Run(field, func(t *testing.T) { + _, found := typ.FieldByName(field) + assert.False(t, found, "field %q must not exist on OktaIDPConfiguration", field) + }) + } +}