Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-6111

Strange Behaviour of the eCommerce Login Link

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: Release Branch 12.04, Release Branch 13.07, Trunk
    • Fix Version/s: 16.11.01
    • Component/s: ecommerce
    • Labels:
      None
    • Sprint:
      Community Day 2 - 2015

      Description

      I've noticed some strange behaviour with the Login link in the eCommerce application. If you're visit the Login link from "main", you're redirected back to the Login view even after logging in:

      1) Visit http://demo-stable-ofbiz.apache.org/ecommerce/control/main
      2) Click "Login" in the upper left
      3) Login as "DemoCustomer" with a password of "ofbiz"
      4) Notice that you're at a new URL, logged in, but the login form is redrawn.

      Compare this with how it's supposed to work:

      1) Logout
      2) Visit http://demo-stable-ofbiz.apache.org/ecommerce/tiny-gismo-GZ-1000-p
      3) Click "Login" in the upper left
      4) Login as "DemoCustomer" with a password of "ofbiz"
      5) Notice that you're at a new URL, but the product page is redrawn correctly.

      It's just really strange behaviour, quite hard to track down, and I can't really find a root cause.

      1. OFBIZ-6111.patch
        0.8 kB
        Taher Alkhateeb
      2. OFBIZ-6111.patch
        1 kB
        Arun Patidar

        Issue Links

          Activity

          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          This has been fixed with OFBIZ-6849

          Show
          jacques.le.roux Jacques Le Roux added a comment - This has been fixed with OFBIZ-6849
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          With OFBIZ-6867 I removed the forceManualJsessionid feature but that did not fix this issue. I have a version where it's fixed but there are other changes related with OFBIZ-6849 (HTTPS only) and it's not stable yet, so this will come later...

          Show
          jacques.le.roux Jacques Le Roux added a comment - With OFBIZ-6867 I removed the forceManualJsessionid feature but that did not fix this issue. I have a version where it's fixed but there are other changes related with OFBIZ-6849 (HTTPS only) and it's not stable yet, so this will come later...
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Thanks Arun, I got sidetracked for misc. reasons but I will soon continue on this and the related email ("Performance over security, is that reasonable?") I sent to the dev ML

          Show
          jacques.le.roux Jacques Le Roux added a comment - Thanks Arun, I got sidetracked for misc. reasons but I will soon continue on this and the related email ("Performance over security, is that reasonable?") I sent to the dev ML
          Hide
          arunpati Arun Patidar added a comment -

          Hi Jacques,

          Assigning it to you for a finalizing conclusion.

          Show
          arunpati Arun Patidar added a comment - Hi Jacques, Assigning it to you for a finalizing conclusion.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I'm very glad you spotted that Scott. I also recently mentionned something about the forceManualJsessionid variable at https://issues.apache.org/jira/browse/OFBIZ-4645?focusedCommentId=15054232 and I agree it should be dropped. I'm also currently writing a somewhat related email to the dev ML, it's even more radical!

          Show
          jacques.le.roux Jacques Le Roux added a comment - I'm very glad you spotted that Scott. I also recently mentionned something about the forceManualJsessionid variable at https://issues.apache.org/jira/browse/OFBIZ-4645?focusedCommentId=15054232 and I agree it should be dropped. I'm also currently writing a somewhat related email to the dev ML, it's even more radical!
          Hide
          lektran Scott Gray added a comment -

          After a closer look this morning, it appears that the session is dropped because it was created during an HTTPS request (logout) and is then passed to a unsecure HTTP request in the URL. Tomcat uses the session for the unsecure request but (correctly) it won't send it back as an unsecure session cookie.

          IMO we shouldn't be passing a secure session ID to an unsecure request.

          In RequestHandler.makeLink(HttpServletRequest, HttpServletResponse, String, boolean, boolean, boolean) we have the following code:

                      // if this isn't a secure page, but we made a secure URL, make sure we manually add the jsessionid since the response.encodeURL won't do that
                      if (!request.isSecure() && didFullSecure) {
                          forceManualJsessionid = true;
                      }
          
                      // if this is a secure page, but we made a standard URL, make sure we manually add the jsessionid since the response.encodeURL won't do that
                      if (request.isSecure() && didFullStandard) {
                          forceManualJsessionid = true;
                      }
          

          But I would argue that in both of the above cases, the reason that response.encodeURL won't include the jsessionid is because it isn't safe to do so from a security point of view. In both cases you've got the potential for session hijacking because either a secure cookie id has been passed in plain text or an unsecure session id will be used in place of a secure one. I think we should remove the logic relating to the forceManualJsessionid variable.

          Show
          lektran Scott Gray added a comment - After a closer look this morning, it appears that the session is dropped because it was created during an HTTPS request (logout) and is then passed to a unsecure HTTP request in the URL. Tomcat uses the session for the unsecure request but (correctly) it won't send it back as an unsecure session cookie. IMO we shouldn't be passing a secure session ID to an unsecure request. In RequestHandler.makeLink(HttpServletRequest, HttpServletResponse, String, boolean, boolean, boolean) we have the following code: // if this isn't a secure page, but we made a secure URL, make sure we manually add the jsessionid since the response.encodeURL won't do that if (!request.isSecure() && didFullSecure) { forceManualJsessionid = true ; } // if this is a secure page, but we made a standard URL, make sure we manually add the jsessionid since the response.encodeURL won't do that if (request.isSecure() && didFullStandard) { forceManualJsessionid = true ; } But I would argue that in both of the above cases, the reason that response.encodeURL won't include the jsessionid is because it isn't safe to do so from a security point of view. In both cases you've got the potential for session hijacking because either a secure cookie id has been passed in plain text or an unsecure session id will be used in place of a secure one. I think we should remove the logic relating to the forceManualJsessionid variable.
          Hide
          lektran Scott Gray added a comment -

          Looks like the issue is caused by a dropped session. A new session is created when the logout occurs and the user is redirected to the main page. Because the redirect switches the user from https to http, the JSESSIONID is included in the URL of the redirect and isn't set as a cookie (I'm not sure why). I think the session id inclusion in the URL is preventing the session id from being sent as a cookie in the subsequent response of the redirect.

          So after this point, when the user submits the add to cart form, there's no session id passed in the request via URL or by cookie and hence a new session is created. The "main" view was saved as the SAVED_VIEW_NAME session attribute but that was lost when the additional session was created, so the request handler defaults back to the default view of "viewCart".

          Something like that anyway. It could possibly be a Tomcat bug given that any of the following would solve the problem:
          1. Tomcat could set the session cookie during the redirect response
          2. Tomcat could set the session cookie in the subsequent response
          Unless I'm missing something.

          Show
          lektran Scott Gray added a comment - Looks like the issue is caused by a dropped session. A new session is created when the logout occurs and the user is redirected to the main page. Because the redirect switches the user from https to http, the JSESSIONID is included in the URL of the redirect and isn't set as a cookie (I'm not sure why). I think the session id inclusion in the URL is preventing the session id from being sent as a cookie in the subsequent response of the redirect. So after this point, when the user submits the add to cart form, there's no session id passed in the request via URL or by cookie and hence a new session is created. The "main" view was saved as the SAVED_VIEW_NAME session attribute but that was lost when the additional session was created, so the request handler defaults back to the default view of "viewCart". Something like that anyway. It could possibly be a Tomcat bug given that any of the following would solve the problem: 1. Tomcat could set the session cookie during the redirect response 2. Tomcat could set the session cookie in the subsequent response Unless I'm missing something.
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          I believe the ecomseo is properly documented at OFBIZ-5312, of course a wiki page would be even better, feel free to create one to complete the documentation in a sense you feel is better

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited I believe the ecomseo is properly documented at OFBIZ-5312 , of course a wiki page would be even better, feel free to create one to complete the documentation in a sense you feel is better
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Actually because of the way I introduced ecomseo (due to Anil's reluctance, see OFBIZ-5312), ecomseo for now depends on (overrides) ecommerce. So it's a bit more complicated and that's why I suggested to make it prominent for the moment. Later we could decide on putting ecommerce to Attic. But Anil's argument about using contents must be considered.

          Show
          jacques.le.roux Jacques Le Roux added a comment - Actually because of the way I introduced ecomseo (due to Anil's reluctance, see OFBIZ-5312 ), ecomseo for now depends on (overrides) ecommerce. So it's a bit more complicated and that's why I suggested to make it prominent for the moment. Later we could decide on putting ecommerce to Attic. But Anil's argument about using contents must be considered.
          Hide
          taher Taher Alkhateeb added a comment -

          This bug was annoying me for a long time, and I know the bug is probably somewhere between controller.xml and web.xml. I started reviewing the differences between ecommerce and ecomseo and it branched out to many files to the process of elimination became a headache and I really like your suggestion Jacques

          Perhaps we can put it up to a vote in the community to do the following
          1- remove ecommerce web app to attic
          2- rename ecomseo to ecommerce
          3- fix all the files and references inside the component accordingly

          That would be actually a more enjoyable job than digging through the code to find out what's blocking the session variables IMHO

          Show
          taher Taher Alkhateeb added a comment - This bug was annoying me for a long time, and I know the bug is probably somewhere between controller.xml and web.xml. I started reviewing the differences between ecommerce and ecomseo and it branched out to many files to the process of elimination became a headache and I really like your suggestion Jacques Perhaps we can put it up to a vote in the community to do the following 1- remove ecommerce web app to attic 2- rename ecomseo to ecommerce 3- fix all the files and references inside the component accordingly That would be actually a more enjoyable job than digging through the code to find out what's blocking the session variables IMHO
          Hide
          pfm.smits Pierre Smits added a comment -

          Having multiple options (ecommerce, ecomseo, ecomclone) in one component doesn't do the adoption much good, when the variations aren't properly documented. How could the potential adopter choose the most applicable one? Expecting to have the (potential) adopter to evaluate the code and base his decision on that is not favourable.

          I agree that ecomseo is an improvement over ecommerce.

          Show
          pfm.smits Pierre Smits added a comment - Having multiple options (ecommerce, ecomseo, ecomclone) in one component doesn't do the adoption much good, when the variations aren't properly documented. How could the potential adopter choose the most applicable one? Expecting to have the (potential) adopter to evaluate the code and base his decision on that is not favourable. I agree that ecomseo is an improvement over ecommerce.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          A solution would be to deprecate the ecommerce web app and use the ecomseo as 1st reference (reverse priority betweent them, we could rename ecomold), but I'm not sure everybody would agree with that...

          Show
          jacques.le.roux Jacques Le Roux added a comment - A solution would be to deprecate the ecommerce web app and use the ecomseo as 1st reference (reverse priority betweent them, we could rename ecomold), but I'm not sure everybody would agree with that...
          Hide
          taher Taher Alkhateeb added a comment - - edited

          Okay, here is what I did:

          1. ./ant clean-all load-demo
          2. ./ant start
          3. go to http://localhost:8080/ecomseo
          4. login as DemoCustomer with password ofbiz
          5. logout
          6. add an item to cart from main page
          7. I'm taken back to the main page, no problem

          Now, again I made the same exact test but instead of http://localhost:8080/ecomseo I went to http://localhost:8080/ecommerce. Now, I confirm the bug happens, but it happens only once, and only in this exact sequence

          The workflow is made painfully complex so I need to try and dig through all the steps. I think the whole workflow is really not well designed. I'll do my best to reverse engineer this mess.

          Show
          taher Taher Alkhateeb added a comment - - edited Okay, here is what I did: ./ant clean-all load-demo ./ant start go to http://localhost:8080/ecomseo login as DemoCustomer with password ofbiz logout add an item to cart from main page I'm taken back to the main page, no problem Now, again I made the same exact test but instead of http://localhost:8080/ecomseo I went to http://localhost:8080/ecommerce . Now, I confirm the bug happens, but it happens only once, and only in this exact sequence The workflow is made painfully complex so I need to try and dig through all the steps. I think the whole workflow is really not well designed. I'll do my best to reverse engineer this mess.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          OK, what's the status here?

          Show
          jacques.le.roux Jacques Le Roux added a comment - OK, what's the status here?
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          Hi Taher,

          Thanks, 1st you do agree with me, right?

          The requests pass by an ASF proxy for security reasons. You may note that when you access the backend it's in HTTP mode. Actually when you get to http://demo-trunk-ofbiz.apache.org/catalog/control/main you are using https://ofbiz-vm.apache.org:8443/catalog/control/main which gets redirected by the proxy

          So we use patches for that there, see tools\demo-backup for all details. Note also that the R13.07 patch is a bit simpler because it uses the -Dportoffset feature.

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited Hi Taher, Thanks, 1st you do agree with me, right? The requests pass by an ASF proxy for security reasons. You may note that when you access the backend it's in HTTP mode. Actually when you get to http://demo-trunk-ofbiz.apache.org/catalog/control/main you are using https://ofbiz-vm.apache.org:8443/catalog/control/main which gets redirected by the proxy So we use patches for that there, see tools\demo-backup for all details. Note also that the R13.07 patch is a bit simpler because it uses the -Dportoffset feature.
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacques, you said above that "the demos are not working the same way than an OOTB version". Can you specify what the exact difference is? This could help us in zooming in on the issue. What's different in the CI deployment configuration?

          Show
          taher Taher Alkhateeb added a comment - Hi Jacques, you said above that "the demos are not working the same way than an OOTB version". Can you specify what the exact difference is? This could help us in zooming in on the issue. What's different in the CI deployment configuration?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Arun, any chances to confirm the behaviour locally?

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Arun, any chances to confirm the behaviour locally?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          It must be a redirection somewhere. Because the demos are not working the same way than an OOTB version which is the real reference in case of ambiguities

          Show
          jacques.le.roux Jacques Le Roux added a comment - It must be a redirection somewhere. Because the demos are not working the same way than an OOTB version which is the real reference in case of ambiguities
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I can't reproduce either on trunk demo, but it still appears on a clean up-to-date local copy after an "ant clean-data load-demo start". Could you please double-check locally with same steps than above:

          1. Login
          2. Logout
          3. Add a product from main page -> you get to the cart page though the check box "Always View Cart After Adding An Item" is not ticked. You should stay on main page.

          I agree it's weird

          Show
          jacques.le.roux Jacques Le Roux added a comment - I can't reproduce either on trunk demo, but it still appears on a clean up-to-date local copy after an "ant clean-data load-demo start". Could you please double-check locally with same steps than above: Login Logout Add a product from main page -> you get to the cart page though the check box "Always View Cart After Adding An Item" is not ticked. You should stay on main page. I agree it's weird
          Hide
          arunpati Arun Patidar added a comment - - edited

          Hi Jacques Le Roux

          I followed all your steps on stable-demo instance and was unable to regenerate this issue. Please share more details to regenerate it on the instance.

          Show
          arunpati Arun Patidar added a comment - - edited Hi Jacques Le Roux I followed all your steps on stable-demo instance and was unable to regenerate this issue. Please share more details to regenerate it on the instance.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Bump...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Bump...
          Hide
          mbrohl Michael Brohl added a comment -

          Arun, please have a look at Jacques' comment and check the solution.

          Thanks, Michael

          Show
          mbrohl Michael Brohl added a comment - Arun, please have a look at Jacques' comment and check the solution. Thanks, Michael
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Yes please Michael, I even wonder if it should not be reverted and another reliable solution used...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Yes please Michael, I even wonder if it should not be reverted and another reliable solution used...
          Hide
          mbrohl Michael Brohl added a comment -

          Just stumbled over this issue while preparing the OFBiz news. Shouldn't this be reopened?

          Show
          mbrohl Michael Brohl added a comment - Just stumbled over this issue while preparing the OFBiz news. Shouldn't this be reopened?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Sorry to say but the behaviour I mentionned it still there :/ This should be reverted

          Show
          jacques.le.roux Jacques Le Roux added a comment - Sorry to say but the behaviour I mentionned it still there :/ This should be reverted
          Hide
          arunpati Arun Patidar added a comment -

          Committed patch on trunk at rev: 1686574
          on 14.12 at rev: 1686575
          and on 13.07 at rev: 1686577

          Thanks Rehan Khan for your contribution.

          Show
          arunpati Arun Patidar added a comment - Committed patch on trunk at rev: 1686574 on 14.12 at rev: 1686575 and on 13.07 at rev: 1686577 Thanks Rehan Khan for your contribution.
          Hide
          arunpati Arun Patidar added a comment -

          Attached modified patch to provide support for rendering on products and categories page or main page after login.

          Show
          arunpati Arun Patidar added a comment - Attached modified patch to provide support for rendering on products and categories page or main page after login.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Mmm, while working on this I stumbled upon another issue which seems to have been introduced since my last comment. Try the scenario I explained you should find the same. Too late now to get further... This one is not related to your patch, you can reproduce it easily on trunk demo, even on stable and old, which seems to mean that it was introduced recently and backported...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Mmm, while working on this I stumbled upon another issue which seems to have been introduced since my last comment. Try the scenario I explained you should find the same. Too late now to get further... This one is not related to your patch, you can reproduce it easily on trunk demo, even on stable and old, which seems to mean that it was introduced recently and backported...
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacques,

          To save time, would you kindly specify if the test requires a full rebuild or do you just visit the website before / after the patch. In other words, do you do an ant clean-all load-demo and test first scenario followed by ant clean-all load-demo and then patch and test the second scenario?

          The reason I ask this is because I want to go through the process of elimination and not get on a wild goose chase. I am suspecting that the culprit here is the JSESSIONID cookie so your confirmation on the above would save me some time in hunting this bug.

          Show
          taher Taher Alkhateeb added a comment - Hi Jacques, To save time, would you kindly specify if the test requires a full rebuild or do you just visit the website before / after the patch. In other words, do you do an ant clean-all load-demo and test first scenario followed by ant clean-all load-demo and then patch and test the second scenario? The reason I ask this is because I want to go through the process of elimination and not get on a wild goose chase. I am suspecting that the culprit here is the JSESSIONID cookie so your confirmation on the above would save me some time in hunting this bug.
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          I double checked in Chrome because I used FF where I have some plugins which can change the behaviour. I get the same difference.

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited I double checked in Chrome because I used FF where I have some plugins which can change the behaviour. I get the same difference.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I don't agree, w/o your patch applied I don't reproduce this behaviour, just tested on trunk demo

          Show
          jacques.le.roux Jacques Le Roux added a comment - I don't agree, w/o your patch applied I don't reproduce this behaviour, just tested on trunk demo
          Hide
          taher Taher Alkhateeb added a comment -

          Okay, I confirm the behavior but it has nothing to do with my patch. If you apply or do not apply the patch in both cases the behavior repeats. I believe this has to do with the session in the url after logout which has the format of something like jsessionid=5D7EA8D44199B81C3C4B2275F6FB08CE.jvm1.

          I recommend we apply the patch for now and figure out that other behavior later or in a different JIRA

          Show
          taher Taher Alkhateeb added a comment - Okay, I confirm the behavior but it has nothing to do with my patch. If you apply or do not apply the patch in both cases the behavior repeats. I believe this has to do with the session in the url after logout which has the format of something like jsessionid=5D7EA8D44199B81C3C4B2275F6FB08CE.jvm1. I recommend we apply the patch for now and figure out that other behavior later or in a different JIRA
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          0) Apply your patch on trunk, build, start
          1) Login
          2) Logout
          3) Add a product from main page -> you get to the cart page though the check box "Always View Cart After Adding An Item" is not ticked. You should stay on main page.

          Show
          jacques.le.roux Jacques Le Roux added a comment - 0) Apply your patch on trunk, build, start 1) Login 2) Logout 3) Add a product from main page -> you get to the cart page though the check box "Always View Cart After Adding An Item" is not ticked. You should stay on main page.
          Hide
          taher Taher Alkhateeb added a comment -

          Hi Jacques,

          Sorry but I do not understand exactly the place where you caught the issue. Can you please specify the exact steps to repeat the problem on my PC?

          Show
          taher Taher Alkhateeb added a comment - Hi Jacques, Sorry but I do not understand exactly the place where you caught the issue. Can you please specify the exact steps to repeat the problem on my PC?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Taher,

          I was ready to commit your patch (R13.07 and R12.04 are also concerned) but I just noticed that now after login (only?) when you logout and add a 1st product in cart you get to the cart page though the check box "Always View Cart After Adding An Item" is not ticked. I still prefer that but it would be good if we could keep the previous "add item"' behaviour...

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Taher, I was ready to commit your patch (R13.07 and R12.04 are also concerned) but I just noticed that now after login (only?) when you logout and add a 1st product in cart you get to the cart page though the check box "Always View Cart After Adding An Item" is not ticked. I still prefer that but it would be good if we could keep the previous "add item"' behaviour...
          Hide
          taher Taher Alkhateeb added a comment -

          I followed the logic of this bug and realized the error is in the controller with a trivial fix. Attached is the patch to resolve this issue

          Show
          taher Taher Alkhateeb added a comment - I followed the logic of this bug and realized the error is in the controller with a trivial fix. Attached is the patch to resolve this issue

            People

            • Assignee:
              jacques.le.roux Jacques Le Roux
              Reporter:
              fbr@14x.net Forrest Rae
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile