Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

UI server refactoring #5580

Merged
merged 31 commits into from
May 24, 2017
Merged

UI server refactoring #5580

merged 31 commits into from
May 24, 2017

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented May 9, 2017 •
edited
Loading

  • Removing separate ethcore-signer crate
  • UI server is now standard Hyper server (with /api and RPC)
  • UI is working as a RPC server middleware, similar to dapps.
  • UI is connecting to WebSockets server using token-based scheme (as previously)
  • /api/apps removed in favour of RPC call

Why?

  • Removes special casing for domains, and CORS headers
  • Clear separation
  • UI works just like dapp (but it uses separate port for security reasons)
  • WS server is now first class citizen - you can connect without token to get standard set of APIs or with token to get a sudo access.

Closes #4389
Closes #2080
Closes #5448
Related #4474

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. M7-ui labels May 9, 2017
@tomusdrw tomusdrw requested review from maciejhirsz and jacogr May 9, 2017 10:06
@tomusdrw tomusdrw added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 9, 2017
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels May 9, 2017
jacogr
jacogr reviewed May 10, 2017
View reviewed changes
js/src/index.js

let token = null;

if (window.location.hash && window.location.hash.indexOf(AUTH_HASH) === 0) {
token = qs.parse(window.location.hash.substr(AUTH_HASH.length)).token;
}

const api = new SecureApi(`${urlScheme}${parityUrl}`, token);
const api = new SecureApi(parityUrl, token);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not convinced we want to lose the scheme here, it was done due to users running over HTTPS on the network. Seems like we only support localhost now?

Copy link
Collaborator Author

@tomusdrw tomusdrw May 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The scheme is resolved inside SecureApi, note that currently parityUrl will be just <host>:<port>

Here:
https://github.com/paritytech/parity/pull/5580/files#diff-d37814c31923681deae2e379c0c5b193R33

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I missed a step although I did see the stuff below. However with line 57 above now, we only do localhost and cannot connect to anything that is on the network?

Copy link
Collaborator Author

@tomusdrw tomusdrw May 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, right you mean connectecting to https remote server from http site?. So it's possible to override (in constructor) protocol returned by SecureApi in case server protocol should be different than the one the UI is running. I can refactor it and detect if protocol was specified in the URL and in such case always use that one if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just worried since the connection to http was logged by a user. (Previously we did ignore protocol here as well)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But now the protocol is handled correctly for all possible cases (including dapps server that had protocol hardcoded).
The only uncovered case is when someone is using SecretApi as a library and wants to connect to https server on a website hosted on http.

@tomusdrw tomusdrw mentioned this pull request May 15, 2017
Closed
@tomusdrw tomusdrw mentioned this pull request May 16, 2017
Closed
@tomusdrw tomusdrw removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label May 19, 2017
@gavofyork gavofyork added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 19, 2017
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. A0-pleasereview 🤓 Pull request needs code review. labels May 22, 2017
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels May 22, 2017
@tomusdrw tomusdrw mentioned this pull request May 22, 2017
Merged
@maciejhirsz
Copy link
Contributor

Still LGTM.

@maciejhirsz maciejhirsz added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 22, 2017
@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels May 23, 2017
@arkpar
Copy link
Collaborator

arkpar commented May 23, 2017

needs a rebase

@tomusdrw tomusdrw mentioned this pull request May 23, 2017
Merged
@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 23, 2017
@NikVolf
Copy link
Contributor

NikVolf commented May 23, 2017

... and needs rebase again

@arkpar arkpar merged commit cbcc369 into master May 24, 2017
@arkpar arkpar deleted the ui-server branch May 24, 2017 10:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jacogr jacogr jacogr left review comments

@maciejhirsz maciejhirsz maciejhirsz left review comments

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tomusdrw @jacogr @maciejhirsz @General-Beck @arkpar @NikVolf @gavofyork