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

Redirect user to original URL instead of proxy if annotation was created by a third-party user #61

Merged
merged 5 commits into from Dec 6, 2017

Conversation

Projects
None yet
2 participants
@robertknight
Contributor

robertknight commented Dec 4, 2017

Depends on #60, will need rebasing.

If a direct-linked annotation was made on a page that embeds Hypothesis and uses third party accounts then the page is highly likely to be broken if delivered through the Via proxy service since Via does not allow cookies to be read/written amongst other issues. Since we know the page already embeds Hypothesis we can just redirect the user to the original URL in this case.

Whether or not an annotation is "third party" is determined by comparing the "authority" field of the Elasticsearch document with a HYPOTHESIS_AUTHORITY setting configured in Bouncer's environment.

Testing steps

  1. Run the client, h and publisher-account-test-site services with the following env vars configured:
    HYPOTHESIS_AUTHORITY=localhost
    BOUNCER_URL=http://127.0.0.1:8000
    
  2. Visit the publisher account test site's demo page and copy a direct link from the "Share" icon in the client
  3. Visit that direct link in a browser that would normally use Via. The local bouncer service should redirect to the original URL instead.
  4. Repeat steps (2) and (3) with a normal Hypothesis annotation in the same browser and Bouncer should attempt to redirect to Via.

@robertknight robertknight added the wip label Dec 4, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Dec 4, 2017

Codecov Report

Merging #61 into master will increase coverage by 0.28%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
+ Coverage   88.44%   88.73%   +0.28%     
==========================================
  Files          10       10              
  Lines         554      568      +14     
==========================================
+ Hits          490      504      +14     
  Misses         64       64
Impacted Files Coverage Δ
bouncer/app.py 0% <ø> (ø) ⬆️
tests/bouncer/views_test.py 100% <100%> (ø) ⬆️
bouncer/util.py 95.65% <100%> (+0.06%) ⬆️
tests/bouncer/util_test.py 100% <100%> (ø) ⬆️
bouncer/views.py 90.09% <100%> (+0.46%) ⬆️

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 3fec6f9...6b85dd5. Read the comment docs.

codecov bot commented Dec 4, 2017

Codecov Report

Merging #61 into master will increase coverage by 0.28%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
+ Coverage   88.44%   88.73%   +0.28%     
==========================================
  Files          10       10              
  Lines         554      568      +14     
==========================================
+ Hits          490      504      +14     
  Misses         64       64
Impacted Files Coverage Δ
bouncer/app.py 0% <ø> (ø) ⬆️
tests/bouncer/views_test.py 100% <100%> (ø) ⬆️
bouncer/util.py 95.65% <100%> (+0.06%) ⬆️
tests/bouncer/util_test.py 100% <100%> (ø) ⬆️
bouncer/views.py 90.09% <100%> (+0.46%) ⬆️

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 3fec6f9...6b85dd5. Read the comment docs.

@robertknight robertknight removed the wip label Dec 5, 2017

@fatbusinessman

I’m having a hard time picking through the history here, trying to figure out which commits are prerequisites for the main change and which are things you spotted along the way. Could you please split the things that can be split into separate pull requests? Anything that would reduce the amount of code that needs digesting at once would be helpful, especially given this is a fairly complicated change that spans multiple programming languages.

@robertknight

This comment has been minimized.

Show comment
Hide comment
@robertknight

robertknight Dec 6, 2017

Contributor

I’m having a hard time picking through the history here, trying to figure out which commits are prerequisites for the main change and which are things you spotted along the way.

Hmm... I've restructured the commits and added more details to the commit messages to try to make things clearer. The first three commits are setup to enable the change and then the last commit actually implements the change. Splitting it out into separate PRs would be awkward at this point because the documentation added in the second commit is altered by the change in the last commit, and splitting out the one-line change in the first commit seems a bit much to me.

Contributor

robertknight commented Dec 6, 2017

I’m having a hard time picking through the history here, trying to figure out which commits are prerequisites for the main change and which are things you spotted along the way.

Hmm... I've restructured the commits and added more details to the commit messages to try to make things clearer. The first three commits are setup to enable the change and then the last commit actually implements the change. Splitting it out into separate PRs would be awkward at this point because the documentation added in the second commit is altered by the change in the last commit, and splitting out the one-line change in the first commit seems a bit much to me.

@fatbusinessman

This mostly looks fine to me. I’ve got a couple of questions and what look like some minor mistakes. The main question I’ve got is about how you settled on the approach you’ve gone with, out of the two options we had available.

*
* This is rendered into Bouncer's interstitial page by the backend service.
*
* @typedef {Object} Settings

This comment has been minimized.

@fatbusinessman

fatbusinessman Dec 6, 2017

Cool! I don’t think I’ve seen typedef declarations before; that’s pretty neat.

@fatbusinessman

fatbusinessman Dec 6, 2017

Cool! I don’t think I’ve seen typedef declarations before; that’s pretty neat.

This comment has been minimized.

@robertknight

robertknight Dec 6, 2017

Contributor

:) - I was wondering if there is something similar for Python dict shapes, given the return type of parse_document, although perhaps that function should just return a class instead?

@robertknight

robertknight Dec 6, 2017

Contributor

:) - I was wondering if there is something similar for Python dict shapes, given the return type of parse_document, although perhaps that function should just return a class instead?

This comment has been minimized.

@fatbusinessman

fatbusinessman Dec 6, 2017

Yeah, I’d usually opt for passing around some kind of class instance rather than a dictionary at that point. By the time you need to document what shape the thing is, I think it’s worth enforcing that shape in code. When it’s a flat structure without a lot of attached functionality, I’m a big fan of namedtuples.

@fatbusinessman

fatbusinessman Dec 6, 2017

Yeah, I’d usually opt for passing around some kind of class instance rather than a dictionary at that point. By the time you need to document what shape the thing is, I think it’s worth enforcing that shape in code. When it’s a flat structure without a lot of attached functionality, I’m a big fan of namedtuples.

Show outdated Hide outdated bouncer/util.py
Show outdated Hide outdated bouncer/views.py
Show outdated Hide outdated tests/bouncer/views_test.py
@@ -43,6 +43,11 @@ ELASTICSEARCH_AWS_{ACCESS_KEY_ID,SECRET_ACCESS_KEY,REGION}
request signing with the given credentials. This allows making requests to an
AWS Elasticsearch domain as an IAM user.
HYPOTHESIS_AUTHORITY

This comment has been minimized.

@fatbusinessman

fatbusinessman Dec 6, 2017

I remember we had a discussion earlier with the option of checking that the authority was in a list of third-party authorities we don’t proxy, rather than checking that it’s not the one first-party authority we do proxy, given the possibility that this approach could potentially cause headaches with our Canvas integration (which looks like it might use a third-party authority but still rely on Via). How did you come to the decision of using this approach instead?

@fatbusinessman

fatbusinessman Dec 6, 2017

I remember we had a discussion earlier with the option of checking that the authority was in a list of third-party authorities we don’t proxy, rather than checking that it’s not the one first-party authority we do proxy, given the possibility that this approach could potentially cause headaches with our Canvas integration (which looks like it might use a third-party authority but still rely on Via). How did you come to the decision of using this approach instead?

This comment has been minimized.

@robertknight

robertknight Dec 6, 2017

Contributor

The main reason I went with checking for "not the first party authority" is because I can't think of a situation in which Via could reasonably work with third party authorities as long as we keep Via's privacy/security features such as not stripping cookie-related headers.

Direct linking to annotations in Canvas will definitely require additional work because the annotated page is not the top-level frame in the document but is hosted inside an iframe. Also, IIRC the plan is that Canvas will always use Via directly rather than going through Bouncer.

Lastly, we also have a few inbound requests from others interested in third party authorities and I would like to lower the barrier to potential users trying out the feature by not requiring a config change to Bouncer to enable new third-party authorities to work.

@robertknight

robertknight Dec 6, 2017

Contributor

The main reason I went with checking for "not the first party authority" is because I can't think of a situation in which Via could reasonably work with third party authorities as long as we keep Via's privacy/security features such as not stripping cookie-related headers.

Direct linking to annotations in Canvas will definitely require additional work because the annotated page is not the top-level frame in the document but is hosted inside an iframe. Also, IIRC the plan is that Canvas will always use Via directly rather than going through Bouncer.

Lastly, we also have a few inbound requests from others interested in third party authorities and I would like to lower the barrier to potential users trying out the feature by not requiring a config change to Bouncer to enable new third-party authorities to work.

This comment has been minimized.

@fatbusinessman

fatbusinessman Dec 6, 2017

Ok. Yup, that all sounds fair to me; thanks for the explanation. If we do find out that this causes some kind of Canvas-related problems, this doesn’t seem like it’ll be hard to adapt later.

@fatbusinessman

fatbusinessman Dec 6, 2017

Ok. Yup, that all sounds fair to me; thanks for the explanation. If we do find out that this causes some kind of Canvas-related problems, this doesn’t seem like it’ll be hard to adapt later.

@fatbusinessman

Cool. I’m happy with this; thanks for making the changes. Rebase the commits if you like, and merge away.

robertknight added some commits Dec 4, 2017

Do not redirect user to proxy for annotations created by third-party …
…users

Annotations in groups belonging to third party authorities can only be
shown using the embedded client on the original URL, not the Via proxy
because configuration from the host page is needed to make the client
show the correct group and log the user in to the third-party managed
account.

Since we know for these annotations that the target URL must embed the
client, we can always redirect the user directly to it, rather than
using the proxy.
Provide a test seam for server-rendered settings in JS tests
This will make it easier to test the behavior of `redirect` in response
to different configuration data provided by the backend.

I also added a missing test to check that reading of configuration data
from the page works as expected.

@robertknight robertknight merged commit 70ec944 into master Dec 6, 2017

5 checks passed

codecov/patch 100% of diff hit (target 88.44%)
Details
codecov/project 88.73% (+0.28%) compared to 3fec6f9
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

@robertknight robertknight deleted the do-not-proxy-third-party-anns branch Dec 6, 2017

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