From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 2A2C31FF0B2 for ; Thu, 20 Aug 2026 14:31:50 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 12A282159E; Thu, 20 Aug 2026 14:31:48 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 20 Aug 2026 14:31:43 +0200 Message-Id: Subject: Re: [PATCH login-manager v2 1/3] fix #4281: ui: add OpenID Connect login flow to login form From: "Shan Shaji" To: "Azharul Haque" , X-Mailer: aerc 0.20.0 References: <20260810144713.75806-1-haque@azharul.com> <20260810144713.75806-4-haque@azharul.com> In-Reply-To: <20260810144713.75806-4-haque@azharul.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1787229078452 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.602 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) KAM_SHORT 0.001 Use of a URL Shortener for very short URL RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: PHUBNAMMOYJCSQS2RLPXBW2J4XG7JT74 X-Message-ID-Hash: PHUBNAMMOYJCSQS2RLPXBW2J4XG7JT74 X-MailFrom: s.shaji@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Hi, one high-level note. There are too many changes in this one patch. It would be better if we could separate it. IMHO, this patch could be split into: 1. Adding the flutter_web_auth plugin. 2. Factor out the _finishLogin function. 3. Add openId specific changes (i.e. the UI related changes and adding the = function call) 4. optional: Separate the password form widget.=20 5. Formatting changes.=20 We normally separate formatting changes and actual code changes into separate patches; otherwise, it will be hard to identify the actual changes= . Please note that the changes I suggested can also be done by me. If you don't have the time, please let me know. I could make those changes and send it as a separate series. On Mon Aug 10, 2026 at 4:47 PM CEST, Azharul Haque wrote: [snip] > =20 > +/// Custom URL scheme the identity provider redirects back to once an > +/// OpenID Connect login completes. Must be registered as a valid redire= ct > +/// URI with the realm's provider, as well as in the platform manifests > +/// (AndroidManifest.xml / Info.plist). > +const String openIdCallbackScheme =3D 'pveauth'; > + If you are sending a v3 by seperating the actual code changes and formattin= g IMO, this can also be changed to use the new scheme. (PATCH login-manager 3= /3) can be dropped then.=20 > class ProxmoxProgressModel { > int inProgress =3D 0; > String message =3D 'Loading...'; > @@ -85,6 +92,8 @@ class _ProxmoxLoginFormState extends State { > ); > } > =20 > + final isOpenId =3D widget.selectedDomain?.isOpenIdRealm ?? false; > + > return AutofillGroup( [snip] > + if (isOpenId) > + Padding( > + padding: const EdgeInsets.symmetric(vertical: 16), > + child: Text( > + "This realm signs you in through your browser. " > + "Tap Continue to proceed.", > + style: Theme.of(context).textTheme.bodyMedium, > + textAlign: TextAlign.center, > + ), > + ) > + else ...[ Instead of "if..else.." block if we use an enum here as I mentioned in the previous reply. We could use the switch expression [0] to match the different realm types. IMHO, that would be much more cleaner and the password form can itself be another widget as well. If you are seperating the widget, it should also be another patch.=20 - [0] https://dart.dev/language/branches#switch-expressions > + TextFormField( > + decoration: const InputDecoration( > + icon: Icon(Icons.person), > + labelText: 'Username', > + ), > + controller: widget.usernameController, > + validator: (value) { > + if (value!.isEmpty) { > + return 'Please enter username'; > + } > + return null; > + }, > + autofillHints: const [AutofillHints.username], > + ), [snip] > + Future _onOpenIdLoginButtonPressed() async { [snip] > + > + await _finishLogin(client, realm: realm, username: username); > + } on proxclient.ProxmoxApiException catch (e) { > + print(e); I believe you copied this block from `_onLoginButtonPressed`. However, dart suggests to avoid adding print statements [0]. Please remove it. - [0] https://dart.dev/tools/linter-rules/avoid_print > + if (mounted) { > + showDialog( > + context: context, > + builder: (context) =3D> ProxmoxApiErrorDialog( > + exception: e, > + ), > + ); > + } > + } catch (e, trace) { > + print(e); > + print(trace); Same here as well. > + if (mounted) { > + if (e.runtimeType =3D=3D HandshakeException) { > + showDialog( > + context: context, > + builder: (context) =3D> const ProxmoxCertificateErrorDialog(= ), > + ); > + } else { > + showDialog( > + context: context, > + builder: (context) =3D> ConnectionErrorDialog(exception: e), > + ); > + } > + } > + } > + setState(() { > + _progressModel.inProgress -=3D 1; > + }); > + } > + > + /// Shared tail of both the password and OpenID login flows: handles a > + /// pending TFA challenge, fetches cluster status, persists the login = and > + /// closes the login page. Returns early (without closing the page) if= the > + /// user cancels a TFA challenge. > + Future _finishLogin( > + ProxmoxApiClient client, { > + required String realm, > + required String username, > + String enteredPassword =3D '', > + String? savedPassword, > + }) async { IMHO, factoring out this function can itself be another patch. [snip] > diff --git a/pubspec.lock b/pubspec.lock > index ac4ba18..9250de9 100644 > --- a/pubspec.lock > +++ b/pubspec.lock > @@ -133,10 +133,10 @@ packages: > dependency: transitive > description: > name: code_assets > - sha256: "83ccdaa064c980b5596c35dd64a8d3ecc68620174ab9b90b6343b753a= a721687" > + sha256: bf394f466ba9205f1812a0433b392d6af280f155f56651eda7c18cc32e= d493b8 > url: "https://pub.dev" > source: hosted > - version: "1.0.0" > + version: "1.2.1" > code_builder: > dependency: transitive > description: > @@ -193,6 +193,14 @@ packages: > url: "https://pub.dev" > source: hosted > version: "3.1.7" [snip] > sdks: > - dart: ">=3D3.10.0 <4.0.0" > - flutter: ">=3D3.35.6" > + dart: ">=3D3.12.0 <4.0.0" > + flutter: ">=3D3.44.0" May I know which flutter version you are using? Looking at this change I believe you are using a different one than the one we are using (v3.41). > diff --git a/pubspec.yaml b/pubspec.yaml > index 4652ced..be38e6d 100644 > --- a/pubspec.yaml > +++ b/pubspec.yaml > @@ -16,6 +16,7 @@ dependencies: > built_collection: ^5.0.0 > proxmox_dart_api_client: > path: ../proxmox_dart_api_client > + flutter_web_auth_2: ^5.0.3 IMHO, the .lock and .yaml file changes can itself be another patch.=20 =20 > =20 > dev_dependencies: