-
Notifications
You must be signed in to change notification settings - Fork 126
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
Adding WebAuthn query parameter switch to msalTestApp #1888
base: dev
Are you sure you want to change the base?
Conversation
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.
Historically, things with extraQP - we've sometimes exposed them to MSAL either directly through extraQP or sometimes exposed them in the Msal Json Config file such as the instance_aware
QP which is hidden behind the multiple_cloud_supported
field in JSON Config. In this case, it seems we are exposing it directly through extraQP. I personally think this is the better & cleaner approach but just wondering if you had considered the other approach as well and you had a specific reason for picking the approach you picked?
LGTM. Just a suggestion, like the way when we click on "deviceIdclaim" button, it shows the json that is getting added in the txt box, can we do something similar for this as well instead of a toggle? |
I actually didn't know some query parameters were being added via the json config files, so thank you for bringing it up. Seems to me that with the config file option, we would have to write some custom logic to look for the WebAuthn value in the json and then add the field+value as a query parameter. One plus is that I think that in the scenario where ESTS would for some reason change the field/value for the webauthn flag, we would just need to update the constants in our libraries, vs having every 1P app make a change. That being said, I don't think ESTS should be changing the field values after the design is complete and committed to. I also think that the config file option generally seems better for features where more than one operation needs to be done upon lighting up a feature flag. The only thing that needs to happen for signaling WebAuthn capability is to send a query parameter, and the host app might as well control this since we've exposed extraQP as a parameter for acquireToken. I don't think there's a need for MSAL to get involved with extra conditional blocks if the host app can directly pass in what needs to go to ESTS. |
I did consider something like this, but the issue is that the extraQP parameter is a List<Map.entry<string, string>>. So supposing we had a text box where we would just put in query string parameters like "&field1=value1&field2=value2...", we would need to convert the string to a List of map entries, and then later logic would just turn it back into a string. Achieves the same thing, but the switch from string to list to string again seemed weird to me. I also considered creating rows with two edittexts (one for field and another for value), and a button could add more rows as necessary. But I was thinking the cost of figuring out that UI would probably outweigh the actual need for it at the moment. (though if we do find that we need to put more into extraQP in the future, I think this discussion should be revisited) |
Makes sense. The way you have it seems good to me! |
@melissaahn Once the feature is released, will this need update to Sample app also? |
Summary
This PR adds a switch to msalTestApp to pass a WebAuthn query key/value to the extra query parameters parameter when on. (Query parameter for WebAuthn will be "&webauthn=1".
This switch is currently set to "off" by default, but once ESTS implements the work for this extra parameter, switching it "on" will show an option for passkeys in the WebView login page.