Skip to content

SSH Agent: Show error messages if there are problems with agent communication#1614

Merged
phoerious merged 1 commit intokeepassxreboot:release/2.3.1from
hifi:fix/agent-error-messages
Mar 6, 2018
Merged

SSH Agent: Show error messages if there are problems with agent communication#1614
phoerious merged 1 commit intokeepassxreboot:release/2.3.1from
hifi:fix/agent-error-messages

Conversation

@hifi
Copy link
Copy Markdown
Contributor

@hifi hifi commented Mar 3, 2018

Implements in-entry messages when using manual add/removal and messages on lock/unlock.

Motivation and context

Error handling is currently silent and is a blocker for a user to figure out what's wrong. Considering this a UX bug.

How has this been tested?

Manually.

Types of changes

  • ✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]


QMap<QString, QSet<OpenSSHKey>> m_keys;
QString m_error;
MessageWidget* m_messageWidget;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a QPointer.

void SSHAgent::init(MessageWidget* messageWidget)
{
m_instance = new SSHAgent(parent);
m_instance = new SSHAgent(messageWidget);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It don't like parenting the SSH agent to a single GUI widget. You should continue using the MainWindow or, even better, the application itself. Either pass in the MessageWidget as a second mandatory parameter or, even better, remove tight coupling altogether and only emit signals which you can connect to the MessageWidget from the outside.

@phoerious phoerious added this to the 2.3.1 milestone Mar 4, 2018
@hifi
Copy link
Copy Markdown
Contributor Author

hifi commented Mar 4, 2018

Connected to global MessageWidget with signals now. Still keeping the in-class error string because the same instance is shared between EditEntryWidget and it should be able to feed the error strings to its own MessageWidget.

void removeIdentityAtLock(const OpenSSHKey& key, const Uuid& uuid);

signals:
void showMessage(const QString& message, MessageWidget::MessageType);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this signal rather be called error or connectionFailed or something?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't directly map it to MessageWidget slot if it doesn't include the second argument so I made it compatible. Can I somehow create a glue between argument incompatible signal/slot easily?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would simply use a separate signal for each type of event (if you have multiple) and then create a function that consumes this event and shows a message with the correct MessageType.

@hifi hifi force-pushed the fix/agent-error-messages branch 2 times, most recently from 9e9bc5a to a30f33a Compare March 6, 2018 16:18
Copy link
Copy Markdown
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fab! Please rebase!

@hifi hifi force-pushed the fix/agent-error-messages branch from a30f33a to 8d37f49 Compare March 6, 2018 17:52
@phoerious phoerious merged commit 0847589 into keepassxreboot:release/2.3.1 Mar 6, 2018
phoerious added a commit that referenced this pull request Mar 6, 2018
- Fix unnecessary automatic upgrade to KDBX 4.0 and prevent challenge-response key being stripped [#1568]
- Abort saving and show an error message when challenge-response fails [#1659]
- Support inner stream protection on all string attributes [#1646]
- Fix favicon downloads not finishing on some websites [#1657]
- Fix freeze due to invalid STDIN data [#1628]
- Correct issue with encrypted RSA SSH keys [#1587]
- Fix crash on macOS due to QTBUG-54832 [#1607]
- Show error message if ssh-agent communication fails [#1614]
- Fix --pw-stdin and filename parameters being ignored [#1608]
- Fix Auto-Type syntax check not allowing spaces and special characters [#1626]
- Fix reference placeholders in combination with Auto-Type [#1649]
- Fix qtbase translations not being loaded [#1611]
- Fix startup crash on Windows due to missing SVG libraries [#1662]
- Correct database tab order regression [#1610]
- Fix GCC 8 compilation error [#1612]
- Fix copying of advanced attributes on KDE [#1640]
- Fix member initialization of CategoryListWidgetDelegate [#1613]
- Fix inconsistent toolbar icon sizes and provide higher-quality icons [#1616]
- Improve preview panel geometry [#1609]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants