Skip to content

Fix checkOwner function lockup when invalid M365 groups are referenced - #147

Closed
Gunjan Datta (gudatta) with Copilot wants to merge 1 commit into
mainfrom
copilot/fix-73
Closed

Fix checkOwner function lockup when invalid M365 groups are referenced#147
Gunjan Datta (gudatta) with Copilot wants to merge 1 commit into
mainfrom
copilot/fix-73

Conversation

Copilot AI commented Aug 19, 2025

Copy link
Copy Markdown
Contributor

The checkOwner function in src/ds.ts would cause the application to lock up and become unresponsive when invalid M365 groups were referenced in SharePoint site permissions. This issue occurred because:

  1. Invalid group IDs: The getGroupId() method returns null for malformed login names that don't contain valid GUIDs
  2. Unfiltered nulls: These null values were being passed directly to Microsoft Graph API batch requests
  3. Missing error handling: When the batch requests failed due to invalid group IDs, there was no error callback to handle the failure
  4. Promise hanging: The promise would never resolve, causing the entire application to become unresponsive

Changes Made

Primary Fixes in src/ds.ts

  • checkOwner method: Added null filtering and error handling for batch requests
  • checkReadOnlySite method: Applied the same fixes for consistency

Additional Fix in src/reports/permissions.ts

  • analyzeGroupIds method: Added error handling to the batch execution

Code Example

Before (problematic):

groupIds.push(this.getGroupId(item.LoginName)); // Could push null
// ...
ds.execute(() => {
    resolve(isSiteAdmin);
}); // No error handling - would hang on failure

After (fixed):

let groupId = this.getGroupId(item.LoginName);
if (groupId) {  // Only add valid group IDs
    groupIds.push(groupId);
}
// ...
ds.execute(() => {
    resolve(isSiteAdmin);
}, () => {
    // Handle batch execution errors gracefully
    resolve(false);
});

Impact

  • Prevents lockup: Application no longer hangs when encountering invalid M365 groups
  • Graceful degradation: Returns false (user is not an admin/owner) instead of crashing
  • Backward compatible: Existing valid groups continue to work normally
  • Minimal changes: Only 24 lines modified across 2 files with surgical precision

The fix ensures robust handling of edge cases while maintaining all existing functionality.

Fixes #73.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI commented Aug 19, 2025

Copy link
Copy Markdown
Contributor Author

Gunjan Datta (@gudatta) 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs.

I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review.

@gudatta

Copy link
Copy Markdown
Contributor

Closing request.

@gudatta
Gunjan Datta (gudatta) deleted the copilot/fix-73 branch August 19, 2025 15:26
Copilot AI restored the copilot/fix-73 branch August 19, 2025 15:26
Copilot AI changed the title [WIP] Check Owner Function Failing Fix checkOwner function lockup when invalid M365 groups are referenced Aug 19, 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.

Check Owner Function Failing

2 participants