Okta conditional access configs #34566
Conversation
getvictor
had a problem deploying
to
Docker Hub
GitHub Actions
Failure
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughPass global ConditionalAccess settings through validation and add Okta conditional-access fields to AppConfig/API; validation and ModifyAppConfig updated, tests and activity entries added for Okta lifecycle, and team-level validation calls updated to use the global settings. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
server/fleet/app.go (1)
607-609: Document Copy() handling for ConditionalAccess field.While the
ConditionalAccessfield contains only value types (making shallow copy safe), the explicit warning comments on lines 241-243 and 619-621 indicate that all new fields should be considered in theCopy()implementation. Consider adding a comment in theCopy()method documenting that this field doesn't require special handling due to its value-type composition.Add a comment in the
Copy()method around line 796:if c.YaraRules != nil { rules := make([]YaraRule, len(c.YaraRules)) copy(rules, c.YaraRules) clone.YaraRules = rules } + + // ConditionalAccess: nothing needs cloning (contains only value types) return &clone }server/fleet/integrations.go (1)
423-439: Avoid false positives; treat Okta “configured” only when all fields are non-emptyCurrent oktaConfigured checks only OktaIDPID and doesn’t trim whitespace. A payload with spaces can bypass the “no integration configured” guard; also a single field present doesn’t mean Okta is actually configured.
Apply one of the following:
Option A — stricter inline check:
- oktaConfigured := conditionalAccessSettings.OktaIDPID.Valid && - conditionalAccessSettings.OktaIDPID.Value != "" + oktaConfigured := + conditionalAccessSettings.OktaIDPID.Valid && + strings.TrimSpace(conditionalAccessSettings.OktaIDPID.Value) != "" && + conditionalAccessSettings.OktaAssertionConsumerServiceURL.Valid && + strings.TrimSpace(conditionalAccessSettings.OktaAssertionConsumerServiceURL.Value) != "" && + conditionalAccessSettings.OktaAudienceURI.Valid && + strings.TrimSpace(conditionalAccessSettings.OktaAudienceURI.Value) != "" && + conditionalAccessSettings.OktaCertificate.Valid && + strings.TrimSpace(conditionalAccessSettings.OktaCertificate.Value) != ""Option B — add ConditionalAccessSettings.OktaConfigured() helper and call it here for consistency.
server/service/appconfig.go (5)
136-145: Clear Entra fields when no integration is foundAvoid stale TenantID/Configured values if ConditionalAccessMicrosoftGet returns nil.
if conditionalAccessIntegration != nil { appConfig.ConditionalAccess.MicrosoftEntraTenantID = conditionalAccessIntegration.TenantID appConfig.ConditionalAccess.MicrosoftEntraConnectionConfigured = conditionalAccessIntegration.SetupDone +} else { + appConfig.ConditionalAccess.MicrosoftEntraTenantID = "" + appConfig.ConditionalAccess.MicrosoftEntraConnectionConfigured = false }
486-493: Order-of-ops note: avoid whitespace-only Okta values appearing “configured”ValidateConditionalAccessIntegration is called before you trim Okta fields. Either:
- make ValidateConditionalAccessIntegration trim (preferred, see suggestion in fleet/integrations.go), or
- move the Okta trimming block above this call.
Current behavior can briefly treat " " as configured.
495-506: License gate should trigger only when setting non-empty valuesRight now, empty strings with Valid=true trip the premium check, blocking clears on Free. Gate only when any Okta field is being set to a non-empty value.
- oktaFieldsBeingSet := (newAppConfig.ConditionalAccess.OktaIDPID.Set && newAppConfig.ConditionalAccess.OktaIDPID.Valid) || - (newAppConfig.ConditionalAccess.OktaAssertionConsumerServiceURL.Set && newAppConfig.ConditionalAccess.OktaAssertionConsumerServiceURL.Valid) || - (newAppConfig.ConditionalAccess.OktaAudienceURI.Set && newAppConfig.ConditionalAccess.OktaAudienceURI.Valid) || - (newAppConfig.ConditionalAccess.OktaCertificate.Set && newAppConfig.ConditionalAccess.OktaCertificate.Valid) + isNonEmpty := func(s optjson.String) bool { + return s.Set && s.Valid && strings.TrimSpace(s.Value) != "" + } + oktaFieldsBeingSet := isNonEmpty(newAppConfig.ConditionalAccess.OktaIDPID) || + isNonEmpty(newAppConfig.ConditionalAccess.OktaAssertionConsumerServiceURL) || + isNonEmpty(newAppConfig.ConditionalAccess.OktaAudienceURI) || + isNonEmpty(newAppConfig.ConditionalAccess.OktaCertificate)
545-561: Tighten ACS URL and certificate validation
- Require a host in ACS URL and use ParseRequestURI for stricter parsing.
- Validate PEM is a valid x509 CERTIFICATE. Support multiple PEM blocks.
- acsURL, err := url.Parse(appConfig.ConditionalAccess.OktaAssertionConsumerServiceURL.Value) - if err != nil || (acsURL.Scheme != "http" && acsURL.Scheme != "https") { + acsURL, err := url.ParseRequestURI(appConfig.ConditionalAccess.OktaAssertionConsumerServiceURL.Value) + if err != nil || ((acsURL.Scheme != "http" && acsURL.Scheme != "https") || acsURL.Host == "") { invalid.Append("conditional_access.okta_assertion_consumer_service_url", - "must be a valid URL with http or https scheme") + "must be a valid URL with http or https scheme and a host") } - block, _ := pem.Decode([]byte(appConfig.ConditionalAccess.OktaCertificate.Value)) - if block == nil { - invalid.Append("conditional_access.okta_certificate", - "must be valid PEM format") - } + // Validate one or more PEM-encoded CERTIFICATE blocks and parse each + rest := []byte(appConfig.ConditionalAccess.OktaCertificate.Value) + certCount := 0 + for { + block, r := pem.Decode(rest) + if block == nil { + break + } + rest = r + if block.Type != "CERTIFICATE" { + invalid.Append("conditional_access.okta_certificate", "PEM block must be a CERTIFICATE") + break + } + if _, err := x509.ParseCertificate(block.Bytes); err != nil { + invalid.Append("conditional_access.okta_certificate", "must be a valid x509 certificate") + break + } + certCount++ + } + if certCount == 0 { + invalid.Append("conditional_access.okta_certificate", "must contain at least one PEM-encoded certificate") + }
708-711: Add activities for Okta config add/remove (per PR objective)PR objective mentions activities for configuring and deleting Okta configs. Consider emitting activities when Okta becomes configured/unconfigured (transition) similar to Conditional Access enable/disable events.
I can draft a minimal patch that computes before/after OktaConfigured() and emits ActivityTypeConfiguredOktaConditionalAccess / ActivityTypeDeletedOktaConditionalAccess (or new types if needed). Want me to open a follow-up?
Also applies to: 969-994
server/service/integration_enterprise_test.go (5)
21346-21355: Build JSON payloads with json.Marshal instead of fmt.Sprintf.Safer and clearer, especially for multiline PEM and future field additions. Use a small helper to emit nulls when pointers are nil.
Example:
func oktaPayload(idp, acs, aud, cert *string) []byte { body := map[string]any{ "conditional_access": map[string]any{ "okta_idp_id": idp, "okta_assertion_consumer_service_url": acs, "okta_audience_uri": aud, "okta_certificate": cert, }, } b, _ := json.Marshal(body) return b }Usage:
- s.DoRaw("PATCH", "/api/latest/fleet/config", []byte(fmt.Sprintf(`{ ... "okta_certificate": %q ... }`, validCert)), http.StatusOK) + s.DoRaw("PATCH", "/api/latest/fleet/config", oktaPayload(&idp, &acs, &aud, &validCert), http.StatusOK) - s.DoRaw("PATCH", "/api/latest/fleet/config", []byte(`{ "conditional_access": { "okta_idp_id": "..." } }`), http.StatusUnprocessableEntity) + s.DoRaw("PATCH", "/api/latest/fleet/config", oktaPayload(&idp, nil, nil, nil), http.StatusUnprocessableEntity)Also applies to: 21330-21345, 21365-21374, 21375-21384
21324-21329: Strengthen the baseline assertions (or drop them if relying on explicit setup).If you keep the baseline check, assert all four Okta fields are empty to catch partial state pollution.
- require.False(t, acResp.ConditionalAccess.OktaIDPID.Valid) + require.False(t, acResp.ConditionalAccess.OktaIDPID.Valid) + require.False(t, acResp.ConditionalAccess.OktaAssertionConsumerServiceURL.Valid) + require.False(t, acResp.ConditionalAccess.OktaAudienceURI.Valid) + require.False(t, acResp.ConditionalAccess.OktaCertificate.Valid)
21365-21374: Add a case for invalid URL scheme (http) to harden validation coverage.Right now you cover a malformed URL; add an http-scheme ACS URL which should also be rejected.
badACS := "http://dev-579.okta.com/sso/saml2/0oaqu0748bP2ELUlJ5d7" s.DoRaw("PATCH", "/api/latest/fleet/config", oktaPayload(&idp, &badACS, &aud, &validCert), http.StatusUnprocessableEntity)
21330-21345: Consider checking error messages for partial updates.Asserting 422 is good; also assert the response message mentions that all 4 Okta fields must be provided together to avoid silent regressions in error text.
21300-21430: Optional: structure as subtests and factor constants.Use t.Run per scenario and extract the repeated URLs into constants to reduce duplication and ease future changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ee/server/service/teams.go(3 hunks)server/fleet/app.go(2 hunks)server/fleet/integrations.go(1 hunks)server/service/appconfig.go(4 hunks)server/service/integration_core_test.go(1 hunks)server/service/integration_enterprise_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/fleet/app.goserver/fleet/integrations.goee/server/service/teams.goserver/service/integration_enterprise_test.goserver/service/integration_core_test.goserver/service/appconfig.go
🔇 Additional comments (7)
ee/server/service/teams.go (1)
284-291: LGTM! Consistent parameter passing for conditional access validation.All three call sites to
ValidateConditionalAccessIntegrationhave been consistently updated to passappCfg.ConditionalAccess, enabling validation to consider global conditional access configuration (Microsoft Entra and Okta settings) during team creation and modification operations.Also applies to: 1162-1169, 1479-1486
server/fleet/app.go (1)
96-101: Validation rule is properly enforced.The validation requirement that all four Okta fields must be set together or all must be empty is correctly implemented in
server/service/appconfig.go(lines 521-543). The code counts valid, non-empty Okta fields and rejects partial configurations. Additional validation also ensures the ACS URL is a valid HTTP/HTTPS URL and the certificate is valid PEM format. Comprehensive integration tests inserver/service/integration_enterprise_test.goverify this behavior across multiple scenarios.server/service/appconfig.go (4)
16-16: Import looks good
200-206: Response wiring LGTMIncluding ConditionalAccess directly on AppConfig and passing through related fields looks correct.
Also applies to: 208-215
507-520: applyOptString helper LGTM
521-544: All-or-none Okta field requirement LGTMSimple and clear.
server/fleet/integrations.go (1)
406-415: Call sites verified: signature change fully appliedAll 4 invocations of
ValidateConditionalAccessIntegrationhave been correctly updated with the newConditionalAccessSettingsparameter:
- server/service/appconfig.go:488 — appConfig flow
- ee/server/service/teams.go:284, 1162, 1479 — team flows (3 sites)
Each call properly passes all five parameters in the correct order: context, service interface, conditional access settings, old enabled value, and new enabled value.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #34566 +/- ##
========================================
Coverage 64.20% 64.20%
========================================
Files 2063 2063
Lines 208401 208552 +151
Branches 6977 7077 +100
========================================
+ Hits 133796 133907 +111
- Misses 64105 64135 +30
- Partials 10500 10510 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# Conflicts: # server/service/integration_enterprise_test.go
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
server/service/integration_enterprise_test.go (3)
19110-19112: Nice: test isolation via t.Cleanup.This addresses prior state‑leak concerns. LGTM.
21489-21491: Good cleanup for Okta test.Per‑test cleanup avoids cross‑test interference. LGTM.
21508-21527: Do not embed a real third‑party certificate — generate a self‑signed PEM at runtime.Avoid shipping a real Okta PEM (includes org/email metadata). Generate a self‑signed test cert instead.
Apply:
- // Valid PEM certificate for testing (real Okta certificate) - validCert := `-----BEGIN CERTIFICATE----- - ... - -----END CERTIFICATE-----` + // Valid PEM certificate for testing (self‑signed, generated at runtime) + validCert := genTestPEMCert(t)Add once in this file (outside the test):
// test-only helper func genTestPEMCert(t *testing.T) string { t.Helper() priv, err := rsa.GenerateKey(rand.Reader, 2048) require.NoError(t, err) tmpl := x509.Certificate{ SerialNumber: big.NewInt(1), NotBefore: time.Now().Add(-time.Hour), NotAfter: time.Now().Add(365 * 24 * time.Hour), KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, Subject: pkix.Name{CommonName: "test.example"}, } der, err := x509.CreateCertificate(rand.Reader, &tmpl, &tmpl, &priv.PublicKey, priv) require.NoError(t, err) var buf bytes.Buffer require.NoError(t, pem.Encode(&buf, &pem.Block{Type: "CERTIFICATE", Bytes: der})) return buf.String() }Required imports at top:
import ( "bytes" "crypto/rand" "crypto/rsa" "crypto/x509" "crypto/x509/pkix" "encoding/pem" "math/big" "time" )
🧹 Nitpick comments (5)
server/service/integration_enterprise_test.go (2)
21529-21533: Use obvious placeholders for example Okta URLs.Prefer non‑realistic hostnames to avoid implying affiliation.
-idp := "https://www.okta.com/saml2/service-provider/spaubuaqdunfbsmoxyhl" -acs := "https://dev-579.okta.com/sso/saml2/0oaqu0748bP2ELUlJ5d7" -aud := "https://www.okta.com/saml2/service-provider/spaubuaqdunfbsmoxyhl" +idp := "https://example.okta.com/saml2/service-provider/spa-example" +acs := "https://example.okta.com/sso/saml2/0oabcdef123456" +aud := idp
21555-21563: Tighten assertions: check Valid=true for all Okta fields after set.Currently only OktaIDPID.Valid is asserted. Add the rest to catch partial state.
s.DoJSON("GET", "/api/latest/fleet/config", nil, http.StatusOK, &acResp) -require.True(t, acResp.ConditionalAccess.OktaIDPID.Valid) +require.True(t, acResp.ConditionalAccess.OktaIDPID.Valid) +require.True(t, acResp.ConditionalAccess.OktaAssertionConsumerServiceURL.Valid) +require.True(t, acResp.ConditionalAccess.OktaAudienceURI.Valid) +require.True(t, acResp.ConditionalAccess.OktaCertificate.Valid) assert.Equal(t, idp, acResp.ConditionalAccess.OktaIDPID.Value) assert.Equal(t, acs, acResp.ConditionalAccess.OktaAssertionConsumerServiceURL.Value) assert.Equal(t, aud, acResp.ConditionalAccess.OktaAudienceURI.Value) assert.Equal(t, validCert, acResp.ConditionalAccess.OktaCertificate.Value)server/fleet/app.go (1)
104-114: Tiny dedupe in OktaConfigured().Consider a small helper isSet := func(s optjson.String) bool { return s.Valid && s.Value != "" } to reduce repetition.
server/service/appconfig.go (2)
558-613: Okta input validation: solid; two small considerations.
- ACS URL: http/https + host validation is good.
- Certificate: robust multi-PEM parse with x509 check is good.
Consider:
- AudienceURI format: many SAML setups use URNs; if you plan to enforce syntax, explicitly allow urn:… and http(s), or keep length-only checks (current approach).
- maxCertLength 8KB may be tight if multiple certs are pasted; bump to 16KB to avoid surprising rejects.
Would you like me to adjust AudienceURI validation and increase the certificate size cap?
1057-1090: Activities for Okta changes: distinguish add vs edit.You emit AddedConditionalAccessOkta for both add and edit. Consider a dedicated EditedConditionalAccessOkta to keep the audit trail precise.
I can add a new activity type and wire tests; want a patch?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/fleetctl/fleetctl/generate_gitops_test.go(2 hunks)docs/Contributing/reference/audit-logs.md(1 hunks)ee/server/service/teams.go(3 hunks)server/fleet/activities.go(2 hunks)server/fleet/app.go(3 hunks)server/fleet/integrations.go(1 hunks)server/service/appconfig.go(5 hunks)server/service/integration_core_test.go(1 hunks)server/service/integration_enterprise_test.go(5 hunks)tools/cloner-check/generated_files/appconfig.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/fleet/activities.goee/server/service/teams.goserver/fleet/integrations.gocmd/fleetctl/fleetctl/generate_gitops_test.goserver/fleet/app.goserver/service/integration_enterprise_test.goserver/service/appconfig.goserver/service/integration_core_test.go
🧠 Learnings (1)
📚 Learning: 2025-10-21T16:04:18.019Z
Learnt from: getvictor
PR: fleetdm/fleet#34566
File: server/service/integration_core_test.go:7500-7511
Timestamp: 2025-10-21T16:04:18.019Z
Learning: Okta conditional access app config in Fleet is Premium-gated and supported both on-prem and in Fleet Cloud; the Cloud-only enforcement applies to the Microsoft compliance partner endpoints, not to the Okta settings.
Applied to files:
server/service/integration_core_test.go
🔇 Additional comments (27)
server/fleet/activities.go (3)
227-228: LGTM!The new activity types are properly registered in
ActivityDetailsListand positioned logically alongside related conditional access activities.
2792-2801: Verify the dual-purpose design of the "added" activity.The documentation states "Generated when Okta is configured or edited for conditional access," suggesting this single activity type handles both initial configuration and subsequent edits. This differs from other resources (e.g., scripts, profiles, software) which have separate "added" and "edited" activity types.
Please confirm this design is intentional for Okta conditional access. If edits should be tracked separately from initial configuration, consider adding an
ActivityTypeEditedConditionalAccessOktaactivity type.
2803-2812: LGTM!The deletion activity implementation is correct and follows the established pattern for conditional access integrations.
docs/Contributing/reference/audit-logs.md (1)
2070-2081: Documentation correctly reflects the implementation.The audit log entries accurately document the new Okta conditional access activities and are properly positioned among related entries. The "configured or edited" wording is consistent with the code implementation in
activities.go.server/service/integration_core_test.go (1)
7597-7607: Thanks for covering the license guard.This check keeps the Okta conditional-access settings aligned with the existing premium-only behavior, mirroring the other licensing safeguards in the suite.
ee/server/service/teams.go (3)
284-289: LGTM! Consistent parameter addition for conditional access validation.The addition of
appCfg.ConditionalAccessas a parameter toValidateConditionalAccessIntegrationcorrectly passes the global conditional access settings for validation. The error handling is already in place and appropriate.
1162-1169: LGTM! Consistent parameter addition for conditional access validation.The addition of
appCfg.ConditionalAccessas a parameter toValidateConditionalAccessIntegrationcorrectly passes the global conditional access settings for validation during team creation from spec.
1479-1486: LGTM! Consistent parameter addition for conditional access validation.The addition of
appCfg.ConditionalAccessas a parameter toValidateConditionalAccessIntegrationcorrectly passes the global conditional access settings for validation during team editing from spec.cmd/fleetctl/fleetctl/generate_gitops_test.go (2)
20-20: LGTM! Appropriate test dependency added.The
assertpackage import is correctly added to support the new test assertions.
762-810: LGTM! Comprehensive test coverage for Okta config exclusion.The test thoroughly verifies that Okta conditional access configurations are excluded from GitOps output by:
- Setting up Okta fields in the app config
- Checking the absence of the
conditional_accesskey in the YAML map- Verifying the YAML string doesn't contain any Okta-related field names
This provides good coverage for the intended behavior.
tools/cloner-check/generated_files/appconfig.txt (1)
187-193: LGTM! Generated file updated with new ConditionalAccessSettings structure.The generated declarations correctly reflect the addition of
AppConfig.ConditionalAccessand its nested fields for Microsoft Entra and Okta configuration. The structure is consistent with the existing patterns in this file.server/service/integration_enterprise_test.go (3)
19212-19215: LGTM: Entra defaults asserted clearly.
19249-19252: LGTM: Post‑delete Entra defaults re‑validated.
21634-21639: ****No separate "Edited" or "Updated" activity type exists for Okta conditional access. The
ActivityTypeAddedConditionalAccessOktatype is designed to handle both add and edit scenarios, as documented: "Generated when Okta is configured or edited for conditional access." The test correctly uses this type for the edit operation, consistent with production code behavior inserver/service/appconfig.go.Likely an incorrect or invalid review comment.
server/fleet/app.go (3)
88-102: Okta CA shape looks good; invariants are clear.Using optjson.String enables partial PATCH semantics; comment about “all-or-none” aligns with service-level validation. LGTM.
619-622: AppConfig: new ConditionalAccess field.Public surface change acknowledged. Ensure call sites deep-copy or treat as read-only (Copy() handles this below). LGTM.
If any external serializers rely on a previous response shape for ConditionalAccess, please confirm no breaking changes in clients.
816-821: Copy(): deep copy of ConditionalAccess is sufficient.Struct contains only value fields; shallow struct copy is fine. LGTM.
server/fleet/integrations.go (1)
405-441: Enable CA only if Entra or Okta is configured.Logic is correct; nil receiver on OktaConfigured() safely returns false. Clear error path. LGTM.
server/service/appconfig.go (9)
16-16: Import added.strings import is needed for trimming Okta fields. LGTM.
136-156: Populate Microsoft Entra fields into AppConfig.ConditionalAccess.Initialization + clear-on-delete is correct. One question: ConditionalAccess is included in AppConfig for all viewers; do we intend to expose Okta cert/URIs and Entra tenant status to non-admins?
If not intended, gate ConditionalAccess similar to SMTP/SSO for non-admins.
211-217: Response assembly includes ConditionalAccess.Consistent with new embedding approach. LGTM.
219-226: Response fields unchanged in behavior.No concerns.
497-504: Nil-guard for ConditionalAccess in Modify flow.Good defensive init for pointer fields.
505-518: Whitespace normalization + partial update helper.applyOptString is a neat pattern; trimming only when Valid is appropriate. LGTM.
519-533: License gate on Okta updates.Allows clears on Free (by setting fields to empty/null) but requires Premium when setting non-empty values. Confirm this policy is desired.
If clears should also be gated, extend oktaFieldsBeingSet to consider Set with Valid=false as a mutation.
534-557: All-or-none enforcement for Okta fields.Counting non-empty Valid fields after applying updates is correct for PATCH behavior. LGTM.
614-621: Validation call wired to include ConditionalAccess settings.Correctly passes Okta/Entra state into the guard. LGTM.
getvictor
requested review from
a team,
lukeheath and
rachaelshaw
as code owners
| if err := svc.NewActivity( | ||
| ctx, | ||
| authz.UserFromContext(ctx), | ||
| fleet.ActivityTypeAddedConditionalAccessOkta{}, |
There was a problem hiding this comment.
So there's no updated or changed activity type? Just added and deleted?
There was a problem hiding this comment.
Yes, we are using the same type for added or changed.
| "must be a valid URL with http or https scheme and a host") | ||
| } | ||
|
|
||
| // Validate one or more PEM-encoded CERTIFICATE blocks and parse each |
There was a problem hiding this comment.
We should probably add a test for a case where there are multiple certificate blocks
There was a problem hiding this comment.
Yes. Can I do that in a follow up PR? So product/frontend doesn't have to re-approve just for a test change?
I will also try to refactor this section into its own method for readability.
There was a problem hiding this comment.
Follow-up PR sounds good 👍
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #34533 This is the first sub-task out of several. Changes file will be added in a subsequent PR. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## New Fleet configuration settings - [x] Setting(s) is/are explicitly **excluded** from GitOps <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added Okta Conditional Access support (IDP, ACS URL, audience, certificate) and exposed conditional access in AppConfig/API * App activity logging for adding/removing Okta conditional access * **Bug Fixes** * Fixed typo in conditional access validation messaging * **Tests** * Added tests for Okta Conditional Access lifecycle, license gating, and GitOps export exclusion * **Documentation** * Added audit-log entries for Okta conditional access add/delete <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Related issue: Resolves #34533
This is the first sub-task out of several. Changes file will be added in a subsequent PR.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
New Fleet configuration settings
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation