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

Adds application permission check on certain command groups / specific commands #5862

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

MathijsVerbeeck
Copy link
Contributor

@MathijsVerbeeck MathijsVerbeeck commented Feb 17, 2024

Closes #4054

Quite a big one, so a little summary below :-)

Command groups:

  • Power Automate
  • Power Platform
  • Power Apps
  • Power BI
  • To Do
  • Yammer

Specific commands

  • outlook message move
  • teams chat send

@milanholemans
Copy link
Contributor

Thank you, we'll try to review it ASAP

@milanholemans
Copy link
Contributor

Could you fix the merge conflicts, please?

@milanholemans milanholemans marked this pull request as draft February 22, 2024 21:28
@MathijsVerbeeck MathijsVerbeeck marked this pull request as ready for review February 23, 2024 10:10
@milanholemans
Copy link
Contributor

@MathijsVerbeeck could you fix the merge conflicts again?

@milanholemans milanholemans marked this pull request as draft March 8, 2024 23:45
@MathijsVerbeeck MathijsVerbeeck marked this pull request as ready for review March 9, 2024 09:27
@milanholemans milanholemans self-assigned this Apr 6, 2024
@milanholemans
Copy link
Contributor

Could you resolve the merge conflicts @MathijsVerbeeck?

@milanholemans milanholemans marked this pull request as draft May 6, 2024 21:34
@MathijsVerbeeck MathijsVerbeeck marked this pull request as ready for review May 9, 2024 20:15
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Great start @MathijsVerbeeck, made a few comments while reviewing. Could you have a look at them, please?
A few comments apply to all tests.

src/utils/accessToken.ts Outdated Show resolved Hide resolved
src/utils/accessToken.ts Outdated Show resolved Hide resolved
src/utils/accessToken.spec.ts Show resolved Hide resolved
src/utils/accessToken.spec.ts Outdated Show resolved Hide resolved
src/utils/accessToken.ts Outdated Show resolved Hide resolved
src/m365/base/ToDoCommand.spec.ts Outdated Show resolved Hide resolved
src/m365/base/ToDoCommand.ts Outdated Show resolved Hide resolved
src/m365/base/VivaEngageCommand.spec.ts Show resolved Hide resolved
src/m365/base/PowerBICommand.spec.ts Show resolved Hide resolved
src/m365/base/PowerPlatformCommand.ts Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft May 14, 2024 21:27
@MathijsVerbeeck MathijsVerbeeck marked this pull request as ready for review May 16, 2024 15:55
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Let's do some more changes. It seems like you missed some of my previous comments, or they were not clear enough. If the latter, sorry for that, you can always ask more context.
Another thing, currently we have a lot of files (pa, pp, flow, ...) where we stub the wrong function. Right now we do:

sinon.stub(accessToken, 'isAppOnlyAccessToken').returns(false);
    auth.connection.accessTokens[auth.defaultResource] = {
      expiresOn: 'abc',
      accessToken: 'abc'
    };

While in fact, we should just stub the assertDelegatedAccessToken function.

src/utils/accessToken.ts Outdated Show resolved Hide resolved
src/utils/accessToken.ts Outdated Show resolved Hide resolved
src/utils/accessToken.ts Outdated Show resolved Hide resolved
src/utils/accessToken.spec.ts Outdated Show resolved Hide resolved
src/utils/accessToken.spec.ts Outdated Show resolved Hide resolved
src/m365/base/PowerBICommand.spec.ts Show resolved Hide resolved
src/m365/base/PowerPlatformCommand.spec.ts Show resolved Hide resolved
src/m365/base/VivaEngageCommand.spec.ts Show resolved Hide resolved
src/m365/base/VivaEngageCommand.spec.ts Show resolved Hide resolved
src/m365/base/PowerBICommand.spec.ts Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft May 17, 2024 23:18
@MathijsVerbeeck
Copy link
Contributor Author

Let's do some more changes. It seems like you missed some of my previous comments, or they were not clear enough. If the latter, sorry for that, you can always ask more context. Another thing, currently we have a lot of files (pa, pp, flow, ...) where we stub the wrong function. Right now we do:

Sorry for the inconvenience... I had some merge issues on my laptop and decided, instead of fixing them, to work on a clean branch on my desktop, which is why I must've messed up some of the comments you had already given.

@MathijsVerbeeck MathijsVerbeeck marked this pull request as ready for review May 18, 2024 06:28
src/utils/accessToken.ts Outdated Show resolved Hide resolved
src/utils/accessToken.ts Outdated Show resolved Hide resolved
src/utils/accessToken.ts Outdated Show resolved Hide resolved
src/utils/accessToken.ts Outdated Show resolved Hide resolved
src/utils/accessToken.spec.ts Outdated Show resolved Hide resolved
src/m365/pp/commands/gateway/gateway-get.spec.ts Outdated Show resolved Hide resolved
src/m365/base/PowerAppsCommand.spec.ts Outdated Show resolved Hide resolved
@@ -39,37 +49,39 @@ describe('PowerAppsCommand', () => {
});

it(`doesn't throw error when not connected`, () => {
auth.connection.active = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's undo this.

src/m365/base/DelegatedGraphCommand.ts Outdated Show resolved Hide resolved
src/m365/base/PowerAppsCommand.spec.ts Show resolved Hide resolved
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

We're getting there, let's do a few more changes to get it on point.

src/m365/base/PowerAutomateCommand.spec.ts Outdated Show resolved Hide resolved
src/m365/base/PowerPlatformCommand.spec.ts Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft May 18, 2024 22:04
@MathijsVerbeeck MathijsVerbeeck marked this pull request as ready for review May 19, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Application permission check on certain commands
3 participants