Conversation
WalkthroughAdds CrowdStrike identity protection support: a new OAuth2-backed IdentityProtectionClient (GraphQL queries, pagination, rate-limit parsing), a security insight resource type and builder, connector internal fields for credentials, and a direct OAuth2 dependency in go.mod. Changes
Sequence DiagramsequenceDiagram
participant Connector
participant SecurityInsight as Security Insight
participant IdentityProtection as Identity Protection Client
participant OAuth2 as OAuth2 Server
participant CrowdStrike as CrowdStrike API
Connector->>SecurityInsight: List(ctx, token)
SecurityInsight->>IdentityProtection: GetIdentityRiskScores(ctx, pageSize, cursor)
IdentityProtection->>OAuth2: Request token (client_id, client_secret)
OAuth2-->>IdentityProtection: Access token
IdentityProtection->>CrowdStrike: POST GraphQL (Bearer token)
CrowdStrike-->>IdentityProtection: GraphQL response (entities, pageInfo, headers)
IdentityProtection->>SecurityInsight: IdentityRiskData[], nextCursor, hasNextPage, RateLimitInfo
loop per identity
SecurityInsight->>SecurityInsight: Build SecurityInsight resource
end
SecurityInsight-->>Connector: Resources[], Annotations (RateLimitInfo)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
mstanbCO
requested review from
a team,
alan-lee-12,
gontzess,
johnallers and
laurenleach
mstanbCO
force-pushed
the
mstanbCO/security_insights
branch
from
0502d6f to
bda5cae
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/connector/security_insight.go`:
- Around line 59-65: The comment says "format as percentage" but
identity.RiskScore is 0–1; update the conversion so the displayed value is a
percent by multiplying identity.RiskScore by 100 before formatting (refer to
identity.RiskScore and the strconv.FormatFloat call) and then pass that string
to rs.WithRiskScore when building traitOpts, or alternatively remove/adjust the
misleading comment if you do not want percentage output (affecting the
riskScoreStr creation and the rs.SecurityInsightTraitOption usage).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/connector/connector.go (1)
39-76:⚠️ Potential issue | 🟡 MinorAdd validation for identity protection endpoint access in Validate().
The
Validatemethod validates access to user and role endpoints but omits the identity protection GraphQL API thatsecurityInsightBuilderuses. If credentials lack permissions for the identity protection API, the connector passes validation but fails during sync.Call
ipClient.ValidateAccess(ctx)in theValidatemethod to ensure credentials have the necessary permissions for the identity protection endpoint before sync begins.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/connector/security_insight.go`:
- Around line 124-131: The comment above the call to
securityInsightResource(identity) is contradictory and the error is swallowed;
update the block so the comment accurately reflects behavior (either remove
"Skip identities..." and the "We still include them" contradiction if you intend
to process all identities, or implement explicit skip logic when the risk score
equals 0) and replace the silent continue with a proper log call that surfaces
the error (e.g., use the module's logger variable to call Logger.Error/Logf with
context and err before continue). Ensure you reference the
securityInsightResource(identity) call and the identity variable when adding the
log or skip logic so the change is easy to locate.
🧹 Nitpick comments (1)
pkg/connector/security_insight.go (1)
20-24: Remove unusedclientfield.The
clientfield is assigned insecurityInsightBuilderbut never accessed by any method on this type. OnlyipClientis used. Consider removing both the field and the corresponding parameter from the builder function.♻️ Proposed fix
type securityInsightResourceType struct { resourceType *v2.ResourceType - client *fClient.CrowdStrikeAPISpecification ipClient *IdentityProtectionClient }And update the builder:
-func securityInsightBuilder(ctx context.Context, client *fClient.CrowdStrikeAPISpecification, clientID, clientSecret, host string) *securityInsightResourceType { +func securityInsightBuilder(ctx context.Context, clientID, clientSecret, host string) *securityInsightResourceType { return &securityInsightResourceType{ resourceType: resourceTypeSecurityInsight, - client: client, ipClient: NewIdentityProtectionClient(ctx, clientID, clientSecret, host), } }
| // Skip identities without a risk score (value of 0 means no assessment yet) | ||
| // We still include them but they will have a risk score of "0" | ||
|
|
||
| resource, err := securityInsightResource(identity) | ||
| if err != nil { | ||
| // Log the error but continue processing other identities | ||
| continue | ||
| } |
There was a problem hiding this comment.
Fix misleading comments and add actual error logging.
Two issues in this block:
-
Contradictory comment (lines 124-126): Says "Skip identities without a risk score" but then immediately says "We still include them." There's no skip logic—all identities are processed. Either add actual skip logic or remove the misleading comment.
-
Silent error swallowing (lines 128-131): Comment says "Log the error" but no logging occurs. Errors are silently discarded, which masks failures and makes debugging difficult.
🐛 Proposed fix
- // Skip identities without a risk score (value of 0 means no assessment yet)
- // We still include them but they will have a risk score of "0"
-
resource, err := securityInsightResource(identity)
if err != nil {
- // Log the error but continue processing other identities
+ // Log and continue - don't fail the entire sync for one bad identity
+ // Consider using a structured logger if available
+ fmt.Printf("warning: failed to create security insight resource for identity %s: %v\n", identity.PrimaryDisplayName, err)
continue
}Or if a proper logger is available in the codebase:
resource, err := securityInsightResource(identity)
if err != nil {
- // Log the error but continue processing other identities
+ l := ctxzap.Extract(ctx)
+ l.Warn("failed to create security insight resource",
+ zap.String("identity", identity.PrimaryDisplayName),
+ zap.Error(err))
continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Skip identities without a risk score (value of 0 means no assessment yet) | |
| // We still include them but they will have a risk score of "0" | |
| resource, err := securityInsightResource(identity) | |
| if err != nil { | |
| // Log the error but continue processing other identities | |
| continue | |
| } | |
| resource, err := securityInsightResource(identity) | |
| if err != nil { | |
| // Log and continue - don't fail the entire sync for one bad identity | |
| // Consider using a structured logger if available | |
| fmt.Printf("warning: failed to create security insight resource for identity %s: %v\n", identity.PrimaryDisplayName, err) | |
| continue | |
| } |
🤖 Prompt for AI Agents
In `@pkg/connector/security_insight.go` around lines 124 - 131, The comment above
the call to securityInsightResource(identity) is contradictory and the error is
swallowed; update the block so the comment accurately reflects behavior (either
remove "Skip identities..." and the "We still include them" contradiction if you
intend to process all identities, or implement explicit skip logic when the risk
score equals 0) and replace the silent continue with a proper log call that
surfaces the error (e.g., use the module's logger variable to call
Logger.Error/Logf with context and err before continue). Ensure you reference
the securityInsightResource(identity) call and the identity variable when adding
the log or skip logic so the change is easy to locate.
2 participants
Description
Summary by CodeRabbit