Is the ACL working correctly?

[Wuntenn]

https://github.com/meanjs/mean/blob/master/modules/articles/server/policies/articles.server.policy.js#L28
The ACL for articles creates a whitelist for users methods GET and POST on the route: '/api/articles' and a GET only on '/api/articles/:articleId'

https://github.com/meanjs/mean/blob/master/modules/articles/client/services/articles.client.service.js
The angular client-side service creates the user method PUT for updating '/api/articles/:articleId'.

The documentation on the Angular side doesn't give any indication that the PUT isn't authentic https://docs.angularjs.org/api/ngResource/service/$resource (scroll down/ find in page: Creating a custom 'PUT' request).

I'm just curious how user is able to update using PUT when it's not been added to the ACL especially after seeing examples showing whitelisting of PUT in the documentation. https://www.npmjs.com/package/acl.

[Wuntenn]

Just remembered - lower down in the policy is the line were we bypass all acl checks as long as the user is the same person that created of the article.

[Wuntenn]

On second thoughts there is a fault here! The fault is that as long as you make a valid request for a article once, this will act as the key to being able to do a subsequent illegal request.

Note that the request that is checked is always the previously cached one. This is also true of the window.user https://github.com/meanjs/mean/blob/master/modules/articles/server/policies/articles.server.policy.js#L49-L54

[mleanos]

@Wuntenn Can you describe the steps to reproduce this issue?

[lirantal]

@Wuntenn that part of the code doesn't show any key caching.. can you be more descriptive where do you think the problem is?

[hyperreality]

If @Wunteen means that it is possible to modify the window user object and use this to bypass the server authentication, I briefly tried this and of course it did not work.

[Wuntenn]

Sorry I didn't explain the steps well.

In your Facebook Dev Account set up a test app and add details for OAuth to your app. Whilst in FB select the app you created, add a test user. Then follow these steps:

1 - Log into the MEANJS app as your normal Facebook user using OAuth (from the details in the
above FB App setup) and create some articles.
2 - Don't log out.
3 - Goto Facebook switch to the test user
(Click your app > Roles in the sidebar > You'll see the table of roles > Add one if you haven't
before. > To the right of the entry for the new user, click the "edit" button > Select log in as
test user)
4 - Return to the app.
5 - Create some more articles
6 - If you look at the creator of the articles, it is the original user (you) and not the test user

This is because of the way that our authentication works. Passport only deals with authentication but nothing after that point. It was designed and scoped to only handle this part.

The problem is that we don't poll FB (or any of the other providers) beyond the point of login to revalidate their authentication when accessing or creating resources. This means that as long as you make a valid login, it stays active and doesn't switch appropriately with new OAuth sessions.

[mleanos]

@Wuntenn This seems like a very complex edge case. In most cases, when testing an app, the dev would just use their own FB account for testing. As you mentioned this is definitely behaving as designed.

IMO, we can't expect to know exactly how a user would want the MEAN application to behave. If the user doesn't explicitly take action, then how do we know that the user doesn't want to remain logged in as the "original" logged in FB user? We can't assume anything in this scenario.

@Wuntenn Can you explain a real-life scenario where this would affect an application's end user? How have other application developers solved such a problem?

For devs, I don't really see a security/data-integrity issue. For end users, it doesn't seem like a typical situation. Perhaps, this could come up in a public/shared device. However, users in these cases should already be aware that they must log out of the application in question.

Most of all, I think the desired behavior can vary greatly from one application, to the next. We should leave this up to the MEAN user to decide what is best for their application's needs.

[Wuntenn]

@mleanos It means that a userA can be logged into the app using any of the providers. That user may not explicitly log out.

If another person userB whilst also logged into any provider and visited the app, they will be actually be resuming action(s) as userA.

If I sit at an internet cafe and public machine and logged into Facebook then visit Trello or Slack or some service where I can use OAuth, I might be surprised to see other users details there. I would see it as a nice to-have if they recognised that I wasn't the original user and subsequently logged me out, but I wouldn't call this their fault (yet); it's more the fault of the user.

If they failed to log me out and I was able to start using the service as if I'm that user and post on their behalf, also if that app has socket chat built in, there may be some entertainment value there!

I don't see this as an edge case. With knowledge of this flaw, apps made with MeanJS could be targeted intentionally to access users personal information so it's definitely nothing other than a security risk.

I've not been able to find implementations where people have implemented the remainder of the OAuth user flow past Passport login. Search results are quite noisy, however within the Facebook SDK they do mention the case in the second paragraph under the section "Roundtrips to Facebook's servers":

https://developers.facebook.com/docs/reference/javascript/FB.getLoginStatus

They use FB.getLoginStatus:

FB.getLoginStatus(function(response) {
  // this will be called when the roundtrip to Facebook has completed
}, true);

The FB SDK is easily added to the core as it's client based. It allows us to use the existing authentication and to hook into it at the stage we're at currently.

I'm thinking, call like this could be wrapped into a middleware and chained into the routes just before the ACL to prevent new data from being accessed. Additionally (and optionally) we can implement a service that wraps the FB call. It would be attached to the core layout template and would use the expiry time as a trigger, checking user login status to automatically log them out if their OAuth is no longer active (we're able to subscribe to to user login events)

[Wuntenn]

@mleanos I've been considering which direction to move with my app. For a moment Facebook SDK seemed like a clear winner but then I looked into Google API and now... I'm confused. They both look good!

One thing I've noticed is that they look like they are both moving away from OAuth in favour of asynchronously loading the JS into the page. I hardly see a reason to keep passport around. Both offer a similar snippet that simplifies the login process. Here the code to use the sdk:

Facebook SDK: https://developers.facebook.com/docs/javascript/quickstart
Google: https://developers.google.com/identity/sign-in/web/sign-in

Both SDKs allow for the user login status to be polled after login:

• Google: https://developers.google.com/identity/sign-in/web/listeners
• FB: above

I'll see how far I can get with a solution and share what I find.

[shanavas786]

@Wuntenn IMO, MEAN or any other application has litte to do with it. A similar scenario is when user logs in using application specific credentials and leaves without logging out. Anyone can access the session !!.

It is not the responsibility of application to keep session in sync with the authorization provider's session. And it shouldn't.

[Wuntenn]

I see your point if you want to say that developers should do it themselves - they should! However the whole point of using a MEAN Stack or the any of Frameworks that comprise it, is to solve commonly faced problems so that the community adopting the stack or framework gain by leveraging time and skills (best practice).

@shanavas786 You're right it's not a problem specific to MEANJS. Anyone who implements authentication using Passport has successfully handled Authentication. However, there are more things to consider beyond authentication.

IMO anyone who pulls user information from a secure source into a browser, subsequently has the responsibility of keeping that data safe! (hence, most/all users of this stack - common problem!)

This project already considers authentication important to the point where this generic problem has been partially solved via integration of Passport. The rest of the problem being equally prevalent should be prioritised accordingly.

It is not the responsibility of application to keep session in sync with the authorisation provider's session. And it shouldn't.

Again, you're right; This is exactly why both Google and Facebook have gone to the efforts of ensuring that their API's handle this case so that application developers don't have to. I'm not saying that we should invent a solution. I'm saying that we should incorporate officially implemented existing ones.

A number of open/closed issues derive from problems caused by keeping the access token in sync and indicate the demand for a best practice solution like this. See issues: #1070, #1062, #288, #511, #1086,

[lirantal]

@Wuntenn can you please sum up in a nutshell the issue and your proposed solution? (also you can submit a PR with the fix so we can review what it entails)

[Wuntenn]

Passport does the authentication step which is all Jared intended it to do. However it is up to us to do things like:

• Making sure that the user who requests access to a resource is the same as the one who authenticated. It's often the case that a person doesn't sign out and another comes after to the same provider. Our service doesn't check that the user is the same. There are calls in Google's API and Facebooks SDK that allow us to make calls to check this.
• We should make sure that users access tokens aren't easily obtained. So minimising cases were we display them or even output them on admin accounts helps (this is why I limited the admin data in my PR Sanitise user #1417).
• If a user is away, signs out, or if another person signs in with the same provider we should log the user out. This is optional but minimises the exposure of their access token.

There are quite a few attacks that make use of obtaining the token, (see fb vid linked above) so taking these kinds of steps at this level would encourage people to continue with best practice.

I've got code doing all three for FB. The real-time call for detecting the user change of status is expensive and uses a flag. Trying to balance using this call vs the cached flag-less call which takes longer to poll.

My code isn't using the latest version of MEANJS (now merging). I diverged so after going Angular Material. I should be able to create and submit PR's for each of the steps once I sort some time out.

[mleanos]

@Wuntenn Is this still an issue? I'm moving this to the back-log since there hasn't been any PR's open to address this.

[Wuntenn]

Thanks @mleanos. This should buy me some time. I have it currently logging users out after users switch devices or log out of their profile but it's not instant. It also logs them out after being inactive for a while. I'm not 100% happy with it.