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 script to automate account deletion #1508

Conversation

edwardchalstrey1
Copy link
Contributor

@edwardchalstrey1 edwardchalstrey1 commented Jul 21, 2023

✅ 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

@alan-turing-institute/data-safe-haven-code-administrators I will add instructions on how/when to use this script in the Turing context to the Turing TRESA docs, but for the DSH docs I think this is sufficient

🌂 Related issues

Turing internal production environments repo: https://github.com/alan-turing-institute/trusted-research/issues/224

🔬 Tests

  • Running the script on a test SHM and checking that users not in a SG are deleted
  • Running with the -dryRun flag users are listed but not deleted

@edwardchalstrey1 edwardchalstrey1 changed the title Add script to automate account deletion [WIP] Add script to automate account deletion Jul 21, 2023
@jemrobinson
Copy link
Member

1. The script that lives directly in the DC - this would need to be manually run by an admin by logging into the SHM after a teardown - I have tested this and it works

2. The script that is run outside the DC and invokes the deletion - Note: currently this seems to hang at the deletion step when I've tested it

The original reason for the 2nd approach was so that it could be run as a final step of the SRE teardown script, but actually, it might be we don't want this, what to people think? If we do, it now occurs to me it may be possible to invoke the script from the 1st approach anyway.

Do the second one. Uploading scripts to the SHM is complex (we upload them to a storage account and then copy them to the SHM). It's only necessary for scripts that need to be run in a GUI (eg. ones that have an AAD login pop-up).

Remote scripts will hang if there is any kind of error in them (also can't be retried for 90 minutes). Make sure you test interactively but non-destructively on the DC.

@edwardchalstrey1
Copy link
Contributor Author

It might be useful to split the operations this way if it is the only (or best) way to log the users being removed and the success/failure of each removal.

The command in this remote script won't give an output, but this should be something that is easy for an SHM admin to check manually

@JimMadge maybe I should add a second part of the script that prints out the remaining users and their security group or something like that, for the purpose of checking?

@JimMadge
Copy link
Member

The command in this remote script won't give an output, but this should be something that is easy for an SHM admin to check manually

@JimMadge maybe I should add a second part of the script that prints out the remaining users and their security group or something like that, for the purpose of checking?

Yes something like a before and after list of users could work. It feels like this is place where it is important to understand what has happened. An admin will need to confirm the users were removed correctly (and fix any cases where removing a user failed).

Copy link
Member

@jemrobinson jemrobinson left a comment

Choose a reason for hiding this comment

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

A couple of thoughts here.

  1. It would be good to add a dry-run flag (typically done using [CmdletBinding(SupportsShouldProcess=$True)] which gives you a -WhatIf flag) to show which users would be removed without doing it
  2. The script called SRE_Delete_Unassigned_Users.ps1 doesn't have anything to do with SREs, right? Maybe change the name.

@edwardchalstrey1 edwardchalstrey1 changed the title [WIP] Add script to automate account deletion Add script to automate account deletion Aug 29, 2023
@edwardchalstrey1 edwardchalstrey1 marked this pull request as ready for review August 29, 2023 13:24
Copy link
Member

@jemrobinson jemrobinson left a comment

Choose a reason for hiding this comment

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

LGTM - if it works :)

@edwardchalstrey1
Copy link
Contributor Author

@jemrobinson please merge I'm not allowed

@jemrobinson jemrobinson merged commit 7e7a1bb into alan-turing-institute:develop Sep 11, 2023
10 checks passed
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