On Sat, Jul 18, 2026 at 03:29:40PM +1000, Seth McDonald via isync-devel wrote:
To authenticate to IMAP servers, mbsync uses SASL if linked with the
Cyrus SASL library (libsasl2) and falls back to LOGIN otherwise.
this is too detailed. the details are not relevant to the next sentence,
so they connect poorly/misleadingly.
This
logic was inline in the IMAP driver but was mostly independent of the
surrounding logic.
Factor out this authentication logic into its own independent interface
(imap_auth_*). This isolates the logic
and thereby allows it to be more
easily extended with a different SASL library (e.g. GNU SASL).
i find that claim debatable. it's also kinda unnecessary, as a good
refactoring should stand on its own merits. one can mention it in
passing as a bonus.
The interface's implementation adds minor changes in behaviour to
simplify the authentication logic. This includes always attempting to
fall back to LOGIN when SASL fails and unifying similar error messages.
semantically, that seems just wrong. you need to work a lot harder to
convince me that this is a wanted change.
structurally, this cannot go into the same patch. do it before or after.
have a look at the patches surrounding previous refactorings to get an
idea of the things i like to separate from them, and how.
i looked through the history for references, and d5a5da947 seems
comparable in scope.
it's clearly changing the code structure in addition to moving the code,
but notably the visible behavior is not changed.
+++ b/src/imap_auth.c
+static const char *
+query_sasl_error( sasl_conn_t *ctx, int rc )
i don't want that obfuscation layer without good reason.
+{
+ const char *err = ctx
+ ? sasl_errdetail( ctx )
+ : sasl_errstring( rc, NULL, NULL );
+ return err ? err : "Unknown error";
what is that fallback for?
+static int
+process_sasl_step(
+ } else {
+ const char *err = query_sasl_error( session, rc );
i don't like these pointless temporaries.
don't try to "hide" style changes in the refactoring.
+ intersect_mechs(
+ common_mechs + 1,
+ size - 1,
+ any_mech ? server_mechs : client_mechs,
+ server_mechs );
that call formatting is rarely used in isync, and looks weird even with
the smaller tabs. don't use it without strong reason (long argument list
starting beyond column ~60, or arguments with long comments).
+++ b/src/imap_auth.h
as i wrote before, i think putting it into imap_p.h would be preferable.
mostly because i don't want the inconsistency with the other extracted
subsystems, and i don't want to create micro-headers for them, either.
+typedef const char *(*imap_auth_cred_t)( void * );
+
the consequences of having these are obviously ugly.
just factor out imap_auth_conf_t as i hinted at previously.
+/*
+ * The client side state of an IMAP authentication process.
+ */
+typedef struct imap_auth_client {
+ /* Callback to obtain username and authorization identity. */
+ imap_auth_cred_t user_cb; /* Is non-NULL. */
+ void *user_arg; /* May be NULL. */
+
+ /* Callback to obtain password. */
+ imap_auth_cred_t pass_cb; /* Is non-NULL. */
+ void *pass_arg; /* May be NULL. */
+
+ const char *domain; /* E.g. "imap.server.com". Is non-NULL. */
+ int force; /* Whether to allow use of LOGIN over an insecure channel. */
+ int saslir; /* Whether the SASL-IR capability is available. */
+ int secure; /* Whether the connection is over a secure channel. */
it's debatable whether these should be structure members. based on their
lifetime, i'd expect function parameters.
+ int cont; /* Whether another authentication step is expected. */
+
+ /*
+ * Library-dependent session state.
+ * Currently holds:
+ * - Cyrus SASL: sasl_conn_t *
+ * - No library: NULL
nope. #ifdef. also above. i don't want dead members.
+/*
+ * Initializes a new auth client with a default state.
+ */
consequently, it should be init_client.
+void imap_auth_new_client( imap_auth_client_t *client );
also, the _client suffixes seem annoying.
instead, the global functions should get _lib suffixes or something.
+/*
+ * Frees all resources associated with an auth client.
+ * The client structure should not be reused unless it is reinitialized.
+ */
here, cleanup_client would be more fitting.
+void imap_auth_free_client( imap_auth_client_t *client );
+/*
+ * Formats a list of SASL mechanisms supported by both client and server side.
+ * The list is space-delimited, including starting and ending with whitespace.
+ * Mechanisms are parsed in a case-insensitive manner.
+ */
+void imap_auth_filter_mechs(
this seems like an implementation detail that should not leak through
the abstraction.
+++ b/src/util.c
+char *
+find_string_list( const string_list_t *list, const char *str )
+
+char *
+find_string_list_case( const string_list_t *list, const char *str )
the return values make no sense. these should be bools, and the
functions called contains_....
related to other comments, i find it debatable that factoring these out
actually makes sense. but if it does, it would have to happen in a
separate patch.
i didn't comment on everything i find objectionable, because i expect
many things to change fundamentally.
i also didn't mention repetitions, because i expect you to find
analogous cases yourself.
as mentioned in several places, this patch still does way too many
things at once. while you don't necessarily need to start _physically_
from scratch, you really need to do that _logically_. fire up git gui
and add each hunk/line separately, checking whether it actually fits the
scope of the commit. stash and rework as necessary to get a nice series
of atomic commits with convincing commit messages. you won't get
"frivolous" changes past me.
just two notes on the followup patch:
i think it would yield cleaner code to amend HAVE_LIBSASL with
SASL_IS_GNU rather than having two mutually exclusive defines.
secondly, reduce the scope of the #ifdefs. minimizing duplication is
very high up on my priority list.
your prior macos keychain patches should be integrated into this series
to reduce overall churn. for that purpose, it's ok to either
- during refactoring, leave code in its old place when it will be
replaced in subsequent patches, or
- to add code in a new file that will be logically created only in the
subsequent refactoring (i think this is the case applicable here).
in either case, forward declarations can be used to overcome the
temporarily missing locality.
finally, as i already pointed out in
https://sourceforge.net/p/isync/mailman/message/59319314/ , i'm not a
fan of verbosity. document only things that are not obvious, and check
first whether they can't be made obvious instead. the criterion for
"obvious" is "obvious after actually considering the prototype and a
cursory reading of the implementation". as everything else,
documentation also needs to pull its own weight. this isn't a library
whose implementation should be treated as a black box.
_______________________________________________
isync-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/isync-devel
