Conversation
msirringhaus
left a comment
msirringhaus
left a comment
There was a problem hiding this comment.
Some small comments
| ) | ||
| .map_err(|e| { | ||
| if let WebAuthnError::TypeError = e { | ||
| tracing::error!("Could not parse passkey creation request. Rejecting request."); |
There was a problem hiding this comment.
I know this is only copy&paste from what was before, but I find this logging a bit weird. We are rejecting the request for all other errors as well, don't we?
There was a problem hiding this comment.
Yeah, we are. Are you saying that it's weird that we're not consistent, or specifically that it's weird that we're including "Rejecting request." here?
I think that it's useful for admins to know from the logs both what caused the error, and what action was taken. I usually try to put that on the same line if possible, so it's easy to find in the logs. But I do notice that we're not consistent in this method, nor is this method responsible for the decision to reject the request or not. I'd be fine with removing "Rejecting request" here and pushing that up into the higher-level method that actually returns the error back to D-Bus.
There was a problem hiding this comment.
I was just wondering why we print a tracing for TypeError only, and not for all other errors. We tell the admin that we are rejecting the request for one specific error only, but silently (?) reject it in all other cases.
| if let WebAuthnError::TypeError = e { | ||
| tracing::error!("Could not parse passkey creation request. Rejecting request."); | ||
| } | ||
| e |
There was a problem hiding this comment.
Also, very much nit picking but: I hate single letter variables :D Can we name this err instead?
2 participants
This improves
RequestEnvironmenttoNavigationContextRequestEnvironment/NavigationContextmixed the concepts of origin type (HTTPS/AppId) and same/cross-origin context. These two concepts are now split intoOriginandNavigationContextmore clearly.