Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configurable success URL for security.loginForm on per component render basis #2

Closed
balapal opened this issue Apr 4, 2014 · 6 comments

Comments

@balapal
Copy link
Contributor

balapal commented Apr 4, 2014

It is possible to configure a default success URL via SecuritySymbols.SUCCESS_URL

I would like add a new optional parameter to security.loginForm to being able to redirect to different page than the one configured via SecuritySymbols.SUCCESS_URL.

The rationale behind that I would like to redirect to different pages depending where the user has come from.

How do you feel guys feel about this? Would you merge this feature to tapestry-security if I fork it?

@kaosko
Copy link
Member

kaosko commented Apr 4, 2014

Yes, federated accounts sign-in components already support returnPageName @parameter (return page is Oauth specific term). While the LoginForm was meant mainly as an example rather than reusable component, we can add support for the more common use cases. Take a look at https://github.com/tynamo/tynamo-federatedaccounts/blob/master/tynamo-federatedaccounts-core/src/main/java/org/tynamo/security/federatedaccounts/base/AbstractOauthSignIn.java and go ahead and implement getSuccessPageUri() similar to getReturnPageUri() (with caret support!) and I'll merge that in.

@balapal
Copy link
Contributor Author

balapal commented Apr 4, 2014

Thanks, will do.

@balapal
Copy link
Contributor Author

balapal commented Apr 5, 2014

I have started to implement this feature based on AbstractOauthSignIn,
If I understand correctly the caret support comes handy if you embed the login form in a page where you would like to return after successful login. For this scenario the context of the page is considered when constructing return URL.

For other pages than the current one it is not possible to pass context or parameters.
For my use case I would need context support. I have log in button on a page what takes the user to the log in page and up on successful log in the user should be redirected to the page where it came from.
To solve this I introduced a @Parameter String successURL to security.loginForm (default value is SecuritySymbols.SUCCESS_URL).
My getSuccessPageUri() simply returns it, so it's the responsibility of the client code to build the custom success URL.

    @Parameter(defaultPrefix = BindingConstants.LITERAL, value = "prop:defaultSuccessURL")
    private String successURL;

    @Inject
    @Symbol(SecuritySymbols.SUCCESS_URL)
    @Property(write = false)
    private String defaultSuccessURL;

    private String getSuccessPageUri()
    {
        if ("^".equals(successURL))
        {
            return pageRenderLinkSource.createPageRenderLink(componentResources.getPage().getClass()).toAbsoluteURI();
        }

        if (StringUtils.hasText(getSuccessURL()))
        {
            return getSuccessURL();
        }
        else
        {
            // Empty returnpageName denotes a base url. Default for default return page may be set to a non empty value.
            // If default is empty and not locally overridden, return the baseurl
            return baseURLSource.getBaseURL(request.isSecure());
        }
    }

    public String getSuccessURL()
    {
        return StringUtils.hasText(successURL) ? successURL : defaultSuccessURL;
    }

I was trying to research how would this should be done in the tapestry way, but this is I could come up.
I can confirm that this works for my use case.

@kaosko is this the right direction to support link context and parameters?

@balapal
Copy link
Contributor Author

balapal commented Apr 5, 2014

The client code looks like this:

Login.tml:

<t:security.loginForm successURL="prop:successURL">
 </t:security.loginForm>

Login.java

    @ActivationRequestParameter("successURL")
    @Property
    private String successURL;

Page that is navigating to Login page with custom successURL.
.tml

    <a t:type="pagelink" t:page="user/Login" t:parameters="loginQueryParams" class="navbar-button">
        <button class="btn btn-primary">${message:login}</button>
    </a>

.java

  @Inject
    private Request request;

    public Map<String, Object> getLoginQueryParams()
    {
        final Map<String, Object> queryParams = new HashMap<>();
        queryParams.put("successURL", this.request.getPath()); //getPath() returns URL containing the page context
        return queryParams;
    }

@kaosko
Copy link
Member

kaosko commented Apr 7, 2014

Shiro's savedRequest cookies is meant exactly for that use case. support for savedRequest is meant to address the use case but comment out because the functionality was pushed down to LoginContextService Impl. You use a query parameter instead - wouldn't it be easier to just override LoginContextService for your purposes? (Just asking, it's quite possible I'm missing something from the picture).

balapal added a commit to balapal/tapestry-security that referenced this issue Apr 27, 2014
kaosko added a commit that referenced this issue Sep 19, 2014
[#2] Configurable success URL for LoginContextService and security.login...
kaosko added a commit that referenced this issue Sep 23, 2014
component render basis 
- remove LoginContextService.getSuccessPage(String successURL). It's
semantically incorrect and unnecessary since the loginContextService
doesn't need to be invoked if you return a user configured success url
- revert changes to Login.java/.tml in
9bd7afe. Passing the success url as a
context parameter makes ugly urls and the author of the pull request
didn't explain why context parameter is in any way preferable to
cookies. Typically, context represents the state and the already
suggested use for the login page state is to convey the state of the
user (unauthenticated, unauthorized, etc.). It's trivial to create a
custom Login page and I expect most users to do so than try to use the
one provided by default
- keep LoginForm's successUrl component parameter and rework the test so
it demonstrates the use case. It's relatively cheap to support and may
have its use cases
@kaosko
Copy link
Member

kaosko commented Sep 23, 2014

Closing. @balapal, see comments in my commit to see what was kept and what changed and why.

@kaosko kaosko closed this as completed Sep 23, 2014
jochenberger pushed a commit to jochenberger/tapestry-security that referenced this issue Jul 30, 2015
jochenberger pushed a commit to jochenberger/tapestry-security that referenced this issue Jul 30, 2015
component render basis 
- remove LoginContextService.getSuccessPage(String successURL). It's
semantically incorrect and unnecessary since the loginContextService
doesn't need to be invoked if you return a user configured success url
- revert changes to Login.java/.tml in
9bd7afe. Passing the success url as a
context parameter makes ugly urls and the author of the pull request
didn't explain why context parameter is in any way preferable to
cookies. Typically, context represents the state and the already
suggested use for the login page state is to convey the state of the
user (unauthenticated, unauthorized, etc.). It's trivial to create a
custom Login page and I expect most users to do so than try to use the
one provided by default
- keep LoginForm's successUrl component parameter and rework the test so
it demonstrates the use case. It's relatively cheap to support and may
have its use cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants