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

Enhance ms teams functionalities part1 #37268

Open
wants to merge 48 commits into
base: master
Choose a base branch
from

Conversation

ShacharKidor
Copy link
Contributor

@ShacharKidor ShacharKidor commented Nov 18, 2024

Contributing to Cortex XSOAR Content

Make sure to register your contribution by filling the contribution registration form

The Pull Request will be reviewed only after the contribution registration form is filled.

Status

  • In Progress
  • Ready
  • In Hold - (Reason for hold)

Related Issues

relates: CIAC-11459

Description

  1. Added the'microsoft-teams-token-permissions-list'command which retrieves the API permissions list of the graph access token.
  2. Added the 'microsoft-teams-create-messaging-endpoint' command which generates the correct messaging endpoint, based on the server URL and the instance configurations.

Must have

  • Tests
  • Documentation

Copy link

github-actions bot commented Nov 18, 2024

Coverage

Coverage Report
FileStmtsMissCoverMissing
Packs/MicrosoftTeams/Integrations/MicrosoftTeams
   MicrosoftTeams.py136936773%112, 118, 182, 184, 206–207, 237, 246, 248, 276–277, 279–282, 284–287, 291, 309, 313, 321–322, 570–571, 602, 647–651, 653–654, 681–682, 695–696, 703–707, 725, 741–746, 748–749, 751–752, 762–763, 779–780, 818–819, 832–835, 837–839, 841–843, 845, 854–855, 859–860, 888, 904–911, 913–917, 919–921, 923–925, 928–931, 933, 935–952, 954, 956–957, 960–963, 965, 967–968, 970–973, 975–976, 978, 983, 985–989, 991–993, 1088–1091, 1099–1102, 1104–1106, 1108–1111, 1113, 1115, 1254, 1256, 1374–1375, 1438, 1510, 1513–1515, 1517–1518, 1520–1521, 1523, 1542, 1547, 1549, 1820–1821, 1906–1909, 2031, 2101, 2110, 2165–2166, 2173, 2192–2193, 2196, 2275–2282, 2284–2290, 2295–2296, 2298–2307, 2311–2314, 2316, 2346, 2354, 2369, 2393–2394, 2397, 2454, 2458, 2476, 2493–2495, 2514, 2560–2561, 2569–2572, 2574–2575, 2577–2583, 2585–2587, 2589–2592, 2594, 2597–2598, 2600, 2602–2605, 2607, 2609–2616, 2618–2627, 2631, 2644–2645, 2648–2652, 2660–2663, 2665, 2700, 2702, 2723–2725, 2727–2728, 2730, 2732–2733, 2735–2736, 2738–2740, 2742, 2744–2748, 2750–2753, 2755–2757, 2759, 2761, 2763–2773, 2775–2781, 2847, 2860, 2901–2904, 2906–2907, 2910, 2918–2919, 2930–2933, 2937–2938, 2944, 2966
TOTAL136936773% 

Tests Skipped Failures Errors Time
113 0 💤 0 ❌ 0 🔥 3.358s ⏱️

Copy link
Contributor

@michal-dagan michal-dagan left a comment

Choose a reason for hiding this comment

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

Great work!!

Copy link
Contributor

@merit-maita merit-maita left a comment

Choose a reason for hiding this comment

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

great work!

Copy link
Contributor

@jbabazadeh jbabazadeh left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -722,8 +723,11 @@ def get_graph_access_token() -> str:
tenant_id = integration_context.get('tenant_id')
if not tenant_id:
raise ValueError(
'Did not receive tenant ID from Microsoft Teams, verify the messaging endpoint is configured correctly. '
'See https://xsoar.pan.dev/docs/reference/integrations/microsoft-teams#troubleshooting for more information'
'Did not receive a tenant ID from Microsoft Teams. Verify that the messaging endpoint in the Demisto bot '
Copy link
Contributor

Choose a reason for hiding this comment

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

i believe we're trying to avoid using the "Demisto" word. consult with @nkanon

@@ -2646,8 +2650,11 @@ def ring_user():
tenant_id: str = integration_context.get('tenant_id', '')
if not tenant_id:
raise ValueError(
'Did not receive tenant ID from Microsoft Teams, verify the messaging endpoint is configured correctly. '
'See https://xsoar.pan.dev/docs/reference/integrations/microsoft-teams#troubleshooting for more information'
'Did not receive a tenant ID from Microsoft Teams. Verify that the messaging endpoint in the Demisto bot '
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicate code, make it a constant.

Comment on lines +2810 to +2812
roles = sorted(roles)
hr = tableToMarkdown(f'The current API permissions in the Teams application are: ({len(roles)})',
roles, headers=['Permission'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
roles = sorted(roles)
hr = tableToMarkdown(f'The current API permissions in the Teams application are: ({len(roles)})',
roles, headers=['Permission'])
hr = tableToMarkdown(f'The current API permissions in the Teams application are: ({len(roles)})',
sorted(roles), headers=['Permission'])

no need for transitive assignment.

else:
hr = 'Graph access token is not set.'

demisto.debug(f"'microsoft-teams-token-permissions-list' command result is: {hr}")
Copy link
Contributor

Choose a reason for hiding this comment

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

also print the AUTH_TYPE

messaging_endpoint = urljoin(urljoin(xsoar_url, 'instance/execute'), instance_name)

else: # XSIAM or XSOAR SAAS
# Add the 'ext-' prefix to the xsoar url
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a link to the CRTX ticket to platform, as to why we do this substitutions

# Replace the '.xdr-' with '.crtx-' for XSIAM tenants
messaging_endpoint = messaging_endpoint.replace('.xdr-', '.crtx-', 1)

hr = f"The messaging endpoint is:\n `{messaging_endpoint}`\n\n The messaging endpoint should be added to the Demisto bot"\
Copy link
Contributor

Choose a reason for hiding this comment

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

same here for Demisto keyword.

'Client Credentials',
'Graph access token is not set.')
])
def test_token_permissions_list_command(mocker, token, decoded_token, auth_type, expected_hr):
Copy link
Contributor

Choose a reason for hiding this comment

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

consider adding ids to the tests.

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

Successfully merging this pull request may close these issues.

6 participants