From c545faa2b96cc233ad529506cec6756cfb5ecaca Mon Sep 17 00:00:00 2001 From: brendanx67 Date: Tue, 1 Sep 2026 15:43:30 -0700 Subject: [PATCH 1/2] Replaced IdentityModel's TokenResponse with an owned waters_connect type * #4613 already does the token request, credential encoding and error classification itself, so IdentityModel was left providing only a JSON-parsed shape. Owning it removes the last dependency on which IdentityModel version is referenced * Parsed with JsonConvert behind static factories, matching the Ardia response types alongside it, with a local TokenErrorType replacing ResponseErrorType * HandleAuthenticationException is unaffected: it re-parses the raw body with JObject rather than reading this type, so invalid_scope, invalid_client and invalid_grant classification is unchanged See TODO-20260901_watersconnect_token_poco.md in pwiz-ai/todos Co-Authored-By: Claude --- .../Shared/CommonMsData/CommonMsData.csproj | 1 + .../WatersConnect/WatersConnectAccount.cs | 21 ++- .../WatersConnectTokenResponse.cs | 140 ++++++++++++++++++ 3 files changed, 151 insertions(+), 11 deletions(-) create mode 100644 pwiz_tools/Shared/CommonMsData/RemoteApi/WatersConnect/WatersConnectTokenResponse.cs diff --git a/pwiz_tools/Shared/CommonMsData/CommonMsData.csproj b/pwiz_tools/Shared/CommonMsData/CommonMsData.csproj index 779c04f366..7e9cc6bdb8 100644 --- a/pwiz_tools/Shared/CommonMsData/CommonMsData.csproj +++ b/pwiz_tools/Shared/CommonMsData/CommonMsData.csproj @@ -129,6 +129,7 @@ + True diff --git a/pwiz_tools/Shared/CommonMsData/RemoteApi/WatersConnect/WatersConnectAccount.cs b/pwiz_tools/Shared/CommonMsData/RemoteApi/WatersConnect/WatersConnectAccount.cs index 95f4385565..dcada1a5de 100644 --- a/pwiz_tools/Shared/CommonMsData/RemoteApi/WatersConnect/WatersConnectAccount.cs +++ b/pwiz_tools/Shared/CommonMsData/RemoteApi/WatersConnect/WatersConnectAccount.cs @@ -27,7 +27,6 @@ using System.Xml; using System.Xml.Linq; using System.Xml.Serialization; -using IdentityModel.Client; using Newtonsoft.Json.Linq; using pwiz.Common.SystemUtil; @@ -74,7 +73,7 @@ public class WatersConnectAccount : RemoteAccount public class TokenCacheEntry { - public TokenResponse TokenResponse { get; set; } + public WatersConnectTokenResponse TokenResponse { get; set; } public DateTime ExpirationDateTime { get; set; } } @@ -202,7 +201,7 @@ public string GetFoldersUrl() private string IdentityConnectEndpoint => @"/connect/token"; - public TokenResponse Authenticate() + public WatersConnectTokenResponse Authenticate() { // First check the cache for a valid token if (_authenticationTokens.TryGetValue(this, out var tokenCacheEntry) && tokenCacheEntry.ExpirationDateTime > DateTime.UtcNow) @@ -236,7 +235,7 @@ public TokenResponse Authenticate() if (newToken.IsError) { AuthenticationException ex; - if (newToken.ErrorType == ResponseErrorType.Exception) + if (newToken.ErrorType == TokenErrorType.Exception) ex = new AuthenticationException(newToken.Error); else ex = new AuthenticationException(string.Format(CultureInfo.CurrentCulture, @@ -254,14 +253,14 @@ public TokenResponse Authenticate() /// POSTs a token request to the identity server and returns the parsed response. Client /// credentials go in an HTTP Basic authorization header with each half URL-escaped per /// RFC 6749 section 2.3.1, matching the wire format of the IdentityModel TokenClient this - /// replaced. Every failure is returned as an error - the same - /// contract TokenClient had - so callers route all failures through the + /// replaced. Every failure is returned as an error - + /// the same contract TokenClient had - so callers route all failures through the /// authentication-error 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. /// - private TokenResponse RequestToken(NameValueCollection form) + private WatersConnectTokenResponse RequestToken(NameValueCollection form) { try { @@ -271,17 +270,17 @@ private TokenResponse RequestToken(NameValueCollection form) EscapeClientCredential(ClientId) + @":" + EscapeClientCredential(ClientSecret)))); httpClient.AddHeader(@"Accept", @"application/json"); var raw = Encoding.UTF8.GetString(httpClient.UploadValues(requestUri, @"POST", form)); - return new TokenResponse(raw); + return WatersConnectTokenResponse.FromJson(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); + return WatersConnectTokenResponse.FromJson(ex.ResponseBody); + return WatersConnectTokenResponse.FromHttpError(ex.StatusCode ?? HttpStatusCode.ServiceUnavailable, ex.Message, ex.ResponseBody); } catch (Exception ex) { - return new TokenResponse(ex); + return WatersConnectTokenResponse.FromException(ex); } } diff --git a/pwiz_tools/Shared/CommonMsData/RemoteApi/WatersConnect/WatersConnectTokenResponse.cs b/pwiz_tools/Shared/CommonMsData/RemoteApi/WatersConnect/WatersConnectTokenResponse.cs new file mode 100644 index 0000000000..4c1864101c --- /dev/null +++ b/pwiz_tools/Shared/CommonMsData/RemoteApi/WatersConnect/WatersConnectTokenResponse.cs @@ -0,0 +1,140 @@ +/* + * Original author: Brendan MacLean , + * MacCoss Lab, Department of Genome Sciences, UW + * 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.Net; +using Newtonsoft.Json; + +namespace pwiz.CommonMsData.RemoteApi.WatersConnect +{ + /// + /// How a token request failed, when it did. + /// + public enum TokenErrorType + { + None, + /// The identity server answered with an OAuth error body. + Protocol, + /// The server answered, but not with a usable token response. + Http, + /// The request never produced a response at all. + Exception + } + + /// + /// The identity server's answer to a token request, parsed from its JSON body. + /// waters_connect makes its own token request through + /// and classifies its own failures, + /// so the only thing IdentityModel was still providing here was this shape. Owning it keeps + /// this code compiling the same way whichever IdentityModel version is referenced - 3.9 and 7 + /// disagree about whether the equivalent type can be constructed at all - and follows the same + /// pattern as the Ardia response types alongside it. + /// + public class WatersConnectTokenResponse + { + private WatersConnectTokenResponse() { } + + [JsonProperty(@"access_token")] + public string AccessToken { get; private set; } + + [JsonProperty(@"refresh_token")] + public string RefreshToken { get; private set; } + + [JsonProperty(@"expires_in")] + public int ExpiresIn { get; private set; } + + [JsonProperty(@"error")] + public string Error { get; private set; } + + [JsonProperty(@"error_description")] + public string ErrorDescription { get; private set; } + + /// + /// The response body verbatim. + /// re-parses this to tell invalid_scope, invalid_client and invalid_grant apart, so it must + /// stay exactly what the server sent. + /// + [JsonIgnore] + public string Raw { get; private set; } + + [JsonIgnore] + public TokenErrorType ErrorType { get; private set; } + + [JsonIgnore] + public bool IsError => ErrorType != TokenErrorType.None; + + /// + /// Parses a token response body. A body that is not JSON, or that carries no access token, + /// is an error rather than an exception - the caller routes every failure through the + /// authentication-error path, and a proxy answering 200 with an HTML page must not throw + /// past it. + /// + public static WatersConnectTokenResponse FromJson(string json) + { + WatersConnectTokenResponse response; + try + { + response = JsonConvert.DeserializeObject(json) + ?? new WatersConnectTokenResponse(); + } + catch (Exception e) + { + return new WatersConnectTokenResponse + { + Raw = json, ErrorType = TokenErrorType.Exception, Error = e.Message + }; + } + + response.Raw = json; + if (!string.IsNullOrEmpty(response.Error)) + response.ErrorType = TokenErrorType.Protocol; + else if (string.IsNullOrEmpty(response.AccessToken)) + response.ErrorType = TokenErrorType.Http; + return response; + } + + /// + /// An HTTP failure carrying no usable OAuth error body. is kept as + /// even when it is a proxy's HTML, so the caller can show it. + /// + public static WatersConnectTokenResponse FromHttpError(HttpStatusCode statusCode, string reason, string body) + { + return new WatersConnectTokenResponse + { + Raw = body, + ErrorType = TokenErrorType.Http, + Error = statusCode.ToString(), + ErrorDescription = reason + }; + } + + /// + /// A request that never reached the server, or whose failure carried no response at all. + /// + public static WatersConnectTokenResponse FromException(Exception exception) + { + return new WatersConnectTokenResponse + { + ErrorType = TokenErrorType.Exception, + Error = exception.Message + }; + } + } +} From 48dff2020de948d46b05fb73a17088243a16076d Mon Sep 17 00:00:00 2001 From: Matt Chambers Date: Wed, 2 Sep 2026 15:00:38 -0400 Subject: [PATCH 2/2] Fixed the stale RemoteApi rule in the smart build trigger * Repointed it at pwiz_tools/Shared/CommonMsData/RemoteApi, where #3170 moved the tree in June 2025, so remote-account changes trigger the TestConnected tests again instead of falling through to the Shared catch-all * Added a rule for pwiz_tools/Skyline/TestConnected so editing the connected tests triggers the config that runs them * Added a WatersConnect rule alongside the existing Ardia, Koina, Panorama and Unifi ones Co-Authored-By: Claude --- scripts/misc/vcs_trigger_and_paths_config.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/misc/vcs_trigger_and_paths_config.py b/scripts/misc/vcs_trigger_and_paths_config.py index fd7b5e9c03..f8b7a930d0 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']),