OSAC-204: Add health and organization-scoped IDP APIs#672
Conversation
|
@CrystalChun: This pull request references OSAC-204 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds health-check trigger and health/test-result protobuf types to public and private APIs, implements slog.LogValuer logging (redacting message content and handling nil receivers) in private and public packages, and adds unit tests validating JSON log redaction. ChangesIdentity Provider Health Monitoring and Telemetry
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Security note: Risk severity — Low. Message contents are redacted in logs; review should verify redaction patterns cover all sensitive fields and that presence flags do not leak secrets. 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proto/private/osac/private/v1/identity_provider_type.proto`:
- Around line 242-255: The enum currently declared as HealthStatus (values
HEALTH_STATUS_*) should be renamed to IdentityProviderHealthStatus to match the
IdentityProvider* naming convention used by IdentityProviderPhase and
IdentityProviderEventType; update the enum identifier from HealthStatus to
IdentityProviderHealthStatus, rename its enum values from HEALTH_STATUS_* to
IDENTITY_PROVIDER_HEALTH_STATUS_* (or the consistent prefix you use), and then
update all references/usages of HealthStatus throughout the codebase (proto
imports, generated stubs, service/method signatures, tests) to the new
IdentityProviderHealthStatus symbol and its new value names, and regenerate
protobuf artifacts so all consumers compile against the renamed enum.
In `@proto/public/osac/public/v1/identity_provider_type.proto`:
- Around line 200-207: The public proto defines
IdentityProviderTestResult.message and IdentityProviderHealth.message as
optional while the private proto declares them as required, causing a schema
mismatch; update the private definitions (IdentityProviderTestResult and
IdentityProviderHealth) so their message fields match the public API by making
them optional (i.e., change `string message = 2` to an optional string) and
regenerate any stubs to ensure both APIs share the same field optionality and
serialization behavior.
In `@proto/public/osac/public/v1/identity_providers_service.proto`:
- Around line 176-198: The user_id filter on
ListOrganizationIdentityProviderEventsRequest enables user enumeration; update
the service implementation for the RPC handling
ListOrganizationIdentityProviderEvents (the server-side handler that consumes
ListOrganizationIdentityProviderEventsRequest) to enforce admin-only access and
to normalize responses so callers cannot infer user existence (e.g., always
return a consistent empty event list or a generic success response when no
events match, rather than an error or differing status for nonexistent users).
Ensure authorization checks map to admin roles before processing the user_id
filter and that the filtering logic treats unknown user_ids the same as valid
user_ids with zero matching events.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 04bf549c-d531-4060-87e8-e1833719452a
⛔ Files ignored due to path filters (12)
internal/api/osac/private/v1/identity_provider_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_provider_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_providers_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_providers_service.pb.gw.gois excluded by!**/*.pb.gw.gointernal/api/osac/private/v1/identity_providers_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_providers_service_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/identity_provider_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/identity_provider_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/identity_providers_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/identity_providers_service.pb.gw.gois excluded by!**/*.pb.gw.gointernal/api/osac/public/v1/identity_providers_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/identity_providers_service_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
proto/private/osac/private/v1/identity_provider_type.protoproto/private/osac/private/v1/identity_providers_service.protoproto/public/osac/public/v1/identity_provider_type.protoproto/public/osac/public/v1/identity_providers_service.proto
CrystalChun
force-pushed
the
idp
branch
2 times, most recently
from
cf2f417 to
2f6278a
Compare
| // Returns the IdP configuration from Keycloak for the specified organization. | ||
| rpc GetOrganizationIdentityProvider(GetOrganizationIdentityProviderRequest) returns (GetOrganizationIdentityProviderResponse) { | ||
| option (google.api.http) = { | ||
| get: "/api/fulfillment/v1/organizations/{organization_name}/identity_provider" |
There was a problem hiding this comment.
We shouldn't have nested collections in the REST API. Indentity providers are already associated to a tenant, like any object, via the metadata.tenant field. A regular tenant user will only see the identity providers of their organization. A cloud administrator can also filter by tenant, using something like filter=this.metadata.tenant = "some-tenant". So I think we don't need these additional methods for listing or getting identity providers.
| // Proxies the test request through Keycloak to validate: | ||
| // - LDAP: Bind credentials and user search | ||
| // - OIDC: Client credentials and token endpoint | ||
| rpc TestOrganizationIdentityProvider(TestOrganizationIdentityProviderRequest) returns (TestOrganizationIdentityProviderResponse) { |
There was a problem hiding this comment.
I think we should avoid this kind of imperative test operation. If we have them, then the fulfillment-grpc-server will need to connect to Keycloak, with the corresponding credentials. Currently we only do that from the fulfillment-controller. To preserve that separation I'd suggest to have in the spec of the identity provider a field that triggers the check:
// This field is used to trigger a heath check of the identity provider. The user
// can write anything here, the value isn't important, the only important thing
// is that when it changes the system will eventually initiate the check, and
// eventually report the result in the `status`.
string check_trigger = 6;The spec of the identity provider should also have a similar field, for example:
// This is used by the system to know when the `spec.status` has changed, and
// then start the check process. It can also be used by the user to see if their
// request to perform the health check has been acknowledged. The system
// will update it to the value of `spec.status` when the check is started.
string check_trigger = 3;The status should also contain other fields or conditions to report the result of the check.
For the user it will be very simple: the look at the spec.check_trigger field and set it to a different value. Then they wait for the changes in the status containing the results.
We can also use a more restrictive type, like an integer, and restrict it to be always incremented, but a random string is also OK.
|
|
||
| // Lists authentication and authorization events from the organization's identity provider. | ||
| // Retrieves events from Keycloak's event store for the specified organization. | ||
| rpc ListOrganizationIdentityProviderEvents(ListOrganizationIdentityProviderEventsRequest) returns (ListOrganizationIdentityProviderEventsResponse) { |
There was a problem hiding this comment.
Do we really need this? I'd say it is not that important, and we can either do it later or not do it at all.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: CrystalChun The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
CrystalChun
force-pushed
the
idp
branch
3 times, most recently
from
91a5598 to
f9cb470
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/osac/public/v1/identity_provider_logging.go`:
- Line 22: The file defines LogValue on *IdentityProviderEvent but the generated
protobuf types IdentityProviderEvent and IdentityProviderEventType (and the enum
constant IdentityProviderEventType_IDENTITY_PROVIDER_EVENT_TYPE_LOGIN_ERROR) do
not exist; fix by either (A) updating the proto definitions to declare
IdentityProviderEvent and IdentityProviderEventType so codegen will produce
those symbols, or (B) change the receiver and enum references in
identity_provider_logging.go to the correct existing generated types used for
IdP events (replace IdentityProviderEvent, IdentityProviderEventType and the
enum constant with the actual protobuf message/enum names), ensuring the
LogValue method signature (LogValue on the correct message type) and any
switch/case branches reference the valid generated symbols.
In `@proto/private/osac/private/v1/identity_provider_type.proto`:
- Around line 221-255: Identity provider private proto lacks fields to propagate
health-check trigger/state across surfaces; add a health-check trigger field to
IdentityProviderSpec and a health payload to IdentityProviderStatus so health
requests/results are not dropped. Specifically, update the IdentityProviderSpec
message to include an optional google.protobuf.Timestamp (or scalar counter)
named health_check_trigger and update IdentityProviderStatus to include an
optional IdentityProviderHealth field named health (or
IdentityProviderHealthStatus + message as needed), and ensure the
google/protobuf/timestamp import is present and field numbers do not collide
with existing ones.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d40f32ab-03ff-44de-a903-3bef302c1a73
⛔ Files ignored due to path filters (4)
internal/api/osac/private/v1/identity_provider_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_provider_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/identity_provider_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/identity_provider_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
internal/api/osac/private/v1/identity_provider_logging.gointernal/api/osac/public/v1/identity_provider_logging.gointernal/api/osac/public/v1/identity_provider_logging_test.goproto/private/osac/private/v1/identity_provider_type.protoproto/public/osac/public/v1/identity_provider_type.proto
CrystalChun
force-pushed
the
idp
branch
2 times, most recently
from
55b861a to
70e28e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/osac/private/v1/identity_provider_logging.go`:
- Around line 22-57: The method LogValue has an undefined receiver type
IdentityProviderEvent causing build failure; update the receiver to the correct
generated protobuf type or add/regenerate the proto so the type exists: locate
the LogValue method in identity_provider_logging.go (function LogValue) and
either change its receiver from IdentityProviderEvent to the actual generated
struct name (e.g., IdentityProviderEventProto or the package-qualified proto
type) used elsewhere, or add the missing proto message and run the
codegen/regeneration step so IdentityProviderEvent is generated, then rebuild to
ensure the undefined-type error is resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6f617214-0534-4672-89e9-2e10921e2fcd
⛔ Files ignored due to path filters (4)
internal/api/osac/private/v1/identity_provider_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_provider_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/identity_provider_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/identity_provider_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
internal/api/osac/private/v1/identity_provider_logging.gointernal/api/osac/public/v1/identity_provider_logging.gointernal/api/osac/public/v1/identity_provider_logging_test.goproto/private/osac/private/v1/identity_provider_type.protoproto/public/osac/public/v1/identity_provider_type.proto
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/osac/public/v1/identity_provider_logging_test.go`:
- Around line 26-84: Add explicit nil-receiver tests to verify
IdentityProviderTestResult.LogValue and IdentityProviderHealth.LogValue safely
return empty slog.Value without panicking or logging fields; create two tests
(e.g., TestIdentityProviderTestResult_LogValue_NilReceiver and
TestIdentityProviderHealth_LogValue_NilReceiver) that declare a nil pointer for
each type, invoke the logger.Info with that nil value, assert there was no panic
and that sensitive/regular fields like "success", "UNHEALTHY" or "has_message"
are not present in the log output, and ensure the tests use the same buffering
logger pattern as the existing tests.
In `@proto/private/osac/private/v1/identity_provider_type.proto`:
- Around line 239-246: The proto comments for the message fields lack warnings
that they may contain sensitive data; update the comment on
IdentityProviderTestResult.message and IdentityProviderHealth.message in both
proto files to explicitly warn that the field may include sensitive information
(bind DNs, credentials, URLs with embedded credentials, hostnames, OAuth
secrets) and instruct consumers not to naively log or expose it; also add a
short note pointing to the generated LogValue implementations
(internal/api/osac/private/v1/identity_provider_logging.go and
internal/api/osac/public/v1/identity_provider_logging.go) as examples of safe
redaction/handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9fd4dea3-a423-4c4f-9fe3-c69b96bb5d02
⛔ Files ignored due to path filters (4)
internal/api/osac/private/v1/identity_provider_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/identity_provider_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/identity_provider_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/identity_provider_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
internal/api/osac/private/v1/identity_provider_logging.gointernal/api/osac/public/v1/identity_provider_logging.gointernal/api/osac/public/v1/identity_provider_logging_test.goproto/private/osac/private/v1/identity_provider_type.protoproto/public/osac/public/v1/identity_provider_type.proto
| // IdentityProviderTestResult contains the results of testing IdP connectivity. | ||
| message IdentityProviderTestResult { | ||
| // Success indicates whether the test was successful. | ||
| bool success = 1; | ||
|
|
||
| // Message provides details about the test result or error information. | ||
| optional string message = 2; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Message fields lack sensitive-data warnings in proto/private/osac/private/v1/identity_provider_type.proto and proto/public/osac/public/v1/identity_provider_type.proto.
Risk: MEDIUM (information disclosure via naive logging). The message fields in IdentityProviderTestResult and IdentityProviderHealth (present in both private and public APIs) will contain error details from failed authentication attempts, including bind DNs, credentials, connection URLs with embedded credentials, internal hostnames, and OAuth secrets. The shared root cause is that the proto schema documents these fields as "details about the test result or error information" and "details about the health status" without warning implementers that the content may be sensitive.
The generated LogValue implementations in internal/api/osac/private/v1/identity_provider_logging.go and internal/api/osac/public/v1/identity_provider_logging.go correctly redact message content to has_message: true (good defense-in-depth), but consumers of these proto types outside the logging path (API clients, external integrations, custom log formatters) lack schema-level guidance and may naively log the raw message content.
Add warnings to the proto comments for both IdentityProviderTestResult.message and IdentityProviderHealth.message in both files, as shown in the per-site comment diffs. The warnings should state that the field may contain sensitive data and reference the LogValue implementations as examples of safe handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@proto/private/osac/private/v1/identity_provider_type.proto` around lines 239
- 246, The proto comments for the message fields lack warnings that they may
contain sensitive data; update the comment on IdentityProviderTestResult.message
and IdentityProviderHealth.message in both proto files to explicitly warn that
the field may include sensitive information (bind DNs, credentials, URLs with
embedded credentials, hostnames, OAuth secrets) and instruct consumers not to
naively log or expose it; also add a short note pointing to the generated
LogValue implementations
(internal/api/osac/private/v1/identity_provider_logging.go and
internal/api/osac/public/v1/identity_provider_logging.go) as examples of safe
redaction/handling.
66ef54c to
52f805a
Compare
| } | ||
|
|
||
| // IdentityProviderTestResult contains the results of testing IdP connectivity. | ||
| message IdentityProviderTestResult { |
There was a problem hiding this comment.
🟡 IdentityProviderTestResult is defined but unused
This message is defined in both public and private protos but no RPC, no field in IdentityProviderStatus or IdentityProviderHealth, and no server code references it. It looks like dead code left over from the imperative TestIdentityProvider RPC that was removed per jhernand's feedback.
It also has LogValue implementations and tests written for it.
Recommendation: Remove from both protos and all hand-written code referencing it. Add it back in the PR that actually uses it.
There was a problem hiding this comment.
Thank you for catching this! Removed the unused code.
| optional string message = 2; | ||
|
|
||
| // LastChecked is when the health was last checked. | ||
| google.protobuf.Timestamp last_checked = 3; |
There was a problem hiding this comment.
💡 last_checked should be optional
When a health check has never been performed, last_checked will default to the zero-value Timestamp (1970-01-01T00:00:00Z), which is semantically different from "never checked." The message field in the same message correctly uses optional, and health on the parent status also uses optional.
Per the project's proto conventions: "Use optional for fields that may not be set."
Recommendation: Change to optional google.protobuf.Timestamp last_checked = 3; in both public and private protos.
There was a problem hiding this comment.
Thank you for letting me know! Modified it to be optional.
There was a problem hiding this comment.
Note that composite types (like google.protobuf.Timestamp) are always optional: putting the optional keyword in front of them doesn't change anything, just adds extra token for the compiler to process. Not a big deal. It does make a difference for primitive types, like string, bool, etc: the generated code will use a pointer, or a similar construct, to be able to check if it is present or not.
There was a problem hiding this comment.
I didn't realize that, thank you for letting me know! Let me remove this.
There was a problem hiding this comment.
No need to remove it, it makes no difference. Just wanted to make sure you know that, for other cases.
| if r == nil { | ||
| return slog.Value{} | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Missing logging test for private API types
The public API has both identity_provider_logging.go and identity_provider_logging_test.go, but the private API has no corresponding test file.
Recommendation: Add identity_provider_logging_test.go for the private package mirroring the public test file.
There was a problem hiding this comment.
Thanks for catching this! Added an associated test file for the private API.
Allows an admin to query the health of an IDP. Also adds organization-level APIs. Assisted-by: Claude Code <noreply@anthropic.com>
4 participants
Description
Allows an admin to query the health of an IDP. Also adds organization-level APIs.
Testing
go run github.com/bufbuild/buf/cmd/buf@latest lintpassesgo build ./...Assisted-by: Claude Code noreply@anthropic.com
/cc @jhernand
Summary by CodeRabbit