Bug: Logout function at server level [Fixed]

[LeoNeeson]

SOLVED!

» Edit #3 (12-05-2020): Now logout is 100% perfect on v2.4 Alpha 8.

» Edit #2 (09-05-2020): This was almost fixed by Rejetto on v2.4 Alpha 5.

» Edit #1 (07-05-2020): I had to edit the title (Confirmed bug: HFS doesn't discard previous auth sessions), because it seems some people have not understood it. Like the title says: HFS doesn't discard the 'session ID' of a authenticated user, when he logouts using a form-based login (we are talking about the 'logout' function at server level, and to reproduce the bug, the user must not use the native login function of the browser).


@Message to Rejetto, Mars, SilentPliz or any other Delphi/Pascal programmer:

• Steps to reproduce this possible bug:
1) Use default 2.3's template along with this form-based login (diff-template).
2) Create a user and have some shared folders protected with a password.
3) Open your browser and use the form-based login to authenticate (do NOT enter credentials on the browser's internal popup login, hit cancel on that popup window).
4) Open several password-protected folders, in several browser tabs, and navigate thought those sub-folders if you want.
5) Click on the 'Logout' button, change to another tab, and navigate on some password-protected resource (you will be automatically logged in again!).

» Why this is a HFS bug and not a fault in the template?...
Please follow the next steps and you will find how it SHOULD work:
A) Follow steps 1 to 5, but after clicking on the logout, temporary close HFS.
B) Open HFS again, and now try to navigate on some password-protected resource (you will NOT be automatically logged in!). Yay! This demonstrates that HFS is not discarding (in his memory) the association between some previously logged in USER and the session ID (SID) he used.

- You may say: but this could be solved on the client side by generating a new 'session ID' cookie. You are right, but if the user had several tabs open (or if he goes back in the browser history), he will be automatically logged in back again, and this is unwanted (and insecure).

- What this means?: This means that when this bug is fixed, no matter if you go back in your browser, once you logout you can't access any password protected resource anymore (no matter if you had multiple tabs open).

If you have any questions or difficulties on reproducing this, please ask me. This thread is open to anyone, so, don't be afraid to leave your question...

Cheers,
Leo.-

[LeoNeeson]

@Rejetto: (have you read this thread or not?). If you read this, please leave a comment about this issue (your opinion is very important). Thanks!


To make you easier to analyze the code, this is the function we need to optimize/fix:

  sessionSetup();
  if data.postVars.indexOfName('__USER') >= 0 then
    begin
    s:=data.postVars.values['__USER'];
    data.account:=getAccount(s);
    if data.account = NIL then
      if s = '' then // logout
        begin
        s:='ok';
        data.usr:='';
        data.pwd:='';
        end
      else
        s:='username not found'
    else
      begin
      data.usr:=s;
      { I opted to use double md5 for this authentication method so that in the
        future we may make this work even if we store hashed password on the server.
        In such case we would not be able to calculate pwd+sessionID because we'd had no clear pwd.
        By relying on md5(pwd) instead of pwd, we will avoid such problem. }
      s:=data.postVars.values['__PASSWORD_MD5'];
      if (s > '') and (s = strMD5(strMD5(data.account.pwd)+data.sessionID))
          or (data.postVars.values['__PASSWORD'] = data.account.pwd) then
        begin
        s:='ok';
        data.pwd:=data.account.pwd;
        data.sessionSet('user', data.usr);
        data.sessionSet('password', data.pwd);
        end
      else
        begin
        s:='bad password';
        data.account:=NIL;
        data.usr:='';
        end;
      end;
    if data.postVars.values['__AJAX'] = '1' then
      begin
      replyWithString(s);
      exit;
      end;
    end;

That is found on main.pas.

@anyone non-programmer: that's Delphi code, not JavaScript.


» EDIT: Please see my next message to find a possible fix.

Could someone please confirm if is able to reproduce this error...
Cheers,
Leo.-
 

[LeoNeeson]

OK, I'll make some tests. What do you think about this?...

.......
    if data.account = NIL then
      if s = '' then // logout
        begin
        s:='ok';
        data.usr:='';
        data.pwd:='';
        data.sessionSet('user', data.usr); //add by leo
        data.sessionSet('password', data.pwd); //add by leo
        Kickidleconnections1Click(NIL); //add by leo
        end
      else
        s:='username not found'
    else
.......

I'll report later, after doing several tests...

@Mars: are you OK with me? (because you seem to ignore my posts, and you almost never participate or reply on any of my messages, even when I quote you). Do you feel offended by something I've said in the past?. Please, forgive me if I've said something that offended you...
 

[LeoNeeson]

Woohoo!! That fix gloriously WORKS! I've compiled HFS by myself and I can confirm THIS fix totally solves the logout problem! Now we could have a perfectly working logout function, by using a form-based login. If you have a better approach to solve this issue, please feel free to submit your proposed code.

» EDIT: I'm now awaiting a reply from Rejetto, and see if he approves this fix for HFS v2.4 RC5.

Cheers,
Leo.-

[SilentPliz]

Hi Leo!

I tried today to compile the 2.3m of rejetto including your modifications ... and it works !!!

I used the diff.tpl provided by you, and tested it on two browsers (FF and Chrome) at the same time, with two different user accounts.

The logout works correctly, as well as the 'Digest access authentication'.

So ... Bravo!! and Bravo!! Well done !!
Since the time users requested these features, this is a big step forward for hfs. It's a great work.

The only small questioning I have is the excessive presence of the browser 'default login box'.
If it were installed as is, the user, I think, would miss using the 'Digest access authentication' feature.
But it's just a detail of form, which must be able to be resolved.

Congratulations again and thank you for your work.

We await with amazement and tremors, the Boss' verdict!

[LeoNeeson]

Thank you for the compliments (I truly appreciate them).

» Technical explanation: Although my fix works, it's not 100% perfect. With my fix, we clear the user & pass data stored along with the session (that's good), and then we kick idle connections (that's not so good, but it's needed at the moment). This is all fine, but the best thing would be removing the 'session ID' from 'sessions.objects' list (and then, perhaps would not be needed to kick idle connections), all this, along with issuing a new 'session ID' cookie at the moment of logout (but it's not clear if that's of much help). Why kicking idle connections is not perfect? because, besides of being a dirty trick, if the user is STILL downloading a password protected file, and he logouts but continues navigating, sometimes he could get logged in back again. That's why I say that we would need a better approach than only kicking idle connections, and that we must remove the 'session ID' (on the server) from 'sessions' list at the moment of logout.

» Design/template: I recognize my form based login is ugly for the new template of v2.4 (it need to be adapted to the new template). Perhaps a modal would fit best, since it doesn't take space, and it's easy to add, or a hidden input (that shows up when you click on it), like the current search box.

Feel anyone free to contribute on all this, and I'm sure Rejetto would come up with great new fresh ideas, for both: on the design and the technical side.

Cheers,
Leo.-

[LeoNeeson]

I would like to bring attention to this thread, because even if Rejetto is not interested on incorporating this fix on a final release of HFS (which is absolutely fine), we still need "some light" on this issue...

@Rejetto: since you internally know how sessions are managed by HFS, could you please say us how we could remove the 'session ID' from 'sessions.objects' list? (to add this on the logout function, and make it work properly). Or at least saying us if we are doing this fix in the right way. My proposed fix works fine, but I consider is not perfect. What do you think about it? (can it get better?)

Doing this, we could mark this issue as fixed, and give a conclusion to this thread, and if you approve the changes, at least this fix could be added on the SilentPliz version (thanks!).

We await with amazement and tremors, the Boss' verdict!

It seems he overlooked and missed this thread, with all the fuss of Delphi 10 upgrade... 

[Mars]

hslib.pas , substitute 'WWW-Auth........'  text will remove request of ident

[LeoNeeson]

hslib.pas , substitute 'WWW-Auth........'  text will remove request of ident

@Mars: I appreciate your comment. Your suggestion is only a detail, not as important as having a proper logout (we are talking about having a logout that actually works when we use a form-based login, something completely different to the login process).

...but if you want to talk about this, I will comment my idea. Instead of removing the login request (like you suggest), I've found here a much better solution. It consist of the server NOT giving the reply 'WWW-Authenticate' when the client send THIS on the header: "X-Requested-With: XMLHttpRequest". That way we keep everything according the standards (and we don't break the normal function).

Currently, we have something like this (on main.pas):

Code: [Select]

.......
    if result then exit;
    conn.reply.realm:=f.getShownRealm();
    runEventScript('unauthorized');
    getPage('unauthorized', data);
    // log anyone trying to guess the password
    if (forceFile = NIL) and stringExists(data.usr, getAccountList(TRUE, FALSE))
    and logOtherEventsChk.checked then
      add2log('Login failed', data);
    end; // accessGranted
.......
With the new changes I propose, it will be something like this (my additions are marked in blue color):

.......
    if result then exit;
    if (conn.getHeader('X-Requested-With') = 'XMLHttpRequest') then //add by leo
        getPage('unauthorized', data); //add by leo
    else //add by leo
        conn.reply.realm:=f.getShownRealm();
        runEventScript('unauthorized');
        getPage('unauthorized', data);
    end; //add by leo
    // log anyone trying to guess the password
    if (forceFile = NIL) and stringExists(data.usr, getAccountList(TRUE, FALSE))
    and logOtherEventsChk.checked then
      add2log('Login failed', data);
    end; // accessGranted
.......

(The code above is untested, but that's the main idea).

Mars: besides that, and talking again about the logout, feel free to ask if you don't understand something or if you can't reproduce it, following my steps. If you need, I can post any of my previous posts on french language (using a translator), but I guess it will be worst, since online translators are not good. Your english is good, but in case of doubt, perhaps SilentPliz can explain to you this better than me (in french).

Vouloir c’est pouvoir.

[rejetto]

sorry i totally missed this topic, i'm reading now

[rejetto]

you made a wonderful work Leo, thanks
i'm now following your suggestion for a better fix removing the whole session.
Have to study it a bit because it's old stuff i don't remember.

[rejetto]

ok, i think this should be a solution

Code: [Select]

procedure TconnData.logout();
begin
freeAndNIL(session);
sessions.delete(sessions.IndexOf(sessionID));
sessionID:='';
usr:='';
pwd:='';
conn.delCookie(SESSION_COOKIE);
end; // logout


[rejetto]

i'm also taking care of the problem that sessions never expire

[rejetto]

please have a look
https://github.com/rejetto/hfs2/releases/tag/v2.4-alpha05

[LeoNeeson]

WOW!! Now you fixed it!!!
(Edit: there is a very small issue still present)

you made a wonderful work Leo, thanks

You welcome, my fixes were nothing compared to your awesome work.   (your logout implementation is fantastic, since you have covered a lot of details). For me, we could give this issue as concluded.


» Edit #1: I've found a super-small detail that you don't need to fix if it's complicated (please read edit #2 to know how to reproduce it). If the user is fast enough, and goes back (or if he had another tabs open) and try to access another couple of protected files (a few seconds after logout), then he could open a few protected files more. This only happens for a few seconds after doing the logout (since if connections get closed by timeout, then he could not regain access to any other protected file). That's why I added a line to "kick idle connections" (but I didn't like to do it, because it could kick an important idle connection from other users). Possible solution to this? Perhaps if an 'extra check' is done on every file request (at least, only for a few seconds after logout was called), or even better, if we could kick idle connections but only from the recently logged-out user, then this would be absolutely 100% perfect. But hey! it's 99.9999% perfect, so, don't take this as a request (it's only a super-ultra-small detail), since for me this is completely solved.


» Edit #2: How to reproduce this small issue?: open several pass-protected pictures (on different tabs, let say: pic1.jpg, pic2.jpg and pic3.jpg), then click on logout, and then quickly go back and click on another pass-protected picture (one that was not previously cached, like pic4.jpg), ...and bang! pic4.jpg is just loaded fine (and it shouldn't be loaded at this point, because we have logged out). This happens meanwhile some idle connection of pic1.jpg, pic2.jpg or pic3.jpg is still active (when those idle connections timeout, then everything works as expected and you can't access any other pass-protected file). And yes, I've tested this using v2.4 Alpha 5, 6 & 7, and the issue is still present. If 'somehow' we can 'accelerate' the closing of those idle connections, then this would be just perfect.


Cheers,
Leo.-