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.

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

Decouple the Hypothesis service URL from the app's base URI #2881

Merged
merged 6 commits into from Jan 22, 2016

Conversation

Projects
None yet
2 participants
@robertknight
Copy link
Member

commented Jan 20, 2016

Previously the URL set as the tag was used indirectly
to specify the URL of the Hypothesis service that various
links referred to as well as for other purposes.

Make things clearer by adding an explicit 'serviceUrl' attribute
to the Hypothesis client settings and use that in place of the
baseURI. This also gets us a step closer towards making it possible
to build a development front-end which uses the production service for debugging etc.

The custom template whitelist configuration has been removed
because all of the templates are embedded in app.html, so the
default value of ['self'], allowing templates to be loaded
only from the same domain as the page, is fine - unless anyone is aware of any scenarios where this would not be the case.


  • I see a local issue with this branch where the Chrome extension fails to connect to the websocket. Need to investigate whether this is related to the changes in this branch. (This was due to be not having whitelisted requests from the chrome-extension://$ID URL of my local extension build)
  • The 'New Group' link in the Chrome extension should use the service URL explicitly.
  • Links in the 'Sign In' menu should use the service URL explicitly.
  • Forgot Password and 'Create an account' links need to include the service URL / be absolute
  • Share links are broken in the Chrome extension

@robertknight robertknight added the WIP label Jan 20, 2016

@robertknight robertknight force-pushed the separate-service-uri-from-base-uri branch 2 times, most recently from 15cff95 to e67d95d Jan 21, 2016

@robertknight robertknight removed the WIP label Jan 21, 2016

@nickstenning nickstenning force-pushed the separate-service-uri-from-base-uri branch from e67d95d to 11a3ee3 Jan 22, 2016

@nickstenning

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2016

Just noting that if I merge #2880 first this will require a rebase as it changes auth-controller.js which turns into a directive in that PR.

@robertknight

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2016

Yes. If #2880 can be merged first I'll rebase this afterwards.

@nickstenning

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2016

Done. Now needs a rebase.

@robertknight robertknight force-pushed the separate-service-uri-from-base-uri branch from 11a3ee3 to b2d5bd5 Jan 22, 2016

@@ -146,6 +146,7 @@ def settings_dict(env):

config.update({
'buildType': build_type_from_api_url(api_url),
'serviceUrl': base_url,

This comment has been minimized.

Copy link
@nickstenning

This comment has been minimized.

Copy link
@robertknight

robertknight Jan 22, 2016

Author Member

Ah, yes indeed.

robertknight added some commits Jan 20, 2016

Decouple the Hypothesis service URL from the app's base URI
Previously the URL set as the <base> tag was used indirectly
to specify the URL of the Hypothesis service that various
links referred to.

Make things clearer by adding an explicit 'serviceUrl' attribute
to the Hypothesis client settings and use that in place of the
baseURI.

The custom template whitelist configuration has been removed
because all of the templates are embedded in app.html, so the
default value of ['self'], allowing templates to be loaded
only from the same domain as the page, is fine.
Use the service URL to determine the link to the 'New Group' page
Previously the 'New Groups' link opened '/groups/new' in a new
window, which only worked if <base> URL matched the URL
of the Hypothesis service. Instead explicitly use
the serviceUrl from settings.

Also remove an empty set of tests for the group list controller.
The tests are now all done at the directive level.
Modify Sign In control to use service URL explicitly
Remove the assumption in the sign-in menu that the
baseURI for the page is the same as the Hypothesis service.

Also use controllerAs and bindToController for consistency
with recently updated directives.
Hard-code schemes to https:// in share dialog
When the Share dialog is hosted in the Chrome extension
and the <base> URI is not set to point to the Hypothesis service,
the share links need to have a scheme set explicitly.
Explicitly specify the service URL in links from the auth form
Make these links work when the app is hosted at a different
base URL from the Hypothesis service.
Explicitly include the service URL in stream links from the annotatio…
…n card

Make these links work when the app is hosted at a different
base URL from the Hypothesis service.

@robertknight robertknight force-pushed the separate-service-uri-from-base-uri branch from b2d5bd5 to 4b7379b Jan 22, 2016

@nickstenning

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2016

LGTM.

nickstenning added a commit that referenced this pull request Jan 22, 2016

Merge pull request #2881 from hypothesis/separate-service-uri-from-ba…
…se-uri

Decouple the Hypothesis service URL from the app's base URI

@nickstenning nickstenning merged commit 59ede8b into master Jan 22, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@nickstenning nickstenning deleted the separate-service-uri-from-base-uri branch Jan 22, 2016

@nickstenning nickstenning referenced this pull request Feb 10, 2016

Closed

Configurable service URL #1976

0 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.