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

Adding externalAppCardActionsForDA capability #2752

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

Conversation

lakhveerk
Copy link
Contributor

For more information about how to contribute to this repo, visit this page.

Description

Added externalAppCardActionsForDA capability with actionOpenUrlDialog function that is designed for bizchat to call. Added UTs, E2e tests, and test app related changes for this capability.

Validation

Validation performed:

  1. <Step 1>
  2. <Step 2>

Unit Tests added:

Unit tests are required for all changes. If no unit tests were added as part of this change, please explain why they aren't necessary.
Yes

End-to-end tests added:

Yes,
This PR have the test cases for isSupported functions. The rest of the e2e tests will be added to host sdk

Additional Requirements

Change file added:

Ensure the change file meets the formatting requirements.

Yes

@lakhveerk lakhveerk requested a review from a team as a code owner March 17, 2025 23:04
Copy link
Contributor

This pull request contains changes to the runtime.ts file. If you, as the author of this PR, have made changes to the Runtime interface please review RUNTIME.md to determine if a new runtime version is required. Please reply to this comment stating what changes, if any, were made to the runtime object and whether a new runtime version was required.

Copy link
Contributor

github-actions bot commented Mar 17, 2025

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/teams-js/dist/esm/packages/teams-js/src/index.js 182.37 KB (+0.61% 🔺) 3.7 s (+0.61% 🔺) 148 ms (-5.35% 🔽) 3.8 s

@lakhveerk
Copy link
Contributor Author

This pull request contains changes to the runtime.ts file. If you, as the author of this PR, have made changes to the Runtime interface please review RUNTIME.md to determine if a new runtime version is required. Please reply to this comment stating what changes, if any, were made to the runtime object and whether a new runtime version was required.

Added a new capability to run time. A new runtime is not needed because it does not edit any of the existing capabilities.

ndangudubiyyam
ndangudubiyyam previously approved these changes Mar 21, 2025
* @throws Error with a message describing the exact validation violation
*/
public constructor(private readonly idAsString: string) {
validateSafeContent(idAsString);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't ValidatedStringId validate the string length too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During my output-validation conversation with security and Teams, it came up that min/max is an old recommendation that stems from C/C++ code where developers had to deal with fixed memory space, reserved in advance, to store property values. And thus, they had to make sure that an incoming property value would not exceed the length they reserved memory for, otherwise that could cause a buffer overrun which used to be a massive attack vector in Windows prior to XP Service Pack 2.
Security was ok to not have the min max length requirement. For that reason, I created a new class that just checks for Opaque and script tags.

Your comment made me thought that we can have a uuid instead. I will check with 3pcxp and will respond back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ndangudubiyyam Niharika, I am going to update 3pcxp. If you agree with this decision to use UUID, can you update hubsdk PR please? I will report here what partners response, but you can keep track of the convo in Teams channel.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure it's really a UUID! We always get tripped up when it's something that looks like a UUID but doesn't actually confirm to the UUID spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have a class UUID that validate that the id is uuid type. I am planning to use that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jekloudaMSFT if we do go with UUID, do you want me to delete the validatedStringId class? OR is it still beneficial to have it around for future use?

MengyiGong
MengyiGong previously approved these changes Mar 21, 2025
@lakhveerk lakhveerk dismissed stale reviews from MengyiGong and ndangudubiyyam via 1f30026 March 21, 2025 20:36
jekloudaMSFT
jekloudaMSFT previously approved these changes Mar 21, 2025
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.

4 participants