Firstly: This is great, thanks!
Now the nit-picking... :)
On Sat, 2012-10-13 at 23:17 -0700, Kevin Cernekee wrote:
> The new prepare_stoken() code was written so that if
> process_auth_form() returns 1 (user cancelled), the software token
> would be bypassed and they could log in manually. This still needs
> work; the CLI doesn't seem to return 1 under any circumstances, and
> the GUI writes to cancel_fd if the user clicks "Cancel", causing the
> SSL connection to abort.
Hm. The CLI should probably return 1 if the user hits Ctrl-D or
something like that. Or escape perhaps. But we're getting into the
realms of more complicated line editors ? we'd maybe end up doing
something like linking against readline, and I'm not convinced it's
worth it.
As for the GUI writing to the cancel pipe when we hit cancel... The
cancel pipe exists to allow us to force the openconnect thread to stop
when it's off doing something on its own, trying to connect to a server
or waiting for a response. When a dialog is *active* there's no need for
that, because we expect the openconnect thread to be waiting for our
answer and returning failure should be perfectly sufficient.
So perhaps that write to cancel_fd should be made conditional? We could
probably make form_shown into a tristate of inactive/pending/active
instead of just a simple true/false to indicate when the UI thread has
finished rendering it. And then the write to cancel_fd could happen only
when the form is inactive, and not in the pending/active states? I think
that should probably work, right?
An alternative is to read from the cancel_fd in prepare_stoken() to
'suck out the poison', which we do elsewhere in the GUI code. But I'm
not convinced I like that.
In fact, stepping back from the implementation and looking purely at it
from the user perspective, what *does* the cancel button mean in that
case? Does it mean to cancel the entire connection, or does it mean just
to cancel the entry of the token password/PIN and try connecting without
using libstoken even though it's been configured to do so? As a user I
think I'd be more inclined to expect the former, surely? If you want to
support that 'fall back to no libstoken' mode, perhaps you could trigger
that with an empty password/PIN entry (user just hits 'enter'). So maybe
this doesn't need to be 'fixed' at all?
Your addition of openconnect_set_stoken_mode() to the library isn't
quite right; it's redefining all the *existing* symbols to version 2.1
at the same time, which means that existing tools linked against version
2.0 of those functions will stop working. It's a complete break in
binary compatibility. It should look more like this:
diff --git a/libopenconnect.map.in b/libopenconnect.map.in
index 8a68b60..e52060f 100644
--- a/libopenconnect.map.in
+++ b/libopenconnect.map.in
@@ -32,5 +32,10 @@ OPENCONNECT_2.0 {
};
+OPENCONNECT_2.1 {
+ global:
+ openconnect_set_stoken_mode;
+} OPENCONNECT_2.0
+
OPENCONNECT_PRIVATE {
global: @SYMVER_TIME@ @SYMVER_ASPRINTF@ @SYMVER_GETLINE@ @SYMVER_PRINT_ERR@
openconnect_SSL_gets;
Finally, can we make the stoken option in the config UI magically
disappear if it's built against a version of openconnect without the
stoken support? I see you've greyed out the text box for entering the
token string, but the choice (added to the .ui file) is always active?
Perhaps we want an openconnect_has_stoken_support() method? Probably
wants to be in 'openconnect --version' output too.
--
Sent with MeeGo's ActiveSync support.
David Woodhouse Open Source Technology Centre
David.Woodhouse at intel.com Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 4370 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/openconnect-devel/attachments/20121014/d0996ba7/attachment.bin>
