Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config to chained validator #692

Merged
merged 8 commits into from
Oct 14, 2024
Merged

Add config to chained validator #692

merged 8 commits into from
Oct 14, 2024

Conversation

Richard87
Copy link
Contributor

No description provided.

@Richard87 Richard87 marked this pull request as ready for review October 10, 2024 11:06
@Richard87 Richard87 self-assigned this Oct 10, 2024
@Richard87 Richard87 requested a review from satr October 10, 2024 11:07
satr
satr previously approved these changes Oct 11, 2024
Comment on lines 18 to 25
for index, validator := range v.validators {
principal, err = validator.ValidateToken(ctx, token)
if err == nil {
return principal, nil
} else if index == len(v.validators)-1 {
return nil, err
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional suggestion - to make it easier to read

Suggested change
for index, validator := range v.validators {
principal, err = validator.ValidateToken(ctx, token)
if err == nil {
return principal, nil
} else if index == len(v.validators)-1 {
return nil, err
}
}
if principal, err = validator.ValidateToken(ctx, token); err == nil {
return principal, nil
} else if index == len(v.validators)-1 {
return nil, err
}

or

Suggested change
for index, validator := range v.validators {
principal, err = validator.ValidateToken(ctx, token)
if err == nil {
return principal, nil
} else if index == len(v.validators)-1 {
return nil, err
}
}
principal, err = validator.ValidateToken(ctx, token)
if err == nil {
return principal, nil
}
if index == len(v.validators)-1 {
return nil, err
}

or

Suggested change
for index, validator := range v.validators {
principal, err = validator.ValidateToken(ctx, token)
if err == nil {
return principal, nil
} else if index == len(v.validators)-1 {
return nil, err
}
}
var principal TokenPrincipal
var err error
for _, validator := range v.validators {
if principal, err = validator.ValidateToken(ctx, token); err == nil {
return principal, nil
}
}
if err != nil {
return nil, err
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed with:

func (v *ChainedValidator) ValidateToken(ctx context.Context, token string) (TokenPrincipal, error) {
	var errs []error

	for _, validator := range v.validators {
		principal, err := validator.ValidateToken(ctx, token)
		if principal != nil {
			return principal, nil
		}
		errs = append(errs, err)
	}

	return nil, fmt.Errorf("%w: %v", errNoIssuersFound, errors.Join(errs...))
}

return &ChainedValidator{validators}
}

func (v *ChainedValidator) ValidateToken(ctx context.Context, token string) (principal TokenPrincipal, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for names in returned values - method returns them explicitly, like return principal, nil or return nil, err

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, returning a list of errors if not found :)

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
satr
satr previously approved these changes Oct 14, 2024
@Richard87 Richard87 merged commit 6a70301 into master Oct 14, 2024
6 checks passed
@Richard87 Richard87 deleted the chained-validator branch October 14, 2024 13:50
Richard87 added a commit that referenced this pull request Oct 16, 2024
Release: Chained validator (#692)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants