TwinePlatform / twine-api

⚠️ DEPRECATED - See https://github.com/TwinePlatform/twine-monolith
GNU Affero General Public License v3.0
3 stars 0 forks source link

Session model and temporary access restriction #110

Closed eliasmalik closed 5 years ago

eliasmalik commented 6 years ago

One of the core mechanisms in the visitor app is the ability of the CB admin to "log in" into a restricted access mode such that the authenticated device can be used as a "public" in order to allow visitors to register visits without allowing them to access privileged information about the CB or other visitors.

In the current version of the visitor app, this mechanism is implemented using an additional "admin-mode" token, which has a short expiry, and establishes that the bearer has full access.

In the API design, it looks like this is going to be implemented using an additional user role ORG_NON_ADMIN. I don't think this is going to work, because CB admins must be able to operate in both modes, and this would therefore break our current constraint of one role per user.

Ultimately I think it's necessary to realise that it's the session that's restricted, not the user. The user has the same role regardless, and should be able to have full access rights in any other context (i.e. if a different twine app is open in another tab).

Therefore the appropriate place to deal with privilege restriction is in the session mechanism (cookies).

Requirements:

IMO, ideally, this means a multiple cookie session model.

For example:

Example auth flow for logging in as CB admin to visitor app:

global session cookie set (or checked) visitor app session-type cookie set to "restricted" confirm password: session-type cookie reset to "full", with short expiry if a request is made with an expired or absent session-type cookie, "restricted" access is assumed (and appropriate cookie is set).

At the moment, this "restricted"/"full" session-type dynamic is relevant only for CB admins using the visitor app. All other user roles will have session-type cookies set to "full" by default, and their scopes will not be set to "restricted" regardless of the value of the session-type cookie.

Scopes are calculated as normal, but then scopes not allowed in "restricted" mode are filtered. Not yet sure how the scopes that are not allowed in "restricted" mode will be stored.

astroash commented 6 years ago

I agree with the multi cookie model.

Scopes are calculated as normal, but then scopes not allowed in "restricted" mode are filtered. Not yet sure how the scopes that are not allowed in "restricted" mode will be stored.

I think it is better to do this with separate schemes for restricted and full routes.

astroash commented 6 years ago

Requirements:

  • Must be restricted by default when request originates from the visitor app
  • Must be un-restricted if privilege is escalated via password confirmation on the visitor app
  • Must be un-restricted when request originates from other apps
  • Ideally does not require re-authentication when switching between apps.

☝️ @eliascodes agree with these requirements

What's the benefit of having a per app & global cookie over a restricted and non-restricted one?


Setup I suggest:

  1. :godmode: Unrestricted admin cookie. Used and handed out on:

    • superadmin dashboard
    • admin section of visitor app comes with a 30 minute logout
  2. 🍪 standard cookie. Used and handed out on:

    • visitor app (for admins)
    • volunteer app (for volunteers and volunteer_admins) comes with 1 day login

Example auth flow

  1. Admin logs into superadmin dashboard and receives :godmode: 🍪 . In another tab they open the visitor app. This grants them access to login, to visitor apps's admin section*. After sipping their coffee and getting distracted on twitter for 37 minutes, on their next navigation, they need to login again.

*for this to work we'll need a funky strategy that checks both cookies for visitor app homepage

2a. I'm an admin setting up the visitor app for the day. I go to the visitor app homepage and login and receive a 🍪 . This allows the visitor app to work.

2b. At lunchtime I jump on the tablet to check the admin section of the visitor app and receive a :godmode: 🍪 . I mess around for 18 minutes looking at my ever growing visitor count. When I'm done I can either press logout to remove my :godmode: 🍪, or after a further 12 minutes it will crumble into the ether, leaving the original 🍪 to allow access to the rest of the app.

eliasmalik commented 6 years ago

What's the benefit of having a per app & global cookie over a restricted and non-restricted one?

IMO flexibility, mostly. The global cookie indicates that a session exists on the platform/API. The per-app one specifies the kind of session that exists on each application. So we can, for example, specify different expiry dates for different apps, and for different session types within those apps. It also allows for potentially adding more cookies in future for other functionality without necessarily messing with how we detect user sessions on the API itself (the global cookie).

For example, Github is storing the following cookies on my browser at the moment:

screen shot 2018-08-23 at 11 06 16

Looks like there's 3 session-related cookies, one for the current session, which will be cleared as soon as the browser is closed, one longer lived one which presumably allows me to come back without logging in again and a third one which I haven't figured out yet.

for this to work we'll need a funky strategy that checks both cookies for visitor app homepage

Are you suggesting that a user visiting visitor.twine-together.com with :godmode: 🍪 will be forwarded to visitor.twine-together.com/dashboard?


What is the implementation going to look like for your suggested setup?

astroash commented 6 years ago

@eliascodes what are you suggesting the domain/app cookie auth flow will be?

for this to work we'll need a funky strategy that checks both cookies for visitor app homepage Are you suggesting that a user visiting visitor.twine-together.com with :godmode: 🍪 will be forwarded to visitor.twine-together.com/dashboard?

No. I think this will cause UX woes as it is not the expected behaviour on this page. My suggestion is:

  1. User logs into super admin dashboard and receives :godmode: 🍪
  2. User goes to visitor.twine-together.com. Home route will
    • have a custom scheme that checks for a standard 🍪. If this is not present then it will check for a :godmode: 🍪
    • user will be send to standard homepage
  3. User clicks on admin dashboard and is granted access

What is the implementation going to look like for your suggested setup?

Login route sets appropriate cookie based on origin/custom headers?

I hadn't considered setting cookies based on headers. This could work well for the two types of admin login for the visitor app

Authenticated routes' scopes and strategy remain unchanged?

scopes stay the same but strategies work as described above

Implementation of the auth strategy now needs to check for presence of both cookies, and alter the value of the scopes accordingly?

If an auth strategy fails (eg cannot verify the cookie) a 401 will be returned. I don't see a need to change the scopes. Admins will always have access to the same routes, it just depends if they have confirmed their login details in the last x minutes.

eliasmalik commented 6 years ago

scopes stay the same but strategies work as described above

Isn't this inconsistent with how we originally designed the permissions system? With what you're suggesting above, we have to identify those routes used by certain roles. This couples the API to the roles, which we were meant to avoid by associating routes with permissions, not roles.

checks for standard cookie, if this is not present checks for :godmode: cookie

This sounds like the restricted strategy contains a re-implementation of the un-restricted strategy?

astroash commented 6 years ago

@eliascodes fair point.

global session cookie set (or checked) visitor app session-type cookie set to "restricted" confirm password: session-type cookie reset to "full", with short expiry

In this suggestion:

if a request is made with an expired or absent session-type cookie, "restricted" access is assumed (and appropriate cookie is set).

What do you mean by this? Surely if a cookie is expired or absent then login is required.

eliasmalik commented 6 years ago

under what circumstances is a global session cookie set?

Only if a pre-existing valid global session cookie is not already present on the request sent to the login (and obviously only if the credentials are valid).

how is restricted access enforced on routes?

A user's role (indirectly) defines their access rights. If a user's session is "restricted", it means their access rights are reduced. This means they have lost some of the permissions they normally have. This should be reflected in the scopes array having fewer members than normal. So the user will not have access to those routes which require scopes that are not present in the scopes array.

What do you mean by this? Surely if a cookie is expired or absent then login is required.

If I'm logged into the admin dashboard, I have a global cookie (indicating I am authenticated on the platform) and a app-specific cookie for the admin dashboard. If I then wander over to the visitor app, I will still have the global cookie, so I shouldn't have to log in again, but I don't have an app-specific cookie, so the API should assume I'm in "restricted" mode and set a "restricted" visitor-app-specific cookie.

The above was just a suggestion though. The alternative is to make both cookies required, and if either are not present, force a login.

eliasmalik commented 6 years ago

For the record: we've decided to go with the scheme outlined in the OP.

eliasmalik commented 6 years ago

OK, you're gonna hate me, but I've come up with a different suggestion which I think is simpler, based on the following scenario:

We should absolutely force a login/session upgrade here.

So the suggestion now is just to stick with one cookie/token, but encode the privilege level in the payload:

{
  userId: number
  organisationId: number
  privilege: 'full' | 'restricted'
}

Expiry can be short (5 mins idle, refreshed on recent activity). I think this is broadly similar to your initial suggestion, but with a single cookie only. Logins from visitor app receive restricted tokens, logins from everywhere else receive a full token.

astroash commented 6 years ago

@eliascodes if this is sent as plain text in the payload, what is to stop someone from editing this themselves?

eliasmalik commented 6 years ago

@astroash The payload is signed using the secret. They won't be able to submit a valid token without knowing it.

eliasmalik commented 6 years ago

I think we can store the "full/restricted" permissions in the access_role_permission table, using an extra column that describes whether that row applies for "full" or "restricted" sessions. Will require a very minor modification to the RolesInterface too.

astroash commented 6 years ago

I thought you meant request payload at first 😳

What's the benefit of storing this data in the payload rather than 2 separate cookies? In both scenarios we will need to write our own custom scheme.

After looking at the the VISITORs routes mentioned in #123 I do see a benefit to having a separate visitor cookie, as they do have access to some routes over ORG_ADMIN

eg. only visitors can create individual visit data.

eliasmalik commented 6 years ago

In both scenarios we will need to write our own custom scheme.

No, storing it in the payload means we can still use the hapi-auth-jwt2 scheme.

eg. only visitors can create individual visit data.

But this is never done except via an authenticated ORG_ADMIN account?

astroash commented 6 years ago

But this is never done except via an authenticated ORG_ADMIN account?

It is, but giving them access to that scope implies that admins are allows to add user data. It makes more sense for this scope to be owned by visitors alone. What happens if later down the line we want to move visitor logins to a native app. By excluding visitors now we are restricting that user type from being able to access routes in future.

eliasmalik commented 6 years ago

By excluding visitors now we are restricting that user type from being able to access routes in future.

We can simply add permissions to the VISITOR role down the line, no?

It is, but giving them access to that scope implies that admins are allows to add user data.

Assuming you mean visit data, I'm not too worried about this right now. All it means is that admins could spoof visit data, but there's no real incentive to do so, and they're barely recording any as it is, so IMO that's one for the future.

astroash commented 6 years ago

IMO that's one for the future

I think if this is something we can accommodate now it's worth it. What's the reason for moving away from the multicookie approach? Is it because we will need to write our own auth scheme?

eliasmalik commented 6 years ago

I think if this is something we can accommodate now it's worth it.

What is "this" though? If it's visitors being able to log in in their own right, that's possible with this approach. If it's CB admin accounts not being able to add visit data, as I said above, IMO it's not a big deal, and in any case it would require changes to how the visitor app works.

What's the reason for moving away from the multicookie approach?

Once Frontline implement their OAuth provider, we'll have to revisit how authentication on the platform works anyway, I don't want to do too much now.

astroash commented 6 years ago

IMO it's not a big deal, and in any case it would require changes to how the visitor app works.

This is possible with the current setup of the visitor app and the auth flow I outlined in #123

Once Frontline implement their OAuth provider, we'll have to revisit how authentication on the platform works anyway, I don't want to do too much now.

My concern is changing the permissions roles have, and removing VISITOR as a type of user for the api. IMO making a scheme now is less work than changing the permissions for 2 types of users and how they interact with the visitor app. We'll most likely end up making our own scheme at somepoint anyway as the current one won't be fit for purpose.

eliasmalik commented 6 years ago

This is possible with the current setup of the visitor app and the auth flow I outlined in #123

I don't think there's any difference from a security perspective between the two, given that CB admins necessarily have access to all of their visitors' QR codes anyway.

My concern is changing the permissions roles have

What is your concern exactly? Being able to change permissions was one of the central reasons for designing the role/permissions system the way it is; decouple roles from permissions, so they can be changed independently.

IMO making a scheme now is less work than changing the permissions for 2 types of users and how they interact with the visitor app

Changing permissions is just changing rows in a database. Making a new scheme is writing actual new code that needs to be thought about and tested and iterated.

We'll most likely end up making our own scheme at somepoint anyway as the current one won't be fit for purpose.

Maybe, but we don't yet know in exactly what ways the current one will be inadequate.

eliasmalik commented 6 years ago

OK, so one of the concerns that was raised earlier was that we're changing scopes on routes. The main changes to api.json in #127 were to do with fixing the conflation between /resource/:id routes and /resource/me routes.

Currently the routes with VISITOR as one of the intended roles (as of abb9b9117259ec3bd21c973bc459b81e9845f28b on #125):

GET /community-businesses/:id/visit-activities: 
  ["visit_activities-child:read", "visit_activties-parent:read"]

POST /community-businesses/:id/visit-logs: 
  ["visit_logs-parent:write"]

POST /users/password/reset:
  ["user_details-own:write"]

GET /users/me:
  ["user_details-own:read"]

PUT /users/me:
  ["user_details-own:write"]

The only one that would need changing is POST /community-businesses/:id/visit-logs, i.e. adding visit_logs-own:write to visit_logs-parent:write. Since both those scopes would be present, adding a VISITOR role in future with visit_logs-parent:write would require no code changes (assuming the handler is complete).

Those scopes on that route imply that the visitor logs as a resource are "owned" by the CB, but able to be written by "child" users. This is the same relation we've imposed on the volunteer logs; owned by CB, editable by "child" users.

astroash commented 5 years ago

Reopening due to TwinePlatform/twine-visitor#552

Visitor app escalates access_mode to 'full', which remains in cookie.

Options for resolving:

  1. 'full' access mode cookies have a 30 min expiry. This would mean that cb's will need to sign back into the app after leaving the admin section. This will cause the app to appear to randomly sign out
  2. All client routes that are not in the admin section deescalate cookie on load. This is the easiest to implement, but adds extra requests.
  3. Admin login sets an additional 'full' access cookie with a 30 minute expiry. This involves writing our own auth scheme to read both cookies.

Third option is my favourite, as the restriction is explicit in the implementation, despite being additional work. Option 2 feels like a work around, and relies on us remembering to deescalate on any routes we write in the distant future.

astroash commented 5 years ago

Alternatively,

  1. Deal with escalation all on the client side. This would involve:
    • on admin check setting an auth value in react state that timesout after 30minutes
    • wrapping all escalated admin routes in a auth HOC to check auth state value
    • removing access_mode from the twine_api

(this may be my new fave...)

eliasmalik commented 5 years ago

For solution 4, how does this deal with the scenario outlined here? Seems like if access mode is removed then the cookie would have full access, allowing anyone who navigated to the super-admin dashboard on the public device full access.

astroash commented 5 years ago

Good point. On first thought: The super admin dashboard is going to be for funding bodies, rather than cbs. Also, as it will relate to finance, there's an argument for forced login on each visit.

It's probably worth us outlining the auth needs of each application and check this with P2C to help design the best solution. I think this should include:

I'll have an attempt at this n posting here.

astroash commented 5 years ago

It's also worth considering the UX for twine admin/funding bodies that will be using the same computer to log into multiple accounts.

Currently logging into the temp-dashboard sets a TWINE_ADMIN cookie. When switching to the visitor app designed for CB_ADMIN the login check returns 403.

🍪 MONO COOKIE APPROACH

  1. Remove the TWINE_ADMIN from the login page
  2. Or always redirect to login on 403 and overwrite the TWINE_ADMIN cookie on successful login

🍪 MULTI COOKIE 🍪 +1 This approach avoids removing another apps cookie

astroash commented 5 years ago
screen shot 2018-11-13 at 6 01 35 pm

@eliascodes ☝️ my current assumptions for session requirements. Anything to add/amend?

eliasmalik commented 5 years ago

I'm not at all sure about the intent for the Frontline Dashboard.

Worth thinking about the native app too, it still uses sessions since the token expires.

As an unrelated point, given we have stateless storage, if we ever wanted to invalidate tokens server side, we need to store an additional bit of info in the token. So each token would have a series or version number or something like that, which we could check with a server-side condition saying "only tokens with version > N are valid". This would be a simple way to revoke access for existing tokens.

astroash commented 5 years ago

single cookie solutions

  1. Visitor app deescalates on all non admin routes

    • ✅ quickest solution
    • 🚫 adds numerous extra requests and requires us to remember to deescalation on additional routes

    💻 technical details

    • add componentDidMount request to /deescalate on all non admin routes
  2. Visitor app locally escalates CB_ADMIN login

    • ✅ simplifies db schema (no need to hold access_mode for permissions)
    • 🚫 allows CB_ADMIN cookie to be used on other apps

    💻 technical details

    • all admin routes in Visitor app wrapped in stateful component that locally stores auth
    • all pages wrapped in <PrivateRoute> HOC that checks auth state

    multi cookie solutions

  3. Additional cookie set on Visitor App only

    • Admin login sets an additional 'full' access cookie with a 30 minute expiry.
    • ✅ can specify lifespan of full access escalation cookies
    • 🚫 big change. involves writing our own auth scheme to read both cookies.

    💻 technical details

    • write own scheme & strategy for auth. Scheme needs to take an array of cookie names to check in order. Strategies can be v similar to current implementation.
    • checks existence of escalation cookie, uses if found
    • checks existence of standard cookie, uses if found
  4. Additional cookies set on all apps

    • Scheme will hold preference for cookie from current app, but able to default to other app cookies in defined order
    • ✅ Full control of cookies on each application eg lifespan
    • 🚫 big change. involves writing our own auth scheme to read both cookies.

    💻 technical details

    • write own scheme & strategy for auth. Scheme needs to take an array of cookie names to check in order. Strategies can be v similar to current implementation.
    • cookie preference array set dependent on request origin eg for VISITOR app this may be ['VISITOR_APP', 'SUPER_ADMIN_DASHBOARD']
eliasmalik commented 5 years ago

I think the reason this is a bit difficult is because this escalation-style feature is more naturally implemented using stateful sessions. We may eventually want to switch to a stateful system.

Anyway...

I don't think either of the single-token options account for visitors navigating to the super-admin dashboard on the public device.

It's not clear to me how the multi-cookie options would handle this case. For option 1, if the escalation cookie is only for the visitor app, does that mean the standard cookie grants full access to the super-admin dashboard? For option 2, if there's a super-admin cookie but no visitor cookie, what scopes does the request get?

Further options for single-tokens, which can be implemented individually or together:

  1. HOC that wraps all admin pages and sets a onBeforePageUnload hook that prompts admin to click "go back home" before leaving the page. This button de-escalates, then forwards to /home.
  2. Check on app load whether session exists and is full or restricted (by calling API):
    • if not exsists: go to /login
    • if restricted: go to /home
    • if full: go to /dashboard
  3. Implement sliding-sessions that expire after periods of inactivity (e.g. 10 mins). This involves setting a new JWT on each request (probs via a hook), and we can use the iat claim (issued at) to measure inactivity.

Option 1 helps in the case of CB navigating away from the visitor app by forcing them to de-escalate first.

Option 2 helps in the case where CB was logged onto super-admin dashboard and is navigating to the visitor app. If they're forwarded to /dashboard they wont leave the tablet lying around until they go back /home.

Option 3 helps us set short expiry times on full sessions without inconveniencing active users.

astroash commented 5 years ago

I don't think either of the single-token options account for visitors navigating to the super-admin dashboard on the public device.

In option 1 you would need to re-enter your password for super-admin (SA) dashboard. Option 2 would allow for no password re-enter.

RE your suggestions: 1 - this is v similar to my first suggestion, but abstracts deescalation to a HOC. I do not think that clientside deescalation is the best approach, but if we decide this is the best approach this offers a nice UX 2 - I'm unsure how this solves the descalation issue 3 - Feels like an app specific feature for auth. What benefit does this have for our other client side apps?

What's your opinion on a multicookie approach? My current preference is to see how much time we have to launch, if there is some available time I would be keen to rewrite auth now, if not defer to my first suggestion as a quick fix hack until we can resolve this properly.

eliasmalik commented 5 years ago

For the record, we implemented the short term fix, and will revisit this after launch.

eliasmalik commented 5 years ago

Closing in favour of specific issues linked to TwinePlatform/twine-visitor#634 (e.g. #335)