Move IDisposable implementation declaration from inheritees to parent

[bad-samaritan]

Hello,
I was using SSH.NET library in my project and I noticed that by following inheritance rules I fell into a trap.

My SFTP wrapper class contains private field of a general AuthenticationMethod type.
It is is assigned in different constructors depending on selected authentication method.

	public class SftpFileHandler : IDisposable
	{
(...)
		private readonly AuthenticationMethod _authMethod;
(...)

		public SftpFileHandler(string handlerName, string address, int port, string? relativeWorkingDir, string? username, string? password, bool? keepConnectionOpen)
		{
(...)
			_authMethod = new PasswordAuthenticationMethod(username, password ?? "");
(...)
		}

		public SftpFileHandler(string handlerName, string address, int port, string? relativeWorkingDir, string? username, string privateKeyFilePath, string? privateKeyPass, bool? keepConnectionOpen)
		{
(...)
			_authMethod = new PrivateKeyAuthenticationMethod(Username, new PrivateKeyFile(privateKeyFilePath, privateKeyPass ?? ""));
(...)
		}

When I was implementing Dispose() for SftpFileHandler I noticed that AuthenticationMethod does not require disposing which seemed a bit suspicious to me since it may contain sensitive data that may require proper disposing.
So I browsed descendants of AuthenticationMethod to discover that all 4 of them implement IDisposable interface.
That being said in other to properly dispose the whole SftpFileHandler I need to do something like:

	public class SftpFileHandler : IDisposable
	{
(...)
		protected virtual void Dispose(bool disposing)
		{
			if (!_disposed)
			{
				if (_authMethod != null && _authMethod is IDisposable disposableAuthMethod)
				{
					disposableAuthMethod.Dispose();
				}
(...)

Which is always true...
Instead of just simply:

	public class SftpFileHandler : IDisposable
	{
(...)
		protected virtual void Dispose(bool disposing)
		{
			if (!_disposed)
			{
				if (_authMethod != null)
				{
					_authMethod .Dispose();
				}
(...)

I would like to propose a small improvement that would make disposing requirement move visible thus making it easier to maintain a little bit more secure code.
My suggestion is to move IDisposable a level above -> to the AuthenticationMethod:

namespace Renci.SshNet
{
    /// <summary>
    /// Base class for all supported authentication methods
    /// </summary>
    public abstract class AuthenticationMethod : IAuthenticationMethod, IDisposable
    {

This way code analyzing tools would warn programmers before making a mistake of not disposing inheritee of AuthenticationMethod class.

I tested changes mentioned above in my code and they work as expected.

What do you think about this improvement?

[IgorMilavec]

Dispose pattern should be implemented in accordance with Implement a Dispose method to avoid code duplication.

[bad-samaritan]

Hey @IgorMilavec, thanks for the feedback.
I moved common Dispose stuff along with some fields to AuthenticationMethod to avoid code duplication - see 8ca32fa.
Looks better?

[IgorMilavec]

Looks much cleaner now. Why not instantiate _authenticationCompleted in AuthenticationMethod's constructor? Also, do not declare finalizers, if they do not actually perform cleanup.

[bad-samaritan]

Why not instantiate _authenticationCompleted in AuthenticationMethod's constructor?

Because derived classes use different types for the EventWaitHandle : PrivateKeyAuthenticationMethod is using ManualResetEvent and the rest three are using AutoResetEvent.
We can also instantiate it with AutoResetEvent in AuthenticationMethod ctor and override in PrivateKeyAuthenticationMethod to be ManualResetEvent. I assumed the first one is clearer because it indicates that this field is used differently in derived classes but I don't have a strong opinion on that.

Also, do not declare finalizers, if they do not actually perform cleanup.

Totally agree! Especially that classes with finalizers can take more time to dispose, so removing them is actaully small performance optimization.
However, this was another part of the code that I did not touch because it was not primary objective of this pull request. But sure, let's also correct that.

Updated code without finalizers on c34ad23.

[IgorMilavec]

Huh, I totally missed different EventWaitHandles... I understand now. :)

[IgorMilavec]

Looking good to me.

[bad-samaritan]

Pleasure doing business with you.

Thanks