From fd84892e621677f27ab7605e0e1f06ce7af08b28 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 10 Feb 2020 18:01:09 +0100 Subject: [PATCH 1/3] Use secret_service_clear_sync() in wxSecretStore There doesn't seem to be any reason to use secret_service_xxx() functions for Save() and Load(), but use simple API function secret_password_clearv_sync() for Delete(). Use the equivalent function from the same API layer for it too instead. --- src/unix/secretstore.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/unix/secretstore.cpp b/src/unix/secretstore.cpp index e72ce36248..fba321a6c2 100644 --- a/src/unix/secretstore.cpp +++ b/src/unix/secretstore.cpp @@ -209,8 +209,9 @@ public: wxString& errmsg) wxOVERRIDE { wxGtkError error; - if ( !secret_password_clearv_sync + if ( !secret_service_clear_sync ( + NULL, // Default service GetSchema(), BuildAttributes(service), NULL, // Can't be cancelled From 5a454d373b6cf03ddef2714ac8fbc84f4949d5d4 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 10 Feb 2020 18:23:59 +0100 Subject: [PATCH 2/3] Optionally return error message from wxSecretStore::IsOk() This allows to give at least some explanation about why the secrets can't be stored to the user, e.g. they could search for a message such as The name org.freedesktop.secrets was not provided by any .service files to find out that gnome-keyring package needs to be installed on their system. Note that this change means that under Unix an attempt to connect to the secret service is now made when wxSecretStore is constructed and not just when it's used for the first time, as before. --- include/wx/private/secretstore.h | 2 + include/wx/secretstore.h | 5 +- interface/wx/secretstore.h | 14 +++-- samples/secretstore/secretstore.cpp | 6 +- src/common/secretstore.cpp | 13 ++++ src/unix/secretstore.cpp | 93 +++++++++++++++++++++++++++-- 6 files changed, 120 insertions(+), 13 deletions(-) diff --git a/include/wx/private/secretstore.h b/include/wx/private/secretstore.h index 4cb52bc91f..443273d627 100644 --- a/include/wx/private/secretstore.h +++ b/include/wx/private/secretstore.h @@ -78,6 +78,8 @@ private: class wxSecretStoreImpl : public wxRefCounter { public: + virtual bool IsOk(wxString* WXUNUSED(errmsg)) const { return true; } + virtual bool Save(const wxString& service, const wxString& username, const wxSecretValueImpl& password, diff --git a/include/wx/secretstore.h b/include/wx/secretstore.h index a81f125501..71c0c4cc64 100644 --- a/include/wx/secretstore.h +++ b/include/wx/secretstore.h @@ -126,8 +126,9 @@ public: ~wxSecretStore(); - // Check if this object is valid. - bool IsOk() const { return m_impl != NULL; } + // Check if this object is valid, i.e. can be used, and optionally fill in + // the provided error message string if it isn't. + bool IsOk(wxString* errmsg = NULL) const; // Store a username/password combination. diff --git a/interface/wx/secretstore.h b/interface/wx/secretstore.h index d301e63585..0af7958bb4 100644 --- a/interface/wx/secretstore.h +++ b/interface/wx/secretstore.h @@ -166,14 +166,16 @@ public: Example of storing credentials using this class: @code wxSecretStore store = wxSecretStore::GetDefault(); - if ( store.IsOk() ) + wxString errmsg; + if ( store.IsOk(&errmsg) ) { if ( !store.Save("MyApp/MyService", username, password) ) wxLogWarning("Failed to save credentials to the system secret store."); } else { - wxLogWarning("This system doesn't support storing passwords securely."); + wxLogWarning("This system doesn't support storing passwords securely " + "(%s).", errmsg); } @endcode @@ -205,9 +207,13 @@ public: static wxSecretStore GetDefault(); /** - Check if this object is valid. + Check if this object can actually be used. + + @param errmsg If not @NULL, this parameter is filled with a + user-readable error message explaining why the secret store can't + be used (this argument is new since wxWidgets 3.1.4) */ - bool IsOk() const; + bool IsOk(wxString* errmsg = NULL) const; /** Store a username/password combination. diff --git a/samples/secretstore/secretstore.cpp b/samples/secretstore/secretstore.cpp index 490be687f3..c9c453ae4d 100644 --- a/samples/secretstore/secretstore.cpp +++ b/samples/secretstore/secretstore.cpp @@ -196,9 +196,11 @@ int main(int argc, char **argv) } wxSecretStore store = wxSecretStore::GetDefault(); - if ( !store.IsOk() ) + wxString errmsg; + if ( !store.IsOk(&errmsg) ) { - wxFprintf(stderr, "Failed to create default secret store.\n"); + wxFprintf(stderr, "Failed to create default secret store (%s)\n", + errmsg); return EXIT_FAILURE; } diff --git a/src/common/secretstore.cpp b/src/common/secretstore.cpp index f00dc3dd26..672fdd17fe 100644 --- a/src/common/secretstore.cpp +++ b/src/common/secretstore.cpp @@ -162,6 +162,19 @@ wxSecretStore::~wxSecretStore() // Methods forwarded to wxSecretStoreImpl // ---------------------------------------------------------------------------- +bool +wxSecretStore::IsOk(wxString* errmsg) const +{ + if ( !m_impl ) + { + if ( errmsg ) + *errmsg = _("Not available for this platform"); + return false; + } + + return m_impl->IsOk(errmsg); +} + bool wxSecretStore::Save(const wxString& service, const wxString& user, diff --git a/src/unix/secretstore.cpp b/src/unix/secretstore.cpp index fba321a6c2..e72bc081c5 100644 --- a/src/unix/secretstore.cpp +++ b/src/unix/secretstore.cpp @@ -119,6 +119,52 @@ private: SecretValue* const m_value; }; +// Dummy implementation used when secret service is not available. +class wxSecretStoreNotAvailableImpl : public wxSecretStoreImpl +{ +public: + explicit wxSecretStoreNotAvailableImpl(const wxString& error) + : m_error(error) + { + } + + virtual bool IsOk(wxString* errmsg) const wxOVERRIDE + { + if ( errmsg ) + *errmsg = m_error; + + return false; + } + + virtual bool Save(const wxString& WXUNUSED(service), + const wxString& WXUNUSED(user), + const wxSecretValueImpl& WXUNUSED(secret), + wxString& errmsg) wxOVERRIDE + { + errmsg = m_error; + return false; + } + + virtual bool Load(const wxString& WXUNUSED(service), + wxString* WXUNUSED(user), + wxSecretValueImpl** WXUNUSED(secret), + wxString& errmsg) const wxOVERRIDE + { + errmsg = m_error; + return false; + } + + virtual bool Delete(const wxString& WXUNUSED(service), + wxString& errmsg) wxOVERRIDE + { + errmsg = m_error; + return false; + } + +private: + const wxString m_error; +}; + // This implementation uses synchronous libsecret functions which is supposed // to be a bad idea, but doesn't seem to be a big deal in practice and as there // is no simple way to implement asynchronous API under the other platforms, it @@ -127,6 +173,25 @@ private: class wxSecretStoreLibSecretImpl : public wxSecretStoreImpl { public: + static wxSecretStoreLibSecretImpl* Create(wxString& errmsg) + { + wxGtkError error; + SecretService* const service = secret_service_get_sync + ( + SECRET_SERVICE_OPEN_SESSION, + NULL, // No cancellation + error.Out() + ); + if ( !service ) + { + errmsg = error.GetMessage(); + return NULL; + } + + // This passes ownership of service to the new object. + return new wxSecretStoreLibSecretImpl(service); + } + virtual bool Save(const wxString& service, const wxString& user, const wxSecretValueImpl& secret, @@ -142,7 +207,7 @@ public: wxGtkError error; if ( !secret_service_store_sync ( - NULL, // Default service + m_service, GetSchema(), BuildAttributes(service, user), SECRET_COLLECTION_DEFAULT, @@ -167,7 +232,7 @@ public: wxGtkError error; GList* const found = secret_service_search_sync ( - NULL, // Default service + m_service, GetSchema(), BuildAttributes(service), static_cast @@ -211,7 +276,7 @@ public: wxGtkError error; if ( !secret_service_clear_sync ( - NULL, // Default service + m_service, GetSchema(), BuildAttributes(service), NULL, // Can't be cancelled @@ -279,6 +344,15 @@ private: NULL )); } + + // Ctor is private, Create() should be used for creating objects of this + // class. + explicit wxSecretStoreLibSecretImpl(SecretService* service) + : m_service(service) + { + } + + wxGtkObject m_service; }; const char* wxSecretStoreLibSecretImpl::FIELD_SERVICE = "service"; @@ -299,8 +373,17 @@ wxSecretValueImpl* wxSecretValue::NewImpl(size_t size, const void *data) /* static */ wxSecretStore wxSecretStore::GetDefault() { - // There is only a single store under Windows anyhow. - return wxSecretStore(new wxSecretStoreLibSecretImpl()); + // Try to create the real implementation. + wxString errmsg; + wxSecretStoreImpl* impl = wxSecretStoreLibSecretImpl::Create(errmsg); + if ( !impl ) + { + // But if we failed, fall back to a dummy one, so that we could at + // least return the error to the code using this class. + impl = new wxSecretStoreNotAvailableImpl(errmsg); + } + + return wxSecretStore(impl); } #endif // wxUSE_SECRETSTORE From f7f90a6111671cf1a49017fa1ae73c1fbdd85828 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 10 Feb 2020 18:27:38 +0100 Subject: [PATCH 3/3] Mention that wxSecretStore ctor can block At least under Unix it can show a dialog asking to create a new key ring. --- interface/wx/secretstore.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/interface/wx/secretstore.h b/interface/wx/secretstore.h index 0af7958bb4..2dbc630c77 100644 --- a/interface/wx/secretstore.h +++ b/interface/wx/secretstore.h @@ -203,6 +203,9 @@ public: Returns the default secrets collection to use. Call IsOk() on the returned object to check if this method succeeded. + + Note that this method may show a dialog to the user under some + platforms, so it can take an arbitrarily long time to return. */ static wxSecretStore GetDefault();