Skip to content

Hosting changes: refactoring & support no-auth setup scenario#384

Open
singhk97 wants to merge 4 commits intonext/corefrom
next/core-token-validation-fix
Open

Hosting changes: refactoring & support no-auth setup scenario#384
singhk97 wants to merge 4 commits intonext/corefrom
next/core-token-validation-fix

Conversation

@singhk97
Copy link
Collaborator

  1. Fix missing RequireAuthorization in CompatBot sample.
  2. Refactor service collection setup.
  3. Use BotConfig as single source for reading & propagating configuration instead of doing that all over.
  4. Move Msal setup logic to separate class
  5. Support scenario where auth is not provided. Warning logs will be emitted. Clients will be created without auth. Incomming token validation will use a bypass scheme.

Logs when setting up auth using the different config formats

No credentials setup

image

Msal Config setup

image

Core config setup

image

BF config setup

image

@singhk97 singhk97 added the CORE label Mar 20, 2026
@rido-min
Copy link
Member

from the screenshots, seems we are duplicating the same Warning multiple times. Can we review those logs to have a single entry?

Copy link
Member

@rido-min rido-min left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd like to validate in a real environment with UMI and FIC before merging

return services.AddUserTokenClient(botConfig);
}

/// <summary>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we are not adding XML comments to private members

TenantId = section["TenantId"] ?? string.Empty,
ClientId = section["ClientId"] ?? string.Empty,
ClientSecret = section["ClientSecret"],
Scope = section["Scope"] ?? configuration["Scope"] ?? BotScope,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to clarify where to read the Scope from, seems we are reading from the root, and from the section

/// <summary>
/// Configures MSAL with User-Assigned Managed Identity (UMI) authentication.
/// </summary>
private static IServiceCollection ConfigureMSALWithUMI(this IServiceCollection services, string tenantId, string clientId, string? managedIdentityClientId = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UMI is not easy to validate, do you need any help to configure and validate?

@singhk97
Copy link
Collaborator Author

singhk97 commented Mar 20, 2026

from the screenshots, seems we are duplicating the same Warning multiple times. Can we review those logs to have a single entry?

yeah, i will do another pass of refactoring to avoid redundant log statements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants