On Mon, May 18, 2015 at 14:37:58 -0400, Yclept Nemo wrote:
> From ab1cb5d40765383ea1c11eca9c3d641f9d49bdd8 Mon Sep 17 00:00:00 2001
> From: Yclept Nemo <pscjtwjdjtAhnbjm/dpn>
> Date: Mon, 18 May 2015 14:07:13 -0400
> Subject: [PATCH] smbclient auth support
>
> There is now a backend block which in turns provides several password
> configuration backends:
I like the feature :) . Comments inline (FWIW, I am not a developer of
MPD; a user an occasional patcher).
> Configuration file:
> backend {
> name "smbclient"
> password_backend "config"
> username "ExampleName"
> password "ExamplePassword"
> }
> Libsecret (keyring):
> backend {
> name "smbclient"
> password_backend "libsecret"
> username "ExampleName"
> }
>
> Some values are optional:
> backend {
> name "smbclient"
> workgroup "FANCYWORKGROUP"
> username "ExampleName"
> }
Supporting a command (for tools such as pass[1]) would also be good.
Also, is there any additional documentation along with this?
> diff --git a/.gitignore b/.gitignore
> index ba6d3b3..e28c23b 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -6,6 +6,7 @@
> *.lo
> *.o
> *.exe
> +*.*.swp
> *~
This hunk is not related to the rest of the changes in this patch.
Please separate it out into a separate patch (at least).
> diff --git a/scripts/mpd.secret.py b/scripts/mpd.secret.py
> new file mode 100755
> index 0000000..30cb497
> --- /dev/null
> +++ b/scripts/mpd.secret.py
> @@ -0,0 +1,111 @@
> +#!/usr/bin/env python3
This looks like a new runtime dependency; is it documented anywhere?
> +import getpass
> +from gi.repository import Secret
New dependencies? Documentation?
> +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?
> +if __name__ == "__main__":
> + mpd_main()
Do you unlock the collection and/or item? Does libsecret do that
automatically? That may be necessary.
> 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...
> diff --git a/src/config/ConfigTemplates.cxx b/src/config/ConfigTemplates.cxx
> index 6fbf025..d283e29 100644
> --- a/src/config/ConfigTemplates.cxx
> +++ b/src/config/ConfigTemplates.cxx
> @@ -90,6 +90,7 @@ const ConfigTemplate config_block_templates[] = {
> { "filter", true },
> { "database", false },
> { "neighbors", true },
> + { "backend", "true" },
A better name? "password_backend"?
> static constexpr unsigned n_config_block_templates =
> diff --git a/src/lib/smbclient/Init.cxx b/src/lib/smbclient/Init.cxx
> index 999e60f..991d02d 100644
> --- a/src/lib/smbclient/Init.cxx
> +++ b/src/lib/smbclient/Init.cxx
> @@ -22,22 +22,146 @@
<snip>
> +#ifdef ENABLE_LIBSECRET
> +#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.
> +#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).
> + 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?
> + ( &mpd_schema , NULL , &error
> + , "server", smb_auth_data->server
> + , "share", smb_auth_data->share
> + , "workgroup", smb_auth_data->workgroup
> + , "username", smb_auth_data->username
> + , NULL
> + );
> +
> + if (error != NULL) {
> + g_error_free(error);
> + return "";
> + }
> +
> + if (password == NULL) {
> + return "";
> + }
> +
> + return password;
> +}
> +#endif
Thanks,
--Ben
[1]http://www.passwordstore.org/
_______________________________________________
mpd-devel mailing list
[email protected]
http://mailman.blarg.de/listinfo/mpd-devel
