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.

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

Conversation

Projects
None yet
2 participants
@robertknight
Contributor

robertknight commented Nov 9, 2017

This fixes an issue where the client could end up using OAuth tokens for a regular Hypothesis account from a previous session when visiting a website which configures the client to use third-party accounts. This happened if the user was anonymous on the third-party website and so the "grantToken" property in the service configuration was null.

Fixes #571

@robertknight robertknight requested a review from seanh Nov 9, 2017

@robertknight robertknight added the WIP label Nov 9, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 9, 2017

Codecov Report

Merging #572 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #572      +/-   ##
==========================================
+ Coverage   90.92%   90.92%   +<.01%     
==========================================
  Files         132      132              
  Lines        5346     5347       +1     
  Branches      928      929       +1     
==========================================
+ Hits         4861     4862       +1     
  Misses        485      485
Impacted Files Coverage Δ
src/sidebar/oauth-auth.js 98.43% <100%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed148af...ea7f5e4. Read the comment docs.

codecov bot commented Nov 9, 2017

Codecov Report

Merging #572 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #572      +/-   ##
==========================================
+ Coverage   90.92%   90.92%   +<.01%     
==========================================
  Files         132      132              
  Lines        5346     5347       +1     
  Branches      928      929       +1     
==========================================
+ Hits         4861     4862       +1     
  Misses        485      485
Impacted Files Coverage Δ
src/sidebar/oauth-auth.js 98.43% <100%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed148af...ea7f5e4. Read the comment docs.

@robertknight robertknight removed the WIP label Nov 9, 2017

@seanh seanh self-assigned this Nov 9, 2017

@seanh

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Nov 13, 2017

Contributor

Works here

Contributor

seanh commented Nov 13, 2017

Works here

} else {
// User is anonymous on the publisher's website.
tokenInfoPromise = Promise.resolve(null);
}

This comment has been minimized.

@seanh

seanh Nov 13, 2017

Contributor

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).

@seanh

seanh Nov 13, 2017

Contributor

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).

This comment has been minimized.

@robertknight

robertknight Nov 13, 2017

Contributor

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.

@robertknight

robertknight Nov 13, 2017

Contributor

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.

return auth.tokenGetter().then(() => {
assert.notCalled(fakeLocalStorage.setObject);
});
});

This comment has been minimized.

@seanh

seanh Nov 13, 2017

Contributor

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

@seanh

seanh Nov 13, 2017

Contributor

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

// User is anonymous on the publisher's website.
authority: 'publisher.org',
grantToken: null,
expectedToken: null,

This comment has been minimized.

@seanh

seanh Nov 13, 2017

Contributor

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.

@seanh

seanh Nov 13, 2017

Contributor

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.

return auth.tokenGetter().then(token => {
assert.equal(token, expectedToken);
assert.notCalled(fakeLocalStorage.getObject);
});

This comment has been minimized.

@seanh

seanh Nov 13, 2017

Contributor

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).

@seanh

seanh Nov 13, 2017

Contributor

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).

This comment has been minimized.

@robertknight

robertknight Nov 13, 2017

Contributor

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

@robertknight

robertknight Nov 13, 2017

Contributor

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

var cfg = serviceConfig(settings);
if (cfg && typeof cfg.grantToken !== 'undefined') {
shouldPersist = false;
}

This comment has been minimized.

@seanh

seanh Nov 13, 2017

Contributor

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.

@seanh

seanh Nov 13, 2017

Contributor

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.

This comment has been minimized.

@robertknight

robertknight Nov 13, 2017

Contributor

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.

@robertknight

robertknight Nov 13, 2017

Contributor

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.

@robertknight

This comment has been minimized.

Show comment
Hide comment
@robertknight

robertknight Nov 13, 2017

Contributor

Depends on #573, will need rebasing after that is merged.

Contributor

robertknight commented Nov 13, 2017

Depends on #573, will need rebasing after that is merged.

@robertknight

This comment has been minimized.

Show comment
Hide comment
@robertknight

robertknight Nov 13, 2017

Contributor

Rebased on #573

Contributor

robertknight commented Nov 13, 2017

Rebased on #573

robertknight added some commits Nov 9, 2017

Fix handling of persisted OAuth tokens for anonymous third-party users
When automatic login to a third-party account is being used, via a grant
token, OAuth tokens persisted by other client sessions should not be
used.

Fix this in the case where the user is anonymous on the publisher's
website and so the publisher will have set the "grantToken" property in
the "services" array to `null`.

Also add a test for the case where the user is logged in and the grant
token is a JWT (although opaque to the client). This case was already
working.
Do not persist refreshed tokens if the original token was temporary
If the initial access token was acquired via an automatic login using a
grant token provided by the publisher, neither the initial access token
nor refreshed tokens should be persisted to local storage.
@seanh

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Nov 13, 2017

Contributor

Rebased

Contributor

seanh commented Nov 13, 2017

Rebased

// 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) {

This comment has been minimized.

@seanh

seanh Nov 13, 2017

Contributor

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.

@seanh

seanh Nov 13, 2017

Contributor

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.

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

This comment has been minimized.

@robertknight

robertknight Nov 21, 2017

Contributor

Add a docstring here.

@robertknight

robertknight Nov 21, 2017

Contributor

Add a docstring here.

@seanh

seanh approved these changes Nov 21, 2017

@seanh

This comment has been minimized.

Show comment
Hide comment
@seanh

seanh Nov 21, 2017

Contributor

Following a Slack chat to explain some of the details we're gonna merge this now in order to not block other ongoing work, but @robertknight has some good ideas for refactoring this file in follow up pull requests that should make the file a lot easier to follow

Contributor

seanh commented Nov 21, 2017

Following a Slack chat to explain some of the details we're gonna merge this now in order to not block other ongoing work, but @robertknight has some good ideas for refactoring this file in follow up pull requests that should make the file a lot easier to follow

@seanh seanh merged commit 868dabd into master Nov 21, 2017

5 checks passed

codecov/patch 100% of diff hit (target 90.92%)
Details
codecov/project 90.92% (+<.01%) compared to ed148af
Details
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@seanh seanh deleted the fix-anon-third-party-user-oauth branch Nov 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment