Details

    • Type: Sub-task Sub-task
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: security-0.4.3
    • Fix Version/s: security-0.4.4
    • Component/s: security
    • Labels:
      None
    • Patch Submitted:
      Yes
    • Number of attachments :
      0

      Description

      The patch erroneously calls

          • WebUtils.issueRedirect(request, response, requestUri, null, false, true);
            where it should be
          • WebUtils.issueRedirect(request, response, requestUri, null, true, true);

      This makes fallbackURL go from web site root, not context root

        Activity

        Hide
        Lenny Primak added a comment -

        This also breaks Redirect-To-Saved request not just fallbackURL

        Show
        Lenny Primak added a comment - This also breaks Redirect-To-Saved request not just fallbackURL
        Hide
        Lenny Primak added a comment -

        As I read more on this thread, I believe here is the confusion:

        The default URLs are /index, /unauthorized and /security/login
        None of these have context attached.

        These should remain consistent, and not have the context in them at all.
        Tynamo should prepend the context (via shiro or on its own) and not confuse
        the issue of supporting different context for fallback URL.

        Other issue is the PageServiceImpl in general.
        All methods there should be protected in the least, thus allowing for overrides.

        You cannot do a custom login screen without overriding this class,
        at least not without copying and pasting all of its code, which is a big no-no.

        Show
        Lenny Primak added a comment - As I read more on this thread, I believe here is the confusion: The default URLs are /index, /unauthorized and /security/login None of these have context attached. These should remain consistent, and not have the context in them at all. Tynamo should prepend the context (via shiro or on its own) and not confuse the issue of supporting different context for fallback URL. Other issue is the PageServiceImpl in general. All methods there should be protected in the least, thus allowing for overrides. You cannot do a custom login screen without overriding this class, at least not without copying and pasting all of its code, which is a big no-no.
        Hide
        Vladimir Velikiy added a comment - - edited

        In all calls to PageServiceImpl.redirectToSavedRequest(String fallbackUrl) inside tapestry-security, it receives a full url (with context path). I do not understand what this patch breaks.

        Show
        Vladimir Velikiy added a comment - - edited In all calls to PageServiceImpl.redirectToSavedRequest(String fallbackUrl) inside tapestry-security, it receives a full url (with context path). I do not understand what this patch breaks.
        Hide
        Lenny Primak added a comment -

        Vladimir, you are right. It was me who was confused about
        the fallbackURL. I always thought the intent was to have everything under
        one context and it was confusing that fallbackURL was different than loginURL and unauthorizedURL.

        Show
        Lenny Primak added a comment - Vladimir, you are right. It was me who was confused about the fallbackURL. I always thought the intent was to have everything under one context and it was confusing that fallbackURL was different than loginURL and unauthorizedURL.
        Hide
        Lenny Primak added a comment -

        I found the real culprit why my code wasn't working.
        The Cookie that was written out does not include the context,
        so when the redirect was issued, it was going against the web site root

            • Patch Attached ***

        Index: src/main/java/org/tynamo/security/internal/SecurityExceptionHandlerAssistant.java
        ===================================================================
        — src/main/java/org/tynamo/security/internal/SecurityExceptionHandlerAssistant.java (revision 2246)
        +++ src/main/java/org/tynamo/security/internal/SecurityExceptionHandlerAssistant.java (working copy)
        @@ -50,9 +50,8 @@
        // Session session = subject.getSession();
        // if (session != null) WebUtils.saveRequest(requestGlobals.getHTTPServletRequest());
        // }

        • String contextPath = servletRequest.getContextPath();
        • if ("".equals(contextPath)) contextPath = "/";
        • cookies.writeCookieValue(WebUtils.SAVED_REQUEST_KEY, WebUtils.getPathWithinApplication(servletRequest), contextPath);
          + pageService.saveRequest();
          +
          return pageService.getLoginPage();
          }
          }
        Show
        Lenny Primak added a comment - I found the real culprit why my code wasn't working. The Cookie that was written out does not include the context, so when the redirect was issued, it was going against the web site root Patch Attached *** Index: src/main/java/org/tynamo/security/internal/SecurityExceptionHandlerAssistant.java =================================================================== — src/main/java/org/tynamo/security/internal/SecurityExceptionHandlerAssistant.java (revision 2246) +++ src/main/java/org/tynamo/security/internal/SecurityExceptionHandlerAssistant.java (working copy) @@ -50,9 +50,8 @@ // Session session = subject.getSession(); // if (session != null) WebUtils.saveRequest(requestGlobals.getHTTPServletRequest()); // } String contextPath = servletRequest.getContextPath(); if ("".equals(contextPath)) contextPath = "/"; cookies.writeCookieValue(WebUtils.SAVED_REQUEST_KEY, WebUtils.getPathWithinApplication(servletRequest), contextPath); + pageService.saveRequest(); + return pageService.getLoginPage(); } }
        Hide
        Kalle Korhonen added a comment -

        0.4.4 is staged at https://nexus.codehaus.org/content/repositories/orgtynamo-027/. I removed the context from the saved request and redirecting to the saved request relative to the context, then added a few tests. Let me know if this doesn't work for you.

        Show
        Kalle Korhonen added a comment - 0.4.4 is staged at https://nexus.codehaus.org/content/repositories/orgtynamo-027/ . I removed the context from the saved request and redirecting to the saved request relative to the context, then added a few tests. Let me know if this doesn't work for you.
        Hide
        Vladimir Velikiy added a comment -

        Works fine in my project

        Show
        Vladimir Velikiy added a comment - Works fine in my project
        Hide
        Lenny Primak added a comment -

        This works for me now as well.
        Thank you!

        Show
        Lenny Primak added a comment - This works for me now as well. Thank you!
        Hide
        Alejandro Scandroli added a comment -

        bulk closing issues that have been resolved for more than a year.

        Show
        Alejandro Scandroli added a comment - bulk closing issues that have been resolved for more than a year.

          People

          • Assignee:
            Unassigned
            Reporter:
            Lenny Primak
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: