-
Notifications
You must be signed in to change notification settings - Fork 172
Remove need to provide client secret when creating workspace and remove Directory.Read.All Dependency from automation admin #4775
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
base: main
Are you sure you want to change the base?
Conversation
…robi/issue2247
Unit Test Results660 tests 660 ✅ 7s ⏱️ Results for commit d4ab72d. ♻️ 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 removes the requirement for users to provide a client secret when creating workspaces and eliminates the Directory.Read.All Microsoft Graph permission dependency from the automation admin identity. The changes introduce automatic workspace app provisioning/import via Terraform with built-in password rotation, simplify the API by removing the extract_workspace_auth_information function, and update all related documentation and scripts.
Key changes include:
- Terraform now provisions or imports the workspace Entra ID app automatically with dual password rotation using
azuread_application_passwordresources - API no longer requires Directory.Read.All permissions as workspace auth information is handled via Terraform outputs
- Major version bump for base workspace bundle (2.8.0 → 3.0.0) due to breaking changes
Reviewed changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/workspaces/base/terraform/workspace.tf | Adds import block for existing workspace apps and removes conditional AAD module creation |
| templates/workspaces/base/terraform/variables.tf | Removes register_aad_application and client_secret variables |
| templates/workspaces/base/terraform/providers.tf | Adds hashicorp/time provider for password rotation |
| templates/workspaces/base/terraform/outputs.tf | Simplifies outputs to always reference AAD module directly |
| templates/workspaces/base/terraform/keyvault.tf | Removes manual client_id and client_secret key vault secret resources |
| templates/workspaces/base/terraform/aad/variables.tf | Adds client_id variable, changes create_aad_groups type to bool |
| templates/workspaces/base/terraform/aad/providers.tf | Adds time provider requirement |
| templates/workspaces/base/terraform/aad/aad.tf | Implements dual password rotation with primary/secondary passwords and intelligent current password selection |
| templates/workspaces/base/terraform/.terraform.lock.hcl | Adds lock file entry for time provider v0.11.0 |
| templates/workspaces/base/template_schema.json | Removes client_secret from schema and moves create_aad_groups to top level |
| templates/workspaces/base/porter.yaml | Major version bump to 3.0.0, removes register_aad_application and client_secret parameters |
| api_app/services/authentication.py | Removes extract_auth_information function |
| api_app/services/access_service.py | Removes extract_workspace_auth_information abstract method |
| api_app/services/aad_authentication.py | Removes _get_app_auth_info and extract_workspace_auth_information implementation |
| api_app/db/repositories/workspaces.py | Removes auth_info parameter from create_workspace_item |
| api_app/api/routes/workspaces.py | Removes extract_auth_information call and auth_info parameter |
| api_app/_version.py | Minor version bump to 0.25.5 |
| api_app/tests_ma/test_services/test_aad_access_service.py | Removes tests for extract_workspace_auth_information |
| api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py | Updates test calls to remove auth_info parameter |
| api_app/tests_ma/test_api/test_routes/test_workspaces.py | Removes extract_auth_information mock patches |
| api_app/tests_ma/test_api/test_routes/test_workspace_users.py | Removes auth_info parameter from sample_workspace |
| docs/tre-developers/end-to-end-tests.md | Adds instructions for adding automation admin as workspace app owner |
| docs/tre-admins/setup-instructions/ui-install-base-workspace.md | Simplifies workspace app creation script usage |
| docs/tre-admins/setup-instructions/installing-base-workspace.md | Removes client_secret from workspace creation example |
| docs/tre-admins/setup-instructions/cicd-pre-deployment-steps.md | Removes TEST_WORKSPACE_APP_SECRET from required secrets |
| docs/tre-admins/identities/workspace.md | Removes client secret references and simplifies workspace app creation |
| docs/tre-admins/identities/application_admin.md | Updates required permissions from Directory.Read.All to Group.Read.All and User.ReadBasic.All |
| docs/tre-admins/environment-variables.md | Updates permission descriptions for auto workspace features |
| docs/tre-admins/auth.md | Updates permission descriptions and removes workspace_api_client_secret |
| devops/scripts/setup_local_debugging.sh | Removes TEST_WORKSPACE_APP_SECRET from environment setup |
| devops/scripts/create_aad_assets.sh | Removes Directory.Read.All from AUTO_WORKSPACE_APP_REGISTRATION permissions and removes automatic workspace app creation |
| devops/scripts/aad/wait_for_new_app_registration.sh | Minor cleanup removing echo statement |
| devops/scripts/aad/create_workspace_application.sh | Significantly simplified to only create minimal app registration without consent/permission setup |
| devops/scripts/aad/add_automation_admin_to_workspace_application.sh | New script for adding automation admin as workspace app owner |
| core/terraform/outputs.sh | Removes TEST_WORKSPACE_APP_SECRET from private.env |
| config_schema.json | Removes workspace_api_client_secret from schema |
| config.sample.yaml | Updates permission descriptions in comments |
Files not reviewed (1)
- templates/workspaces/base/terraform/.terraform.lock.hcl: Language not supported
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
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
Copilot reviewed 37 out of 38 changed files in this pull request and generated 6 comments.
Files not reviewed (1)
- templates/workspaces/base/terraform/.terraform.lock.hcl: Language not supported
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
… into marrobi/issue2247
|
/test |
|
🤖 pr-bot 🤖 🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/19742606600 (with refid (in response to this comment from @marrobi) |
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
Copilot reviewed 54 out of 55 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- templates/workspaces/base/terraform/.terraform.lock.hcl: Language not supported
|
/test-extended |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/19763777793 (with refid (in response to this comment from @marrobi) |
|
/test-extended cb9d51d |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/19768116977 (with refid (in response to this comment from @marrobi) |
|
/test-extended b6c60d1 |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/19768874181 (with refid (in response to this comment from @marrobi) |
|
/test-extended d4ab72d |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/19770486572 (with refid (in response to this comment from @marrobi) |
Uh oh!
There was an error while loading. Please reload this page.