feat: Feature authorization middleware

[ltwlf]

Description

This PR adds a claim validation middleware for secure bot to bot communication. Users should whitelist which consumers can call a composer skill bot. By default the feature is activated (secure by config) and adds AllowedCallers=["*"] to the appsettings.json. The middleware implementation is included in the runtime as C# file so that devs can customize the validation.

This implementation should be compatible with older bot versions. Middleware will not be activated when the feature flag in the setting is missing.

Task Item

closes #4084

[coveralls]

Coverage remained the same at 55.739% when pulling 6dc883c on ltwlf:authorization into 8ad58bd on microsoft:main.

[cwhitten]

Thank you for the contribution @ltwlf - @carlosscastro will review soon.

Are you able to add any tests with this change?

[ltwlf]

I will check tests when the draft is confirmed as an useful feature for composer.

[carlosscastro]

Thanks for your contribution! This is great and perfect timing. The change is very close to being in a mergeable state. Just 2 things that need to be addressed: 1) I added a comment about naming and the granularity of this feature. Please take a look. and 2) Tomorrow, we're having a discussion on how allowed claims lists will be configured in the UI. From that meeting, there might be some extra requirements around this. If that is the case, I'll update the Pr and we can discuss.

[carlosscastro]

Naming: Features should be higher level feature ideally. A more high level, user facing feature I'm thinking here would be is Is this a Skill? or Is this a Skill Host?. In that context, we could have 2 settings: IsSkill and IsSkillHost, and when IsSkill is true, then one of the consequences is that we enable this authorization claims validator.

I encourage you to provide feedback on this suggestion, would love to hear your thoughts on it.

[ltwlf]

Will check the feedback tomorrow in detail. The idea with the higher level feature make sense. As far as I know claims validation makes only sense for skills and not for consumers. As soon as you activate that the bot can be consumed as a skill the claim validation should be activated and the skill manifest should become part of the composer project (not sure how this is handled) currently. Let’s see what the others say.

[ltwlf]

@carlosscastro do you have feedback from the meeting? Would like to work on this at the weekend.
And do you have any hint for me where is the best place to integrate the feature on a higher level?

[carlosscastro]

Hello @ltwlf! After the meeting, the direction we discussed looks good. Some more details below:

• Addition of a new IsSkill setting
• If IsSkill is set to false, in startup.cs we do not register the bot as a skill here: https://github.com/microsoft/BotFramework-Composer/blob/main/runtime/dotnet/azurewebapp/Startup.cs#L141 and in line 143. Line 142 is about being a skill host which is a different story and we'll likely hide it behind other settings in a near future but you don't need to worry about it.
• If IsSkill is set to true, in startup.cs we register the bot as a skill https://github.com/microsoft/BotFramework-Composer/blob/main/runtime/dotnet/azurewebapp/Startup.cs#L141 and in line 143
• If IsSkill is set to true, we configure your claim validator.
• The AllowedCallers list is empty by default, as opposed to '*'. We'll add some Ui and docs around this in order to improve the experience and avoid frustration. However the current * configuration is allowing a lot of access skills and bot developers may not be aware which we think is not ideal.

Let me know if any of that doesn't make sense, happy to discuss. I'll be around over the weekend so just tag me if you have questions!

[ltwlf]

@carlosscastro here is my new draft for skill authorization. I've created an own class for skill settings, because there may will be more skill specific settings. I create default settings like this

skill: {
  isSkill:false,
  allowedCallers:['*']
} 

is skill could then either be configured via settings.json or UI

Looking forward to your feedback.

Best wishes,
Christian

[carlosscastro]

Looking good. Last details on my comments: 1) copyright header missing, 2) possible null ref exception in startup.cs and 3) potential improvement in registration (which may not have a happy solution, just a suggestion). Once we have these, we should be ready to go.

Separately, FYI we will create items to port this to the (in progress) js runtime and azure functions. But that can be done by other contributors, or by you in a separate PR -- your call. Definitely let's focus on this in this PR, since it's almost ready to go. Thanks again for iterating on this.

[carlosscastro]

In general we want to move away from doing GetService in startup.cs. If we could optionally do a services.AddSingleton<AuthenticationConfiguration> and have DI wire it into the adapter's constructor , and also do services.AddSingleton<ICredentialProvider>(new ConfigurationCredentialProvider...), then we could avoid the getService. Not 100% sure that would work because of the numerous overloads of BotFrameworkHttpAdapter constructor. If that doesn't work, then as-is is good. We are currently refactoring the adapter and will soon refactor this startup.cs, so we should clean this up soon.

[ltwlf]

I couldn't find a way to inject the Authorization. That's why I used GetService.

[carlosscastro]

settings could be null, right?

[carlosscastro]

If yes, we cannot throw NullRef

[ltwlf]

IsSkill returns false when null:

        public bool IsSkill(BotSettings settings)
        {
            return settings?.Skill?.IsSkill == true;
        }

So it should be fine

[ltwlf]

@carlosscastro I changed from PR draft and added some unit tests. Looking forward to your feedback,
After this is done, I'm glad to support the other runtimes. Can you please mention or assign me in this issues.

[carlosscastro]

Minor, optional: Ideally we strive for either xml docs (/// format) or no docs.

[ltwlf]

Hi, can I do anything to finalize this PR? When this PR is merged, I would like to the same for Azure Funcs and then for JS...
@carlosscastro @cwhitten

[cwhitten]

@ltwlf for some reason the unit tests are failing so I've re-run CI. We need that to pass before we can merge.

[cwhitten]

Summary of all failing tests
FAIL packages/server/src/controllers/tests/project.test.ts (8.628 s)
● skill operation › should retrieve skill manifest

expect(jest.fn()).toHaveBeenCalledWith(...expected)

Expected: 200
Received: 404

Number of calls: 1

  339 |     } as Request;
  340 |     await ProjectController.getSkill(mockReq, mockRes);
> 341 |     expect(mockRes.status).toHaveBeenCalledWith(200);
      |                            ^
  342 |   }, 10000);
  343 | });
  344 | 

  at Object.toHaveBeenCalledWith (src/controllers/__tests__/project.test.ts:341:28)
      at runMicrotasks (<anonymous>)

Test Suites: 1 failed, 217 passed, 218 total
Tests: 1 failed, 3 skipped, 1 todo, 840 passed, 845 total
Snapshots: 5 passed, 5 total
Time: 410.851 s
Ran all test suites in 13 projects.
Force exiting Jest: Have you considered using --detectOpenHandles to detect async operations that kept running after all tests finished?
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Error: Process completed with exit code 1.

[cwhitten]

@ltwlf The test failure is unrelated to this change. We'll merge this and address the failure in a follow-up PR