Pass navigator.maxTouchPoints from Web UI to auth service
#61777
Conversation
There's already post and postFormData. Similar to deleteWithOptions, instead of adding just another positional arg with custom headers to either post or postFormData, I've decided to create a third function with a more flexible API. We could deprecate the other two functions but I don't know when I'll backport this change to v18 yet. The post function should be enough for 95% of cases.
ravicious
added
the
no-changelog
ravicious
force-pushed
the
r7s/detect-ipados
branch
from
ec207a1 to
37edce7
Compare
| } | ||
| var receivedWebToken *devicepb.DeviceWebToken | ||
| authServer.SetCreateDeviceWebTokenFunc(func(ctx context.Context, dwt *devicepb.DeviceWebToken) (*devicepb.DeviceWebToken, error) { | ||
| receivedWebToken = dwt |
There was a problem hiding this comment.
How bad is this? receivedWebToken is shared between subtests but for now there is only a single test.
There was a problem hiding this comment.
What if we changed the function to return nil, nil maxTouchPoints > 0 and added an iPadOS test case? I believe that is how it would work in practice, since we don't support registration of iPad devices.
ravicious
requested review from
kimlisa
and removed request for
gzdunek and
ryanclark
| } | ||
| var receivedWebToken *devicepb.DeviceWebToken | ||
| authServer.SetCreateDeviceWebTokenFunc(func(ctx context.Context, dwt *devicepb.DeviceWebToken) (*devicepb.DeviceWebToken, error) { | ||
| receivedWebToken = dwt |
There was a problem hiding this comment.
What if we changed the function to return nil, nil maxTouchPoints > 0 and added an iPadOS test case? I believe that is how it would work in practice, since we don't support registration of iPad devices.
| return res, trace.Wrap(err) | ||
| } | ||
|
|
||
| const headerMaxTouchPoints = "Max-Touch-Points" |
There was a problem hiding this comment.
My 2c: I would move this into "clientMetaFromReq" and just hard-code the header name in the test. Or hard-code the header name in both places. Headers are more a case of "well-known string" than "magic string".
codingllama
changed the title
nagivator.maxTouchPoints from Web UI to auth servicenavigator.maxTouchPoints from Web UI to auth service
There was a problem hiding this comment.
As such, while clientMetaFromReq always attempts to read the header, it'll be sent from the frontend app only to those two endpoints.
Is it wise to cherry-pick the endpoints? I'm concerned this lead to a "misunderstanding" bug, because looking at the Go sources it seems like the header is always there.
Maybe add a comment to ForwardedClientMetadata.MaxTouchPoints saying it's only available in select endpoints?
In the Device Trust service, we want to be able to differentiate between iPadOS and macOS. This is so that when we add Device Trust for mobile devices and someone adds just a macOS device to their trusted devices, they won't be prompted for Device Trust on iPadOS and vice versa.
However, since iPadOS 13, it's impossible to tell it apart from macOS based on the user agent alone since it started using the same user agent as macOS. To work around this, people typically use
navigator.maxTouchPointsto detect if the device supports touch controls. If yes, then we can assume the user is on an iPad.To pass this information to the auth service, I modified the frontend app to send
navigator.maxTouchPointsthrough theMax-Touch-Pointsheader. It seemed more fitting to send some additional metadata this way rather than through the payloads of specific endpoints. The user agent is sent in a header too after all.The next question was which endpoints should receive
Max-Touch-Points. The user agent is currently read from the header in four differentlib/webendpoints (clientMetaFromReqreads the header):mfaLoginFinishSession- finishes MFA ceremony and returns a web session.mfaLoginFinish– like above, but returns an SSH cert instead.createWebSession– creates a new web session when using TOTP.headlessLogin– used for headless logins, returns an SSH cert.From these four, only two can result in a Device Trust prompt being shown:
mfaLoginFinishSessionandcreateWebSession. As such, whileclientMetaFromReqalways attempts to read the header, it'll be sent from the frontend app only to those two endpoints.Minor refactorings
To enable this, I had to change two small things done in the first two commits.
First, Connect was constructing the device web token message from protobufs by hand. This meant that each time a new field was added, it had to be added by hand in this one place with a default value, otherwise it wouldn't pass the type checker. I refactored code in Connect to use
DeviceWebToken.createwhich creates a new message with default values. I also removed an unnecessary layer from the code, opting to use the gRPC client directly (ClustersServiceused to be the only place with access to the gRPC client, but it hasn't been the case for a long time now).Next, the Web UI has two functions for sending POST requests,
postandpostFormData, and none of them supported sending custom headers. Nic had a similar problem with DELETE requests, where we haddelete, thendeleteWithHeadersand then we needed both headers and some extra custom arguments and we finally ended up withdeleteWithOptions. Similarly, instead of adding a third function calledpostWithHeadersI addedpostWithOptionswhich accepts regular data, form data, and headers.