-
Notifications
You must be signed in to change notification settings - Fork 172
Use Group.Create instead of Group.ReadWrite.All for group creation #4774
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
Conversation
Unit Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 0ee7c65. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR reduces the scope of Microsoft Entra ID permissions required for workspace group creation by replacing the overly broad Group.ReadWrite.All permission with the more restrictive Group.Create permission. To maintain management capabilities, the Application Admin (via data.azuread_client_config.current.object_id) is added as an owner on all created groups.
Key changes:
- Updated Microsoft Graph permission from
Group.ReadWrite.AlltoGroup.Createacross documentation and deployment scripts - Added Application Admin as an owner to workspace security groups (owners, researchers, airlock managers)
- Bumped workspace bundle version from 2.7.1 to 2.8.0 following semantic versioning for MINOR functionality changes
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/workspaces/base/terraform/aad/aad.tf | Added Application Admin as owner to three workspace security groups to enable management with reduced permissions |
| templates/workspaces/base/porter.yaml | Incremented version from 2.7.1 to 2.8.0 for the permission change |
| docs/tre-admins/identities/application_admin.md | Updated documentation to reflect Group.Create permission instead of Group.ReadWrite.All |
| docs/tre-admins/environment-variables.md | Updated AUTO_WORKSPACE_GROUP_CREATION documentation and removed trailing blank line |
| devops/scripts/create_aad_assets.sh | Changed script to request Group.Create permission instead of Group.ReadWrite.All |
| config.sample.yaml | Updated comment for AUTO_WORKSPACE_GROUP_CREATION and removed trailing blank line |
| CHANGELOG.md | Added changelog entry for the permission change |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
/test-extended |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/19708120869 (with refid (in response to this comment from @marrobi) |
JC-wk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
/test-force-approve Passed: #4774 (comment) |
|
🤖 pr-bot 🤖 ✅ Marking tests as complete (for commit 0ee7c65) (in response to this comment from @marrobi) |
Resolves #4772
What is being addressed
AzureTRE currently grants the Group.ReadWrite.All permission to its Application Admin for group creation, which is broader than necessary. The update aligns permissions with the actual needs by utilizing the Group.Create permission instead.
How is this addressed
-Made Application Admin an owner on the group so it can manage them.