Refactor error handling that includes logging and update test cases

- Removed WINGET_CATALOG_CATCH_STORE macro from ExecutionContext.h. - Refactored GetPackageCatalogOperationStatus in Converters.h. - Updated PackageCatalogReference.cpp to use HandleException. - Refactored PackageManager.cpp for detailed exception handling. - Added new test cases for insecure URI and invalid type scenarios. - Updated PackageCatalogInterop.cs tests to use InvalidOptions status. - Added comments for admin validation checks in InProc/OutOfProc calls.
microsoft · Oct 21, 2024 · b4db8df · b4db8df
1 parent 016c618
commit b4db8df
Show file tree

Hide file tree

Showing 5 changed files with 155 additions and 60 deletions.
diff --git a/src/AppInstallerCLICore/ExecutionContext.h b/src/AppInstallerCLICore/ExecutionContext.h
@@ -25,13 +25,6 @@
         WINGET_CATCH_STD_EXCEPTION_STORE(exceptionHR, genericHR) \
         WINGET_CATCH_ALL_EXCEPTION_STORE(exceptionHR, genericHR)
 
-#define WINGET_CATALOG_CATCH_STORE(exceptionHR, genericHR) \
-        WINGET_CATCH_RESULT_EXCEPTION_STORE(exceptionHR) \
-        WINGET_CATCH_HRESULT_EXCEPTION_STORE(exceptionHR) \
-        WINGET_CATCH_POLICY_EXCEPTION_STORE(exceptionHR) \
-        WINGET_CATCH_STD_EXCEPTION_STORE(exceptionHR, genericHR) \
-        WINGET_CATCH_ALL_EXCEPTION_STORE(exceptionHR, genericHR)
-
 // Terminates the Context with some logging to indicate the location.
 // Also returns from the current function.
 #define AICLI_TERMINATE_CONTEXT_ARGS(_context_,_hr_,_ret_) \

diff --git a/src/AppInstallerCLIE2ETests/Interop/PackageCatalogInterop.cs b/src/AppInstallerCLIE2ETests/Interop/PackageCatalogInterop.cs
@@ -101,7 +101,7 @@ public async Task AddPackageCatalog_DuplicateName_ReturnsSourceNameExists()
             await this.AddAndValidatePackageCatalogAsync(options, AddPackageCatalogStatus.Ok);
 
             // Add the same package catalog again.
-            await this.AddAndValidatePackageCatalogAsync(options, AddPackageCatalogStatus.CatalogError, Constants.ErrorCode.ERROR_SOURCE_NAME_ALREADY_EXISTS);
+            await this.AddAndValidatePackageCatalogAsync(options, AddPackageCatalogStatus.InvalidOptions, Constants.ErrorCode.ERROR_SOURCE_NAME_ALREADY_EXISTS);
 
             // Remove the tests source if it exists.
             RemovePackageCatalogOptions removePackageCatalogOptions = this.TestFactory.CreateRemovePackageCatalogOptions();
@@ -126,7 +126,7 @@ public async Task AddPackageCatalog_DuplicateSourceUri_ReturnSourceArgAlreadyExi
 
             // Add the same package catalog again.
             options.Name = "TestSource2";
-            await this.AddAndValidatePackageCatalogAsync(options, AddPackageCatalogStatus.CatalogError, Constants.ErrorCode.ERROR_SOURCE_ARG_ALREADY_EXISTS);
+            await this.AddAndValidatePackageCatalogAsync(options, AddPackageCatalogStatus.InvalidOptions, Constants.ErrorCode.ERROR_SOURCE_ARG_ALREADY_EXISTS);
 
             // Remove the tests source if it exists.
             RemovePackageCatalogOptions removePackageCatalogOptions = this.TestFactory.CreateRemovePackageCatalogOptions();
@@ -150,6 +150,37 @@ public async Task AddPackageCatalogWithInvalidSourceUri()
             await this.AddAndValidatePackageCatalogAsync(options, AddPackageCatalogStatus.InternalError);
         }
 
+        /// <summary>
+        /// Add package catalog with insecure source uri.
+        /// </summary>
+        /// <returns>representing the asynchronous unit test.</returns>
+        [Test]
+        public async Task AddPackageCatalogWithHttpSourceUri()
+        {
+            AddPackageCatalogOptions options = this.TestFactory.CreateAddPackageCatalogOptions();
+            options.SourceUri = "http://microsoft.com";
+            options.Name = "Insecure";
+            options.TrustLevel = PackageCatalogTrustLevel.Trusted;
+            options.AcceptSourceAgreements = true;
+
+            await this.AddAndValidatePackageCatalogAsync(options, AddPackageCatalogStatus.InvalidOptions, Constants.ErrorCode.ERROR_SOURCE_NOT_SECURE);
+        }
+
+        /// <summary>
+        /// Add package catalog with invalid type.
+        /// </summary>
+        /// <returns>representing the asynchronous unit test.</returns>
+        [Test]
+        public async Task AddPackageCatalogWithInvalidType()
+        {
+            AddPackageCatalogOptions options = this.TestFactory.CreateAddPackageCatalogOptions();
+            options.SourceUri = Constants.TestSourceUrl;
+            options.Name = Constants.TestSourceName;
+            options.Type = "InvalidType";
+
+            await this.AddAndValidatePackageCatalogAsync(options, AddPackageCatalogStatus.InvalidOptions, Constants.ErrorCode.ERROR_INVALID_SOURCE_TYPE);
+        }
+
         /// <summary>
         /// Add package catalog with source agreement not accepted.
         /// </summary>
@@ -245,7 +276,7 @@ public async Task RemoveNonExistingPackageCatalog()
             RemovePackageCatalogOptions removePackageCatalogOptions = this.TestFactory.CreateRemovePackageCatalogOptions();
             removePackageCatalogOptions.Name = Constants.TestSourceName;
 
-            await this.RemoveAndValidatePackageCatalogAsync(removePackageCatalogOptions, RemovePackageCatalogStatus.CatalogError, Constants.ErrorCode.ERROR_SOURCE_NAME_DOES_NOT_EXIST);
+            await this.RemoveAndValidatePackageCatalogAsync(removePackageCatalogOptions, RemovePackageCatalogStatus.InvalidOptions, Constants.ErrorCode.ERROR_SOURCE_NAME_DOES_NOT_EXIST);
         }
 
         /// <summary>

diff --git a/src/Microsoft.Management.Deployment/Converters.h b/src/Microsoft.Management.Deployment/Converters.h
@@ -143,59 +143,93 @@ namespace winrt::Microsoft::Management::Deployment::implementation
     }
 
     template <typename TStatus>
-    TStatus GetPackageCatalogOperationStatus(winrt::hresult hresult)
+    TStatus HandleCommonCatalogOperationStatus(winrt::hresult hresult)
     {
-        // Applicable only for AddPackageCatalogStatus
-        if constexpr (std::is_same_v<TStatus, winrt::Microsoft::Management::Deployment::AddPackageCatalogStatus>)
+        // Common status handling for AddPackageCatalogStatus and RemovePackageCatalogStatus.
+        if constexpr (std::is_same_v<TStatus, winrt::Microsoft::Management::Deployment::AddPackageCatalogStatus> || std::is_same_v<TStatus, winrt::Microsoft::Management::Deployment::RemovePackageCatalogStatus>)
         {
             switch (hresult)
             {
-            case APPINSTALLER_CLI_ERROR_SOURCE_AGREEMENTS_NOT_ACCEPTED:
-                return TStatus::SourceAgreementsNotAccepted;
-            case APPINSTALLER_CLI_ERROR_AUTHENTICATION_TYPE_NOT_SUPPORTED:
-                return TStatus::AuthenticationError;
-            default:
-                break;
-            }
-        }
-
-        // Applicable only for AddPackageCatalogStatus and RemovePackageCatalogStatus
-        if constexpr (std::is_same_v<TStatus, winrt::Microsoft::Management::Deployment::AddPackageCatalogStatus>
-            || std::is_same_v<TStatus, winrt::Microsoft::Management::Deployment::RemovePackageCatalogStatus>)
-        {
-            switch (hresult)
-            {
-            case APPINSTALLER_CLI_ERROR_INVALID_CL_ARGUMENTS:
-            case E_INVALIDARG:
-                return TStatus::InvalidOptions;
             case APPINSTALLER_CLI_ERROR_COMMAND_REQUIRES_ADMIN:
             case E_ACCESSDENIED:
                 return TStatus::AccessDenied;
-            default:
+            case APPINSTALLER_CLI_ERROR_INVALID_CL_ARGUMENTS:
+            case E_INVALIDARG:
+                return TStatus::InvalidOptions;
+            default:
                 break;
             }
         }
 
-        // Handle the common status codes across add, remove and refresh operations.
+        // Common status handling for AddPackageCatalogStatus, RemovePackageCatalogStatus, and RefreshPackageCatalogStatus.
         switch (hresult)
         {
         case S_OK:
             return TStatus::Ok;
         case APPINSTALLER_CLI_ERROR_BLOCKED_BY_POLICY:
             return TStatus::GroupPolicyError;
-        case APPINSTALLER_CLI_ERROR_SOURCES_INVALID:
-        case APPINSTALLER_CLI_ERROR_SOURCE_DATA_MISSING:
-        case APPINSTALLER_CLI_ERROR_SOURCE_NAME_ALREADY_EXISTS:
-        case APPINSTALLER_CLI_ERROR_SOURCE_NAME_DOES_NOT_EXIST:
-        case APPINSTALLER_CLI_ERROR_SOURCE_ARG_ALREADY_EXISTS:
-        case APPINSTALLER_CLI_ERROR_SOURCE_NOT_SECURE:
-        case APPINSTALLER_CLI_ERROR_SOURCE_NOT_REMOTE:
         case APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE:
-        case APPINSTALLER_CLI_ERROR_SOURCE_OPEN_FAILED:
             return TStatus::CatalogError;
         case APPINSTALLER_CLI_ERROR_INTERNAL_ERROR:
         default:
             return TStatus::InternalError;
         }
     }
+
+    template <typename TStatus>
+    TStatus GetAddPackageCatalogOperationStatus(winrt::hresult hresult)
+    {
+        switch (hresult)
+        {
+        case APPINSTALLER_CLI_ERROR_SOURCE_AGREEMENTS_NOT_ACCEPTED:
+            return TStatus::SourceAgreementsNotAccepted;
+        case APPINSTALLER_CLI_ERROR_AUTHENTICATION_TYPE_NOT_SUPPORTED:
+            return TStatus::AuthenticationError;
+        case APPINSTALLER_CLI_ERROR_SOURCE_NOT_SECURE:
+        case APPINSTALLER_CLI_ERROR_INVALID_SOURCE_TYPE:
+        case APPINSTALLER_CLI_ERROR_SOURCE_NOT_REMOTE:
+        case APPINSTALLER_CLI_ERROR_SOURCE_NAME_ALREADY_EXISTS:
+        case APPINSTALLER_CLI_ERROR_SOURCE_ARG_ALREADY_EXISTS:
+            return TStatus::InvalidOptions;
+        case APPINSTALLER_CLI_ERROR_SOURCE_OPEN_FAILED:
+            return TStatus::CatalogError;
+        default:
+            return HandleCommonCatalogOperationStatus<TStatus>(hresult);
+        }
+    }
+
+    template <typename TStatus>
+    TStatus GetRemovePackageCatalogOperationStatus(winrt::hresult hresult)
+    {
+        switch (hresult)
+        {
+        case APPINSTALLER_CLI_ERROR_SOURCE_NAME_DOES_NOT_EXIST:
+            return TStatus::InvalidOptions;
+        case APPINSTALLER_CLI_ERROR_INVALID_SOURCE_TYPE:
+            return TStatus::CatalogError;
+        default:
+            return HandleCommonCatalogOperationStatus<TStatus>(hresult);
+        }
+    }
+
+    template <typename TStatus>
+    TStatus GetPackageCatalogOperationStatus(winrt::hresult hresult)
+    {
+        if constexpr (std::is_same_v<TStatus, winrt::Microsoft::Management::Deployment::AddPackageCatalogStatus>)
+        {
+            return GetAddPackageCatalogOperationStatus<AddPackageCatalogStatus>(hresult);
+        }
+        else if constexpr (std::is_same_v<TStatus, winrt::Microsoft::Management::Deployment::RemovePackageCatalogStatus>)
+        {
+            return GetRemovePackageCatalogOperationStatus<RemovePackageCatalogStatus>(hresult);
+        }
+        else if constexpr (std::is_same_v<TStatus, winrt::Microsoft::Management::Deployment::RefreshPackageCatalogStatus>)
+        {
+            return HandleCommonCatalogOperationStatus<RefreshPackageCatalogStatus>(hresult);
+        }
+        else
+        {
+            throw winrt::hresult_error(E_UNEXPECTED);
+        }
+    }
 }
diff --git a/src/Microsoft.Management.Deployment/PackageCatalogReference.cpp b/src/Microsoft.Management.Deployment/PackageCatalogReference.cpp
@@ -302,12 +302,8 @@ namespace winrt::Microsoft::Management::Deployment::implementation
 
     winrt::Windows::Foundation::IAsyncOperationWithProgress<winrt::Microsoft::Management::Deployment::RefreshPackageCatalogResult, double> PackageCatalogReference::RefreshPackageCatalogAsync()
     {
-        ::AppInstaller::Logging::Telemetry().SetCaller(GetCallerName());
-        ::AppInstaller::Logging::Telemetry().LogStartup(true);
-
         HRESULT terminationHR = S_OK;
-        try
-        {
+        try {
             // Check for permissions and get caller info for telemetry
             THROW_IF_FAILED(EnsureComCallerHasCapability(Capability::PackageQuery));
 
@@ -327,7 +323,10 @@ namespace winrt::Microsoft::Management::Deployment::implementation
 
             this->m_sourceReference.Update(progress);
         }
-        WINGET_CATALOG_CATCH_STORE(terminationHR, APPINSTALLER_CLI_ERROR_INTERNAL_ERROR);
+        catch (...)
+        {
+            terminationHR = AppInstaller::CLI::Workflow::HandleException(nullptr, std::current_exception());
+        }
 
         co_return GetRefreshPackageCatalogResult(terminationHR);
     }

diff --git a/src/Microsoft.Management.Deployment/PackageManager.cpp b/src/Microsoft.Management.Deployment/PackageManager.cpp
@@ -1013,15 +1013,51 @@ namespace winrt::Microsoft::Management::Deployment::implementation
 
         ::AppInstaller::Repository::Source source = ::AppInstaller::Repository::Source{ name, sourceUri, type, trustLevel, options.Explicit() };
 
-        // This will throw if the source details are not initialized properly, acting as a validation check for the source object.
-        source.GetDetails();
-
         std::string customHeader = winrt::to_string(options.CustomHeader());
         if (!customHeader.empty())
         {
             source.SetCustomHeader(customHeader);
         }
 
+        try
+        {
+            auto sourceInfo = source.GetInformation();
+
+            if (sourceInfo.Authentication.Type == ::AppInstaller::Authentication::AuthenticationType::Unknown)
+            {
+                throw winrt::hresult_error(APPINSTALLER_CLI_ERROR_AUTHENTICATION_TYPE_NOT_SUPPORTED);
+            }
+
+            if (!source.CheckSourceAgreements())
+            {
+                if(!options.AcceptSourceAgreements())
+                {
+                    throw winrt::hresult_error(APPINSTALLER_CLI_ERROR_SOURCE_AGREEMENTS_NOT_ACCEPTED);
+                }
+
+                source.SaveAcceptedSourceAgreements();
+            }
+        }
+        catch (const winrt::hresult_error& hre)
+        {
+            if (hre.code() == APPINSTALLER_CLI_ERROR_AUTHENTICATION_TYPE_NOT_SUPPORTED)
+            {
+                THROW_HR(APPINSTALLER_CLI_ERROR_AUTHENTICATION_TYPE_NOT_SUPPORTED);
+            }
+            else if (hre.code() == APPINSTALLER_CLI_ERROR_SOURCE_AGREEMENTS_NOT_ACCEPTED)
+            {
+                THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_AGREEMENTS_NOT_ACCEPTED);
+            }
+            else
+            {
+                THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_OPEN_FAILED);
+            }
+        }
+        catch (...) // Catch all exceptions
+        {
+            THROW_HR(APPINSTALLER_CLI_ERROR_SOURCE_OPEN_FAILED);
+        }
+
         return source;
     }
 
@@ -1305,18 +1341,12 @@ namespace winrt::Microsoft::Management::Deployment::implementation
         try {
 
             // Check if running as admin
+            // [NOTE:] For OutOfProc calls, the Windows Package Manager Service executes in the context initiated by the caller process,
+            //so the same admin validation check is applicable for both InProc and OutOfProc calls.
             THROW_HR_IF(APPINSTALLER_CLI_ERROR_COMMAND_REQUIRES_ADMIN, !AppInstaller::Runtime::IsRunningAsAdmin());
 
             ::AppInstaller::Repository::Source sourceToAdd = CreateSourceFromOptions(options);
 
-            THROW_HR_IF(APPINSTALLER_CLI_ERROR_AUTHENTICATION_TYPE_NOT_SUPPORTED, sourceToAdd.GetInformation().Authentication.Type == ::AppInstaller::Authentication::AuthenticationType::Unknown);
-
-            if (!sourceToAdd.CheckSourceAgreements())
-            {
-                THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_AGREEMENTS_NOT_ACCEPTED, !options.AcceptSourceAgreements());
-                sourceToAdd.SaveAcceptedSourceAgreements();
-            }
-
             auto report_progress{ co_await winrt::get_progress_token() };
             co_await winrt::resume_background();
 
@@ -1333,7 +1363,10 @@ namespace winrt::Microsoft::Management::Deployment::implementation
 
             sourceToAdd.Add(progress);
         }
-        WINGET_CATALOG_CATCH_STORE(terminationHR, APPINSTALLER_CLI_ERROR_INTERNAL_ERROR);
+        catch (...)
+        {
+            terminationHR = AppInstaller::CLI::Workflow::HandleException(nullptr, std::current_exception());
+        }
 
         co_return GetAddPackageCatalogResult(terminationHR);
     }
@@ -1356,6 +1389,8 @@ namespace winrt::Microsoft::Management::Deployment::implementation
         HRESULT terminationHR = S_OK;
         try {
             // Check if running as admin
+            // [NOTE:] For OutOfProc calls, the Windows Package Manager Service executes in the context initiated by the caller process,
+            //so the same admin validation check is applicable for both InProc and OutOfProc calls.
             THROW_HR_IF(APPINSTALLER_CLI_ERROR_COMMAND_REQUIRES_ADMIN, !AppInstaller::Runtime::IsRunningAsAdmin());
 
             auto matchingSource = GetMatchingSource(winrt::to_string(options.Name()));
@@ -1388,7 +1423,10 @@ namespace winrt::Microsoft::Management::Deployment::implementation
                 sourceToRemove.Remove(progress);
             }
         }
-        WINGET_CATALOG_CATCH_STORE(terminationHR, APPINSTALLER_CLI_ERROR_INTERNAL_ERROR);
+        catch (...)
+        {
+            terminationHR = AppInstaller::CLI::Workflow::HandleException(nullptr, std::current_exception());
+        }
 
         co_return GetRemovePackageCatalogResult(terminationHR);
     }