diff --git a/Jamroot.jam b/Jamroot.jam index 95103f3fa5e..31766c2fabc 100644 --- a/Jamroot.jam +++ b/Jamroot.jam @@ -951,6 +951,14 @@ else .installer-vendor-files-location = $(PWIZ_BUILD_PATH)/without-cxt/$(PLATFORM) ; } +# Assemblies that every installer already declares for itself. They reach the +# enumeration below because a vendor API references them, but listing them here as well would +# declare the same file twice and fail the WiX link with LGHT0091 (duplicate symbol). +# pwiz.CommonUtil.dll is ours rather than a vendor redistributable, and is named directly by +# scripts/wix/pwiz-setup.wxs.template, pwiz_tools/Bumbershoot/idpicker/Deploy/Deploy.wxs.template +# and pwiz_tools/Skyline/Executables/Installer/FileList64-template.txt. +.installer-vendor-files-exclusions = pwiz.CommonUtil.dll ; + rule make_INSTALLER_VENDOR_FILES ( targets * : sources * : properties * ) { # enumerate .NET assemblies and their native DLL dependencies @@ -961,7 +969,8 @@ rule make_INSTALLER_VENDOR_FILES ( targets * : sources * : properties * ) for local assembly in $(assemblies) { local assembly-path = [ path.basename [ $(assembly).name ] ] ; - if ! $(assembly-path:L) in $(.unique-vendor-files:L) + if ! $(assembly-path:L) in $(.unique-vendor-files:L) && + ! $(assembly-path:L) in $(.installer-vendor-files-exclusions:L) { .unique-vendor-files += $(assembly-path) ; } diff --git a/libraries/boost-build/src/tools/msvc.jam b/libraries/boost-build/src/tools/msvc.jam index f98b0b56766..3b34847ef2a 100644 --- a/libraries/boost-build/src/tools/msvc.jam +++ b/libraries/boost-build/src/tools/msvc.jam @@ -861,6 +861,11 @@ rule set-assemblies ( target : properties * ) { for local assembly in $(assemblies) { + # an assembly can be generated by this build rather than vendored as a prebuilt file, + # in which case the compile referencing it with /FU has to wait for it to be built; + # copy-assemblies already declares the same edge for the copy it makes + DEPENDS $(target) : [ $(assembly).actualize ] ; + local assembly-path = [ $(assembly).name ] ; if $(assembly-path:S) != ".pdb" { @@ -883,6 +888,8 @@ rule set-assemblies ( target : properties * ) local assembly-include-paths ; for local dependency in $(assembly-dependencies) { + DEPENDS $(target) : [ $(dependency).actualize ] ; + local dependency-path = [ $(dependency).name ] ; if ! [ path.is-rooted $(dependency-path) ] { diff --git a/pwiz_aux/msrc/utility/vendor_api/UNIFI/Jamfile.jam b/pwiz_aux/msrc/utility/vendor_api/UNIFI/Jamfile.jam index 18cf0e4e3f5..ff4839cfb57 100644 --- a/pwiz_aux/msrc/utility/vendor_api/UNIFI/Jamfile.jam +++ b/pwiz_aux/msrc/utility/vendor_api/UNIFI/Jamfile.jam @@ -45,9 +45,54 @@ if [ modules.peek : NT ] System.Memory.dll System.Numerics.Vectors.dll System.Runtime.CompilerServices.Unsafe.dll + System.Text.Json.dll System.Threading.Tasks.Extensions.dll System.ValueTuple.dll ; +# pwiz.CommonUtil.dll supplies OAuthPasswordGrantClient - the IdentityModel-7-compatible OAuth +# password-grant request shared with the managed WatersConnect/Unifi account classes, so this +# native reader does not duplicate that request logic in C++/CLI. Unlike everything in +# .shared-assemblies it is built from this repo rather than vendored, so it is declared as a +# target here and referenced by target ID below: is a dependency feature, so bjam +# builds this before compiling anything that references it, and msvc.jam's copy-assemblies +# stages it next to each consuming binary. +# +# MSBuild, not "dotnet build": the .NET SDK's MSBuild resolves cultures through ICU, which does +# not know the legacy zh-CHS of CommonUtil's satellite .resx files, so it gives them the same +# manifest name as the neutral resources and fails with MSB3577. Framework/VS MSBuild uses NLS +# and still recognizes zh-CHS. +rule build-common-util-properties ( targets + : sources * : properties * ) +{ + local .msvcSetupScript = [ get-current-msvc-setup-script $(properties) ] ; + JAM_SEMAPHORE on $(targets) = "dotNetSemaphore" ; + MSVC_CURRENT_SETUP_SCRIPT on $(targets) = $(.msvcSetupScript) ; +} + +rule do_build_common_util ( targets + : sources * : properties * ) +{ + return [ build-common-util-properties $(targets) : $(sources) : $(properties) ] ; +} + +actions do_build_common_util +{ + $(MSVC_CURRENT_SETUP_SCRIPT) + echo Building pwiz.CommonUtil.dll for pwiz_vendor_api_unifi + msbuild "$(PWIZ_ROOT_PATH)\pwiz_tools\Shared\CommonUtil\CommonUtil.csproj" /p:Configuration=Release;Platform=x64;OutDir=$(<:D)\ /nologo /verbosity:minimal + set status=%ERRORLEVEL% + exit %status% +} + +make pwiz.CommonUtil.dll + : # sources + : # actions + @do_build_common_util + : # requirements + @no-express-requirement + @msvc-dotnet-requirement + ; + +explicit pwiz.CommonUtil.dll ; + rule vendor-api-requirements ( properties * ) { local result ; @@ -67,6 +112,7 @@ rule vendor-api-requirements ( properties * ) #result += $(dll_location)/unifi-protobuf-net.dll ; result += $(PWIZ_ROOT_PATH)/pwiz_aux/msrc/utility/vendor_api/ABI/protobuf-net.dll ; result += $(PWIZ_ROOT_PATH)/pwiz_tools/Shared/Lib/$(.shared-assemblies) ; + result += $(PWIZ_ROOT_PATH)/pwiz_aux/msrc/utility/vendor_api/UNIFI//pwiz.CommonUtil.dll ; result += $(dll_location)/System.Runtime.Caching.Generic.dll ; result += $(dll_location)/ParallelExtensionsExtras.dll ; result += $(dll_location) ; diff --git a/pwiz_aux/msrc/utility/vendor_api/UNIFI/UnifiData.cpp b/pwiz_aux/msrc/utility/vendor_api/UNIFI/UnifiData.cpp index cee6bdb74e3..25e243e1b88 100644 --- a/pwiz_aux/msrc/utility/vendor_api/UNIFI/UnifiData.cpp +++ b/pwiz_aux/msrc/utility/vendor_api/UNIFI/UnifiData.cpp @@ -63,8 +63,8 @@ using System::Threading::Tasks::Task; using System::Threading::Tasks::TaskScheduler; using System::Threading::Tasks::Schedulers::QueuedTaskScheduler; using System::Uri; -using IdentityModel::Client::TokenClient; using IdentityModel::Client::TokenResponse; +using pwiz::Common::SystemUtil::OAuthPasswordGrantClient; using std::size_t; @@ -1002,14 +1002,8 @@ class UnifiData::Impl password = userPassPair[1]; } - auto fields = gcnew System::Collections::Generic::Dictionary(); - fields->Add(IdentityModel::OidcConstants::TokenRequest::GrantType, IdentityModel::OidcConstants::GrantTypes::Password); - fields->Add(IdentityModel::OidcConstants::TokenRequest::UserName, username); - fields->Add(IdentityModel::OidcConstants::TokenRequest::Password, password); - fields->Add(IdentityModel::OidcConstants::TokenRequest::Scope, _clientScope); - - auto tokenClient = gcnew TokenClient(tokenEndpoint(), _clientId, _clientSecret, nullptr, IdentityModel::Client::AuthenticationStyle::BasicAuthentication); - TokenResponse^ response = tokenClient->RequestAsync(fields, System::Threading::CancellationToken::None)->Result; + TokenResponse^ response = OAuthPasswordGrantClient::RequestToken(gcnew Uri(tokenEndpoint()), _clientId, _clientSecret, + OAuthPasswordGrantClient::PasswordGrantForm(username, password, _clientScope)); if (response->IsError) throw user_error("authentication error: incorrect hostname, username or password? (" + ToStdString(response->Error) + ")"); diff --git a/pwiz_aux/msrc/utility/vendor_api/UNIFI/WatersConnectData.ipp b/pwiz_aux/msrc/utility/vendor_api/UNIFI/WatersConnectData.ipp index e57eb39eeba..71526221da1 100644 --- a/pwiz_aux/msrc/utility/vendor_api/UNIFI/WatersConnectData.ipp +++ b/pwiz_aux/msrc/utility/vendor_api/UNIFI/WatersConnectData.ipp @@ -61,8 +61,8 @@ using System::Threading::Tasks::TaskScheduler; using System::Threading::Tasks::Schedulers::QueuedTaskScheduler; using System::Net::Http::HttpClient; using System::Uri; -using IdentityModel::Client::TokenClient; using IdentityModel::Client::TokenResponse; +using pwiz::Common::SystemUtil::OAuthPasswordGrantClient; using std::size_t; #include "WatersConnectProtobuf.hpp" @@ -475,14 +475,8 @@ private: static Object^ getAccessTokenResult(String^ uri, AccessTokenRequest^ request) { - auto fields = gcnew System::Collections::Generic::Dictionary(); - fields->Add(IdentityModel::OidcConstants::TokenRequest::GrantType, IdentityModel::OidcConstants::GrantTypes::Password); - fields->Add(IdentityModel::OidcConstants::TokenRequest::UserName, request->Username); - fields->Add(IdentityModel::OidcConstants::TokenRequest::Password, request->Password); - fields->Add(IdentityModel::OidcConstants::TokenRequest::Scope, request->Scope); - - auto tokenClient = gcnew TokenClient(request->Uri, request->ClientId, request->Secret, nullptr); - TokenResponse^ response = tokenClient->RequestAsync(fields, System::Threading::CancellationToken::None)->Result; + TokenResponse^ response = OAuthPasswordGrantClient::RequestToken(gcnew Uri(request->Uri), request->ClientId, request->Secret, + OAuthPasswordGrantClient::PasswordGrantForm(request->Username, request->Password, request->Scope)); if (response->IsError) throw user_error("authentication error: incorrect hostname, username or password? (" + ToStdString(response->Error) + ")"); return gcnew KeyValuePair(response->AccessToken, DateTime::UtcNow.AddSeconds(response->ExpiresIn)); diff --git a/pwiz_tools/MSConvertGUI/Jamfile.jam b/pwiz_tools/MSConvertGUI/Jamfile.jam index dd8c382d4a7..ec0a16c18c1 100644 --- a/pwiz_tools/MSConvertGUI/Jamfile.jam +++ b/pwiz_tools/MSConvertGUI/Jamfile.jam @@ -57,11 +57,33 @@ if [ modules.peek : NT ] local location = [ path.make [ feature.get-values location : $(properties) ] ] ; local output_path = [ path.native $(location)/ ] ; # OutDir requires trailing slash OUTPUT_PATH on $(<[1]) = $(output_path) ; - local intermediate_path = "BaseIntermediateOutputPath=$(PWIZ_BUILD_PATH)/obj/" ; + + # One intermediate directory per OutDir, and none of them the directory bjam + # stages assemblies into. BaseIntermediateOutputPath is a global property, so + # every project in the solution compiles there - CommonUtil included - while the + # UNIFI vendor API declares pwiz.CommonUtil.dll as an , which makes + # copy-assemblies hardlink it into build-nt-x86\obj\$(PLATFORM)\$(config) + # alongside the pwiz_bindings_cli installed there. Staging actions carry no + # JAM_SEMAPHORE, so sharing that directory races the hardlink against csc and + # csc fails with CS2012, "being used by another process". The solution is also + # built once per (the bin copy and the copy gui_tools stages), hence + # keying on OutDir. Keep the trees under PWIZ_BUILD_PATH so clean removes them, + # and out of OutDir so the installer harvest does not see them. + local build_path = [ path.make $(PWIZ_BUILD_PATH) ] ; + local obj_subdir = [ path.relative $(location) $(build_path) : no-error ] ; + if $(obj_subdir) = not-a-child + { + obj_subdir = [ path.basename $(location) ] ; + } + local intermediate_path = "BaseIntermediateOutputPath=$(PWIZ_BUILD_PATH)/obj/$(obj_subdir)/" ; + local native_deps_path = [ path.native $(PWIZ_BUILD_PATH)/MSConvertGUI/bin/$(PLATFORM)/$(config)/ ] ; # PwizCli properties must use separate /p: args to avoid cmd.exe misinterpreting \; in paths # PwizCliHintPath points to install-native-dependencies location, not OutDir (they may differ) - MSBUILD_PARAMETERS on $(<[1]) = "/p:Configuration=$(config);Platform=$(PLATFORM);$(intermediate_path);OutDir=$(output_path) /p:PwizCliAssembly=pwiz_bindings_cli /p:PwizCliHintPath=$(native_deps_path)pwiz_bindings_cli.dll" ; + # PwizBuildPath is derived from BaseIntermediateOutputPath in MSConvertGUI.csproj and is + # how it resolves pwiz_bindings_cli.dll, so pass it now that the intermediate directory + # is no longer a single level under the build path. + MSBUILD_PARAMETERS on $(<[1]) = "/p:Configuration=$(config);Platform=$(PLATFORM);$(intermediate_path);OutDir=$(output_path) /p:PwizBuildPath=$(PWIZ_BUILD_PATH) /p:PwizCliAssembly=pwiz_bindings_cli /p:PwizCliHintPath=$(native_deps_path)pwiz_bindings_cli.dll" ; JAM_SEMAPHORE on $(targets) = "dotNetSemaphore" ; MSVC_CURRENT_SETUP_SCRIPT on $(targets[1]) = [ get-current-msvc-setup-script $(properties) ] ; } diff --git a/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiAccount.cs b/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiAccount.cs index 56ad0fc1265..953a0ae40cc 100644 --- a/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiAccount.cs +++ b/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiAccount.cs @@ -16,6 +16,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +using System; using System.Collections.Generic; using System.Linq; using System.Net.Http; @@ -127,15 +128,19 @@ public string GetFoldersUrl() public TokenResponse Authenticate() { - var tokenClient = new TokenClient(IdentityServer + IdentityConnectEndpoint, ClientId, - ClientSecret, new HttpClientHandler()); - return tokenClient.RequestResourceOwnerPasswordAsync(Username, Password, ClientScope).Result; + // Shared with WatersConnectAccount.RequestToken, which authenticates against a + // sibling Waters-hosted identity server the same way - see OAuthPasswordGrantClient + // for the POST, response parsing, and why both needed to stop constructing + // TokenResponse directly once IdentityModel 7 removed its constructors. + return OAuthPasswordGrantClient.RequestToken(new Uri(IdentityServer + IdentityConnectEndpoint), ClientId, ClientSecret, + OAuthPasswordGrantClient.PasswordGrantForm(Username, Password, ClientScope)); } public IEnumerable GetFolders() { - var httpClient = GetAuthenticatedHttpClient(); - var response = httpClient.GetAsync(GetFoldersUrl()).Result; + using var httpClient = GetAuthenticatedHttpClient(); + using var request = new HttpRequestMessage(HttpMethod.Get, GetFoldersUrl()); + using var response = httpClient.SendRequest(request); string responseBody = response.Content.ReadAsStringAsync().Result; var jsonObject = JObject.Parse(responseBody); @@ -149,9 +154,10 @@ public IEnumerable GetFolders() public IEnumerable GetFiles(UnifiFolderObject folder) { - var httpClient = GetAuthenticatedHttpClient(); + using var httpClient = GetAuthenticatedHttpClient(); string url = string.Format(@"/unifi/v1/folders({0})/items", folder.Id); - var response = httpClient.GetAsync(ServerUrl + url).Result; + using var request = new HttpRequestMessage(HttpMethod.Get, ServerUrl + url); + using var response = httpClient.SendRequest(request); string responseBody = response.Content.ReadAsStringAsync().Result; var jsonObject = JObject.Parse(responseBody); var itemsValue = jsonObject[@"value"] as JArray; @@ -162,13 +168,21 @@ public IEnumerable GetFiles(UnifiFolderObject folder) return itemsValue.OfType().Select(f => new UnifiFileObject(f)); } - public HttpClient GetAuthenticatedHttpClient() + public HttpClientWithProgress GetAuthenticatedHttpClient() { var tokenResponse = Authenticate(); - var httpClient = new HttpClient(); - httpClient.SetBearerToken(tokenResponse.AccessToken); - httpClient.DefaultRequestHeaders.Remove(@"Accept"); - httpClient.DefaultRequestHeaders.Add(@"Accept", @"application/json;odata.metadata=minimal"); + if (tokenResponse.IsError) + { + // Without this the request goes out with an empty bearer token and the server + // answers 401, hiding what the identity server actually said. Matches what + // WatersConnectAccount.Authenticate does for the sibling server. + throw new RemoteServerException(string.Format( + UnifiResources.UnifiAccount_GetAuthenticatedHttpClient_Failed_to_authenticate_UNIFI_account__0__with_error___1_, + Username, tokenResponse.ErrorDescription ?? tokenResponse.Error), tokenResponse.Raw); + } + var httpClient = new HttpClientWithProgress(); + httpClient.AddAuthorizationHeader(@"Bearer " + tokenResponse.AccessToken); + httpClient.AddHeader(@"Accept", @"application/json;odata.metadata=minimal"); return httpClient; } diff --git a/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiResources.designer.cs b/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiResources.designer.cs index 6c491b3cfa6..7c51183f1b8 100644 --- a/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiResources.designer.cs +++ b/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiResources.designer.cs @@ -60,6 +60,16 @@ internal UnifiResources() { } } + /// + /// Looks up a localized string similar to Failed to authenticate UNIFI account {0} with error: {1}. + /// + public static string UnifiAccount_GetAuthenticatedHttpClient_Failed_to_authenticate_UNIFI_account__0__with_error___1_ { + get { + return ResourceManager.GetString("UnifiAccount_GetAuthenticatedHttpClient_Failed_to_authenticate_UNIFI_account__0__" + + "with_error___1_", resourceCulture); + } + } + /// /// Looks up a localized string similar to Cannot find account for username {0} and server {1}.. /// diff --git a/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiResources.resx b/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiResources.resx index ed279a64a31..5072c5ee820 100644 --- a/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiResources.resx +++ b/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiResources.resx @@ -118,6 +118,9 @@ System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + Failed to authenticate UNIFI account {0} with error: {1} + Cannot find account for username {0} and server {1}. diff --git a/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiSession.cs b/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiSession.cs index d6345caa555..7a57580297a 100644 --- a/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiSession.cs +++ b/pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiSession.cs @@ -19,6 +19,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Net.Http; using Newtonsoft.Json.Linq; using pwiz.Common.Collections; @@ -44,9 +45,11 @@ public override bool AsyncFetchContents(RemoteUrl remoteUrl, out RemoteServerExc private ImmutableList GetFolders(Uri requestUri) { - var httpClient = UnifiAccount.GetAuthenticatedHttpClient(); - var response = httpClient.GetAsync(requestUri).Result; - response.EnsureSuccessStatusCode(); + using var httpClient = UnifiAccount.GetAuthenticatedHttpClient(); + // SendRequest already throws NetworkRequestException on a non-2xx response, so there + // is nothing left for an EnsureSuccessStatusCode() to catch here. + using var request = new HttpRequestMessage(HttpMethod.Get, requestUri); + using var response = httpClient.SendRequest(request); string responseBody = response.Content.ReadAsStringAsync().Result; var jsonObject = JObject.Parse(responseBody); @@ -60,9 +63,9 @@ private ImmutableList GetFolders(Uri requestUri) private ImmutableList GetFiles(Uri requestUri) { - var httpClient = UnifiAccount.GetAuthenticatedHttpClient(); - var response = httpClient.GetAsync(requestUri).Result; - response.EnsureSuccessStatusCode(); + using var httpClient = UnifiAccount.GetAuthenticatedHttpClient(); + using var request = new HttpRequestMessage(HttpMethod.Get, requestUri); + using var response = httpClient.SendRequest(request); string responseBody = response.Content.ReadAsStringAsync().Result; var jsonObject = JObject.Parse(responseBody); var itemsValue = jsonObject[@"value"] as JArray; diff --git a/pwiz_tools/Shared/CommonMsData/RemoteApi/WatersConnect/WatersConnectAccount.cs b/pwiz_tools/Shared/CommonMsData/RemoteApi/WatersConnect/WatersConnectAccount.cs index 95f43855651..d5b55edaa23 100644 --- a/pwiz_tools/Shared/CommonMsData/RemoteApi/WatersConnect/WatersConnectAccount.cs +++ b/pwiz_tools/Shared/CommonMsData/RemoteApi/WatersConnect/WatersConnectAccount.cs @@ -21,9 +21,7 @@ using System.Collections.Generic; using System.Collections.Specialized; using System.Globalization; -using System.Net; using System.Security.Authentication; -using System.Text; using System.Xml; using System.Xml.Linq; using System.Xml.Serialization; @@ -226,13 +224,7 @@ public TokenResponse Authenticate() } } // Otherwise, request a new token using the username and password - var newToken = RequestToken(new NameValueCollection - { - [@"grant_type"] = @"password", - [@"username"] = Username, - [@"password"] = Password, - [@"scope"] = ClientScope - }); + var newToken = RequestToken(OAuthPasswordGrantClient.PasswordGrantForm(Username, Password, ClientScope)); if (newToken.IsError) { AuthenticationException ex; @@ -263,35 +255,11 @@ public TokenResponse Authenticate() /// private TokenResponse RequestToken(NameValueCollection form) { - try - { - var requestUri = new Uri(IdentityServer + IdentityConnectEndpoint); - using var httpClient = new HttpClientWithProgress(); - httpClient.AddAuthorizationHeader(@"Basic " + Convert.ToBase64String(Encoding.UTF8.GetBytes( - EscapeClientCredential(ClientId) + @":" + EscapeClientCredential(ClientSecret)))); - httpClient.AddHeader(@"Accept", @"application/json"); - var raw = Encoding.UTF8.GetString(httpClient.UploadValues(requestUri, @"POST", form)); - return new TokenResponse(raw); - } - catch (NetworkRequestException ex) - { - if (ex.StatusCode == HttpStatusCode.BadRequest && !string.IsNullOrEmpty(ex.ResponseBody)) - return new TokenResponse(ex.ResponseBody); - return new TokenResponse(ex.StatusCode ?? HttpStatusCode.ServiceUnavailable, ex.Message, ex.ResponseBody); - } - catch (Exception ex) - { - return new TokenResponse(ex); - } - } - - /// - /// RFC 6749 section 2.3.1: client_id and client_secret are form-urlencoded before being - /// combined into the Basic authorization credential. - /// - private static string EscapeClientCredential(string value) - { - return Uri.EscapeDataString(value ?? string.Empty).Replace(@"%20", @"+"); + // Shared with UnifiAccount.Authenticate, which authenticates against a sibling + // Waters-hosted identity server the same way - see OAuthPasswordGrantClient for the + // POST, response parsing, and why both needed to stop constructing TokenResponse + // directly once IdentityModel 7 removed its constructors. + return OAuthPasswordGrantClient.RequestToken(new Uri(IdentityServer + IdentityConnectEndpoint), ClientId, ClientSecret, form); } public static AuthenticationErrorType HandleAuthenticationException(AuthenticationException ex, out string message) @@ -305,18 +273,26 @@ public static AuthenticationErrorType HandleAuthenticationException(Authenticati try { var tokenResponse = JObject.Parse((string)ex.Data[TOKEN_DATA]); + // error_description is frequently empty (e.g. Waters' invalid_scope response is just + // {"error":"invalid_scope"}), so fall back to the bare error code rather than leaving + // the caller with nothing to show - every classified branch below sets message for the + // same reason. Only EditRemoteAccountDlg's InvalidClientSecret case overrides this with + // a friendlier string; the others show this raw (deliberately non-L10N) server text. string error = (tokenResponse[@"error_description"] ?? tokenResponse[@"error"] ?? "").ToString(); var errorType = (tokenResponse[@"error"] ?? "").ToString(); if (errorType == @"invalid_scope") { + message = error; return AuthenticationErrorType.InvalidClientScope; } else if (errorType == @"invalid_client") { + message = error; return AuthenticationErrorType.InvalidClientSecret; } else if (errorType == @"invalid_grant") { + message = error; return AuthenticationErrorType.InvalidPassword; } else if (!string.IsNullOrEmpty(error)) @@ -326,6 +302,7 @@ public static AuthenticationErrorType HandleAuthenticationException(Authenticati } else { + message = ex.Message; return AuthenticationErrorType.InvalidIdentityServer; } } diff --git a/pwiz_tools/Shared/CommonUtil/CommonUtil.csproj b/pwiz_tools/Shared/CommonUtil/CommonUtil.csproj index 160dfcab877..096445ea804 100644 --- a/pwiz_tools/Shared/CommonUtil/CommonUtil.csproj +++ b/pwiz_tools/Shared/CommonUtil/CommonUtil.csproj @@ -90,6 +90,9 @@ false + + ..\Lib\IdentityModel.dll + ..\Lib\JetBrains.Annotations.dll @@ -245,6 +248,7 @@ + diff --git a/pwiz_tools/Shared/CommonUtil/SystemUtil/OAuthPasswordGrantClient.cs b/pwiz_tools/Shared/CommonUtil/SystemUtil/OAuthPasswordGrantClient.cs new file mode 100644 index 00000000000..bd65a0e35aa --- /dev/null +++ b/pwiz_tools/Shared/CommonUtil/SystemUtil/OAuthPasswordGrantClient.cs @@ -0,0 +1,133 @@ +/* + * Original author: Matt Chambers + * AI assistance: Claude Code (Claude Opus 5) + * + * Copyright 2026 University of Washington - Seattle, WA + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +using System; +using System.Collections.Specialized; +using System.Diagnostics; +using System.Net; +using System.Net.Http; +using System.Text; +using IdentityModel.Client; + +namespace pwiz.Common.SystemUtil +{ + /// + /// Posts an OAuth 2.0 token request (RFC 6749) to an identity server and parses the result, + /// shared by every account type that authenticates against one directly rather than through + /// a UI-driven login (CommonMsData's WatersConnectAccount and UnifiAccount - both connect to + /// a Waters-hosted identity server of the same design, one with a password grant and one with + /// password+refresh) as well as the native C++/CLI UNIFI/WatersConnect vendor readers, which + /// call this directly instead of duplicating the IdentityModel-7 request shape in C++/CLI. + /// + public static class OAuthPasswordGrantClient + { + /// + /// Builds the RFC 6749 4.3 "resource owner password credentials" grant form - the same + /// four fields for every caller here, so this is the one place that spells them out. + /// + public static NameValueCollection PasswordGrantForm(string username, string password, string scope) + { + return new NameValueCollection + { + [@"grant_type"] = @"password", + [@"username"] = username, + [@"password"] = password, + [@"scope"] = scope + }; + } + + /// + /// POSTs to with the client + /// credentials in an HTTP Basic authorization header, and returns the parsed response. + /// Every failure - protocol, HTTP, or transport - is returned as an error + /// rather than thrown, so callers route all failures through + /// a single path: a 400 is an OAuth protocol error whose JSON body carries + /// error/error_description; any other HTTP failure becomes an HTTP-error response + /// (IsError true even when the body is a proxy's HTML page); a transport or URL-format + /// exception becomes an exception-type response. + /// + public static TokenResponse RequestToken(Uri tokenEndpoint, string clientId, string clientSecret, NameValueCollection form) + { + try + { + using var httpClient = new HttpClientWithProgress(); + httpClient.AddAuthorizationHeader(@"Basic " + Convert.ToBase64String(Encoding.UTF8.GetBytes( + EscapeClientCredential(clientId) + @":" + EscapeClientCredential(clientSecret)))); + httpClient.AddHeader(@"Accept", @"application/json"); + var raw = Encoding.UTF8.GetString(httpClient.UploadValues(tokenEndpoint, @"POST", form)); + return ParseTokenResponse(HttpStatusCode.OK, raw); + } + catch (NetworkRequestException ex) + { + // HttpClientWithProgress throws on every non-2xx, so the OAuth error body arrives + // here rather than as a response. Hand the status and body to IdentityModel and let + // it decide protocol-error vs HTTP-error instead of restating those rules. + try + { + return ParseTokenResponse(ex.StatusCode ?? HttpStatusCode.ServiceUnavailable, ex.ResponseBody, ex.Message); + } + catch (Exception parseEx) + { + // Reconstituting the response is the one step outside the try above, and no + // failure may escape this method - ReasonPhrase, for one, rejects a message + // that contains a line break. Report the network failure rather than the + // parse failure: it is the error the user needs, and passing a message to + // FromException would replace it. + Debug.WriteLine($@"Failed to rebuild the token response: {parseEx.Message}"); + return ProtocolResponse.FromException(ex); + } + } + catch (Exception ex) + { + return ProtocolResponse.FromException(ex); + } + } + + /// + /// Builds a from a status and body. IdentityModel 7 left + /// with only a parameterless constructor; + /// replaced the ones this code was + /// originally written against, and it takes an . + /// never yields one for a failure - it throws - so the + /// failure path in reconstitutes the response here. + /// + private static TokenResponse ParseTokenResponse(HttpStatusCode status, string body, string reason = null) + { + using var response = new HttpResponseMessage(status); + response.Content = new StringContent(body ?? string.Empty, Encoding.UTF8, @"application/json"); + if (reason != null) + response.ReasonPhrase = reason; + // The body is already in memory, so FromHttpResponseAsync's only await completes + // synchronously and the returned task is ALREADY COMPLETE. There is no continuation to + // post to the captured WinForms SynchronizationContext, so reading .Result cannot + // deadlock and needs no Task.Run to escape it. That safety depends on the content being + // in memory; do not turn this into an async chain. + return ProtocolResponse.FromHttpResponseAsync(response).Result; + } + + /// + /// RFC 6749 section 2.3.1: client_id and client_secret are form-urlencoded before being + /// combined into the Basic authorization credential. + /// + private static string EscapeClientCredential(string value) + { + return Uri.EscapeDataString(value ?? string.Empty).Replace(@"%20", @"+"); + } + } +} diff --git a/pwiz_tools/Shared/Lib/IdentityModel.dll b/pwiz_tools/Shared/Lib/IdentityModel.dll index fef274ed38c..0ef9a3f83ff 100644 Binary files a/pwiz_tools/Shared/Lib/IdentityModel.dll and b/pwiz_tools/Shared/Lib/IdentityModel.dll differ diff --git a/pwiz_tools/Shared/Lib/System.Text.Json.dll b/pwiz_tools/Shared/Lib/System.Text.Json.dll new file mode 100644 index 00000000000..e8bee3a0f34 Binary files /dev/null and b/pwiz_tools/Shared/Lib/System.Text.Json.dll differ diff --git a/pwiz_tools/Skyline/Alerts/ArdiaLoginDlg.cs b/pwiz_tools/Skyline/Alerts/ArdiaLoginDlg.cs index 5822b8651ca..8744ef1749c 100644 --- a/pwiz_tools/Skyline/Alerts/ArdiaLoginDlg.cs +++ b/pwiz_tools/Skyline/Alerts/ArdiaLoginDlg.cs @@ -1227,7 +1227,10 @@ private async Task RequestUserTokenAsync(DeviceAuthorizationRespo { Address = tokenEndpoint, ClientId = @"ardia.device.client.registration", // Hard coded for initial connection to get device registration - DeviceCode = deviceAuthorizationResponse.DeviceCode + // IdentityModel 7 annotates DeviceCode as nullable; a successful device + // authorization always carries one, and passing it on unchanged keeps the + // server's own error the one the user sees if it somehow did not. + DeviceCode = deviceAuthorizationResponse.DeviceCode! }); if (!response.IsError) diff --git a/pwiz_tools/Skyline/Executables/Installer/FileList64-template.txt b/pwiz_tools/Skyline/Executables/Installer/FileList64-template.txt index 34af5c17329..fcf4929362c 100644 --- a/pwiz_tools/Skyline/Executables/Installer/FileList64-template.txt +++ b/pwiz_tools/Skyline/Executables/Installer/FileList64-template.txt @@ -292,7 +292,7 @@ System.Reflection.Emit.Lightweight.dll System.Runtime.Caching.Generic.dll (included automatically from ProteoWizard; DO NOT ADD TO THE WXS TEMPLATE!) System.Runtime.CompilerServices.Unsafe.dll System.Text.Encodings.Web.dll -System.Text.Json.dll +System.Text.Json.dll (included automatically from ProteoWizard; DO NOT ADD TO THE WXS TEMPLATE!) ThermoFisher.CommonCore.Data.dll (included automatically from ProteoWizard; DO NOT ADD TO THE WXS TEMPLATE!) ThermoFisher.CommonCore.RawFileReader.dll (included automatically from ProteoWizard; DO NOT ADD TO THE WXS TEMPLATE!) timsdata.dll (included automatically from ProteoWizard; DO NOT ADD TO THE WXS TEMPLATE!) diff --git a/pwiz_tools/Skyline/Executables/Installer/Product-template.wxs b/pwiz_tools/Skyline/Executables/Installer/Product-template.wxs index 4ef8e0a3281..978bf442efa 100644 --- a/pwiz_tools/Skyline/Executables/Installer/Product-template.wxs +++ b/pwiz_tools/Skyline/Executables/Installer/Product-template.wxs @@ -276,9 +276,6 @@ - - - diff --git a/pwiz_tools/Skyline/TestConnected/UnifiFunctionalTest.cs b/pwiz_tools/Skyline/TestConnected/UnifiFunctionalTest.cs index 90c7326471b..04cf7c80497 100644 --- a/pwiz_tools/Skyline/TestConnected/UnifiFunctionalTest.cs +++ b/pwiz_tools/Skyline/TestConnected/UnifiFunctionalTest.cs @@ -40,6 +40,7 @@ public class UnifiFunctionalTest : AbstractFunctionalTestEx private string[] _dataPath; private string[] _filenames; private string _selectItem; + private int _curvesPerReplicate; private PointF? _chromatogramPoint; [TestMethod] @@ -55,6 +56,7 @@ public void TestUnifi() _dataPath = new[] { "Company", "Demo Department", "Peptides", }; _filenames = new[] { "Hi3_ClpB_MSe_01" }; _selectItem = "Molecule:/sp|P0A6A8|ACP_ECOLI/ITTVQAAIDYINGHQA"; + _curvesPerReplicate = 1; _chromatogramPoint = new PointF(4.0f, 3.25f); RunFunctionalTest(); } @@ -72,6 +74,7 @@ public void TestWatersConnect() _dataPath = new[] { "Company", "Skyline", "SmallMolOptimization", "Scheduled", }; _filenames = new[] { "ID33140_03a_WAA253_4814_092017", "ID33141_03a_WAA253_4814_092017" }; _selectItem = "Molecule:/Nucleotide metabolism/UDP"; + _curvesPerReplicate = 2; _chromatogramPoint = null; RunFunctionalTest(); @@ -107,25 +110,50 @@ protected override void DoTest() RunUI(() => editAccountDlg.SetRemoteAccount(_testAccount.ChangeServerUrl("https://asdfdsafads.local"))); // non-resolving hostname AssertAlertDlgContainsMessage(() => editAccountDlg.TestSettings(), "The remote name could not be resolved"); - // Test invalid client id, scope, and secret - RunUI(() => editAccountDlg.SetRemoteAccount((_testAccount as WatersConnectAccount)!.ChangeClientId("foobar"))); - AssertAlertDlgContainsMessage(() => editAccountDlg.TestSettings(), ToolsUIResources.EditRemoteAccountDlg_TestWatersConnectAccount_invalid_client_id_or_secret); - RunUI(() => editAccountDlg.SetRemoteAccount((_testAccount as WatersConnectAccount)!.ChangeClientSecret("foobar"))); - AssertAlertDlgContainsMessage(() => editAccountDlg.TestSettings(), ToolsUIResources.EditRemoteAccountDlg_TestWatersConnectAccount_invalid_client_id_or_secret); - RunUI(() => editAccountDlg.SetRemoteAccount((_testAccount as WatersConnectAccount)!.ChangeClientScope("foobar"))); - AssertAlertDlgContainsMessage(() => editAccountDlg.TestSettings(), "invalid_scope"); // not L10N - - // Test invalid password, the error message tested is a non-L10N string from Waters server - RunUI(() => editAccountDlg.SetRemoteAccount(_testAccount.ChangePassword("wrongpassword"))); - AssertAlertDlgContainsMessage(() => editAccountDlg.TestSettings(), "password entered for this user is incorrect"); + // waters_connect only below this point: hard-cast client id/scope/secret manipulation, + // and the invalid-password message text, which is the wire text from the Waters server + // and not something Unifi's server necessarily matches. Added in d1c5c45927 (#3386) for + // waters_connect and never guarded, so it null-referenced TestUnifi the first time these + // ran against a real Unifi account (_testAccount as WatersConnectAccount is null there). + if (_testAccount is WatersConnectAccount) + { + // Test invalid client id, scope, and secret + RunUI(() => editAccountDlg.SetRemoteAccount((_testAccount as WatersConnectAccount)!.ChangeClientId("foobar"))); + AssertAlertDlgContainsMessage(() => editAccountDlg.TestSettings(), ToolsUIResources.EditRemoteAccountDlg_TestWatersConnectAccount_invalid_client_id_or_secret); + RunUI(() => editAccountDlg.SetRemoteAccount((_testAccount as WatersConnectAccount)!.ChangeClientSecret("foobar"))); + AssertAlertDlgContainsMessage(() => editAccountDlg.TestSettings(), ToolsUIResources.EditRemoteAccountDlg_TestWatersConnectAccount_invalid_client_id_or_secret); + RunUI(() => editAccountDlg.SetRemoteAccount((_testAccount as WatersConnectAccount)!.ChangeClientScope("foobar"))); + AssertAlertDlgContainsMessage(() => editAccountDlg.TestSettings(), "invalid_scope"); // not L10N + + // Test invalid password, the error message tested is a non-L10N string from Waters server + RunUI(() => editAccountDlg.SetRemoteAccount(_testAccount.ChangePassword("wrongpassword"))); + AssertAlertDlgContainsMessage(() => editAccountDlg.TestSettings(), "password entered for this user is incorrect"); + } RunUI(() => editAccountDlg.SetRemoteAccount(_testAccount)); OkDialog(editAccountDlg, editAccountDlg.OkDialog); - RunUI(() => + if (_testAccount is WatersConnectAccount) { - openDataSourceDialog.SetCurrentDirectory((openDataSourceDialog.CurrentDirectory as RemoteUrl)!.ChangePathParts(_dataPath)); - }); + // waters_connect's ListContents resolves a full, multi-level ChangePathParts jump + // directly. + RunUI(() => + { + openDataSourceDialog.SetCurrentDirectory((openDataSourceDialog.CurrentDirectory as RemoteUrl)!.ChangePathParts(_dataPath)); + }); + } + else + { + // Unifi's UnifiSession.ListContents matches children only by the parent folder's + // real Id (a GUID assigned incrementally as each level is opened - see + // UnifiUrl.Id/ChangeId), so jumping straight to a multi-level ChangePathParts path + // never resolves: Id stays empty and ListContents(navUrl) returns nothing, which + // left openDataSourceDialog.ListItemNames permanently empty and hung the later + // WaitForConditionUI in OpenFile for the full 720-second timeout. Navigate one + // level at a time instead, exactly as clicking through the tree would. + foreach (var pathSegment in _dataPath) + OpenFile(openDataSourceDialog, pathSegment); + } foreach (var filename in _filenames) OpenFile(openDataSourceDialog, filename, false); RunUI(openDataSourceDialog.Open); @@ -141,11 +169,27 @@ protected override void DoTest() if (_selectItem == null) return; + // Multiple replicates dock their chromatogram graphs as tabs, and a GraphChromatogram + // that is not showing draws nothing, so tile them to make every replicate's graph + // visible before counting curves. + RunUI(SkylineWindow.ArrangeGraphsTiled); RunUI(() => SkylineWindow.SelectElement(ElementRefs.FromObjectReference(ElementLocator.Parse(_selectItem)))); - var chromGraph = FindOpenForm(); - WaitForConditionUI(5000, () => chromGraph.CurveCount == _filenames.Length); - Assert.AreEqual(_filenames.Length, chromGraph.CurveCount); + // Skyline creates one GraphChromatogram per replicate (SkylineWindow.GraphChromatograms), + // so FindOpenForm, which asserts the form is unique, only works while a single file is + // imported. Look up each replicate's own graph by the name the document ended up with: + // ImportResultsNameDlg above removes the common prefix and suffix, which leaves names + // that are nothing like _filenames (ID33140_03a_... and ID33141_03a_... become 0 and 1). + var replicateNames = SkylineWindow.Document.Settings.MeasuredResults.Chromatograms + .Select(chromatogramSet => chromatogramSet.Name).ToArray(); + Assert.AreEqual(_filenames.Length, replicateNames.Length); + foreach (var replicateName in replicateNames) + { + var chromGraph = SkylineWindow.GetGraphChrom(replicateName); + Assert.IsNotNull(chromGraph, replicateName); + WaitForConditionUI(5000, () => chromGraph.CurveCount == _curvesPerReplicate); + RunUI(() => Assert.AreEqual(_curvesPerReplicate, chromGraph.CurveCount, replicateName)); + } if (_chromatogramPoint != null) { diff --git a/scripts/misc/vcs_trigger_and_paths_config.py b/scripts/misc/vcs_trigger_and_paths_config.py index fd7b5e9c03e..f8b7a930d0a 100644 --- a/scripts/misc/vcs_trigger_and_paths_config.py +++ b/scripts/misc/vcs_trigger_and_paths_config.py @@ -104,13 +104,15 @@ ("scripts/.*", targets['All']), ("pwiz_tools/BiblioSpec/.*", merge(targets['Core'], targets['Skyline'], targets['Container'])), ("pwiz_tools/Bumbershoot/.*", targets['Bumbershoot']), - ("pwiz_tools/Skyline/Model/Results/RemoteApi/.*", merge(targets['SkylineWithTestConnected'], targets['Container'])), + ("pwiz_tools/Skyline/TestConnected/.*", merge(targets['SkylineWithTestConnected'], targets['Container'])), ("pwiz_tools/Skyline/.*Ardia.*", merge(targets['SkylineWithTestConnected'], targets['Container'])), ("pwiz_tools/Skyline/.*Koina.*", merge(targets['SkylineWithTestConnected'], targets['Container'])), ("pwiz_tools/Skyline/.*Panorama.*", merge(targets['SkylineWithTestConnected'], targets['Container'])), ("pwiz_tools/Skyline/.*Unifi.*", merge(targets['SkylineWithTestConnected'], targets['Container'])), + ("pwiz_tools/Skyline/.*WatersConnect.*", merge(targets['SkylineWithTestConnected'], targets['Container'])), ("pwiz_tools/Skyline/.*DataSource.*", merge(targets['SkylineWithTestConnected'], targets['Container'])), ("pwiz_tools/Skyline/.*", merge(targets['Skyline'], targets['Container'])), + ("pwiz_tools/Shared/CommonMsData/RemoteApi/.*", merge(targets['SkylineWithTestConnected'], targets['Container'])), ("pwiz_tools/Shared/.*", merge(targets['Skyline'], targets['BumbershootRelease'], targets['Container'])), ("pwiz_tools/Osprey/.*", targets['OspreyWindowsNet']), ("pwiz_tools/.*", targets['All']),