ohmage / gwt-front-end

The javascript/GWT code behind the front end.
6 stars 2 forks source link

get rid of confusing `authToken` cookie #262

Closed jeroen closed 11 years ago

jeroen commented 11 years ago

The session is handled naturally by the server which sets a cookie named auth_token after successful authentication. This cookie is then included as a request header to every subsequent request, which automatically makes every request part of the session.

However, the front-end is doing something weird, which I is legacy behavior that was implemented by John Hicks. It seems to read the token from the response body, and manually write a second cookie named authToken. Then it will include the value for this token as the token parameter for every ohmage request. Hence every request includes 3 tokens! :

Now you might think there is no harm in sending the token 3 times instead of once, but there actually is some problems. First of all it is ambiguous which one the server will use, which can lead to bugs. Or more likely: bugs that go unnoticed when using the front-end, but that appear for 3rd party clients.

Furthermore, the authToken cookie that the front-end manually hacks in, is not honoring expiration time, which is a security flaw. Finally in the situation when other dashboards are running on the same server, the situation can arise when auth_token and authToken have different values. Also, if there is an existing session (e.g. authenticated through a dashboard or easyPost), and then the user navigates through the front-end, it will ask for re-authentication, because it ignores the auth_token cookie, and only looks for the authToken cookie.

The solution is simple: just remove all of the stuff related to authToken cookie and the stuff that manually adds the token parameter to requests. The session should be handled only and automatically by the auth_token cookie only, which is set by the server to the correct domain, will expire in time, and defines a session that can be used by any dashboard or frontend. The front-end should look for the auth_token cookie to see if there is an existing session, not authToken.

As a very simple test: when you authenticate at the easypost page, and then navigate to the front-end, it should not be asking you to authenticate again. It should be picking up the auth_token cookie that is already there.

screenshot

jeroen commented 11 years ago

The auth code is here, originally written by John Hicks:

https://github.com/cens/ohmageFrontEnd/blob/master/src/edu/ucla/cens/mobilize/client/common/TokenLoginManager.java

The static cookie hack is defined here:

edu/ucla/cens/mobilize/client/AwConstants.java:    public final static String cookieAuthToken = "authToken";

All of the code where gwt manually messes around with cookies should be stripped. We should remove the places where the frontend manually adds the token as a parameter to every call:

edu/ucla/cens/mobilize/client/dataaccess/MockDataService.java:598:      params.put("auth_token", "my_auth_token");
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:410:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:449:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:637:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:813:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:1114:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:1151:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:1168:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:1381:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:1457:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:1493:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:1503:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:1512:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:1533:    params.put("auth_token", this.authToken());
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:1575:    params.put("auth_token", this.authToken());
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:1645:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:1815:      params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:1857:      params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/AndWellnessDataService.java:1897:      params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/requestparams/ClassSearchParams.java:20:    params.put("auth_token", authToken);
edu/ucla/cens/mobilize/client/dataaccess/requestparams/CampaignReadParams.java:35:    params.put("auth_token", authToken);
edu/ucla/cens/mobilize/client/dataaccess/requestparams/SurveyResponseReadParams.java:100:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/requestparams/UserSearchParams.java:31:    params.put("auth_token", authToken);
edu/ucla/cens/mobilize/client/dataaccess/requestparams/UserUpdateParams.java:29:    params.put("auth_token", authToken);
edu/ucla/cens/mobilize/client/dataaccess/requestparams/SurveyResponseUpdateParams.java:24:    params.put("auth_token", authToken);
edu/ucla/cens/mobilize/client/dataaccess/requestparams/AuditLogSearchParams.java:31:    params.put("auth_token", authToken);
edu/ucla/cens/mobilize/client/dataaccess/requestparams/DocumentReadParams.java:22:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/requestparams/ClassUpdateParams.java:26:    params.put("auth_token", this.authToken);
edu/ucla/cens/mobilize/client/dataaccess/requestparams/UserCreateParams.java:26:    params.put("auth_token", authToken);
edu/ucla/cens/mobilize/client/utils/AwUrlBasedResourceUtils.java:26:    params.put("auth_token", Cookies.getCookie(AwConstants.cookieAuthToken));
edu/ucla/cens/mobilize/client/utils/AwUrlBasedResourceUtils.java:40:    params.put("auth_token", Cookies.getCookie(AwConstants.cookieAuthToken));

And also the stuff where it manually manipulates the cookie (cookies should only be created by the server using the setCookie header):

edu/ucla/cens/mobilize/client/common/TokenLoginManager.java:39:    private final static String AUTH_TOKEN_COOKIE = AwConstants.cookieAuthToken;
edu/ucla/cens/mobilize/client/common/TokenLoginManager.java:68:        if (currentCookieNames.contains(AUTH_TOKEN_COOKIE) &&
edu/ucla/cens/mobilize/client/common/TokenLoginManager.java:93:        Cookies.setCookie(AUTH_TOKEN_COOKIE, authToken, null, null, "/", false);
edu/ucla/cens/mobilize/client/common/TokenLoginManager.java:122:        String authToken = Cookies.getCookie(AUTH_TOKEN_COOKIE);
edu/ucla/cens/mobilize/client/common/TokenLoginManager.java:172:        Cookies.setCookie(AUTH_TOKEN_COOKIE, null, removeExpire, null, "/", false);
jshslsky commented 11 years ago

For reference cens/ohmageServer#468.

jeroen commented 11 years ago

You can test the behavior on the lausd1 test server. The server hosts 3 ohmage clients:

Both easypost and the snackboard behave correctly. If you login using either for the 3 clients, and then go to the snackboard, it will pick up on the session. Also you can use easypost without re-authenticating. Then if you logout using easypost, and refresh snackboard, it will also be logged out.

However, the front-end does not work correctly. If you login using snackboard or easypost, and then go to the front-end, it will ask you for authentication. This should be fixed such that it does a call to /user/whoami to detect if there is an active session.

jshslsky commented 11 years ago

authToken has been replaced with auth_token. There is a new cookie called "u" for the username. In the future the "u" cookie will go away in favor of local storage, but for now this is the solution to maintaining state across complete app reloads (i.e., that is the way the app works today and it is a much bigger effort to change that behavior).

Users will now stay logged-in across web apps served up by ohmage server from the same domain / subdomain.

stevenolen commented 11 years ago

@joshuaselsky after we moved the location of the ohmageFrontend to /web, the u cookie is available only under the /web domain. could you set this cookie as available from root, /?

jeroen commented 11 years ago

What do you need that for?

sent from my droid On Mar 29, 2013 2:12 PM, "stevenolen" notifications@github.com wrote:

@joshuaselsky https://github.com/joshuaselsky after we moved the location of the ohmageFrontend to /web, the u cookie is available only under the /web domain. could you set this cookie as available from root, /?

— Reply to this email directly or view it on GitHubhttps://github.com/cens/ohmageFrontEnd/issues/262#issuecomment-15660957 .

stevenolen commented 11 years ago

see the referenced issue above for details.

jeroen commented 11 years ago

It might be safer to have the cookies only available for /app so that you need to do a whoami call to figure out who you are.

stevenolen commented 11 years ago

right now it's available for /web (the frontend uses it). all of the other cookies the frontend creates are stored for /, so this one seems more of an oversight than anything else. from @joshuaselsky 's comment above, it looks like the intent of u was for all webapps hosted on our server:

Users will now stay logged-in across web apps served up by ohmage server from the same domain / subdomain

so why not give them access to u if they want it while it exists?...I agree though, /app/user/whoami is preferred, which I've mentioned in my comment.

On Fri, Mar 29, 2013 at 3:14 PM, Jeroen notifications@github.com wrote:

It might be safer to have the cookies only available for /app so that you need to do a whoami call to figure out who you are.

— Reply to this email directly or view it on GitHubhttps://github.com/cens/ohmageFrontEnd/issues/262#issuecomment-15663235 .

jeroen commented 11 years ago

| so why not give them access to u if they want it while it exists?

Well it is potentially privacy sensitive information. Also the value of u might be outdated, if e.g. the session has expired. I think the most elegant design is just require apps to do a call to /whoami. That way they verify that the session is actually still valid, and the server stays in control of private information.

jshslsky commented 11 years ago

The u cookie is set by the front end, not the server.