search

AzureAD / microsoft-authentication-library-common-for-android

Common code used by both the Active Directory Authentication Library (ADAL) and the Microsoft Authentication Library (MSAL)
MIT License
41 stars 35 forks source link

Move addition of passkey header from BaseController to WebView fragment #2237

Closed melissaahn closed 11 months ago

melissaahn commented 11 months ago

Summary

The initial work I did to add the passkey protocol header occurs in the BaseController and the PrtV3 authorization strategy classes. Adding the header in these two places covers the scenarios that MSAL/Broker needs. However, in their non-brokered auth flow, OneAuth utilizes the AuthorizationActivityFactory.getAuthorizationActivityIntent directly, so they are not making use of any controllers in this case (though they do use BrokerMsalController for broker scenarios). (For context, getAuthorizationActivityIntent is called down the line by our strategies). Thus, with the header logic the way it is, OneAuth is going to need to add the passkey header themselves to their getAuthorizationActivityIntent call.

They brought up the idea that if I were to move the addition of the passkey header to a class that's covered by getAuthorizationActivityIntent, then they would be able to get the logic automatically. If I move the passkey header logic to WebViewAuthorizationFragment, my understanding is that it would be inherited by both MSAL and broker (so I can also remove the logic from PrtV3 authorization strategy, iiuc) as well as OneAuth. And I also wouldn't have to check for authorization agent == WEBVIEW anymore. But this change would not be correlating to where we typically add headers (which is in BaseController and the PrtV3 auth strategy).

I want to get some input from the team about this before commiting to making such a change. Decided to make the change.

rpdome commented 11 months ago

1) Do we plan to support passkey in both standalone MSAL and OneAuth?

2) Will this be configurable flow by flow?

--

The original design is letting the method that constructs the request to decide if passkey should be supported. This change means 2) is no longer doable. (e.g. you don't want concurrent requests to modify a global static variable)

If 1) and 2) is not a concern (e.g. every request header from MSAL/OneAuth/Broker would always have this value), then this seems OK.

melissaahn commented 11 months ago
  1. Do we plan to support passkey in both standalone MSAL and OneAuth?
  2. Will this be configurable flow by flow?

--

The original design is letting the method that constructs the request to decide if passkey should be supported. This change means 2) is no longer doable. (e.g. you don't want concurrent requests to modify a global static variable)

If 1) and 2) is not a concern (e.g. every request header from MSAL/OneAuth/Broker would always have this value), then this seems OK.

  1. We do plan to support this in standalone MSAL and OneAuth (for 1P apps only). Noting that this header controls whether or not we want to use the custom passkey protocol, which is necessary for cases where WebView is the authorization agent. It does not change the UX of the login page, which is handled by a different query string parameter that is configurable flow by flow.
  2. No, it shouldn't be. Android's WebView does not currently have the capability to support WebAuthn calls automatically like CCT does, which is why the custom protocol was built. So anytime a WebView is being used as the auth agent (whether it's for MSAL, OneAuth, or Broker), we want to state to the server that the passkey protocol should be used, should any passkey request come in. Another thing about this solution is that if in the future, Android does implement WebAuthn support for WebView, we can turn off this custom protocol in one shot, instead of making the same changes in broker and OneAuth individually.