Skip to content
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

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

melissaahn
Copy link
Contributor

@melissaahn melissaahn commented Aug 23, 2023

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.
image

@melissaahn melissaahn added the No-Changelog This change does not update the changelog. label Aug 23, 2023
@melissaahn melissaahn marked this pull request as ready for review August 23, 2023 15:44
@melissaahn melissaahn requested a review from a team as a code owner August 23, 2023 15:44
Copy link
Contributor

@shahzaibj shahzaibj left a 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?

@somalaya
Copy link
Contributor

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?

@melissaahn
Copy link
Contributor Author

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?

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.
This is just what I'm thinking though, so let me know if there are other things I should be considering.

@melissaahn
Copy link
Contributor Author

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 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)

@shahzaibj
Copy link
Contributor

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?

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. This is just what I'm thinking though, so let me know if there are other things I should be considering.

Makes sense. The way you have it seems good to me!

@negoe
Copy link
Contributor

negoe commented Oct 8, 2023

@melissaahn Once the feature is released, will this need update to Sample app also?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No-Changelog This change does not update the changelog. testapps
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants