SessionHelper#signalNegotiationEnd should pass reason through to listener

[duco-lw]

mina-sshd/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/SessionHelper.java

Line 1149 in 71b842f

listener.sessionNegotiationEnd(this, c2sOptions, s2cOptions, negotiatedGuess, null);

The reason is not passed through to the listener:

    protected void signalNegotiationEnd(
            SessionListener listener,
            Map<KexProposalOption, String> c2sOptions, Map<KexProposalOption, String> s2cOptions,
            Map<KexProposalOption, String> negotiatedGuess, Throwable reason) {
        if (listener == null) {
            return;
        }

        listener.sessionNegotiationEnd(this, c2sOptions, s2cOptions, negotiatedGuess, null);
    }

The doc for this says the negotiation is successful if this is null:

    /**
     * Signals the end of the negotiation options handling
     *
     * @param session           The referenced {@link Session}
     * @param clientProposal    The client proposal options (un-modifiable)
     * @param serverProposal    The server proposal options (un-modifiable)
     * @param negotiatedOptions The successfully negotiated options so far - even if exception occurred (un-modifiable)
     * @param reason            Negotiation end reason - {@code null} if successful
     */
    default void sessionNegotiationEnd(
            Session session,
            Map<KexProposalOption, String> clientProposal,
            Map<KexProposalOption, String> serverProposal,
            Map<KexProposalOption, String> negotiatedOptions,
            Throwable reason) {
        // ignored
    }

mina-sshd/sshd-core/src/main/java/org/apache/sshd/common/session/SessionListener.java

Line 151 in 71b842f

* @param reason Negotiation end reason - {@code null} if successful

This looks like this will always be "successful", even in the error case?

        } catch (IOException | RuntimeException | Error e) {
            signalNegotiationEnd(c2sOptions, s2cOptions, negotiatedGuess, e);
            throw e;
        }

mina-sshd/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java

Line 2259 in 71b842f

signalNegotiationEnd(c2sOptions, s2cOptions, negotiatedGuess, e);

Would changing the line from

        listener.sessionNegotiationEnd(this, c2sOptions, s2cOptions, negotiatedGuess, null);

to

        listener.sessionNegotiationEnd(this, c2sOptions, s2cOptions, negotiatedGuess, reason);

break anything?

[tomaswolf]

Thanks for pointing this out.

Would ... break anything?

No, it would not. Feel free to provide a PR.