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

Refactor and move security validations #804

Merged
merged 40 commits into from Nov 14, 2019
Merged

Refactor and move security validations #804

merged 40 commits into from Nov 14, 2019

Conversation

XVincentX
Copy link
Contributor

@XVincentX XVincentX commented Nov 13, 2019

This PR will address most of the technical debt we had in the security validations. In particular:

  • Moves the HTTP security validations to the HTTP package and not keep them in core. They didn't make sense since in the core package we do not know if we're handling HTTP stuff. This allowed me to remove a lot of unknown and weird lodash.get calls and use precise types
  • Validate Security is now standardised and uses the same ValidatorFn shape we're using for valdidateInput and validateOutput
  • The "securityHandler" look up function has been replaced with a static one which is type safer and probably even faster. The runtime option didn't make sense.
  • Removed useless parameters that were not used, as well some optional that, well, were not optional
  • Narrowed the types; security is only checking security, we do not need to pass the whole HTTP Request
  • Tests and some minor bugs have been fixed because of the more precise typing

There are some other internal refactors that need to be done (although not urgent) because the current code is really hard to reason about. I'll tame this beast in the next airplane (tomorrow)

@XVincentX XVincentX force-pushed the feat/security- branch 2 times, most recently from 6124d1b to 12538e9 Compare November 13, 2019 18:20
@XVincentX XVincentX marked this pull request as ready for review November 14, 2019 10:48
@XVincentX XVincentX force-pushed the feat/security- branch 2 times, most recently from d4d731f to f689fd3 Compare November 14, 2019 11:17
Copy link
Contributor

@karol-maciaszek karol-maciaszek left a comment

Choose a reason for hiding this comment

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

Looks great, yet another piece of code is polished 👍

Couple minor comments added.

Copy link
Contributor

@karol-maciaszek karol-maciaszek left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@XVincentX XVincentX merged commit 0969677 into master Nov 14, 2019
@XVincentX XVincentX deleted the feat/security- branch November 14, 2019 15:48
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

2 participants