SMBLibrary icon indicating copy to clipboard operation
SMBLibrary copied to clipboard

Exposes client connection and logged in state to the public API

Open KieranDevvs opened this issue 2 years ago • 6 comments

Currently, consumers of the client API's have to store and track the client state themselves when the client already contains this data. This pull request exposes these properties on the client interface and updates v1 & v2 of the SMB clients to expose their internal state via the interface.

KieranDevvs avatar May 21 '22 17:05 KieranDevvs

Hi Kieran, Thanks for the pull request, Given that you get a clean indication during the Connect and Login phase whether those operations were successful, it's not clear to me where this information is going to be useful, could you please shed light on this? in which scenario those properties are helpful?

TalAloni avatar May 25 '22 19:05 TalAloni

Hi Kieran, Thanks for the pull request, Given that you get a clean indication during the Connect and Login phase whether those operations were successful, it's not clear to me where this information is going to be useful, could you please shed light on this? in which scenario those properties are helpful?

Well this is my point, you're making the user track the state of the client outside of the client which is:

  1. Redundant as the state already persists within the client
  2. Leads to bug prone code as users may miss certain state changes, or the clients state might change without calling user code and now, the client's internal state and the user tracked state no longer align.

Take this example below, what happens if there's a socket disconnect because of invalid packets or a network drop? The user tracked state of the client is no longer valid. However, if you were to use the clients tracked state and there is a socket disconnect, the socket disconnect is caught within the client and the internal state is updated.

(This is just sudo, the method names may differ in the actual API)

private bool isConnected = false;
private bool isLoggedIn = false;

private SMB2Client _client = new SMB2Client();

public void RenderMyUI()
{
    if(!isConnected)
    {
        // Render connection form UI.
    }
    else if (isConnected && !isLoggedIn)
    {
        // Render login form UI
    }
    else if (isConnected && IsLoggedIn)
    {
        // Render file browser
    }
}

public void Connect(string hostname)
{
    var state = _client.Connect(hostname);
    if (state == NT_SUCCESS)
    {
        isConnected = true;
    }
}

KieranDevvs avatar May 26 '22 09:05 KieranDevvs

Its the same reason System.Net.Sockets.Socket has the public property Connected https://docs.microsoft.com/en-us/dotnet/api/system.net.sockets.socket.connected?view=net-6.0

KieranDevvs avatar May 26 '22 11:05 KieranDevvs

@TalAloni Can I get an update on this please?

KieranDevvs avatar Jun 30 '22 19:06 KieranDevvs

Hi Kieran, I apologize, I've taken a few responsibilities and I can't seem to find the time to give thought to this non-urgent request. I hope I'll find the time in the coming weeks.

TalAloni avatar Jul 02 '22 18:07 TalAloni

Hi Kieran, I apologize, I've taken a few responsibilities and I can't seem to find the time to give thought to this non-urgent request. I hope I'll find the time in the coming weeks.

Ok thanks, appreciate it.

KieranDevvs avatar Jul 03 '22 12:07 KieranDevvs

Hi Kieran, I've exposed IsConnected given that indeed this property can be changed by the server and may be useful to have available. Regarding IsLoggedIn - I'm not sure I see as much value in that given that the client is in complete control of the login which should immediately follow the Connect phase (i.e. the server cannot Logoff a user - only to disconnect the connection). I hope this helps. thank you.

TalAloni avatar Oct 28 '22 11:10 TalAloni

@TalAloni Looks like something is wrong with the 1.4.9 release, your changes don't appear to be in that release binary or source code.

KieranDevvs avatar Nov 06 '22 21:11 KieranDevvs

These changes were merged after the 1.4.9 release (I've remembered about them after already releasing 1.4.9).

TalAloni avatar Nov 07 '22 05:11 TalAloni