> agent-manager: Don't ever fail the secrets requests if they are user initiated

> documented behavoir of GetSecrets() is just to return any secrets we have

typo "behavior"

>-		request_next_agent (parent);
>+		if (   req->flags & NM_SECRET_AGENT_GET_SECRETS_FLAG_USER_REQUESTED
>+		    && !parent->pending) {

request_next_agent() also has a special case for !parent->pending... Should we just put the "USER_REQUESTED => success" special case in request_next_agent() rather than here?


> core: Don't wipe out VPN secrets if we're changing the connection

>+		/* Empty hash received, while the hashes default to NULL.
>+		 * Don't match against the default, consider it unset. */
>+		if (G_VALUE_HOLDS (value, G_TYPE_HASH_TABLE)) {
>+			GHashTable *hash = (GHashTable *) g_value_get_boxed (value);
>+
>+			if (g_hash_table_size (hash) == 0)
>+				return;
>+		}

The comment isn't necessarily true at the point in the code where it appears; it's only true if both if()s are true. So the comment needs to be conditional too. Eg:

  /* If an empty hash is received, consider it unset. */



> cli: Error out early if we fail parsing header fields and don't handle errors

I don't get what this is doing... the commit message talks about programmer errors, but that code looks like it's dealing with user errors to me.

(In reply to comment #1)
> > agent-manager: Don't ever fail the secrets requests if they are user initiated
> 
> > documented behavoir of GetSecrets() is just to return any secrets we have
> 
> typo "behavior"
> 
> >-		request_next_agent (parent);
> >+		if (   req->flags & NM_SECRET_AGENT_GET_SECRETS_FLAG_USER_REQUESTED
> >+		    && !parent->pending) {
> 
> request_next_agent() also has a special case for !parent->pending... Should we
> just put the "USER_REQUESTED => success" special case in request_next_agent()
> rather than here?

Yeah, request->flags is only valid for Get requests so that should be OK.  (request_next_agent() is used for Get, Save, and Delete FWIW).

FLAG_USER_REQUESTED was added in 74e262b3032b0e53df0cd0c64a752ad628042cf1 where the commit message is:

    Allows agents to provide different behavior depending on whether the
    secrets request was initiated by a user (eg by picking a connection
    from a UI menu or by 'nmcli con up') or was automatically started by
    NetworkManager.
    
    See https://bugzilla.gnome.org/show_bug.cgi?id=660293

The intent of that patch was that agents would make secrets requests modal (eg, dialog taking over everything) if the user explicitly requested the action, but automatic activation stuff by NM wouldn't be modal, becuase the user hadn't requested it.  The modal/non-modal dialog thing may be bogus these days, but I don't think the original intent is really wrong.

The issue here is impl_settings_connection_get_secrets() which is what editors/applets call to get secrets, and they typically don't care about errors they just want whatever they can get even if it's incomplete.

But I don't think we can make the internal users of FLAG_USER_REQUESTED (eg, nm-vpn-connection.c and nm-activation-request.c) ignorant of errors like the patch does, because then I think VPNs or connections would continue trying to connect even if they hadn't received any secrets.

So heres what I think should happen:

1) add an internal flag (like NM_SECRET_AGENT_GET_SECRETS_FLAG_ONLY_SYSTEM) that impl_settings_connection_get_secrets() would set that indicates that it doesn't actually care about errors

2) make sure to mask that flag off in nm_secret_agent_get_secrets(), or better just mask anything with the 0x80000000 bit set

3) modify request_next_agent() to check for that flag in the !pending case and just return success instead of error

Does that make sense?


> > core: Don't wipe out VPN secrets if we're changing the connection
> 
> >+		/* Empty hash received, while the hashes default to NULL.
> >+		 * Don't match against the default, consider it unset. */
> >+		if (G_VALUE_HOLDS (value, G_TYPE_HASH_TABLE)) {
> >+			GHashTable *hash = (GHashTable *) g_value_get_boxed (value);
> >+
> >+			if (g_hash_table_size (hash) == 0)
> >+				return;
> >+		}
> 
> The comment isn't necessarily true at the point in the code where it appears;
> it's only true if both if()s are true. So the comment needs to be conditional
> too. Eg:
> 
>   /* If an empty hash is received, consider it unset. */

Also I would place the hash table check under the "if (flags & NM_SETTING_PARAM_SECRET)" check, otherwise it might capture other property types.

I don't have any comments on the other two patches besides what danw said.  So once he's satisfied with them, I am too.

(In reply to comment #1)
> > agent-manager: Don't ever fail the secrets requests if they are user initiated
> 
> > documented behavoir of GetSecrets() is just to return any secrets we have
> 
> typo "behavior"

Fixed

> > core: Don't wipe out VPN secrets if we're changing the connection
> 
> >+		/* Empty hash received, while the hashes default to NULL.
> >+		 * Don't match against the default, consider it unset. */
> >+		if (G_VALUE_HOLDS (value, G_TYPE_HASH_TABLE)) {
> >+			GHashTable *hash = (GHashTable *) g_value_get_boxed (value);
> >+
> >+			if (g_hash_table_size (hash) == 0)
> >+				return;
> >+		}
> 
> The comment isn't necessarily true at the point in the code where it appears;
> it's only true if both if()s are true. So the comment needs to be conditional
> too. Eg:
> 
>   /* If an empty hash is received, consider it unset. */

Fixed

> > cli: Error out early if we fail parsing header fields and don't handle errors
> 
> I don't get what this is doing... the commit message talks about programmer
> errors, but that code looks like it's dealing with user errors to me.

Tried to fix up the wording. Does it make more sense now?

(What happened is that I failed to separate columns in the header file properly and I got a NULL reference in error handling code. I'd have preferred a more sensible error.)

(In reply to comment #2)
> So heres what I think should happen:
> 
> 1) add an internal flag (like NM_SECRET_AGENT_GET_SECRETS_FLAG_ONLY_SYSTEM)
> that impl_settings_connection_get_secrets() would set that indicates that it
> doesn't actually care about errors
> 
> 2) make sure to mask that flag off in nm_secret_agent_get_secrets(), or better
> just mask anything with the 0x80000000 bit set

Not sure what you meant here. 0x80000000 is NM_SECRET_AGENT_GET_SECRETS_FLAG_ONLY_SYSTEM, not the new flag that. Did you mean that we should just mask all user flags away with a common mask?

> 3) modify request_next_agent() to check for that flag in the !pending case and
> just return success instead of error

We'd have to modify it to accept ConnectionRequest instead of Request then (or move flags to Request). Do we want to do either?

> Does that make sense?

> > > core: Don't wipe out VPN secrets if we're changing the connection
> > 
> > >+		/* Empty hash received, while the hashes default to NULL.
> > >+		 * Don't match against the default, consider it unset. */
> > >+		if (G_VALUE_HOLDS (value, G_TYPE_HASH_TABLE)) {
> > >+			GHashTable *hash = (GHashTable *) g_value_get_boxed (value);
> > >+
> > >+			if (g_hash_table_size (hash) == 0)
> > >+				return;
> > >+		}
> > 
> > The comment isn't necessarily true at the point in the code where it appears;
> > it's only true if both if()s are true. So the comment needs to be conditional
> > too. Eg:
> > 
> >   /* If an empty hash is received, consider it unset. */
> 
> Also I would place the hash table check under the "if (flags &
> NM_SETTING_PARAM_SECRET)" check, otherwise it might capture other property
> types.

Yep, but that would be correct. We don't care about the negative case; if something can't contain secrets we're sure it won't contain secrets, whether it's actually a secret property or not.

Pushed the updated branch.

>> agent-manager: Don't ever fail the secrets requests from GetSecrets()
                 ^^^^^^

-    NM_SECRET_AGENT_GET_SECRETS_FLAG_NO_ERRORS = 0x40000000
+    NM_SECRET_AGENT_GET_SECRETS_FLAG_NO_ERRORS = 0x40000000,


whitespace in src/settings/nm-settings-connection.c



                    } else {
                         request_next_agent (parent);
                    }

no braces          ^^^





>> core: Don't wipe out VPN secrets if we're changing the connection

This change is only there for NMSettingVpn. Just hard code against that type.

The abstraction for secrets for NMSettingVpn and "normal" settings is broken (otherwise, we would call something like is_secret_prop() or nm_setting_get_secret_flags() to access the secrets).
Trying to use a broken abstraction is more confusing (and not generic anyway).


if (NM_IS_SETTING_VPN (setting)) {
    if (nm_setting_vpn_get_num_secrets (NM_SETTING_VPN(setting)))
       *((gboolean *) user_data) = TRUE;
    return;
}
// continue with regular GParamSpec check.



>> cli: Error out early if we fail parsing header fields and don't handle errors

the commit subject does not describe that the patch only adds an assert. Adding an assertion should have no effect.

anyway. Why would parse_output_fields() crash with error==NULL? If it crashes, the crash should be fixed instead :)
Also, there are quite some callers that don't pass an error.

(In reply to comment #4)
> >> agent-manager: Don't ever fail the secrets requests from GetSecrets()
>                  ^^^^^^
> 
> -    NM_SECRET_AGENT_GET_SECRETS_FLAG_NO_ERRORS = 0x40000000
> +    NM_SECRET_AGENT_GET_SECRETS_FLAG_NO_ERRORS = 0x40000000,

Ok

> whitespace in src/settings/nm-settings-connection.c

Fixed

>                     } else {
>                          request_next_agent (parent);
>                     }
> 
> no braces          ^^^

Fixed

> >> core: Don't wipe out VPN secrets if we're changing the connection
> 
> This change is only there for NMSettingVpn. Just hard code against that type.
> 
> The abstraction for secrets for NMSettingVpn and "normal" settings is broken
> (otherwise, we would call something like is_secret_prop() or
> nm_setting_get_secret_flags() to access the secrets).
> Trying to use a broken abstraction is more confusing (and not generic anyway).
> 
> 
> if (NM_IS_SETTING_VPN (setting)) {
>     if (nm_setting_vpn_get_num_secrets (NM_SETTING_VPN(setting)))
>        *((gboolean *) user_data) = TRUE;
>     return;
> }
> // continue with regular GParamSpec check.

Sounds good, adjusted it

> >> cli: Error out early if we fail parsing header fields and don't handle errors
> 
> the commit subject does not describe that the patch only adds an assert. Adding
> an assertion should have no effect.

Tried to reword it.

> anyway. Why would parse_output_fields() crash with error==NULL? If it crashes,
> the crash should be fixed instead :)

I figure it's easier to fix when an assert indicates what went wrong.

> Also, there are quite some callers that don't pass an error.

And none of them should cause the function to fail.

Pushed an updated branch.

    parse_output_fields() crashes if it encounters an error (nonexistend column)
    and it is called with error == NULL. That should never happen, as it's never
    called with NULL error against user input. However, it is useful to assert
    that is the case -- it gives us a more useful crash in case of a programmer
    error.

why does it crash? Can you not avoid the crash? :)
Otherwise, assert against being called with a nonexisting column.

Rest LGTM

> cli: assert that there's an GError in case of an error in parse_output_fields()

I can't see why this function should fail because of the missing error, I agree it's wrong to put the assert here.  It seems like the problem is more with one of the callers (and yeah, there are many).  Could we get a backtrace?

Also, I'm happy to have everything except this patch merged ASAP.

(In reply to comment #7)
> > cli: assert that there's an GError in case of an error in parse_output_fields()
> 
> I can't see why this function should fail because of the missing error, I agree
> it's wrong to put the assert here.  It seems like the problem is more with one
> of the callers (and yeah, there are many).  Could we get a backtrace?
> 
> Also, I'm happy to have everything except this patch merged ASAP.

Done