Implement secret storage using libsecret #206
Conversation
There was a problem hiding this comment.
Nice! I left some comments, but I haven't worked with the codebase in a long time. As far as I see it, two of them would be blockers:
- uninitialized variable
attempted_fetch_from_secret_store - lifetime of objects used in the lookup_secret callback is not guaranteed
| return remote_id_; | ||
| }; | ||
|
|
||
| } // namespace |
There was a problem hiding this comment.
Suggest to move this into the anonymous namespace at the top of the file
| { | ||
| Glib::ustring remote_id = get_remote_hostname(xmpp); | ||
| info.attempted_fetch_from_secret_store = true; | ||
| lookup_secret(remote_id, username, [=] (std::optional<std::string> password) { |
There was a problem hiding this comment.
- use explicit capture list
- the name
passwordshadows the variable calledpasswordin the outer scope--would suggest to give it a different name (especially if you capture the whole outer scope)
| secret_password_store_finish(result, &error); | ||
| if (error != nullptr) | ||
| { | ||
| std::cerr << "Failed to store secret: " << error->message << std::endl; |
There was a problem hiding this comment.
If I remember correctly, most other notifications in Gobby use the status bar?
I would either make this a class that can have the status bar as a member, so it can show the error there, or propagate the error back to the caller.
| gchar* password = secret_password_lookup_finish(result, &error); | ||
| if (error != nullptr) | ||
| { | ||
| std::cerr << "Failed to lookup secret: " << error->message << std::endl; |
There was a problem hiding this comment.
Same here about error handling
| } | ||
| // Restart this function and do not re-query the secret store. | ||
| sasl_callback(session, xmpp, prop); | ||
| }); |
There was a problem hiding this comment.
I think this is dangerous--there is nothing that guarantees that this is still valid by the time the callback is invoked. Some action would need to be taken in the destructor that either cancels the secret lookup or makes the callback ignore any action once invoked.
Other than this, also session and xmpp are not guaranteed to still exist. It could probably be addressed with reference counting?
| store_secret( | ||
| get_remote_hostname(connection), | ||
| iter->second.username, | ||
| iter->second.last_password); |
There was a problem hiding this comment.
Might be wise to erase iter->second.last_password once it has been stored in the secret store, to avoid having it lying around in the memory? Possibly a future addition, might be worth a TODO comment if you think it's a good idea.
Persist password from successful connection attempts using libsecret into the local keyring (on !Windows, as libsecret requires a *nix). The passwords are keyed by target server and username.
Fixes #205