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

Add AzureAD/EntraID functionality #1778

Merged

Conversation

jemrobinson
Copy link
Member

@jemrobinson jemrobinson commented Apr 10, 2024

✅ Checklist

  • You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
  • You are targeting the appropriate branch. If you're not certain which one this is, it should be develop.
  • Your branch is up-to-date with the target branch (it probably was when you started, but it may have changed since then).
  • You have marked this pull request as a draft and added '[WIP]' to the title if needed (if you're not yet ready to merge).
  • You have formatted your code using appropriate automated tools (for example ./tests/AutoFormat_Powershell.ps1 -TargetPath <path to file or directory> for Powershell).

⤴️ Summary

  • Add necessary AzureAD/EntraID functionality for managing users and groups directly in the cloud
  • Add remove, register and unregister methods to AzureADUsers
  • Fix default GraphAPI scopes for SRE deployment
  • Add function to create a service principal for AzureAD applications
  • Update create_group function to no longer require gidNumber
  • Drop support for external linux_schema in AzureAD
  • Add functions to grant approval for application/delegated permissions
  • Add remove_user function to GraphAPI
  • Only remove_user_from_group if the user belongs to the group
  • Grant requested permissions to applications during refresh, in case these weren't done at creation time

🌂 Related issues

First step towards #1570

🔬 Tests

Tested on a new deployment

@jemrobinson jemrobinson force-pushed the 1570-add-azuread-features branch 6 times, most recently from a091222 to 05d192a Compare April 10, 2024 12:26
@jemrobinson jemrobinson changed the title [WIP] Add AzureAD/EntraID functionality Add AzureAD/EntraID functionality Apr 10, 2024
@jemrobinson jemrobinson marked this pull request as ready for review April 10, 2024 13:03
@jemrobinson jemrobinson requested a review from a team April 10, 2024 13:04
@martintoreilly
Copy link
Member

👀 Does this remove the "local" SHM AD servers and the SCE NPS servers?

@jemrobinson
Copy link
Member Author

👀 Does this remove the "local" SHM AD servers and the SCE NPS servers?

  • This PR just adds some necessary functionality to our GraphAPI/AzureAPI wrappers.
  • Use Apricot for authentication/identity #1772 adds a new SRE identity server that gets its users directly from AzureAD/EntraID
  • TBD next PR will remove the SHM AD servers

We've already dropped the NPS servers in 4.2.0 as part of dropping support for Microsoft Remote Desktop

Copy link
Member

@JimMadge JimMadge left a comment

Choose a reason for hiding this comment

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

A few suggestions and questions.

I really like using dynamic components to keep things "in Pulumi" where we can.

I think I struggled a bit with the Graph API stuff because it feels quite abstract.
Regarding the user management commands, do we want CLI commands for convenience (avoid using a browser) or because it automates some quirks or extra steps DSH requires?

data_safe_haven/administration/users/azure_ad_users.py Outdated Show resolved Hide resolved
data_safe_haven/external/api/graph_api.py Outdated Show resolved Hide resolved
data_safe_haven/external/api/graph_api.py Outdated Show resolved Hide resolved
data_safe_haven/external/api/graph_api.py Outdated Show resolved Hide resolved
data_safe_haven/external/api/graph_api.py Show resolved Hide resolved
data_safe_haven/external/api/graph_api.py Outdated Show resolved Hide resolved
data_safe_haven/external/api/graph_api.py Show resolved Hide resolved
data_safe_haven/external/api/graph_api.py Outdated Show resolved Hide resolved
@jemrobinson
Copy link
Member Author

Regarding the user management commands, do we want CLI commands for convenience (avoid using a browser) or because it automates some quirks or extra steps DSH requires?

I guess initially because I want to maintain our current setup of adding/removing users and groups using the CLI. We can definitely remove this in future, but I think it should be a conscious decision, not a by-product of another large change.

Co-authored-by: Jim Madge <jim+github@jmadge.com>
@jemrobinson jemrobinson force-pushed the 1570-add-azuread-features branch from 8ac5650 to f627151 Compare April 11, 2024 13:24
@jemrobinson jemrobinson requested a review from JimMadge April 12, 2024 08:43
Copy link
Member

@JimMadge JimMadge left a comment

Choose a reason for hiding this comment

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

A few suggestions on tidying things up 👍.

@jemrobinson jemrobinson requested a review from JimMadge April 12, 2024 14:25
@jemrobinson
Copy link
Member Author

Thanks - I especially liked the clarity on explaining our API (not Microsoft's)!

@JimMadge JimMadge merged commit aa9176e into alan-turing-institute:develop Apr 12, 2024
11 checks passed
@jemrobinson jemrobinson deleted the 1570-add-azuread-features branch April 19, 2024 11:02
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.

3 participants