Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue that could lead to user being logged into normal Hypothesis account on websites using third-party accounts #572

Merged
merged 2 commits into from
Nov 21, 2017
Merged

Fix issue that could lead to user being logged into normal Hypothesis account on websites using third-party accounts #572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fda9286
Fix handling of persisted OAuth tokens for anonymous third-party users
robertknight Nov 9, 2017
ea7f5e4
Do not persist refreshed tokens if the original token was temporary
robertknight Nov 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 29 additions & 20 deletions src/sidebar/oauth-auth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,6 @@ function auth($http, $rootScope, $window,
return $http.post(url, data, requestConfig);
}

function grantTokenFromHostPage() {
var cfg = serviceConfig(settings);
if (!cfg) {
return null;
}
return cfg.grantToken;
}

/**
* Return the storage key used for storing access/refresh token data for a given
* annotation service.
Expand Down Expand Up @@ -223,17 +215,25 @@ function auth($http, $rootScope, $window,
*/
function tokenGetter() {
if (!tokenInfoPromise) {
var grantToken = grantTokenFromHostPage();

if (grantToken) {
// Exchange host-page provided grant token for a new access token.
tokenInfoPromise = exchangeJWT(grantToken).then((tokenInfo) => {
return tokenInfo;
}).catch(function(err) {
showAccessTokenExpiredErrorMessage(
'You must reload the page to annotate.');
throw err;
});
var cfg = serviceConfig(settings);

// Check if automatic login is being used, indicated by the presence of
// the 'grantToken' property in the service configuration.
if (cfg && typeof cfg.grantToken !== 'undefined') {
if (cfg.grantToken) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in Slack there's an unhandled case here when there is a service config but it doesn't contain any grantToken (not even null). This should be fixed elsewhere, in a separate PR: a service config without a grantToken should be considered invalid and rejected. The docs for grantToken need to be updated to say that it's required and does not default to null.

// User is logged-in on the publisher's website.
// Exchange the grant token for a new access token.
tokenInfoPromise = exchangeJWT(cfg.grantToken).then((tokenInfo) => {
return tokenInfo;
}).catch(function(err) {
showAccessTokenExpiredErrorMessage(
'You must reload the page to annotate.');
throw err;
});
} else {
// User is anonymous on the publisher's website.
tokenInfoPromise = Promise.resolve(null);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here, if I understand it correctly, is that whenever there's a truthy serviceConfig() (which currently I believe is equivalent to whenever the embedding page contains a js-hypothesis-config or a window.hypothesisConfig() with a services object) then first-party accounts are disabled and third-party accounts are used.

But in the longer term I think what we want is for the default hypothes.is service to be a service like any other, one that appears in the services object alongside any other service configs, it's just that it's found in a services object in the app.html page instead of in the embedding page.

Do you agree? And if so, do we need another way of deciding when to enable third-party accounts, instead of checking for the presence of a service config?

I guess this also introduces another way to disable Hypothesis on a site, by embedding the client and including a services config (e.g. one with an authority that doesn't exist server-side and a grant token that doesn't work).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here, if I understand it correctly, is that whenever there's a truthy serviceConfig() (which currently I believe is equivalent to whenever the embedding page contains a js-hypothesis-config or a window.hypothesisConfig() with a services object) then first-party accounts are disabled and third-party accounts are used.

Here we aren't testing for first vs third party services but rather the use of automatic login via a grant token. That is what matters because this method is responsible for obtaining an access token from somewhere. In future if h becomes "just another annotation service" then it would, in theory, be eligible to use grant tokens for login as well.

But in the longer term I think what we want is for the default hypothes.is service to be a service like any other, one that appears in the services object alongside any other service configs, it's just that it's found in a services object in the app.html page instead of in the embedding page.

Yes, that is where I expect we'll eventually end up.

} else if (authCode) {
// Exchange authorization code retrieved from login popup for a new
// access token.
Expand Down Expand Up @@ -264,9 +264,18 @@ function auth($http, $rootScope, $window,
}

if (Date.now() > token.expiresAt) {
var shouldPersist = true;

// If we are using automatic login via a grant token, do not persist the
// initial access token or refreshed tokens.
var cfg = serviceConfig(settings);
if (cfg && typeof cfg.grantToken !== 'undefined') {
shouldPersist = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tokenGetter() is getting pretty long (over 70 lines). It probably needs to be split up very soon, if not overdue. It's doing a lot of different things

  1. Decides whether automatic login is in use
  2. Exchanges grant token for access token
  3. Shows error for expired access token
  4. Exchanges auth code for access token
  5. Attempts to load tokens from previous session
  6. Decides whether tokens should be persisted
  7. Attempts to refresh tokens
  8. ...

This is probably a sign that oauth-auth.js itself has gotten too long and needs to be broken up soon.

Copy link
Member Author

@robertknight robertknight Nov 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. There are three things going on here:

  1. Fetch a token from one of three sources (automatic login, auth code, browser storage) if we haven't got one yet.
  2. Await the fetched token
    3a. Return the fetched token if valid
    3b. Otherwise, refresh the token and await the result.

I could split out (1) into separate functions but I'd prefer to do that as a separate PR to avoid distracting from the bug being fixed here.


// Token expired. Attempt to refresh.
tokenInfoPromise = refreshAccessToken(token.refreshToken, {
persist: true,
persist: shouldPersist,
}).catch(() => {
// If refreshing the token fails, the user is simply logged out.
return null;
Expand Down
71 changes: 53 additions & 18 deletions src/sidebar/test/oauth-auth-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,12 +181,6 @@ describe('sidebar.oauth-auth', function () {
});
});

it('should not persist access tokens fetched using a grant token', function () {
return auth.tokenGetter().then(() => {
assert.notCalled(fakeLocalStorage.setObject);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so this test was replaced by the parametrize-style one below


context('when the access token request fails', function() {
beforeEach('make access token requests fail', function () {
fakeHttp.post.returns(Promise.resolve({status: 500}));
Expand Down Expand Up @@ -353,6 +347,33 @@ describe('sidebar.oauth-auth', function () {
});
});

[{
// User is logged-in on the publisher's website.
authority: 'publisher.org',
grantToken: 'a.jwt.token',
expectedToken: 'firstAccessToken',
},{
// User is anonymous on the publisher's website.
authority: 'publisher.org',
grantToken: null,
expectedToken: null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add more cases to the tokenGetter() tests above this? In describe('#tokenGetter' I see it('should request an access token if a grant token was provided', but that's all. Looking at the code I think there are a number of cases to test, seems a likely candidate for a parametrized test like this one:

  1. No service config at all
  2. Service config contains no grantToken
  3. Service config contains a grantToken but it's null. I believe other falsey values e.g. false may behave the same as null too.
  4. Service config contains a truthy grantToken

Case 2 is also tested currently, in it('should return null if no grant token was provided', but it's way further down in the file than it('should request an access token if a grant token was provided',, may be worth bringing them together in one parametrized test and adding additional cases.

}].forEach(({ authority, grantToken, expectedToken }) => {
it('should not persist access tokens if a grant token was provided', () => {
fakeSettings.services = [{ authority, grantToken }];
return auth.tokenGetter().then(() => {
assert.notCalled(fakeLocalStorage.setObject);
});
});

it('should not read persisted access tokens if a grant token was set', () => {
fakeSettings.services = [{ authority, grantToken }];
return auth.tokenGetter().then(token => {
assert.equal(token, expectedToken);
assert.notCalled(fakeLocalStorage.getObject);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these tests be in describe('#tokenGetter'? They look like tests for tokenGetter() to me, although I think the existence of describe('persistence of tokens to storage' confuses things (all the other describes in the file are for describing functions: describe('#tokenGetter', describe('#login', describe('#logout',, and then this one is different).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Reading tokens from & writing tokens to storage is part of what is triggered by by tokenGetter().

});
});

it('persists tokens retrieved via auth code exchanges to storage', () => {
fakeSettings.services = [];

Expand All @@ -367,6 +388,20 @@ describe('sidebar.oauth-auth', function () {
});
});

function expireAndRefreshAccessToken() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a docstring here.

fakeLocalStorage.setObject.reset();
fakeHttp.post.returns(Promise.resolve({
status: 200,
data: {
access_token: 'secondToken',
expires_in: DEFAULT_TOKEN_EXPIRES_IN_SECS,
refresh_token: 'secondRefreshToken',
},
}));
expireAccessToken();
return auth.tokenGetter();
}

it('persists refreshed tokens to storage', () => {
fakeSettings.services = [];

Expand All @@ -375,17 +410,7 @@ describe('sidebar.oauth-auth', function () {
return auth.tokenGetter();
}).then(() => {
// 2. Refresh access token.
fakeLocalStorage.setObject.reset();
fakeHttp.post.returns(Promise.resolve({
status: 200,
data: {
access_token: 'secondToken',
expires_in: DEFAULT_TOKEN_EXPIRES_IN_SECS,
refresh_token: 'secondRefreshToken',
},
}));
expireAccessToken();
return auth.tokenGetter();
return expireAndRefreshAccessToken();
}).then(() => {
// 3. Check that updated token was persisted to storage.
assert.calledWith(fakeLocalStorage.setObject, TOKEN_KEY, {
Expand All @@ -396,9 +421,19 @@ describe('sidebar.oauth-auth', function () {
});
});

it('does not persist refreshed tokens if the original token was temporary', () => {
fakeSettings.services = [{ authority: 'publisher.org', grantToken: 'a.jwt.token' }];

return auth.tokenGetter().then(() => {
return expireAndRefreshAccessToken();
}).then(() => {
// Check that updated token was not persisted to storage.
assert.notCalled(fakeLocalStorage.setObject);
});
});

it('fetches and returns tokens from storage', () => {
fakeSettings.services = [];

fakeLocalStorage.getObject.withArgs(TOKEN_KEY).returns({
accessToken: 'foo',
refreshToken: 'bar',
Expand Down