Thanks, re:
On Mon, May 18, 2015 at 5:27 PM, Ben Boeckel <[email protected]> wrote:
> Supporting a command (for tools such as pass[1]) would also be good.
> Also, is there any additional documentation along with this?
>
So a password_backend ("command") that runs a command and retrieves stdout
and return code? Any recommendations boost, popen, etc?
> This hunk is not related to the rest of the changes in this patch.
> Please separate it out into a separate patch (at least).
>
Will do.
> > +#!/usr/bin/env python3
> This looks like a new runtime dependency; is it documented anywhere?
>
This (mpd.secret.py) is not a hard runtime dependency; it is a script to
help end-users setup libsecret keyrings. But yes I'll write some
documentation concerning the backend block to the mpd.conf man page and
also mention this helper script. And as Max mentioned, it should be
installed to $bindir.
> New dependencies? Documentation?
>
Right, and I'll mention libsecret in INSTALL
>
> > +def mpd_collection_ensure(label):
> > + service =
> Secret.Service.get_sync(Secret.ServiceFlags.LOAD_COLLECTIONS)
> > + collections = Secret.Service.get_collections(service)
> > + labels = [collection.get_label() for collection in collections]
>
> Not all collections have a label (I'm assuming this is the
> org.freedesktop.Secret.Collection.Label property) or an alias; I
> wouldn't rely on it. Or is this basically the path to the collection?
>
Correct me if I'm wrong, but all collections do have a label; the alias is
optional [1]. The label is used to form the path.
> Do you unlock the collection and/or item? Does libsecret do that
> automatically? That may be necessary.
>
Heh, really just wrote this and posted with minimal testing. However I've
tested automatic unlocking (with the gnome-keyring backed) and it works
nicely:
* script: automatically prompts to unlock keyring when storing password, if
locked
* mpd: automatically prompts to unlock keyring when fetching password, if
locked
In both cases the prompt includes an option to automatically unlock the
relevent collection/keyring on login, via the default/login
collection/keyring.
> diff --git a/src/config/ConfigOption.hxx b/src/config/ConfigOption.hxx
> > index 5acd6fd..8d11a3f 100644
> > --- a/src/config/ConfigOption.hxx
> > +++ b/src/config/ConfigOption.hxx
> > @@ -90,6 +90,7 @@ enum class ConfigBlockOption {
> > AUDIO_FILTER,
> > DATABASE,
> > NEIGHBORS,
> > + BACKEND,
>
> A more descriptive name for this would be better...
>
I disagree, the BACKEND is intended to be a single top-level keyword to
configure multiple backends. So the backend block can configure the
smbclient backend (via name param), in which case the password_backend
param is specific to the smbclient backend. This makes sense as different
backends may have different password/attribute requirements, such as a
hypothetical sftp backend.
> +#pragma GCC diagnostic push
> > +#pragma GCC diagnostic ignored "-Wmissing-field-initializers"
>
> Not everything is GCC. I'd say this should be wrapped in __GNUC__
> checks.
>
Check.
> +#ifdef ENABLE_LIBSECRET
> > +const char*
> > +mpd_smbc_get_auth_data_secret
> > + ( SmbAuthData* smb_auth_data
> > + )
> > {
> > - // TODO: implement
> > - strcpy(wg, "WORKGROUP");
> > - strcpy(un, "");
> > - strcpy(pw, "");
> > + GError *error = NULL;
> > +
> > + // When to free?
>
> The caller should probably own the data returned here (other backends
> use strdup for valid values, and NULL be used on error paths).
>
I'm not really sure what you mean by other backends? I took a cursory look
at:
* (ConfigBlock*)config_get_block(...);
* (...)GetBlockValue(...);
In both cases the data returned is not owned by the caller. Correct me if
I'm wrong, but I'm not supposed to delete/free this data?
That said, I'll probably add a bool value to SmbAuthData indicating the
password field needs to be destroyed via secret_password_free.
>
> > + const char* password = (const char*)secret_password_lookup_sync
>
> Should the event loop really be blocked? Also, are collections and/or
> items unlocked here properly?
>
Yeah wasn't really sure about this. My reasoning is, why not block if the
music_directory isn't even available yet. Also, I wasn't sure how to hook
into the event loop.
[1]
https://people.gnome.org/~stefw/libsecret-docs/SecretCollection.html#secret-collection-create-sync
_______________________________________________
mpd-devel mailing list
[email protected]
http://mailman.blarg.de/listinfo/mpd-devel
