[webkit-reviews] review denied: [Bug 183530] [WebAuthn] Roll back the newly created credential if an error happens afterwards : [Attachment 398712] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 183530: [WebAuthn] Roll back the newly created credential if an error
happens afterwards
https://bugs.webkit.org/show_bug.cgi?id=183530

Attachment 398712: Patch

https://bugs.webkit.org/attachment.cgi?id=398712&action=review




--- Comment #4 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 398712
  --> https://bugs.webkit.org/attachment.cgi?id=398712
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398712&action=review

I think this looks good, but I think you made a typo "NSDate instead of
NSData".

> Source/WebKit/ChangeLog:3
> +	   [WebAuthn] Roll back the newly created credential if an error
happens afterwards

Maybe "Roll back newly created credentials if an error occurs"?

> Source/WebKit/ChangeLog:9
> +	   Otherwise, the newly created credential will be a dangling
credential.

Maybe "We should clean up any newly created credentials if an error occurs
before the relying party registers the identity. Otherwise we are left with a
dangling credential."

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.h:78
> +    RetainPtr<NSDate> m_newlyCreatedCredentialId;

This should be an NSData, I think (not an NSDate).

Maybe we should call this "m_provisionalCredentialId"?

Also: Should this be nulled at some point when the credential moves from
'provisional' to 'committed' (or 'complete') ?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:340
> +	   m_newlyCreatedCredentialId = toNSData(credentialId);

m_provisionalCredentialId

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:605
> +	       LOG_ERROR(makeString("Couldn't delete older credential: "_s,
status).utf8().data());

I think this should say something like:

"Couldn't delete provisional credential while handling error: "