Created attachment 149706 [details] [review]
NM_PIN_Rejected1.patch

Created attachment 149708 [details] [review]
MM_PIN_Rejected2.patch

(looks better)

It would be nice to get a more detailed interpretation of the AT+CPIN= error messages, but I don't know if that's possible.

This is, for instance how my ZTE MF628 "behaves":

Pin OK:

--> 'AT+CPIN="1234"<CR>'
<-- 'AT+CPIN="1234"'
<-- '<CR>'
<-- '<CR><LF>OK<CR><LF>'

Wrong PIN:

--> 'AT+CPIN="4564"<CR>'
<-- 'AT+CPIN="4564"'
<-- '<CR>'
<-- '<CR><LF>+CME ERROR: incorrect password<CR><LF>'

PIN has already been sent:

--> 'AT+CPIN="1234"<CR>'
<-- 'AT+CPIN="123'
<-- '4'
<-- '"<CR><CR><LF>+CME ERROR: operation not allowed<CR><LF>'

Created attachment 149993 [details] [review]
Modem Manager Patch (PINPUK)

Created attachment 149994 [details] [review]
Network Manager Patch (PINPUK)

Created attachment 149996 [details] [review]
NM Applet Patch (PINPUK)

New patches: 

* PUK requests also get handled.
* Ask Pin&Puk dialog improved.

So a few comments; NetworkManager shouldn't really be doing PUK handling during the connection process because if you screw up the PUK, you're pretty much done.  ModemManager shouldn't be handling the PUK during the SimpleConnect stuff either, for the same reason.  PUK handling should be out-of-band completely.  I think it should basically go like this:

1) Try to connect
2) ModemManager needs a PIN; returns the "need a PIN" error to NetworkManager
3) If the user provided a PIN to NetworkManager, NetworkManager sends the PIN *once*
4) if the PIN is correct, connection proceeds
5) if the PIN is incorrect, NM fails the connection attempt completely with a "need pin" error
6) the applet sees the "need pin" error and asks the user for the correct PIN, stating that the existing PIN was incorrect and warning the user that entering the PIN too many times isn't a good thing to do
7) the user enters the new PIN, and nm-applet requests that NM activate the modem again
8) hopefully everything works this time

For PUK handling I think it should roughly be:

1) If a PUK is required, NM sets an internal flag saying that a PUK is required and fails the connection immediately
2) the applet notices this and asks the user for the PUK warning the user if they get the PUK wrong, it's really bad and they have to call their provider to unlock the device
3) applet sends the PUK to NM, NM sends it to ModemManager
4) if the PUK works, the user clicks the connection again and it starts
5) if the PUK is wrong, I think NM/MM should require that the modem be unplugged or something until you can try the PUK again

Basically, PIN is OK to have some automation for (at least for one try).  PUK should *never* be automatically handled because it's pretty dangerous.  PUK should always require direct user interaction.

In this scheme, we don't want the PUK sent along to NM in the NMSettingGsm at all.  I'd rather have D-Bus methods on the NMGsmDevice object for Unlock(string type, string secret) that the applet calls for PUK.  There's quite a few other codes (like PIN2, PUK2, and a few more) that the applet could also need to send at various times so we have to handle those as well, hence a generic "Unlock" method.

Does that sound like a good approach to you?

(In reply to comment #7)
> PUK handling should be out-of-band
> completely.  

I guess PIN handling as well. When do you enter the PIN into your cell-phone? When you turn it on, and not when you make a call. So probably the "ask for PIN" (or PUK) dialog should rather appear when the modem is plugged in. Or if it's already plugged in on MM/NM startup, there could be an "unlock device" button somewhere.

Unlocking the modem at an earlier stage should also allow displaying signal strengths etc. before the actual connect... (this is how umtsmon handles it as well)

Another question might be: Why can't the applet talk to MM directly for the "unlock-process"?
 
> Does that sound like a good approach to you?

Sounds like heading in the right direction. But it seems to require a pretty sophisticated state-model inside the applet. And I don't see the big disadvantage of the current callback scheme (keeping the state-model inside NM).

I'm really not an expert on the overall NM/MM design. I just tried to make PIN/PUK work within the current model. Quite sluggish, but at least it solves 90% of the problem. As I'm always setting retry_secret = TRUE for PUK requests, the danger of screwing up the SIM shouldn't be too high (as users can still hit the cancel button). But a big warning message should definitely be added to the PUK dialog.

(Actually I did kill one of my SIMs when developing this patch, but fortunately it was just a 10 Euro pre-payed ;-)

Anyway, I can't judge whether it's right to go for the ideal solution now, or just live with a quick and dirty fix for a while...

(In reply to comment #8)
> (In reply to comment #7)
> > PUK handling should be out-of-band
> > completely.  
> 
> I guess PIN handling as well. When do you enter the PIN into your cell-phone?
> When you turn it on, and not when you make a call. So probably the "ask for
> PIN" (or PUK) dialog should rather appear when the modem is plugged in. Or if
> it's already plugged in on MM/NM startup, there could be an "unlock device"
> button somewhere.

Yeah, that's probably a better approach.  After successfully grabbing the device, MM could check the device's PIN status and then set a flag on the modem D-Bus object (called "PinRequired" on the org.freedesktop.ModemManager.Modem interface perhaps).  That would be a string property that would contain 'pin', 'puk', 'pin2', etc depending on what the device wanted.

That would let apps like nm-applet get the device state immediately and unlock the device when they discovered it.

> Unlocking the modem at an earlier stage should also allow displaying signal
> strengths etc. before the actual connect... (this is how umtsmon handles it as
> well)

Right.

> Another question might be: Why can't the applet talk to MM directly for the
> "unlock-process"?

Because ModemManager deals directly with hardware and runs with root privileges because it needs to talk to udev and handle serial ports and other stuff.  We need to be very careful about what holes we open to allow normal users to manipulate hardware.  If we're not careful, a malicious application could dial the modem and start charging you money.  We solve this in NetworkManager by using PolicyKit to ensure that the user has the priviliges required and to ensure that it's the *user* doing the request, not a malicious program behind the scenes.

So we can either start making exposing MM D-Bus methods to normal users and protecting them with PolicyKit, or we can essentially duplicate some of the methods that nm-applet would need to use (pin unlock, pin change, scan, etc) in the NetworkManager D-BUs interface, and have the applet talk to NM (which then validates the request) and passes it along to ModemManager.

I tend to think that PolicyKit-protecting the ModemManager methods would be the better approach for the time being.  We can use D-Bus policy to unlock specific methods to all users and then use PK to add fine-grained protection on them.

But yeah, it's probably easier to handle the unlock via a property that nm-applet can query and then do the right thing.

We still have to handle the generic unlock case if the user hits the "cancel" button on the unlock dialog.  At that point we'd have to re-present the unlock dialog when a scan or activate operation is requested anyway.

First would be to add the "UnlockRequired" property to the .Modem interface in introspection/mm-modem.xml; with something like this just below the "Enabled" property:

    <property name="UnlockRequired type="s" access="read">
      <tp:docstring>
        Empty if the device is usable without an unlock code or has already been unlocked.  If the device needs to be unlocked before becoming usable this property contains the specific unlock code required.  Valid unlock code values are "" (blank), "sim-pin", "sim-puk", "ph-sim-pin", "ph-fsim-pin", "ph-fsim-puk", "sim-pin2", "sim-puk2", "ph-net-pin", "ph-net-puk", "ph-netsub-pin", "ph-netsub-puk", "ph-sp-pin", "ph-sp-puk", "ph-corp-pin", and "ph-corp-puk".
      </tp:docstring>
    </property>

Next we need to add the new property to the MMModem interface and then to the subclasses that implement MMModem:

- src/mm-modem.h
#define MM_MODEM_IP_METHOD       "ip-method"
#define MM_MODEM_ENABLED         "enabled"
#define MM_MODEM_UNLOCK_REQUIRED "unlock-required"
#define MM_MODEM_VALID           "valid"      /* not exported */

- src/mm-mdoem.c near the top:
Add MM_MODEM_PROP_UNLOCK_REQUIRED to the end of the MMModemProp struct.

- src/mm-modem.c::mm_modem_init():
Define the property right after the "Enabled" property:

    g_object_interface_install_property
        (g_iface,
         g_param_spec_string (MM_MODEM_UNLOCK_REQUIRED,
                               "UnlockRequired",
                               "Whether or not the modem requires an unlock code to become usable, and if so, which unlock code is required",
                               NULL,
                               G_PARAM_READABLE));


- src/mm-modem-base.c::mm_modem_base_class_init()
Override the property in the base class the implements the MMModem interface so that subclasses can set its value.  Right before the mm_properties_changed_signal_new(), put:

    g_object_class_override_property (object_class,
                                      MM_MODEM_PROP_UNLOCK_REQUIRD,
                                      MM_MODEM_UNLOCK_REQUIRED);

- Now we make the MMModemBase class the primary handler of the property; subclasses like MMGenericGSM and MMGenericCDMA will just call a method on MMModemBase (their superclass) to when the device knows that it needs a specific unlock code.  So we now make MMModemBase respond to get/set requests for the property...

- Add a new "char *unlock_required" item to MMModemBasePrivate near the top.  Make sure you add a corresponding "g_free (priv->unlock_required);" in finalize() near the bottom to free the value.

- Now add a case to get_property() for MM_MODEM_PROP_UNLOCK_REQUIRED which just does:
     g_value_set_string (value, priv->unlock_required ? priv->unlock_required : "");

- Next add a method to mm-modem-base.c and .h called:

void mm_modem_base_set_unlock_required (MMModemBase *self, const char *unlock_required)
{
    MMModemBasePrivate *priv;

    g_return_if_fail (self != NULL);
    g_return_if_fail (MM_IS_MODEM_BASE (self));

    priv = MM_MODEM_BASE_GET_PRIVATE (object);

    /* Only do something if the value changes */
    if (   priv->unlock_required == unlock_required
        || (priv->unlock_required && unlock_required && !strcmp (priv->unlock_required, unlock_required)))
        return;

    g_free (priv->unlock_required);
    priv->unlock_required = g_strdup (unlock_required);
    g_object_notify (G_OBJECT (self), MM_MODEM_UNLOCK_REQUIRED);
}


- Now we can actually implement the logic to detect the PIN in the MMGenericGSM class.  More on that after we get this part done....

Created attachment 151335 [details] [review]
p01_unlock_required_property.patch

Here is the patch to introduce the "UnlockRequired" property.

There is only one thing that is different to your proposal:

get_property() for MM_MODEM_PROP_UNLOCK_REQUIRED returns NULL and not "" in case the property is NULL.

I think NULL is a better indicator that unlock is not required.

Created attachment 151336 [details] [review]
p02_initial_enable.patch

as you told me on IRC. 

tries to find out whether PIN/PUK unlock is necessary after grabbing a port.

uses the "enable" method, as it checks for the PIN, and does a "disable" right afterwards (on success).

I also tried to clear the "unlock_required" property when the user has successfully provided a PIN or PUK. what's missing is updating "unlock_required" when sending the PIN or PUK has failed.

p01 looks good, thanks!

I was thinking for p02 to instead of doing the full enable, since that may also send commands that the modem (especially your MF628) will reject (like init, CFUN, etc) that we should simply try to send CPIN? instead.  You should be able to do this with a new short function called from mm_generic_gsm_grab_port() where your patch calls mm_modem_enable.  More or less like this:

static void
initial_pin_check_done (MMModem *modem, GError *error, gpointer user_data)
{
    MMGenericGsmPrivate *priv;
    gchar *unlock_required_str = NULL;

    g_return_if_fail (MM_IS_GENERIC_GSM (modem));

    priv = MM_GENERIC_GSM_GET_PRIVATE (modem);

    priv->pin_checked = TRUE;

    if (error) {
        switch (error->code) {
           <your error -> PIN string code>
        }
        mm_modem_base_set_unlock_required (MM_MODEM_BASE(modem), unlock_required_str);
    }

    if (mm_serial_port_open (priv->primary, NULL))
        mm_serial_port_close (priv->primary);

    check_valid (MM_GENERIC_GSM (modem));
}

static void
initial_pin_check (MMGenericGsm *self)
{
    GError *error = NULL;

    g_return_if_fail (priv->primary != NULL);

    if (!mm_serial_port_open (priv->primary, &error)) {
        MMCallbackInfo *info;

        g_assert (error);
        info = mm_callback_info_new (modem, callback, user_data);
        info->error = error;
        mm_callback_info_schedule (info);
        return;
    }

    mm_generic_gsm_check_pin (self, initial_pin_check_done, NULL);
}


Trying to enable the modem may well do a lot more than we want but I think this code above should limit command exposure to just pin checking.  Thoughts?

Thanks!

*** Bug 605058 has been marked as a duplicate of this bug. ***

Created attachment 151647 [details] [review]
p02_1_initial_pin_check.patch

.. didn't know that I just have to open the serial port. So "enable" is not required here. :-)

Here is a new patch to trigger an initial PIN check as you suggested.

One question: Should the "unlock_required" only reflect the result of this initial PIN-check, or should it also react to state changes. like: 

* clear it after successfully unlocking the SIM, or
* sending wrong PIN -> PUK unlock required (unlock_required = "sim-puk")?

So what's next? I guess send_pin/puk_done() needs proper parsing of the result.
(Which i tried to fix in this earlier patch http://bugzilla-attachments.gnome.org/attachment.cgi?id=149993)

It's not always safe to just open the serial port and send commands without doing the modem setup, but in this case we're limited in what we're doing so I think it's OK.  All the 3G devices I've found are pretty sane with default serial settings anyway.

Enable() is supposed to ensure that the device is powered up and ready to use by sending any necessary initialization commands the module might require, and also ensuring that the device's radios are powered up for use in things like network scanning and registration.  CPIN checking shouldn't require that level of functionality.

WRT to pin_check_done() and updating unlock_required, good point.  We should update unlock_required any time we think the device needs an unlock.  So moving the unlock_required logic to pin_check_done() is probably a good idea, and it helps consolidate the code that parses PIN responses too.

That way in pin_check_done() you can do something like:

    if (error)
        info->error = g_error_copy (error);
    else if (g_str_has_prefix (response->str, "+CPIN: ")) {
        const char *str = response->str + 7;

        if (g_str_has_prefix (str, "READY")) {
            parsed = TRUE;
            mm_modem_base_set_unlock_required (MM_MODEM_BASE (modem), NULL);
        } else {
            CPinResult *iter = &results[0];

            /* Translate the error */
            while (iter->result) {
                if (g_str_has_prefix (str, iter->result)) {
                    char *tmp;

                    info->error = mm_mobile_error_for_code (iter->code);
                    parsed = TRUE;

                    tmp = g_ascii_strdown (iter->result);
                    mm_modem_base_set_unlock_required (MM_MODEM_BASE (modem), tmp);
                    g_free (tmp);
                    break;
                }
                iter++;
            }
        }
    }

    if (!parsed) {
        // FIXME: assume unlocked?
        mm_modem_base_set_unlock_required (MM_MODEM_BASE (modem), NULL);
    }


And yeah, if the sent PIN is wrong too many times and the modem says it needs the PUK after sending the wrong PIN the 3rd time, we should update unlock_required at that time as well.

Thanks again!

I took it upon myself to finish this up this weekend; can you checkout current git master and see if that works for you?  I'll start taking a look at the nm-applet bits too; though we'll need some PolicyKit bits in MM to ensure that the rogue apps can't hammer the PUK before we can get the nm-applet bits in.

In any case, the ModemManager side of this is pretty much done.

(In reply to comment #17)
> I took it upon myself to finish this up this weekend; can you checkout current
> git master and see if that works for you?  

Sorry, I didn't have time the last month. Yesterday I tested a little bit. Congratulations - PIN/PUK handling works a lot better now! :-)

And the display of network strengths is very cool as well!

Here are some thoughts/experiences - probably GUI related...

*) feedback when the wrong pin has been entered would be cool in the PIN dialog...

*) "puk required" dialog doesn't show up after entering the wrong pin 3 times (the pin dialog is repeated forever)

*) when plugging the modem with mm already running: after entering the correct pin, a "correct pin entered" message should show.
At the moment there is no feedback at all.
(perhaps even a "do you want to connect now?" dialog would be cool - also when the sim is not pin locked. I guess if someone plugs a modem "live", his/her intention is to connect)

other issues (probably unrelated)

-) ZTE MF628 works a lot better now (even when PIN locked)
https://bugzilla.gnome.org/show_bug.cgi?id=604369 (but i have to eject the zerocd manually with nautilus)

-) the hawaei 1552 still sometimes refuses to connect ("no carrier") https://bugzilla.gnome.org/show_bug.cgi?id=591047 (when the network has not been specified). If this cannot be fixed, I guess it would at least be better to display the actual "no carrier" message, rather than nothing.

-) sometimes MM doesn't recognize modems when they are already plugged before MM is launched.

(In reply to comment #18)
> (In reply to comment #17)
> > I took it upon myself to finish this up this weekend; can you checkout current
> > git master and see if that works for you?  
> 
> Sorry, I didn't have time the last month. Yesterday I tested a little bit.
> Congratulations - PIN/PUK handling works a lot better now! :-)
> 
> And the display of network strengths is very cool as well!
> 
> Here are some thoughts/experiences - probably GUI related...
> 
> *) feedback when the wrong pin has been entered would be cool in the PIN
> dialog...

I thought there was some, but only if the Modem responded with an error we detect.  We still need to fix the dialog to wait around for a bit and handle when the UnlockRequired property changes.

> *) "puk required" dialog doesn't show up after entering the wrong pin 3 times
> (the pin dialog is repeated forever)

Yeah, that's wrong.  I have 20 used SIM cards coming in the mail that I can PIN/PUK lock until I PUK2 lock them all, so I'll be able to actually test this soon.  There's also a bug where PUK2 triggers the PUK dialog, not the PUK2 dialog (which we don't actually handle yet since I don't want to PUK lock one of my good SIMs).

> *) when plugging the modem with mm already running: after entering the correct
> pin, a "correct pin entered" message should show.
> At the moment there is no feedback at all.

Yeah, perhaps the dialog should stick around until we know the complete result.  Feedback would be good, but the interaction here is a bit odd.  We'd have to change the "Unlock" button to an "OK" button half-way through and require a second click to get rid of the dialog.  That may be the right thing to do though, since at least you'd get some feedback.

> (perhaps even a "do you want to connect now?" dialog would be cool - also when
> the sim is not pin locked. I guess if someone plugs a modem "live", his/her
> intention is to connect)
> 
> other issues (probably unrelated)
> 
> -) ZTE MF628 works a lot better now (even when PIN locked)
> https://bugzilla.gnome.org/show_bug.cgi?id=604369 (but i have to eject the
> zerocd manually with nautilus)

Yeah, I'm keeping my 626 PIN-locked for a while until I know it's working well.

You might look into usb_modeswitch 1.1.0, which is working well for me with my ZTE devices.  I don't even need to eject the fake CD.

> -) the hawaei 1552 still sometimes refuses to connect ("no carrier")
> https://bugzilla.gnome.org/show_bug.cgi?id=591047 (when the network has not
> been specified). If this cannot be fixed, I guess it would at least be better
> to display the actual "no carrier" message, rather than nothing.

Hmm, my 1550 seems to work OK, but i"ll look into this.  I have noticed some automatic registration issues that could cause the modem to either not connect, or to connect before it's ready.  Any chance you could attach a debug output from MM when you see this?  I bet it's a case of the modem not being in the right registration state or something before connect is tried.

> -) sometimes MM doesn't recognize modems when they are already plugged before
> MM is launched.

Hmm, any chance you can grab the debug output here?