Account Provisioning: Credential details #263
Conversation
|
Warning Rate limit exceeded@phoebesimon has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughThis pull request introduces extensive validation functionality for new credential-related message types across multiple files. It adds validation methods for Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (5)
pkg/connectorbuilder/connectorbuilder.go (2)
546-565: Refactor Duplicated Validation Logic into a Helper FunctionThe validation logic for
CapabilityAccountProvisioningandCapabilityCredentialRotationis similar. Consider refactoring this code to reduce duplication and improve maintainability.Apply this diff to refactor the validation logic:
func validateCapabilityDetails(ctx context.Context, credDetails *v2.CredentialDetails) error { + validate := func(option v2.CapabilityDetailCredentialOption, supportedOptions []v2.CapabilityDetailCredentialOption, action string) error { + if option == v2.CapabilityDetailCredentialOption_CAPABILITY_OPTIONS_UNSPECIFIED { + return status.Errorf(codes.InvalidArgument, "error: preferred %s option is not set", action) + } + if !slices.Contains(supportedOptions, option) { + return status.Errorf(codes.InvalidArgument, "error: preferred %s option is not part of the supported options", action) + } + return nil + } if credDetails.CapabilityAccountProvisioning != nil { - if credDetails.CapabilityAccountProvisioning.PreferredCredentialOption == v2.CapabilityDetailCredentialOption_CAPABILITY_OPTIONS_UNSPECIFIED { - return status.Error(codes.InvalidArgument, "error: preferred credential creation option is not set") - } - if !slices.Contains(credDetails.CapabilityAccountProvisioning.SupportedCredentialOptions, credDetails.CapabilityAccountProvisioning.PreferredCredentialOption) { - return status.Error(codes.InvalidArgument, "error: preferred credential creation option is not part of the supported options") - } + if err := validate( + credDetails.CapabilityAccountProvisioning.PreferredCredentialOption, + credDetails.CapabilityAccountProvisioning.SupportedCredentialOptions, + "credential creation", + ); err != nil { + return err + } } if credDetails.CapabilityCredentialRotation != nil { - if credDetails.CapabilityCredentialRotation.PreferredCredentialOption == v2.CapabilityDetailCredentialOption_CAPABILITY_OPTIONS_UNSPECIFIED { - return status.Error(codes.InvalidArgument, "error: preferred credential rotation option is not set") - } - if !slices.Contains(credDetails.CapabilityCredentialRotation.SupportedCredentialOptions, credDetails.CapabilityCredentialRotation.PreferredCredentialOption) { - return status.Error(codes.InvalidArgument, "error: preferred credential rotation option is not part of the supported options") - } + if err := validate( + credDetails.CapabilityCredentialRotation.PreferredCredentialOption, + credDetails.CapabilityCredentialRotation.SupportedCredentialOptions, + "credential rotation", + ); err != nil { + return err + } } return nil }
573-592: Refactor Duplicated Code ingetCredentialDetailsFunctionThe logic for gathering capability details from
AccountManagerandCredentialManagerimplementations is similar. Consider refactoring to reduce code duplication.Apply this diff to refactor the code:
func getCredentialDetails(ctx context.Context, b *builderImpl) (*v2.CredentialDetails, error) { l := ctxzap.Extract(ctx) rv := &v2.CredentialDetails{} + type capabilityDetail struct { + getDetails func(ctx context.Context) (interface{}, annotations.Annotations, error) + assign func(interface{}) + name string + } + + var capabilities []capabilityDetail for _, rb := range b.resourceBuilders { - if am, ok := rb.(AccountManager); ok { - accountProvisioningCapabilityDetails, _, err := am.CreateAccountCapabilityDetails(ctx) - if err != nil { - l.Error("error: getting account provisioning details", zap.Error(err)) - return nil, fmt.Errorf("error: getting account provisioning details: %w", err) - } - rv.CapabilityAccountProvisioning = accountProvisioningCapabilityDetails - } - - if cm, ok := rb.(CredentialManager); ok { - credentialRotationCapabilityDetails, _, err := cm.RotateCapabilityDetails(ctx) - if err != nil { - l.Error("error: getting credential management details", zap.Error(err)) - return nil, fmt.Errorf("error: getting credential management details: %w", err) - } - rv.CapabilityCredentialRotation = credentialRotationCapabilityDetails - } + capabilities = []capabilityDetail{ + { + getDetails: func(ctx context.Context) (interface{}, annotations.Annotations, error) { + if am, ok := rb.(AccountManager); ok { + return am.CreateAccountCapabilityDetails(ctx) + } + return nil, nil, nil + }, + assign: func(detail interface{}) { + rv.CapabilityAccountProvisioning = detail.(*v2.CredentialDetailsAccountProvisioning) + }, + name: "account provisioning", + }, + { + getDetails: func(ctx context.Context) (interface{}, annotations.Annotations, error) { + if cm, ok := rb.(CredentialManager); ok { + return cm.RotateCapabilityDetails(ctx) + } + return nil, nil, nil + }, + assign: func(detail interface{}) { + rv.CapabilityCredentialRotation = detail.(*v2.CredentialDetailsCredentialRotation) + }, + name: "credential rotation", + }, + } + + for _, capDetail := range capabilities { + detail, _, err := capDetail.getDetails(ctx) + if err != nil { + l.Error(fmt.Sprintf("error: getting %s details", capDetail.name), zap.Error(err)) + return nil, fmt.Errorf("error: getting %s details: %w", capDetail.name, err) + } + if detail != nil { + capDetail.assign(detail) + } + } } err := validateCapabilityDetails(ctx, rv) if err != nil { return nil, fmt.Errorf("error: validating capability details: %w", err) } return rv, nil }pb/c1/connector/v2/connector.pb.validate.go (1)
820-821: Consider Adding Validation Rules forPreferredCredentialOptionFieldsThe
PreferredCredentialOptionfields inCredentialDetailsAccountProvisioningandCredentialDetailsCredentialRotationcurrently have no validation rules. If there are constraints on these fields, consider specifying them in the proto definition to generate appropriate validation logic.Also applies to: 927-928
proto/c1/connector/v2/resource.proto (2)
140-144: Add Comments to New Credential Option TypesConsider adding documentation comments to the
NoPasswordandSSOmessages to clarify their purpose and usage.
143-143: Add Validation Rule forsso_providerFieldTo ensure data integrity, consider adding a validation rule to the
sso_providerfield to enforce that it is not empty.Apply this change to add a validation rule:
message SSO { - string sso_provider = 1; + string sso_provider = 1 [(validate.rules).string = {min_bytes: 1, max_bytes: 1024}]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/resource.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
pb/c1/connector/v2/connector.pb.validate.go(2 hunks)pb/c1/connector/v2/resource.pb.validate.go(2 hunks)pkg/connectorbuilder/connectorbuilder.go(4 hunks)proto/c1/connector/v2/connector.proto(1 hunks)proto/c1/connector/v2/resource.proto(1 hunks)
🔇 Additional comments (4)
proto/c1/connector/v2/connector.proto (1)
62-83: LGTM!
The additions of the enum CapabilityDetailCredentialOption and the messages CredentialDetails, CredentialDetailsAccountProvisioning, and CredentialDetailsCredentialRotation are well-structured and enhance the credential management capabilities.
Also applies to: 87-87
pb/c1/connector/v2/resource.pb.validate.go (3)
1821-1902: LGTM: CredentialOptions validation implementation is correct
The implementation properly validates the NoPassword and SSO options in the CredentialOptions message, following the standard protobuf validation pattern with appropriate nil checks for oneof fields.
4101-4130:
Consider adding validation rules for SsoProvider
The SSO validation implementation currently has no validation rules for the SsoProvider field. This could potentially allow invalid SSO provider values to be specified, which might cause issues during account provisioning.
Consider adding validation rules to:
- Ensure SsoProvider is not empty if required
- Validate against a list of supported SSO providers
- Add any provider-specific validation rules
Run the following script to check the SSO provider usage in the codebase:
3998-4025: Verify if NoPassword validation requirements are complete
The NoPassword validation implementation is minimal with no specific validation rules. While this is technically correct for a simple flag, please verify if this aligns with the intended design and if there are any additional validation requirements that should be added.
Run the following script to check for any validation-related comments or discussions in the proto files:
✅ Verification successful
Let me check the proto definition more thoroughly to understand the complete context of NoPassword validation.
Based on the proto definition and validation rules inspection, I can now generate the final response:
NoPassword validation implementation is correct
The NoPassword message is intentionally defined as an empty message in the proto definition, serving as a flag-only option within CredentialOptions. The current validation implementation correctly reflects this design choice, as there are no fields to validate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for validation-related comments for NoPassword
# Test: Look for comments or annotations about validation requirements
rg -A 5 "NoPassword" proto/
Length of output: 659
Script:
#!/bin/bash
# Check for any validation rules or constraints defined in the proto files
ast-grep --pattern 'message NoPassword {
$$$
}'
# Also check for any validate.rules annotations
rg -B 2 -A 2 'validate\.rules' proto/c1/connector/v2/resource.proto
Length of output: 2527
| @@ -56,10 +56,12 @@ type CreateAccountResponse interface { | |||
|
|
|||
| type AccountManager interface { | |||
| CreateAccount(ctx context.Context, accountInfo *v2.AccountInfo, credentialOptions *v2.CredentialOptions) (CreateAccountResponse, []*v2.PlaintextData, annotations.Annotations, error) | |||
| CreateAccountCapabilityDetails(ctx context.Context) (*v2.CredentialDetailsAccountProvisioning, annotations.Annotations, error) | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Breaking Changes Confirmed - Implementation Updates Required
The addition of CreateAccountCapabilityDetails method to the AccountManager interface requires updates to the following implementations:
internal/connector/noop_provisioner.go: ThenoopProvisionerstruct implementsAccountManagerbut lacks the new methodpkg/tasks/local/accounter.go: ThelocalAccountManagerstruct implementsAccountManagerbut lacks the new method
Both implementations need to be updated to include the new CreateAccountCapabilityDetails method to satisfy the interface.
🔗 Analysis chain
Potential Breaking Changes Due to Interface Method Additions
Adding new methods to the AccountManager and CredentialManager interfaces may break existing implementations that satisfy these interfaces. Ensure that all implementations are updated accordingly.
Run the following script to identify implementations of AccountManager and CredentialManager interfaces that may need updating:
Also applies to: 64-64
Let me gather more information about the interfaces and their implementations.
Let me gather more specific information about the AccountManager interface and its implementations.
Let me check for any existing implementations of the new method.
Let me check one more thing to find all potential implementations of AccountManager.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of AccountManager and CredentialManager interfaces.
# Find implementations of AccountManager
rg --type go --files-with-matches 'type .*AccountManager interface' \
| xargs -I{} rg --type go 'func.*\(.*\).*CreateAccount\(.*\)'
# Find implementations of CredentialManager
rg --type go --files-with-matches 'type .*CredentialManager interface' \
| xargs -I{} rg --type go 'func.*\(.*\).*Rotate\(.*\)'
Length of output: 1075
Script:
#!/bin/bash
# Find the interface definitions and their implementations
echo "=== Interface Definitions ==="
rg --type go -A 5 'type .*AccountManager interface'
rg --type go -A 5 'type .*CredentialManager interface'
echo -e "\n=== Implementations ==="
# Find struct types that might implement these interfaces
ast-grep --pattern 'type $_ struct {
$$$
}'
# Find all files that contain CreateAccountCapabilityDetails
echo -e "\n=== CreateAccountCapabilityDetails Usage ==="
rg --type go 'CreateAccountCapabilityDetails'
Length of output: 66166
Script:
#!/bin/bash
# Find the AccountManager interface definition and its implementations
echo "=== AccountManager Interface ==="
rg --type go -A 5 'type AccountManager interface'
echo -e "\n=== CreateAccountCapabilityDetails Implementations ==="
ast-grep --pattern 'func $RECV CreateAccountCapabilityDetails($_) ($_, $_, $_) {
$$$
}'
echo -e "\n=== Files importing AccountManager ==="
rg --type go 'AccountManager'
Length of output: 6455
Script:
#!/bin/bash
# Search for implementations of the new CreateAccountCapabilityDetails method
echo "=== Searching for CreateAccountCapabilityDetails implementations ==="
rg --type go -A 3 'func.*CreateAccountCapabilityDetails'
# Search for noopProvisioner implementation
echo -e "\n=== Checking noopProvisioner implementation ==="
rg --type go -A 10 'type noopProvisioner struct'
# Search for any other potential implementations
echo -e "\n=== Checking other potential implementations ==="
rg --type go -A 10 'type.*struct.*implements.*AccountManager'
Length of output: 1598
Script:
#!/bin/bash
# Find all files in internal/ and pkg/ directories that might implement AccountManager
echo "=== Potential AccountManager implementations ==="
fd -e go . internal pkg | xargs rg --type go -l 'CreateAccount.*\(ctx context.Context'
# Check the content of these files
echo -e "\n=== Implementation details ==="
fd -e go . internal pkg | xargs rg --type go -A 5 'CreateAccount.*\(ctx context.Context'
Length of output: 2644
Introduces the concept of "Credential Details." These are included in the capabilities in the metadata and used by c1 to determine how the CreateAccount request can/should be crafted.
This also introduces some changes to the AccountManager and CredentialManager interfaces to require that someone implementing those also must specify how users/credentials can be created/rotated.
Summary by CodeRabbit
New Features
NoPassword,SSO, andRandomPassword.ConnectorCapabilitiesto include detailed credential management options.Bug Fixes
Documentation