Re: [PATCH login-manager v2 1/3] fix #4281: ui: add OpenID Connect login flow to login form

["Shan Shaji" <s.shaji@proxmox.com>]

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. 
5. Formatting changes. 

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]

>  
> +/// Custom URL scheme the identity provider redirects back to once an
> +/// OpenID Connect login completes. Must be registered as a valid redirect
> +/// URI with the realm's provider, as well as in the platform manifests
> +/// (AndroidManifest.xml / Info.plist).
> +const String openIdCallbackScheme = 'pveauth';
> +

If you are sending a v3 by seperating the actual code changes and formatting
IMO, this can also be changed to use the new scheme. (PATCH login-manager 3/3)
can be dropped then. 

>  class ProxmoxProgressModel {
>    int inProgress = 0;
>    String message = 'Loading...';
> @@ -85,6 +92,8 @@ class _ProxmoxLoginFormState extends State<ProxmoxLoginForm> {
>        );
>      }
>  
> +    final isOpenId = 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. 

- [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<void> _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) => ProxmoxApiErrorDialog(
> +            exception: e,
> +          ),
> +        );
> +      }
> +    } catch (e, trace) {
> +      print(e);
> +      print(trace);

Same here as well.

> +      if (mounted) {
> +        if (e.runtimeType == HandshakeException) {
> +          showDialog(
> +            context: context,
> +            builder: (context) => const ProxmoxCertificateErrorDialog(),
> +          );
> +        } else {
> +          showDialog(
> +            context: context,
> +            builder: (context) => ConnectionErrorDialog(exception: e),
> +          );
> +        }
> +      }
> +    }
> +    setState(() {
> +      _progressModel.inProgress -= 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<void> _finishLogin(
> +    ProxmoxApiClient client, {
> +    required String realm,
> +    required String username,
> +    String enteredPassword = '',
> +    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: "83ccdaa064c980b5596c35dd64a8d3ecc68620174ab9b90b6343b753aa721687"
> +      sha256: bf394f466ba9205f1812a0433b392d6af280f155f56651eda7c18cc32ed493b8
>        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: ">=3.10.0 <4.0.0"
> -  flutter: ">=3.35.6"
> +  dart: ">=3.12.0 <4.0.0"
> +  flutter: ">=3.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. 
  
>  
>  dev_dependencies: