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.

Sign up for GitHub

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

Jump to bottom

feat: added option to enable perUserMfaState #23

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: added option to enable perUserMfaState #23

wants to merge 2 commits into from

Conversation

mspreeuwenberg
Copy link

No description provided.

@JeroenBL JeroenBL added the needs-review Indicates that the PR is ready and waiting for review label Dec 19, 2024
Rick-Jongbloed
Rick-Jongbloed requested changes Dec 19, 2024
View reviewed changes
Copy link
Member

@Rick-Jongbloed Rick-Jongbloed left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey Mark, ziet er echt top uit. Ik heb nog wel een paar kleine puntjes. Verder kan je misschien een kleine readme tikken en die in dit mapje plaatsen? Hier kan je de instructie en requirements (rechten) voor het gebruik van deze scripts noteren. Hier kan je dan ook een opmerking plaatsen voor het beta endpoint.

permissions/perUserMfaState/grantPermission.ps1 Outdated
@@ -0,0 +1,322 @@
###############################################
# Please not the scripting uses a beta endpoint
Copy link
Member

Choose a reason for hiding this comment

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

Please use the default headers and add this as comment. Also not -> note

Copy link
Author

Choose a reason for hiding this comment

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

gefixt

permissions/perUserMfaState/grantPermission.ps1 Outdated
$WarningPreference = "Continue"

#region functions
function Convert-StringToBoolean($obj) {
Copy link
Member

Choose a reason for hiding this comment

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

functie wordt niet gebruikt in scipt

Copy link
Author

Choose a reason for hiding this comment

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

is verwijderd

permissions/perUserMfaState/revokePermission.ps1 Outdated
@@ -0,0 +1,322 @@
###############################################
Copy link
Member

Choose a reason for hiding this comment

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

Please use the default headers and add this as comment. Also not -> note

Copy link
Author

Choose a reason for hiding this comment

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

default header toegevoegd

permissions/perUserMfaState/permissions.ps1
@@ -0,0 +1,10 @@

Copy link
Member

Choose a reason for hiding this comment

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

Should we use a header here as well?

Copy link
Author

Choose a reason for hiding this comment

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

done

permissions/perUserMfaState/revokePermission.ps1 Outdated

$outputContext.AuditLogs.Add([PSCustomObject]@{
# Action = "" # Optional
Message = "Skipped setting perUserMfaState to [$($state)] for account with AccountReference: $($actionContext.References.Account | ConvertTo-Json). Old value: [$($currentPerUserMfaState)]. New value: [$($state)]. Reason: No changes."
Copy link
Member

Choose a reason for hiding this comment

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

Old value en new value lijkt me niet nodig als er geen changes zijn. Beter iets melden in de trend van
'Skipped setting perUserMfaState as state is already set to the desired state' of 'Skipped setting perUserMfaState as state is already set $state'

Copy link
Author

Choose a reason for hiding this comment

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

verwijderd

permissions/perUserMfaState/revokePermission.ps1
#region Verify account reference and required properties
$actionMessage = "verifying account reference and required properties"
if ([string]::IsNullOrEmpty($($actionContext.References.Account))) {
throw "The account reference could not be found"
Copy link
Member

Choose a reason for hiding this comment

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

dit lijkt me niet handig bij een revoke.

Bij grant en notfound: error
Bij revoke en notfound: geen error, maar skip van actie (en audit bericht met success op true)

Copy link
Author

Choose a reason for hiding this comment

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

dit gaat om de account reference. Die zou altijd gevuld moeten tijn. Echter, in basis zouden we inderdaad ook moeten checken of de gebruiker nog bestaat. Dit zouden we in meerdere scripts moeten toevoegen. Dit gaat voor nu te ver

permissions/perUserMfaState/grantPermission.ps1 Outdated

$outputContext.AuditLogs.Add([PSCustomObject]@{
# Action = "" # Optional
Message = "$state perUserMfaState [$($actionContext.References.Permission.Name)] for account with AccountReference: $($actionContext.References.Account | ConvertTo-Json). Old value: [$($currentPerUserMfaState)]. New value: [$($state)]."
Copy link
Member

Choose a reason for hiding this comment

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

new state heb je al voorin de message staan. is het nodig om dit 2x in het audit bericht te plaatsen?

Copy link
Author

Choose a reason for hiding this comment

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

aangepast

permissions/perUserMfaState/grantPermission.ps1 Outdated

$outputContext.AuditLogs.Add([PSCustomObject]@{
# Action = "" # Optional
Message = "Skipped setting perUserMfaState to [$($state)] for account with AccountReference: $($actionContext.References.Account | ConvertTo-Json). Old value: [$($currentPerUserMfaState)]. New value: [$($state)]. Reason: No changes."
Copy link
Member

Choose a reason for hiding this comment

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

Old value en new value lijkt me niet nodig als er geen changes zijn. Beter iets melden in de trend van
'Skipped setting perUserMfaState as state is already set to the desired state' of 'Skipped setting perUserMfaState as state is already set $state'

Copy link
Author

Choose a reason for hiding this comment

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

aangepast

permissions/perUserMfaState/grantPermission.ps1
$warningMessage = "Error at Line [$($ex.InvocationInfo.ScriptLineNumber)]: $($ex.InvocationInfo.Line). Error: $($ex.Exception.Message)"
}

Write-Warning $warningMessage
Copy link
Member

Choose a reason for hiding this comment

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

Waarschijnlijk even uitbreiden met 'Unable to set $state for person X. Error message: '

Copy link
Author

Choose a reason for hiding this comment

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

Lijkt me overbodig, dit blijkt al uit de actie

permissions/perUserMfaState/revokePermission.ps1
$warningMessage = "Error at Line [$($ex.InvocationInfo.ScriptLineNumber)]: $($ex.InvocationInfo.Line). Error: $($ex.Exception.Message)"
}

Write-Warning $warningMessage
Copy link
Member

Choose a reason for hiding this comment

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

Waarschijnlijk even uitbreiden met 'Unable to set $state for person X. Error message: '

Copy link
Author

Choose a reason for hiding this comment

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

Lijkt me overbodig, dit blijkt al uit de actie

Rick-Jongbloed
Rick-Jongbloed requested changes Dec 19, 2024
View reviewed changes
permissions/perUserMfaState/revokePermission.ps1 Outdated
$WarningPreference = "Continue"

#region functions
function Convert-StringToBoolean($obj) {
Copy link
Member

Choose a reason for hiding this comment

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

functie wordt niet gebruikt in scipt

Copy link
Author

Choose a reason for hiding this comment

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

verwijderd

@Rick-Jongbloed Rick-Jongbloed added pending-author-update PR is waiting for the author to make necessary modifications or updates and removed needs-review Indicates that the PR is ready and waiting for review labels Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rick-Jongbloed Rick-Jongbloed Rick-Jongbloed requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@Rick-Jongbloed Rick-Jongbloed

Labels
current-sprint pending-author-update PR is waiting for the author to make necessary modifications or updates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mspreeuwenberg @Rick-Jongbloed @JeroenBL