microsoft/botbuilder-js

CertificateAppCredentials defaults tenant to botframework.com

XVincentX opened this issue · 4 comments

XVincentX commented

https://github.com/microsoft/botbuilder-js/blob/main/libraries/botframework-connector/src/auth/certificateAppCredentials.ts#L39

The CertificateAppCredential class does not require a tenant id when creating a new instance; yet I found out (and make me lose hours) is that if you are not providing it, it will automatically set it to botframework.com - which does not look like a good default

image

I am not too sure why such parameter is optional, since the authentication just cannot happen without a tenant ID, and using botframework.com as default does not look a good backup plan

botbuilder-js/libraries/botframework-connector/src/auth/authenticationConstants.ts

Line 31 in f487de2

export const DefaultChannelAuthTenant = 'botframework.com';

sw-joelmut commented

Hi @XVincentX,

The CertificateAppCredential class has the tenantId parameter marked as optional and defaults to botframework.com because of MultiTenant apps.
BotBuilder supports multiple types of apps to authenticate, MultiTenant, SingleTenant and UserAssignedMSI (more info). Thus, by providing a tenantId for MultiTenant apps other than the default, the authentication will not work.
Additionally, the default value is also required for MultiTenant, and changing it will break MultiTenant authentication.

tracyboehrer commented

Since any change would be breaking, lets add comments around this behavior in that class. There is probably a bit more history here since the other app types were added later.

  • 👍2
XVincentX commented

@sw-joelmut

  1. Aren't multi tenant application supposed to use common as tenant name? Why botframework.com?
  2. Would it make sense to have whatever value is required as default of the constructor, rather than setting it somewhere in the code, making unclear what is really happening?
tracyboehrer commented

"botframework.com" has been the default tenant in AppCredentials, however subclasses can change this. As in the case of Gov cloud where its "MicrosoftServices.onmicrosoft.us". Though the option is there to specify a different tenant in the constructor.

Most of this dates back far, when there was only MultiTenant auth. It would appear that it just wouldn't be possible to change without creating a breaking change. There probably is a clearer way if we were green rooming it.

Also, the credential factories, which is central to CloudAdapter have tenantId as optional too. For example, CertificateServiceClientCredentialsFactory.