search

apinstein / phocoa

PHOCOA php framework
http://phocoa.com
15 stars 3 forks source link

Support temporary login sessions #58

Closed parisholley closed 8 years ago

parisholley commented 8 years ago

Allow web application to change the authorization per request instead of modifying session for active user

This change is Reviewable

apinstein commented 8 years ago

Reviewed 1 of 1 files at r1. Review status: all files reviewed at latest revision, 1 unresolved discussion.

phocoa/framework/WFAuthorization.php, line 546 at r1 (raw file):

Previously, parisholley (Paris Holley) wrote… > in my implementation, i just fetch the user out of the db based on the request header and set this for the request only. by adding "temporary" it prevents writing into the session and influencing other requests however everything that is using the shared manager static method can see this "temporary" user until the request is over. >

Maybe it'd make more sense if I saw the other side of the code, but feels pretty hacky. Are you calling I am having trouble reasoning about what might break b/c of where you patched in.

This func loginAsAuthorizationInfo that you patched, it is interestingly only used by a few tests. Not actual code. It's only use in non-test code seems to be when it's used by the WFAuthorizationManager class internally.

After looking at it more now I realized I didn't notice before that you allow the "instance" var to be set but avoid persisting stuff to the session. Presumably you're just calling $mgr->loginAsAuthorizationInfo(.., .., true) with the data from the magic header to implement this?

I think I see how you got it to work.

Here's my areas of concern: 

     * Change the current session authorization info to the specified WFAuthorizationInfo object.
     * This is useful for SSO or other internal user-switching capabilities.

That func also ensures that the session data matches the authInfo data. That's the risky part for me. I am not sure the subtle ways it could break.

I feel like a safer solution might be to alter the auth mgr class to have a delegate for authorizationInfo() and just implement your "temp hack" in userland?

So the only change to phocoa is to WFAuthorization::authorizationInfo(), to leverage a delegate func of same name (add it to WFAuthorizationDelegate), 

    function authorizationInfo()
    {
        // give delegate a chance to munge current auth-info
        if (method_exists($this->authorizationDelegate, 'loginInvocationPath'))
        {
            return $this->authorizationDelegate->authorizationInfo($this->authorizationInfo);
        }
        else
        {
            return $this->authorizationInfo;
        }
    }```

and then in userland you'd just munge it there. Then, there are no weird race conditions with how the temp authinfo gets set, or places where the session / instance are out-of-sync, etc.

Maybe a little pedantic, but I think it's easier to reason about and seems less risky from security perspective. Seems less side-effecty than the current implementation, and also has the effect of consolidating the "check header" logic and "munge auth info" logic into the same spot.

Thoughts?

---

*Comments from [Reviewable](https://reviewable.io:443/reviews/apinstein/phocoa/58)*
<!-- Sent from Reviewable.io -->
parisholley commented 8 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.

phocoa/framework/WFAuthorization.php, line 546 at r1 (raw file):

Previously, apinstein (Alan Pinstein) wrote… > Maybe it'd make more sense if I saw the other side of the code, but feels pretty hacky. Are you calling I am having trouble reasoning about what might break b/c of where you patched in. > > This func `loginAsAuthorizationInfo` that you patched, it is interestingly only used by a few tests. Not actual code. It's only use in non-test code seems to be when it's used by the `WFAuthorizationManager` class internally. > > After looking at it more now I realized I didn't notice before that you allow the "instance" var to be set but avoid persisting stuff to the session. Presumably you're just calling `$mgr->loginAsAuthorizationInfo(.., .., true)` with the data from the magic header to implement this? > > I think I see how you got it to work. > > Here's my areas of concern: > - there are places where the auth mgr references the session directly, such as in `isLoggedIn()` and `recentLoginTime`, and having that data out-of-sync w/the `authInfo` seems risky. Despite the `loginAsAuthorizationInfo()` docs that indicate this is a great place to do this, > > ``` > * Change the current session authorization info to the specified WFAuthorizationInfo object. > * This is useful for SSO or other internal user-switching capabilities. > ``` > > That func also ensures that the session data matches the authInfo data. That's the risky part for me. I am not sure the subtle ways it could break. > > I feel like a safer solution might be to alter the auth mgr class to have a delegate for `authorizationInfo()` and just implement your "temp hack" in userland? > > So the only change to phocoa is to `WFAuthorization::authorizationInfo()`, to leverage a delegate func of same name (add it to `WFAuthorizationDelegate`), > > `````` > function authorizationInfo() > { > // give delegate a chance to munge current auth-info > if (method_exists($this->authorizationDelegate, 'loginInvocationPath')) > { > return $this->authorizationDelegate->authorizationInfo($this->authorizationInfo); > } > else > { > return $this->authorizationInfo; > } > }``` > > and then in userland you'd just munge it there. Then, there are no weird race conditions with how the temp authinfo gets set, or places where the session / instance are out-of-sync, etc. > > Maybe a little pedantic, but I think it's easier to reason about and seems less risky from security perspective. Seems less side-effecty than the current implementation, and also has the effect of consolidating the "check header" logic and "munge auth info" logic into the same spot. > > Thoughts? > >

Correct, I went this route as the user land implementation you mentioned didn't exist (but seems more appropriate). There is already a convention of going to "user land" to delegate the login logic and many auth frameworks also have the same process for delegating how the session is serialized/unserialized to produce a "user object".

That being said, I'm not sure the delegate will solve any sync/out of sync issue. This call was being made right after manager is initialized, and you will still have the potential risk of logic somewhere depending on the session/auth object being in sync (but in theory, no one should be inspecting the session directly).

Comments from Reviewable

apinstein commented 8 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions.

phocoa/framework/WFAuthorization.php, line 538 at r1 (raw file):

     * @param object WFAuthorizationInfo The new authorization info to set for the current session.
     */
    function loginAsAuthorizationInfo($authInfo, $authorizeRecentLogin = true, $temporary = false)

if you do keep this, doc the param please.

_phocoa/framework/WFAuthorization.php, line 546 at r1 (raw file):_

Previously, parisholley (Paris Holley) wrote… > Correct, I went this route as the user land implementation you mentioned didn't exist (but seems more appropriate). There is already a convention of going to "user land" to delegate the login logic and many auth frameworks also have the same process for delegating how the session is serialized/unserialized to produce a "user object". > > That being said, I'm not sure the delegate will solve any sync/out of sync issue. This call was being made right after manager is initialized, and you will still have the potential risk of logic somewhere depending on the session/auth object being in sync (but in theory, no one should be inspecting the session directly). >

I think you're right in terms of it doesn't change either the fact that there is data out-of-sync, or what happens if somone reaches into the sessino (tho they deserve it in that case :)

I think the only argument to move it to delegate is just it's slightly more readable/grokkable/consistent, which I think you agree with.

I am comfortable with whichever way you choose. Thanks for the discussion.

Comments from Reviewable

parisholley commented 8 years ago

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.

_phocoa/framework/WFAuthorization.php, line 538 at r1 (raw file):_

Previously, apinstein (Alan Pinstein) wrote… > if you do keep this, doc the param please. >

implemented delegate approach

Comments from Reviewable

apinstein commented 8 years ago
:lgtm:

Reviewed 1 of 1 files at r2. Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable