-
Notifications
You must be signed in to change notification settings - Fork 113
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
LG-13937 Validate secret token for socure DocV webhook #11118
Conversation
|
||
def token_valid? | ||
authorization_header = request.headers['Authorization']&.split&.last | ||
authorization_header == IdentityConfig.store.socure_webhook_secret_key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
authorization_header == IdentityConfig.store.socure_webhook_secret_key | |
ActiveSupport::SecurityUtils.secure_compare( | |
authorization_header, | |
IdentityConfig.store.socure_webhook_secret_key, | |
) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make the config an array so we can smoothly support a transition between an old & new key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think @zachmargolis makes a great point here ☝🏿
if token_valid? | ||
render json: { message: 'Secret token is valid.' } | ||
else | ||
render status: :unauthorized, json: { message: 'Invalid secret token.' } | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we be doing more validation in the future? Should we start doing form validation pattern?
if token_valid? | |
render json: { message: 'Secret token is valid.' } | |
else | |
render status: :unauthorized, json: { message: 'Invalid secret token.' } | |
end | |
form = SocureWebhookValidationForm.new | |
result = form.submit(headers: request.headers) | |
if result.success? | |
render json: { message: 'Secret token is valid.' } | |
else | |
render status: :unauthorized, json: { message: result.first_error_message } | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we will be; my understanding is that we'll use the webhook just for status updates on submitted requests, and the secret token is just an anti-ddos measure.
Certainly, if we do end up using information from the webhook, then we should follow the form validation pattern we already have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, the actual PII data from Socure will be acquired through a separate API call to them.
Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov>
changelog: Upcoming Features, Doc Auth, add secret validation for socure webhook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested against the review app, and in the solipet env.
if token_valid? | ||
render json: { message: 'Secret token is valid.' } | ||
else | ||
render status: :unauthorized, json: { message: 'Invalid secret token.' } | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, the actual PII data from Socure will be acquired through a separate API call to them.
🎫 Ticket
LG-13937
🛠 Summary of changes
Socure has the ability to send a secret token with the webhook. We want to validate that secret whenever the webhook is used.
📜 Testing Plan
Provide a checklist of steps to confirm the changes.