[2.2] [Security] Add an option to disable the hasPreviousSession() check in AbstractAuthenticationListener #4776

Merged
merged 1 commit into from Mar 23, 2013

Projects

None yet
@adrienbrault
Contributor

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: Build Status
Fixes the following tickets: #3703
Todo: Add this option to the symfony doc security configuration reference
License of the code: MIT
Documentation PR: N/A

As stated in #3703, all authentication listeners that inherit from AbstractAuthenticationListener, only work when a previous session has been created.
This PR allows to change the default behavior in the security.yml file.

Example:

security:
    firewalls:
        secured_area:
            pattern:    ^/demo/secured/
            form_login:
                check_path: /demo/secured/login_check
                login_path: /demo/secured/login
                require_previous_session: false # The default value is true
            logout:
                path:   /demo/secured/logout
                target: /demo/
            #anonymous: ~
            #http_basic:
            #    realm: "Secured Demo Area"

PS: While removing my old commit, it closed the #4774 PR ...

@fabpot
Member
fabpot commented Jul 9, 2012

I would just remove the need for a session on the login page instead of creating yet another setting.

@adrienbrault
Contributor

Here the aim is to cover specific cases where developers wants to disable the previous session check.
I think the default behavior of throwing an exception when the client didn't send any session cookie is good in most cases, because it will not fail silently.

@fabpot
Member
fabpot commented Jul 11, 2012

Why is it good? When the form is submitted, the session will be started anyway.

@asm89
Contributor
asm89 commented Jul 11, 2012

@fabpot You should checkout the related issue #3703. If you want to use form login without CSRF to post from non-symfony pages then users might not already have a symfony session.

@adrienbrault
Contributor

@fabpot Yes, but if users have issues with cookies (they send a different or no PHPSESSID), they won't have a "previous php" session. So when they submit the form to check_path, the authentication will fail because they didn't send the session cookie the server gave them on login_path ...
However when the check is removed, the authentication won't fail if no previous session was detected (as it's not the default, it's the developer's responsibility to change this).
Some cases require need this check to be disabled to work, as @asm89 said.

@mathielen

+1

@fredjiles

+1

@makasim
Contributor
makasim commented Oct 8, 2012

-1

@stof
Member
stof commented Oct 13, 2012

@schmittjoh @fabpot what is the status of this PR ?

@lmcnearney
Contributor

+1

@cigoe
cigoe commented Feb 12, 2013

+1

@alanbem
alanbem commented Feb 15, 2013

+1

@WishCow
WishCow commented Mar 1, 2013

+1

@fabpot fabpot added a commit that referenced this pull request Mar 23, 2013
@fabpot fabpot merged branch adrienbrault/security-feature (PR #4776)
This PR was merged into the master branch.

Discussion
----------

[2.2] [Security] Add an option to disable the hasPreviousSession() check in AbstractAuthenticationListener

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: [![Build Status](https://secure.travis-ci.org/adrienbrault/symfony.png?branch=security-feature)](http://travis-ci.org/adrienbrault/symfony)
Fixes the following tickets: #3703
Todo: Add this option to the symfony doc security configuration reference
License of the code: MIT
Documentation PR: N/A

As stated in #3703, all authentication listeners that inherit from AbstractAuthenticationListener, only work when a previous session has been created.
This PR allows to change the default behavior in the security.yml file.

Example:

```yml
security:
    firewalls:
        secured_area:
            pattern:    ^/demo/secured/
            form_login:
                check_path: /demo/secured/login_check
                login_path: /demo/secured/login
                require_previous_session: false # The default value is true
            logout:
                path:   /demo/secured/logout
                target: /demo/
            #anonymous: ~
            #http_basic:
            #    realm: "Secured Demo Area"
```

PS: While removing my old commit, it closed the #4774 PR ...

Commits
-------

0562463 [Security] Add an option to disable the hasPreviousSession() check in AbstractAuthenticationListener
aa26e66
@fabpot fabpot merged commit 0562463 into symfony:master Mar 23, 2013
@WishCow WishCow referenced this pull request in FriendsOfSymfony/FOSFacebookBundle Apr 3, 2013
Open

"your session has timed out or you have disabled cookies" from iPhone #247

@bendavies
Contributor

@fabpot this is marked as a 2.2. milestone, but i guess it won't be merged into 2.2 because it is a new feature?

@bendavies
Contributor

note that the docs have not been updated for this PR, which I thought was a requirement for all new features now...

@fredjiles fredjiles referenced this pull request in symfony/symfony-docs Apr 12, 2013
Closed

Add Previous session option to security configuration #2497

@fredjiles

I added a PR to the docs for the new configuration setting.
symfony/symfony-docs#2497

@dlin-me
dlin-me commented May 1, 2013

+1, I want to log the user in with just a token in the URL, without being able to disable the previous session check, I have to refresh the page once at least

@dlin-me
dlin-me commented May 1, 2013

By the way, I've found a work around:
If you have can access the Request object (i.e. kenel.request event), the following two lines will fool the hasPreviousSession check.

$this->container->get('session')->set('foo', ''); //create a fake session
$request->cookies->set($this->container->get('session')->getName(), ''); //create a fake cookie

@weaverryan
Member

Hi Fred!

Thanks so much for this! For the rare cases where a code PR gets merged without a doc PR, it's very important that we get that taken care of! So thanks a lot for picking this up!

And it's also nice to see a fellow Michigander on here! I'm from Saline originally... and I'm afraid to say that I'm a UM fan. I hope we can still be friends ;).

Thanks!

@fredjiles

No problem. Glad I could help out. It was a good first contribution
(small and straight forward). Hopefully I can find some more ways to
contribute.

I won't hold the UM thing against you. There are a couple good people in
the world that are UM fans :)

On Fri, May 3, 2013 at 2:26 PM, Ryan Weaver notifications@github.comwrote:

Hi Fred!

Thanks so much for this! For the rare cases where a code PR gets merged
without a doc PR, it's very important that we get that taken care of! So
thanks a lot for picking this up!

And it's also nice to see a fellow Michigander on here! I'm from Saline
originally... and I'm afraid to say that I'm a UM fan. I hope we can still
be friends ;).

Thanks!


Reply to this email directly or view it on GitHubhttps://github.com/symfony/symfony/pull/4776#issuecomment-17410273
.

@fabpot fabpot added a commit to fabpot/symfony that referenced this pull request May 5, 2013
@fabpot fabpot [Security] fixed wrong merge (refs #4776) 1856df3
@fabpot fabpot added a commit that referenced this pull request May 5, 2013
@fabpot fabpot merged branch fabpot/require-previous-session (PR #7938)
This PR was merged into the master branch.

Discussion
----------

[Security] fixed wrong merge (refs #4776)

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

This fixes a bad conflict resolution when #4776 was merged.

Commits
-------

1856df3 [Security] fixed wrong merge (refs #4776)
674b30f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment