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 allowed signing algorithms in validator #128

Merged
merged 1 commit into from
Dec 7, 2021

Conversation

sergiught
Copy link
Contributor

@sergiught sergiught commented Dec 6, 2021

Description

In this PR we add allowed signing algorithms for the validator.

References

Testing

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have read and agreed to the terms within the Auth0 Code of Conduct.
  • I have read the Auth0 General Contribution Guidelines.
  • I have reviewed my own code beforehand.
  • I have added documentation for new/changed functionality in this PR.
  • All active GitHub checks for tests, formatting, and security are passing.
  • The correct base branch is being used, if not master.

@sergiught sergiught self-assigned this Dec 6, 2021
@sergiught sergiught requested a review from a team as a code owner December 6, 2021 09:58
evansims
evansims previously approved these changes Dec 6, 2021
Copy link
Member

@evansims evansims left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

PS384 = SignatureAlgorithm("PS384") // RSASSA-PSS using SHA384 and MGF1-SHA384
PS512 = SignatureAlgorithm("PS512") // RSASSA-PSS using SHA512 and MGF1-SHA512
)

// Validator to use with the jose v2 package.
type Validator struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this type need to be exported? It has a field of type jwt.Expected, which is from the underlying library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately we can't make it private because of the following linting error:

validator/validator.go:65:4: unexported-return: exported func New returns unexported type *validator.validator, which can be annoying to use (revive)
) (*validator, error) {

The field type of jwt.Expected is not exported so there should not be any issues IMO:)

Copy link
Contributor

@Widcket Widcket left a comment

Choose a reason for hiding this comment

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

Looks good, just left a comment about the Validator struct.

Base automatically changed from patch/refactor-tests to master December 7, 2021 07:33
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2021

Codecov Report

Merging #128 (5d3c76a) into master (92afdf4) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #128   +/-   ##
=======================================
  Coverage   89.32%   89.32%           
=======================================
  Files           7        7           
  Lines         178      178           
=======================================
  Hits          159      159           
  Misses         12       12           
  Partials        7        7           
Impacted Files Coverage Δ
validator/validator.go 89.47% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92afdf4...5d3c76a. Read the comment docs.

@sergiught
Copy link
Contributor Author

Had to push again as the github actions were stuck.

@sergiught sergiught merged commit 1ba26fe into master Dec 7, 2021
@sergiught sergiught deleted the patch/signing-method branch December 7, 2021 08:57
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.

None yet

5 participants