LG-14198: AAGUID logging #11138
LG-14198: AAGUID logging #11138
Conversation
There was a problem hiding this comment.
I'd expect we'd do this entirely on the backend without any JavaScript, because we're already sending authenticator data (through attestationObject), which is available for us to access through the webauthn-ruby gem.
There was a problem hiding this comment.
This migration will need to happen in a separate deploy to avoid 50/50 state issues.
There was a problem hiding this comment.
I think that part of that guide is for a field that is expected to be backfilled
AFAICT this PR is OK because we're writing to the column but not reading from it. And the boxes that spin up and write to the column will have the correct code, and the migration completed, and the old boxes that don't know about the field will ignore it
There was a problem hiding this comment.
Okay, that makes sense. Part of the ticket is to include the database value in authentication attempts, where I'd expect we'd be reading from the record. Need more clarity on the issues raised in #11138 (comment) .
There was a problem hiding this comment.
okay ill make a separate pr since im reading from it now. with just the migration.
Can you elaborate on what you mean by this? |
| @@ -110,6 +110,7 @@ def valid_attestation_response?(protocol) | |||
| ) | |||
|
|
|||
| begin | |||
| @aaguid = attestation_response.authenticator_data.aaguid | |||
There was a problem hiding this comment.
Couple minor comments:
- If we're not expecting this to raise an exception, I think it'd be best to limit the
rescue'dbeginblock to code which is expected to raise. - Assigning it as a side-effect of validation feels like it creates a fragile relationship between assignment and use. Since we have
attestation_responseas a consumed parameter insubmitand the value is derived from that, we could create a method which derives directly from that and avoids another instance variable.
def aaguid
attestation_response&.authenticator_data&.aaguid
end| @@ -172,6 +174,7 @@ def extra_analytics_attributes | |||
| pii_like_keypaths: [[:mfa_method_counts, :phone]], | |||
| authenticator_data_flags: authenticator_data_flags, | |||
| unknown_transports: invalid_transports.presence, | |||
| aaguid: aaguid, | |||
There was a problem hiding this comment.
Can we add corresponding YARDoc updates for method documentation in analytics_events.rb?
I would have also expected this to be enforced by UndocumentedParamsChecker and fail the build, which makes me worried we might not have full coverage for the code which would log this event.
Looking at this aaguid throws an error when I attempt to call for it in webauthn verification form. I think this is due to https://developer.mozilla.org/en-US/docs/Web/API/Web_Authentication_API/Authenticator_data#attestedcredentialdata, we are verifying the credentials and thus aaguid doesnt exist and throws a formating error. |
Oh, I was expecting that we'd log the value from the database record, not what's given to us from the client-side authentication. |
Ah got you. I have no problem with that |
mdiarra3
force-pushed
the
LG-14198-store-aaguid-in-webauthn-configuration
branch
from
f07d030 to
eb1209e
Compare
🎫 Ticket
Link to the relevant ticket:
LG-14198
🛠 Summary of changes
This will add aaguid to the webauthn configuration, this will allow us to keep track of what kind of devices are used to create passkeys.
*note it seems like we dont have a way to check this in verification ,and also aaguid is nil for security keys right now, but Fido seems to imply that that shouldnt be?