bgmulinari / B1SLayer

A lightweight SAP Business One Service Layer client for .NET
MIT License
122 stars 42 forks source link

Invalid session #46

Closed it-AhmedTaha closed 11 months ago

it-AhmedTaha commented 11 months ago
          This should be fixed with version [1.3.1](https://github.com/bgmulinari/B1SLayer/releases/tag/1.3.1). Sorry for the delay on this release.

Thanks for your report, @it-AhmedTaha.

Originally posted by @bgmulinari in https://github.com/bgmulinari/B1SLayer/issues/31#issuecomment-1653879012

Hi, thank you for the new update. I have performed some tests with the new update, but the issue still persists. I believe there might be a problem in the logic of the ExecuteLoginAsyncmethod, specifically in determining whether a new login should happen or not.

Here's the part of the code that I think might be incorrect:

if (DateTime.Now.Subtract(this._lastRequest).TotalMinutes < (double)(this._loginResponse.SessionTimeout - 1)) The issue seems to be that the _lastRequestvariable is always updated with each new request, so the result of DateTime.Now.Subtract(this._lastRequest).TotalMinutes will never be greater than _loginResponse.SessionTimeout.

As a potential solution, I suggest utilizing the LastLogintime instead to calculate the time difference accurately. Here's a possible modification:

var timeDifference = DateTime.Now - _loginResponse.LastLogin;
if (timeDifference.TotalMinutes < 29)
    return expectReturn ? LoginResponse : null;
bgmulinari commented 11 months ago

Hi, @it-AhmedTaha. Thanks again for your report on this.

The way Service Layer's session works is based on a timeout of 30 minutes (by default). That means, your session will not expire unless you go for 30+ minutes without making any request with the active session. That's why I update _lastRequest each request, to check if the time between the last request and now is greater than the session timeout value.

I'm curious to know your scenario where you observe this invalid session error. Are you able to write a code snippet to simulate the issue?

bgmulinari commented 11 months ago

@it-AhmedTaha, I did some more testing and I think I was able to reproduce the issue.

I pushed a fix to the dev branch. Can you check if it works for you? Thanks.

it-AhmedTaha commented 11 months ago

The way Service Layer's session works is based on a timeout of 30 minutes (by default). That means, your session will not expire unless you go for 30+ minutes without making any request with the active session.

I have done many tests with services layer session, and session timeout is 30 minutes even if you do a request at minute 29. After 30 minutes you have to do another a new login request to get a new session.

That's why I update _lastRequest each request, to check if the time between the last request and now is greater than the session timeout value.

The time between _lastRequest and now will never be grater than the _loginResponse.SessionTimeout because each time there is a new request you are setting the value of _lastRequest = DateTime.Now.

I'm curious to know your scenario where you observe this invalid session error. Are you able to write a code snippet to simulate the issue?

Sure, this is one of the requests I do with NumberOfAttempts = 1 :

await serviceLayer.Request("Invoices").PostAsync(invoices);

Do a request and then wait for about 32 minutes, and then try again you will get "Invalid session"

it-AhmedTaha commented 11 months ago

@it-AhmedTaha, I did some more testing and I think I was able to reproduce the issue.

I pushed a fix to the dev branch. Can you check if it works for you? Thanks.

I did some tests with it and so far I haven't got any "invalid session". However I think checking against the last login time is better than checking for Unauthorizedstatus. It is faster to check against last login time than wait for Unauthorizedresponse.

bgmulinari commented 11 months ago

@it-AhmedTaha, I also did some extensive testing with the session management in Service Layer and from my experience it works exactly as I described. You can use the same session indefinitely and it will not expire, provided you don't go for 30+ minutes without making any request with the active session. If that happens, a new login should be performed to get a new valid session.

That means, if I used the login time (like you suggested) to check whether a session is valid, I would end up creating a new session without the need for one in most cases.

Here's a quick test I did, check out the login time and the session ID: 1

Here's a random GET request, almost 45 minutes later, still using the same session ID: 2

it-AhmedTaha commented 11 months ago

I understand why you might not want to rely solely on the login time for session management. Let's examine your logic using two different scenarios.

1.Scenario:

1.Case:
- A new request is made at 8:00 am, creating a new login session.
- Another request is made at 8:05 am. 
- Let's calculate the time elapsed since the last request:
    - lastRequest is set to DateTime.Now, which is 8:05.
    - Check if the time elapsed (DateTime.Now.Subtract(this._lastRequest).TotalMinutes=> (8:05 am - 8:05 am)) is less than the session timeout (30 - 1).
    - The calculation is (0.777), which is indeed less than 29 minutes.
As a result, the condition is true, and the session is considered valid.

2. Case:
- Another request is made at 8:28 am. 
- Let's calculate the time elapsed since the last request:
    - lastRequest is set to DateTime.Now, which is 8:28 am.
    - Check if the time elapsed (DateTime.Now.Subtract(this._lastRequest).TotalMinutes=>(8:28 am - 8:28 am)) is less than the session timeout (30 - 1).
    - The calculation is (0.777), which is indeed less than 29 minutes.
As a result, the condition is true, and the session is considered valid.

So far, the logic appears to be correct. However, there is another scenario that would return invalid session:

  1. Scenario:
- A new request is made at 10:00 am, creating a new login session.
- Another request is made at 10:45 am. 
- Let's calculate the time elapsed since the last request:
     - lastRequest is set to DateTime.Now, which is 10:45 am before LoginInternalAsync.
     - Check if the time elapsed (DateTime.Now.Subtract(this._lastRequest).TotalMinutes=> (10:45 am - 10:45 am)) is less than the session timeout  (30 - 1).
     - The calculation is (0.777), which is indeed less than 29 minutes.

Unfortunately, the result is true, indicating that the session is still valid, leading to incorrect behavior. In reality, the session created at 10:00 am has already expired, and a new login request should be performed.

I think the problem is that you are setting the _lastRequest = DateTime.Now; before calling LoginInternalAsync();

image

If I am still missing something and not really understanding the logic correctly, than I think a good solution would be this fix.

@it-AhmedTaha, I did some more testing and I think I was able to reproduce the issue.

I pushed a fix to the dev branch. Can you check if it works for you? Thanks.

rcalv002 commented 11 months ago

I understand why you might not want to rely solely on the login time for session management. Let's examine your logic using two different scenarios.

1.Scenario:

1.Case:
- A new request is made at 8:00 am, creating a new login session.
- Another request is made at 8:05 am. 
- Let's calculate the time elapsed since the last request:
    - lastRequest is set to DateTime.Now, which is 8:05.
    - Check if the time elapsed (DateTime.Now.Subtract(this._lastRequest).TotalMinutes=> (8:05 am - 8:05 am)) is less than the session timeout (30 - 1).
    - The calculation is (0.777), which is indeed less than 29 minutes.
As a result, the condition is true, and the session is considered valid.

2. Case:
- Another request is made at 8:28 am. 
- Let's calculate the time elapsed since the last request:
    - lastRequest is set to DateTime.Now, which is 8:28 am.
    - Check if the time elapsed (DateTime.Now.Subtract(this._lastRequest).TotalMinutes=>(8:28 am - 8:28 am)) is less than the session timeout (30 - 1).
    - The calculation is (0.777), which is indeed less than 29 minutes.
As a result, the condition is true, and the session is considered valid.

So far, the logic appears to be correct. However, there is another scenario that would return invalid session:

  1. Scenario:
- A new request is made at 10:00 am, creating a new login session.
- Another request is made at 10:45 am. 
- Let's calculate the time elapsed since the last request:
     - lastRequest is set to DateTime.Now, which is 10:45 am before LoginInternalAsync.
     - Check if the time elapsed (DateTime.Now.Subtract(this._lastRequest).TotalMinutes=> (10:45 am - 10:45 am)) is less than the session timeout  (30 - 1).
     - The calculation is (0.777), which is indeed less than 29 minutes.

Unfortunately, the result is true, indicating that the session is still valid, leading to incorrect behavior. In reality, the session created at 10:00 am has already expired, and a new login request should be performed.

I think the problem is that you are setting the _lastRequest = DateTime.Now; before calling LoginInternalAsync();

image

If I am still missing something and not really understanding the logic correctly, than I think a good solution would be this fix.

@it-AhmedTaha, I did some more testing and I think I was able to reproduce the issue. I pushed a fix to the dev branch. Can you check if it works for you? Thanks.

I can confirm this is also still happening for me on long lived processes, eventually, we start getting perpetual invalid session error

bgmulinari commented 11 months ago

Hi, @it-AhmedTaha. Thanks for your detailed scenarios on this.

After taking a closer look, setting the last request time before the login was indeed an oversight on my part that would cause the issue you detailed in your scenario 2.

I pushed another commit to the dev branch that should fix this. Are you able to test it, please? Thanks!

it-AhmedTaha commented 11 months ago

Hi, @it-AhmedTaha. Thanks for your detailed scenarios on this.

After taking a closer look, setting the last request time before the login was indeed an oversight on my part that would cause the issue you detailed in your scenario 2.

I pushed another commit to the dev branch that should fix this. Are you able to test it, please? Thanks!

Hello, I did some tests with the new changes, and I did not get any invalid sessions errors.

bgmulinari commented 11 months ago

@it-AhmedTaha, @rcalv002

This fix is now available in version 1.3.2. Thanks for the reports and the help with testing!