-
Notifications
You must be signed in to change notification settings - Fork 343
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
Hybrid Free Busy Configuration Checker #1934
base: main
Are you sure you want to change the base?
Conversation
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.
Started to review the script. there are a few changes that need to be made before we can merge and release this script. Please address the current comments, then will do another review.
#> | ||
#region Properties and Parameters | ||
|
||
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Variables are being used')] |
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.
You have a single script file, this shouldn't be here.
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.
However, you do have a lot of lines in this file. This should be broken up into multiple files.
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.
Divided into multiple files now.
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.
Let's remove this, this should only be used for parameters that are never used in the initial script file. Which all of them appear to be. Then it is just a matter of addressing the other problems below.
|
||
Required Permissions: | ||
- Organization Management | ||
- Domain Admins (only necessary for the DCCoreRatio parameter) |
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.
DCCoreRatio
isn't a parameter
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.
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.
@MarcoLFrancisco it appears you haven't committed the changes and pushed to your branch.
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.
I think I missed the push. Now commited and pushed.
} | ||
#DisConnect-ExchangeOnline -Confirm:$False | ||
|
||
[System.Console]::Clear() |
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.
We shouldn't clear the console in our scripts.
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.
exit | ||
} | ||
|
||
Add-PSSnapin microsoft.exchange.management.powershell.Snapin |
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.
Unsupported way to load Exchange On Prem cmdlets.
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.
if ($Help) { | ||
Write-Host $bar | ||
ShowHelp | ||
$bar |
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.
$bar
isn't defined yet, so it wouldn't show up.
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.
function UserOnlineCheck { | ||
Write-Host -ForegroundColor Green "Online Mailbox: $UserOnline" | ||
Write-Host "Press the Enter key if OK or type an Exchange Online Email address and press the Enter key" | ||
$UserOnlineCheck = [System.Console]::ReadLine() |
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.
No read lines, everything should be a parameter if we want user input or a should process function.
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.
We will remove all console inputs. And only include parameters on script call.
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.
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.
Removed all ReadLine() and ReadKey()
Write-Host -ForegroundColor White " Exchange Online Mailbox:" | ||
Write-Host -ForegroundColor Green " $UserOnline" | ||
|
||
$script:html = "<!DOCTYPE html> |
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.
This should likely be its own file.
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.
Html output now in its own file
Write-Host -ForegroundColor Green " Get-OrganizationRelationship | Where{($_.DomainNames -like $ExchangeOnlineDomain )} | Select Identity,DomainNames,FreeBusy*,TarGet*,Enabled, ArchiveAccessEnabled" | ||
Write-Host $bar | ||
|
||
$OrgRel |
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.
This variable is set somewhere else, should be either passed to this function by a parameter or set within this function.
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.
#region DAuth Functions | ||
|
||
function OrgRelCheck { | ||
Write-Host $bar |
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.
Should make this $bar
a function really to do this line.
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.
$tdFBAccessLevel = "FreeBusyAccessLevel is set to AvailabilityOnly" | ||
$tdFBAccessLevelColor = "green" | ||
} | ||
if ($OrgRel.FreeBusyAccessLevel -like "LimitedDetails" ) { | ||
Write-Host -ForegroundColor Green " FreeBusyAccessLevel is set to LimitedDetails" | ||
$tdFBAccessLevel = "FreeBusyAccessLevel is set to LimitedDetails" | ||
$tdFBAccessLevelColor = "green" | ||
} | ||
if ($OrgRel.FreeBusyAccessLevel -ne "LimitedDetails" -AND $OrgRel.FreeBusyAccessLevel -ne "AvailabilityOnly" ) { | ||
Write-Host -ForegroundColor Red " FreeBusyAccessEnabled : False" | ||
$tdFBAccessLevel = "FreeBusyAccessEnabled : False" | ||
$tdFBAccessLevelColor = "Red" | ||
$countOrgRelIssues++ | ||
} | ||
#TarGetApplicationUri | ||
Write-Host -ForegroundColor White " TarGetApplicationUri:" | ||
if ($OrgRel.TarGetApplicationUri -like "Outlook.com" ) { | ||
Write-Host -ForegroundColor Green " TarGetApplicationUri is Outlook.com" | ||
$tdTarGetApplicationUri = "TarGetApplicationUri is Outlook.com" | ||
$tdTarGetApplicationUriColor = "green" | ||
} else { | ||
Write-Host -ForegroundColor Red " TarGetApplicationUri should be Outlook.com" | ||
$tdTarGetApplicationUri = "TarGetApplicationUri should be Outlook.com" | ||
$tdTarGetApplicationUriColor = "red" | ||
$countOrgRelIssues++ | ||
} | ||
#TarGetOwAUrl | ||
Write-Host -ForegroundColor White " TarGetOwAUrl:" | ||
if ($OrgRel.TarGetOwAUrl -like "https://outlook.com/owa/$ExchangeOnlineDomain" -or $OrgRel.TarGetOwAUrl -like $Null) { | ||
if ($OrgRel.TarGetOwAUrl -like "http://outlook.com/owa/$ExchangeOnlineDomain") { | ||
Write-Host -ForegroundColor Green " TarGetOwAUrl is http://outlook.com/owa/$ExchangeOnlineDomain. This is a possible standard value. TarGetOwAUrl can also be configured to be Blank." | ||
$tdOrgRelTarGetOwAUrl = " $($OrgRel.TarGetOwAUrl) - TarGetOwAUrl is http://outlook.com/owa/$ExchangeOnlineDomain. This is a possible standard value. TarGetOwAUrl can also be configured to be Blank." | ||
$tdOrgRelTarGetOwAUrlColor = "green" | ||
} | ||
if ($OrgRel.TarGetOwAUrl -like "https://outlook.office.com/mail") { | ||
Write-Host -ForegroundColor Green " TarGetOwAUrl is https://outlook.office.com/mail. This is a possible standard value. TarGetOwAUrl can also be configured to be Blank or http://outlook.com/owa/$ExchangeOnlineDomain." | ||
$tdOrgRelTarGetOwAUrl = " $($OrgRel.TarGetOwAUrl) - TarGetOwAUrl is https://outlook.office.com/mail. TarGetOwAUrl can also be configured to be Blank or http://outlook.com/owa/$ExchangeOnlineDomain." | ||
$tdOrgRelTarGetOwAUrlColor = "green" | ||
} | ||
if ($OrgRel.TarGetOwAUrl -like $Null) { | ||
Write-Host -ForegroundColor Green " TarGetOwAUrl is Blank, this is a standard value. " | ||
Write-Host " TarGetOwAUrl can also be configured to be https://outlook.com/owa/$ExchangeOnlineDomain or https://outlook.office.com/mail" | ||
$tdOrgRelTarGetOwAUrl = "$($OrgRel.TarGetOwAUrl) . TarGetOwAUrl is Blank, this is a standard value. TarGetOwAUrl can also be configured to be http://outlook.com/owa/$ExchangeOnlineDomain or http://outlook.office.com/mail. " | ||
$tdOrgRelTarGetOwAUrlColor = "green" | ||
if ($OrgRel.TarGetOwAUrl -like "https://outlook.com/owa/$ExchangeOnlineDomain") { | ||
Write-Host -ForegroundColor Green " TarGetOwAUrl is https://outlook.com/owa/$ExchangeOnlineDomain. This is a possible standard value. TarGetOwAUrl can also be configured to be Blank or http://outlook.office.com/mail." | ||
$tdOrgRelTarGetOwAUrl = " $($OrgRel.TarGetOwAUrl) - TarGetOwAUrl is https://outlook.com/owa/$ExchangeOnlineDomain. This is a possible standard value. TarGetOwAUrl can also be configured to be Blank or http://outlook.office.com/mail." | ||
$tdOrgRelTarGetOwAUrlColor = "green" | ||
} | ||
} | ||
} else { | ||
Write-Host -ForegroundColor Red " TarGetOwAUrl seems not to be Blank or https://outlook.com/owa/$ExchangeOnlineDomain. These are the standard values." | ||
$countOrgRelIssues++ | ||
$tdOrgRelTarGetOwAUrl = " TarGetOwAUrl seems not to be Blank or https://outlook.com/owa/$ExchangeOnlineDomain. These are the standard values." | ||
$tdOrgRelTarGetOwAUrlColor = "red" | ||
} | ||
#TarGetSharingEpr | ||
Write-Host -ForegroundColor White " TarGetSharingEpr:" | ||
if ([string]::IsNullOrWhitespace($OrgRel.TarGetSharingEpr) -or $OrgRel.TarGetSharingEpr -eq "https://outlook.office365.com/EWS/Exchange.asmx ") { | ||
Write-Host -ForegroundColor Green " TarGetSharingEpr is ideally blank. This is the standard Value. " | ||
Write-Host " If it is set, it should be Office 365 EWS endpoint. Example: https://outlook.office365.com/EWS/Exchange.asmx " | ||
$tdTarGetSharingEpr = " TarGetSharingEpr is ideally blank. This is the standard Value. | ||
If it is set, it should be Office 365 EWS endpoint. Example: https://outlook.office365.com/EWS/Exchange.asmx " | ||
$tdTarGetSharingEprColor = "green" | ||
} else { | ||
Write-Host -ForegroundColor Red " TarGetSharingEpr should be blank or https://outlook.office365.com/EWS/Exchange.asmx" | ||
Write-Host " If it is set, it should be Office 365 EWS endpoint. Example: https://outlook.office365.com/EWS/Exchange.asmx " | ||
$tdTarGetSharingEpr = " TarGetSharingEpr should be blank or https://outlook.office365.com/EWS/Exchange.asmx | ||
If it is set, it should be Office 365 EWS endpoint. Example: https://outlook.office365.com/EWS/Exchange.asmx " | ||
$tdTarGetSharingEprColor = "red" | ||
$countOrgRelIssues++ | ||
} | ||
#FreeBusyAccessScope | ||
Write-Host -ForegroundColor White " FreeBusyAccessScope:" | ||
if ([string]::IsNullOrWhitespace($OrgRel.FreeBusyAccessScope)) { | ||
Write-Host -ForegroundColor Green " FreeBusyAccessScope is blank, this is the standard Value. " | ||
$tdFreeBusyAccessScope = " FreeBusyAccessScope is blank, this is the standard Value." | ||
$tdFreeBusyAccessScopeColor = "green" | ||
} else { | ||
Write-Host -ForegroundColor Red " FreeBusyAccessScope is should be Blank, that is the standard Value." | ||
$tdFreeBusyAccessScope = " FreeBusyAccessScope is should be Blank, that is the standard Value." | ||
$tdFreeBusyAccessScopeColor = "red" | ||
$countOrgRelIssues++ | ||
} | ||
#TarGetAutoDiscoverEpr: | ||
$OrgRelTarGetAutoDiscoverEpr = $OrgRel.TarGetAutoDiscoverEpr | ||
if ([string]::IsNullOrWhitespace($OrgRelTarGetAutoDiscoverEpr)) { | ||
$OrgRelTarGetAutoDiscoverEpr = "Blank" | ||
} | ||
Write-Host -ForegroundColor White " TarGetAutoDiscoverEpr:" | ||
if ($OrgRel.TarGetAutoDiscoverEpr -like "https://AutoDiscover-s.outlook.com/AutoDiscover/AutoDiscover.svc/WSSecurity" ) { | ||
Write-Host -ForegroundColor Green " TarGetAutoDiscoverEpr is correct" | ||
$tdTarGetAutoDiscoverEPR = " TarGetAutoDiscoverEpr is https://AutoDiscover-s.outlook.com/AutoDiscover/AutoDiscover.svc/WSSecurity" | ||
$tdTarGetAutoDiscoverEPRColor = "green" | ||
} else { | ||
Write-Host -ForegroundColor Red " TarGetAutoDiscoverEpr is not correct. Should be https://AutoDiscover-s.outlook.com/AutoDiscover/AutoDiscover.svc/WSSecurity" | ||
$tdTarGetAutoDiscoverEPR = " TarGetAutoDiscoverEpr is $OrgRelTarGetAutoDiscoverEpr . Should be https://AutoDiscover-s.outlook.com/AutoDiscover/AutoDiscover.svc/WSSecurity" | ||
$tdTarGetAutoDiscoverEPRColor = "Red" | ||
$countOrgRelIssues++ | ||
} |
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.
This code should be reduced. You have a lot of repeated strings creating new variables for different settings. Try adding values you want to a list and using that list. Each entry in the list contains the Name, Value, and Color in the form the object.
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.
|
||
if (-not $OnPremEWSUrl) { | ||
|
||
$EWSVirtualDirectory = Get-WebServicesVirtualDirectory -server $Server -ErrorAction SilentlyContinue |
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.
Need to verify that we are running from an Exchange Server first before we just assume that is what we are doing.
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.
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Variables are being used in functions')] | ||
[CmdletBinding()] | ||
param ( | ||
[Parameter(Mandatory = $false)] | ||
$TestingDAuthConfiguration = "---------------------------------------Testing DAuth Configuration-----------------------------------------------", | ||
$TestingOAuthConfiguration = " ---------------------------------------Testing OAuth Configuration----------------------------------------------- ", | ||
$CollectingExoAvailabilityInformation = " Collecting Exchange Online Availability Information", | ||
$ExchangeOnlinePowershellModuleMessage = "`n Exchange Online Powershell Module is required to Check Free Busy Configuration on Exchange Online side. Installing Module", | ||
$ExchangeOnlineModuleAvailableMessage = "`n ExchangeOnlineManagement module is available.", | ||
$TestingExoDAuthConfiguration = " ---------------------------------------Testing DAuth Configuration----------------------------------------------- ", | ||
$TestingExoOAuthConfiguration = " ---------------------------------------Testing OAuth Configuration----------------------------------------------- ", | ||
$ThatIsAllForTheExchangeOnlineSide = " That is all for the Exchange Online Side" | ||
) |
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.
Not sure what is going on here. We started a new file and started it off like a completely new script with [CmdletBinding()]
on the top with param
This should only need to be done for the main script and new Function
(s)
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.
Mistake, removed
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Variables are being used in functions')] | ||
[CmdletBinding()] | ||
param ( | ||
[Parameter(Mandatory = $false)] | ||
[String] $tdIntraOrgTarGetAddressDomain | ||
) |
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.
Again, we shouldn't be having this type of a start of a file that we are just importing into a different main script.
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.
Mistake, removed
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Variables are being used in functions')] | ||
[CmdletBinding()] | ||
param ( | ||
[Parameter(Mandatory = $false)] | ||
[String] $OrgRelTarGetAutoDiscoverEpr | ||
) |
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.
Again, we shouldn't be having this type of a start of a file that we are just importing into a different main script.
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.
Mistake, removed
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Variables are being used in functions')] | ||
[CmdletBinding()] | ||
param ( | ||
[Parameter(Mandatory = $false)] | ||
[String] $tdEXOIntraOrgConTarGetAddressDomains | ||
) |
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.
Again, we shouldn't be having this type of a start of a file that we are just importing into a different main script.
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.
Mistake, removed
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Variables are being used in functions')] | ||
[CmdletBinding()] | ||
param ( | ||
[Parameter(Mandatory = $false)] | ||
[String] $tdEXOOrgRelDomainNamesColor | ||
) |
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.
Again, we shouldn't be having this type of a start of a file that we are just importing into a different main script.
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.
Mistake, removed
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Variables are being used in functions')] | ||
[CmdletBinding()] | ||
param ( | ||
[Parameter(Mandatory = $false)] | ||
[String] $tdDomainNamesColor | ||
) |
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.
Again, we shouldn't be having this type of a start of a file that we are just importing into a different main script.
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.
Mistake, removed
@MarcoLFrancisco do you still work on this PR? Please re-request a review once this is ready to be reviewed. Thanks. |
$exchangeServer = Get-ExchangeServer -ErrorAction SilentlyContinue | Where-Object { $_.Name -eq $Server } | ||
if (!$exchangeServer) { | ||
Write-Output "$Server is not an Exchange Server. This script need to be run in an Exchange Server 2013, 2016 or 2019" | ||
break |
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.
Break doesn't exit the script. We will till continue.
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.
Changed to Return. Correction will be available when changes are pushed.
param ( | ||
[string]$Server | ||
) | ||
$exchangeServer = Get-ExchangeServer -ErrorAction SilentlyContinue | Where-Object { $_.Name -eq $Server } |
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.
This is a very expensive call to be making. you are returning all the servers vs just trying to find a single server that is much quicker.
try {
Get-ExchangeServer -ErrorAction Stop $Server | Out-Null
} catch {
Write-Host "$Server is not an Exchange Server. This script needs to be run on an Exchange Server."
exit
}
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.
Changed to:
$exchangeServer = Get-ExchangeServer $server -ErrorAction SilentlyContinue
if (!$exchangeServer) {
Write-Output "$Server is not an Exchange Server. This script need to be run in an Exchange Server version 2013, 2016 or 2019"
return
}
Correction will be available when changes are pushed.
} | ||
|
||
if (-not (Get-Module -Name ExchangeOnlineManagement -ListAvailable)) { | ||
Disconnect-ExchangeOnline -Confirm:$False |
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.
We should not be disconnecting from sessions within our scripts, unless we are the ones that created it but we also don't like doing that.
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.
Removed from code. Change will be available when changes are pushed.
} end { | ||
function InstallRequiredModules { | ||
try { | ||
Get-Command -Module ActiveDirectory -ErrorAction Stop >$null |
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.
We should be using #Requires -Module ActiveDirectory
in the script inside of doing this.
https://powershellisfun.com/2023/04/24/using-the-requires-statement-in-powershell/
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.
Adding #Requires -Module ActiveDirectory and removing this check
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.
Removed this funciton and function call as it is not needed
function InstallRequiredModules {
try {
Get-Command -Module ActiveDirectory -ErrorAction Stop >$null
} catch {
Import-Module ActiveDirectory
}
if (-not (Get-Module -Name ExchangeOnlineManagement -ListAvailable)) {
Disconnect-ExchangeOnline -Confirm:$False
}
}
function EWSVirtualDirectoryCheck { | ||
Write-Host -ForegroundColor Green " Get-WebServicesVirtualDirectory -Server $($server)| Select Identity,Name,ExchangeVersion,*Authentication*,*url" | ||
PrintDynamicWidthLine | ||
$Script:WebServicesVirtualDirectory = Get-WebServicesVirtualDirectory -Server $server | Select-Object Identity, Name, ExchangeVersion, *Authentication*, *url -ErrorAction SilentlyContinue |
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.
This is the second time running this cmdlet, we should avoid doing that.
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.
Corrected, Get-WebServicesVirtualDirectory is only exectuted if not execiuted before. If it has been previuosly exevuted it now uses variable content. Correction will be available when changes are pushed.
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.
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Variables are being used in functions')] | ||
param ( | ||
[Parameter(Mandatory = $false)] | ||
[String] $tdEXOIntraOrgConTarGetAddressDomains | ||
) |
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.
Not needed
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.
Removed from code. Correction will be available when changes are pushed.
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Variables are being used in functions')] | ||
param ( | ||
[Parameter(Mandatory = $false)] | ||
[String] $tdEXOOrgRelDomainNamesColor | ||
) |
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.
Not needed
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.
Removed from code. Correction will be available when changes are pushed.
if (-not (Get-Module -Name ExchangeOnlineManagement -ListAvailable)) { | ||
# If not installed, then install the module | ||
Write-Host -ForegroundColor Yellow $ExchangeOnlinePowershellModuleMessage | ||
Install-Module -Name ExchangeOnlineManagement -Force | ||
PrintDynamicWidthLine |
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.
Let's use #Requires
for the module check
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.
If the user wishes to check only On Premises settings, Exchange Online Management module is not needed. This is very common on DAuth Free busy. Should I implement Requires in this case?
} | ||
#endregion | ||
|
||
Disconnect-ExchangeOnline -Confirm:$False |
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.
We should not be disconnecting from Exchange Online powershell session. We don't want to be modifying admins current connections.
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.
Removing
PrintDynamicWidthLine | ||
} | ||
|
||
Connect-ExchangeOnline -ShowBanner:$false |
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.
Do not connect, this should already be done by the admin prior to running the script. Only thing we should be doing is verifying that we are connected.
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.
Removing
@MarcoLFrancisco do you still want to work on this PR? Otherwise, we will close this out. |
Closing out as no updates since March. |
Good morning, my appologies but I missed last comments. Can the pull request be reactivated? |
Reopened as requested by @MarcoLFrancisco |
Good morning, I will start to address the last recomendations this weekend. I believe i can probably finish them during the weekend, if not they should all be addressed no latter than the following weekend. |
Please comment on here once you are ready for us to review the script again. |
Pushing requested changes
I believe changes are now commited |
Not sure why, I did spelcheck on two different machines, PC and Mac. I will correct this items. |
Pushing requested Changes
@MarcoLFrancisco I was running into the same problem. Looks like we need to update on the client side.
|
Thank you so much, I even tested on two PC's . I will update and review again. |
Pushing changes after Spell check update
Spell Check on PC worked, returned spell errors and i could address them. |
$exchangeServer = Get-ExchangeServer $server -ErrorAction SilentlyContinue | ||
if (!$exchangeServer) { | ||
Write-Output "$Server is not an Exchange Server. This script need to be run in an Exchange Server version 2013, 2016 or 2019" | ||
return |
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.
A return
is only going to return
from the current function and all the script to move forward and you don't stop the script here. We need to switch this to an exit
.
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.
Changed to exit
) | ||
$exchangeServer = Get-ExchangeServer $server -ErrorAction SilentlyContinue | ||
if (!$exchangeServer) { | ||
Write-Output "$Server is not an Exchange Server. This script need to be run in an Exchange Server version 2013, 2016 or 2019" |
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.
Let's remove Exchange Version number in a hard coded string display.
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.
Removed
- Exchange Server 2013 | ||
- Exchange Server 2016 | ||
- Exchange Server 2019 |
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.
Maybe change this to just Exchange Server
$Script:AutoDiscoveryVirtualDirectory = $null | ||
$Script:OrgRel = $null | ||
$Script:SPDomainsOnprem = $null | ||
$AvailabilityAddressSpace = $null |
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.
This isn't required to be here as you set it in the other functions and only use it there.
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.
Removed
$countOrgRelIssues++ | ||
} | ||
# Display the settings list | ||
if ($countOrgRelIssues -eq '0') { |
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.
We shouldn't be doing this; you are comparing an int
to a string
. PowerShell likely works, but this is a bad habit.
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.
Changed to "if ($countOrgRelIssues -eq 0)"
$AvailabilityAddressSpace = $null | ||
$Script:WebServicesVirtualDirectory = $null | ||
$ConsoleWidth = $Host.UI.RawUI.WindowSize.Width | ||
$BuildVersion = "" |
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.
Move to the function where we use this variable.
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.
Moved
$Server = hostname | ||
$LogFile = "$PSScriptRoot\FreeBusyChecker.txt" | ||
$startingDate = (Get-Date -Format yyyyMMdd_HHmmss) | ||
$LogFileName = [System.IO.Path]::GetFileNameWithoutExtension($LogFile) + "_" + $startingDate + ([System.IO.Path]::GetExtension($LogFile)) |
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.
Move to the function where we are using this variable.
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.
Moved
$LogFile = "$PSScriptRoot\FreeBusyChecker.txt" | ||
$startingDate = (Get-Date -Format yyyyMMdd_HHmmss) | ||
$LogFileName = [System.IO.Path]::GetFileNameWithoutExtension($LogFile) + "_" + $startingDate + ([System.IO.Path]::GetExtension($LogFile)) | ||
$htmlFile = "$PSScriptRoot\FBCheckerOutput_$($startingDate).html" |
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.
Set this to be a $Script:
variable, as we do use this in multiple locations.
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.
Corrected
#> | ||
#region Properties and Parameters | ||
|
||
[Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', '', Justification = 'Variables are being used')] |
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.
Let's remove this, this should only be used for parameters that are never used in the initial script file. Which all of them appear to be. Then it is just a matter of addressing the other problems below.
Syntax: | ||
|
||
FreeBusyChecker.ps1 |
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.
use the same format that is in SourceSideValidations.md
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.
I believe it is as intended now.
Good morning. I have been out of office for some days. Will implement corrections as soon as possible. |
Changes are almost completed. Expect to be concluded and tested in the next days. |
Edited CommonFunctions.ps1; hostOutput.ps1; htmlContent.ps1; OnPremDAuthFunctions.ps1; FreeBusyChecker.ps1
Requested changes are now published |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
[string]$Server | ||
) | ||
$exchangeServer = Get-ExchangeServer $Script:Server -ErrorAction SilentlyContinue |
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.
Using two different variables here for $Server
. The parameter of $Server
is local, where the $Script:Server
is a script scope variable. So you can have these as two different variables.
Since this function is only really used once, is it really needed if it isn't complicated?
$LogFileName = [System.IO.Path]::GetFileNameWithoutExtension($LogFile) + "_" + $Script:startingDate + ([System.IO.Path]::GetExtension($Script:LogFile)) | ||
Write-Host " `n`n " | ||
Write-Host " `n`n " | ||
Start-Transcript -Path $LogFileName -Append |
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.
Since you only appear to have Write-Host
in the script this works. However, it makes the end user text file rather wordy. A better option is to only display the needed information, and then for diagnostic purposes, like script troubleshooting, have that information in Write-Verbose
which is not captured in Start-Transcript
unless they provide -Verbose
.
We do have logging functions already created in the share, but this isn't required at this time.
To View this Project at GitHub! [GitHub Repository](https://github.com/MarcoLFrancisco/Hybrid-Free-Busy-Configuration-Checker) | ||
|
||
To Download the latest release: [FreeBusyChecker.ps1](https://github.com/MarcoLFrancisco/Hybrid-Free-Busy-Configuration-Checker/releases/download/Version1/FreeBusyChecker.ps1) |
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.
update links to point to our repository and using the correct download link location.
- This is a Beta Version. Please double check on any information provided by this script before proceeding to address any changes to your Environment. Be advised that there may be incorrect content in the provided output. | ||
|
||
Use: Collects OAuth and DAuth Hybrid Availability Configuration Settings Both for Exchange On Premises and Exchange Online | ||
|
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.
Need to include information about how they need to connect to Exchange Online with the -Prefix
Connect-ExchangeOnline -Prefix EO
if ($Org -contains 'ExchangeOnPremise' -or -not $Org) { | ||
#region DAuth Checks | ||
if ($Auth -like "DAuth" -OR -not $Auth -or $Auth -like "All") { | ||
Write-Host $TestingDAuthConfiguration |
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.
Empty variable
if ($Script:tdExoOrgRelDomainNamesData -or $Script:tdExoOrgRelDomainNamesColor) { | ||
} |
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.
Looks like we are missing something here.
if ($Script:tdExoOrgRelFreeBusyAccessEnabled -or $Script:tdExoOrgRelFreeBusyAccessEnabledColor) { | ||
} |
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.
Looks like we are missing something here.
PrintDynamicWidthLine | ||
$Script:OAuthConnectivity = Test-OAuthConnectivity -Service EWS -TarGetUri https://outlook.office365.com/EWS/Exchange.asmx -Mailbox $Script:UserOnPrem | ||
if ($Script:OAuthConnectivity.ResultType -eq 'Success' ) { | ||
#$Script:OAuthConnectivity.ResultType |
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.
Empty if
statement with a comment. Determine if this is needed, otherwise, remove if
statement.
$AD = $AutoDiscoveryVirtualDirectoryOAuth | Format-List | ||
$AD | ||
AutoDVirtualDCheckOauthHtmlHead | ||
if ($Auth -contains "OAuth") { |
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.
Empty if
statement
if ($Auth -contains "OAuth") { | ||
} |
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.
Empty if
statement
Due to a code formatting rule change, you will need to run that locally and address the issues seen there. |
@MarcoLFrancisco please review the PR and make the required adjustments. |
Issue:
Collect Exchange Hybrid Free Configuration Information in Exchange deployments is often time consuming and a big effort
Reason:
This script is aimed to assist in collecting Hybrid Free busy configuration information, both for OAuth and DAuth and both from On Premise and Exchange online side. It provides also information for what would be expected for each component reviewed and can assist our customer and our support engineers.
Fix:
Script collects relevant information as described in Microsoft Internal Documentation. Script does not produce changes on customer side other than generating txt and html Output
Validation:
Utilized on several occasions both on lab environments and with active support cases. It has proven to be useful.