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

Create groups for AAD app reg roles #2532

Merged
merged 18 commits into from
Sep 14, 2022
Merged

Conversation

marrobi
Copy link
Member

@marrobi marrobi commented Aug 30, 2022

Resolves #2480

What is being addressed

  • Adds groups to AAD terraform
  • Set workspace owner as owner of the groups.
  • Adds boolean to create groups to porter.yaml
  • Updates aad schema to provide group boolean.
  • Add environment variable to enable the feature
  • Update app reg scripts to grant required permissions to app registration
  • Update changelog

@github-actions
Copy link

github-actions bot commented Aug 30, 2022

Unit Test Results

503 tests  +486   501 ✔️ +484   15s ⏱️ +8s
    1 suites ±    0       2 💤 +    2 
    1 files   ±    0       0 ±    0 

Results for commit c890c1e. ± Comparison against base commit 03a3aab.

This pull request removes 17 and adds 503 tests. Note that renamed tests count towards both.
test_provisioned_health_api ‑ test_health
test_shared_service_templates ‑ test_get_shared_service_template[tre-shared-service-firewall]
test_shared_service_templates ‑ test_get_shared_service_template[tre-shared-service-gitea]
test_shared_service_templates ‑ test_get_shared_service_templates[tre-shared-service-firewall]
test_shared_service_templates ‑ test_get_shared_service_templates[tre-shared-service-gitea]
test_ui ‑ test_ui
test_workspace_service_templates ‑ test_create_workspace_service_templates
test_workspace_service_templates ‑ test_get_workspace_service_template[tre-service-azureml]
test_workspace_service_templates ‑ test_get_workspace_service_template[tre-service-guacamole]
test_workspace_service_templates ‑ test_get_workspace_service_template[tre-service-innereye]
…
tests_ma.test_api.test_errors.test_422_error ‑ test_frw_validation_error_format
tests_ma.test_api.test_errors.test_error ‑ test_frw_validation_error_format
tests_ma.test_api.test_routes.test_airlock.TestAirlockRoutesThatRequireAirlockManagerRights ‑ test_post_create_airlock_review_approves_airlock_request_returns_200
tests_ma.test_api.test_routes.test_airlock.TestAirlockRoutesThatRequireAirlockManagerRights ‑ test_post_create_airlock_review_input_is_malformed_returns_400
tests_ma.test_api.test_routes.test_airlock.TestAirlockRoutesThatRequireAirlockManagerRights ‑ test_post_create_airlock_review_with_event_grid_not_responding_returns_503
tests_ma.test_api.test_routes.test_airlock.TestAirlockRoutesThatRequireAirlockManagerRights ‑ test_post_create_airlock_review_with_illegal_status_change_returns_400
tests_ma.test_api.test_routes.test_airlock.TestAirlockRoutesThatRequireOwnerOrResearcherRights ‑ test_get_airlock_container_link_cancelled_request_returns_400
tests_ma.test_api.test_routes.test_airlock.TestAirlockRoutesThatRequireOwnerOrResearcherRights ‑ test_get_airlock_container_link_in_progress_request_returns_400
tests_ma.test_api.test_routes.test_airlock.TestAirlockRoutesThatRequireOwnerOrResearcherRights ‑ test_get_airlock_container_link_no_airlock_request_found_returns_404
tests_ma.test_api.test_routes.test_airlock.TestAirlockRoutesThatRequireOwnerOrResearcherRights ‑ test_get_airlock_container_link_no_workspace_request_found_returns_404
…

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@tamirkamara tamirkamara left a comment

Choose a reason for hiding this comment

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

What about adding the groups to their respective roles in the workspace app?

api_app/schemas/azuread.json Outdated Show resolved Hide resolved
templates/workspaces/base/terraform/aad/aad.tf Outdated Show resolved Hide resolved
@marrobi marrobi requested a review from tamirkamara September 2, 2022 15:09
@marrobi
Copy link
Member Author

marrobi commented Sep 2, 2022

@tamirkamara became a bit more involved than I expected. Appreciate feedback, and if someone has time to test it. I have and got desired result:

image

@marrobi
Copy link
Member Author

marrobi commented Sep 2, 2022

Might need to output the group IDs too, then can link to a URI such as:

https://account.activedirectory.windowsazure.com/r#/manageMembership?objectType=Group&objectId=xxxx

To manage group members.

@marrobi marrobi marked this pull request as ready for review September 2, 2022 15:34
@marrobi marrobi self-assigned this Sep 5, 2022
Copy link
Collaborator

@tamirkamara tamirkamara left a comment

Choose a reason for hiding this comment

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

Looks good with a couple of small comments.

api_app/schemas/azuread.json Outdated Show resolved Hide resolved
templates/workspaces/base/porter.yaml Show resolved Hide resolved
templates/workspaces/base/template_schema.json Outdated Show resolved Hide resolved
@marrobi
Copy link
Member Author

marrobi commented Sep 6, 2022

/test

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/2999673697 (with refid 9f314729)

(in response to this comment from @marrobi)

@marrobi marrobi enabled auto-merge (squash) September 6, 2022 11:12
@marrobi
Copy link
Member Author

marrobi commented Sep 7, 2022

/test

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/3007192495 (with refid 9f314729)

(in response to this comment from @marrobi)

@marrobi
Copy link
Member Author

marrobi commented Sep 7, 2022

/test

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/3009145901 (with refid 9f314729)

(in response to this comment from @marrobi)

@marrobi
Copy link
Member Author

marrobi commented Sep 7, 2022

/test-destroy-env

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

Destroying PR test environment (RG: rg-tre9f314729)... (run: https://github.com/microsoft/AzureTRE/actions/runs/3009327735)

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

PR test environment destroy complete (RG: rg-tre9f314729)

@marrobi
Copy link
Member Author

marrobi commented Sep 7, 2022

/test

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/3009450629 (with refid 9f314729)

(in response to this comment from @marrobi)

@marrobi
Copy link
Member Author

marrobi commented Sep 8, 2022

/test

@github-actions
Copy link

github-actions bot commented Sep 8, 2022

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/3013623377 (with refid 9f314729)

(in response to this comment from @marrobi)

@marrobi
Copy link
Member Author

marrobi commented Sep 9, 2022

/test

@github-actions
Copy link

github-actions bot commented Sep 9, 2022

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/3021309456 (with refid 9f314729)

(in response to this comment from @marrobi)

@marrobi
Copy link
Member Author

marrobi commented Sep 13, 2022

/test

@github-actions
Copy link

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/3047996550 (with refid 9f314729)

(in response to this comment from @marrobi)

@tamirkamara
Copy link
Collaborator

/test

@github-actions
Copy link

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/3050648354 (with refid 9f314729)

(in response to this comment from @tamirkamara)

@marrobi
Copy link
Member Author

marrobi commented Sep 14, 2022

@github-actions
Copy link

🤖 pr-bot 🤖

✅ Marking tests as complete (for commit c890c1e)

(in response to this comment from @marrobi)

@marrobi marrobi merged commit caf6a9c into microsoft:main Sep 14, 2022
@marrobi marrobi deleted the marrobi/issue2480 branch September 14, 2022 16:03
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.

When auto creating app registrations, create a user group for each role and assign to the app role
2 participants