Easier Postmessage Origin handling & Popup Build Script by dan-f · Pull Request #30 · nodeSolidServer/solid-auth-client

dan-f · 2017-09-08T19:53:54Z

This PR:

Removes the need to statically template in the application origin
Adds a distributed CLI script to more easily build the popup

coveralls · 2017-09-08T19:57:29Z

Coverage remained the same at 85.773% when pulling ce3a4e9 on dan-f/popup-build-script into 492b294 on master.

coveralls · 2017-09-08T19:59:04Z

Coverage remained the same at 85.773% when pulling ce3a4e9 on dan-f/popup-build-script into 492b294 on master.

coveralls · 2017-09-08T20:11:33Z

Coverage remained the same at 85.773% when pulling 99dab3c on dan-f/popup-build-script into 492b294 on master.

coveralls · 2017-09-08T20:22:04Z

Coverage remained the same at 85.773% when pulling 0fd6f74 on dan-f/popup-build-script into 492b294 on master.

coveralls · 2017-09-13T17:32:46Z

Coverage increased (+0.2%) to 86.2% when pulling 657569b on dan-f/popup-build-script into b457128 on master.

Additionally, add `solid-auth-client` CLI for generating the popup.

coveralls · 2017-09-13T17:40:22Z

Coverage increased (+0.2%) to 86.2% when pulling aecd02e on dan-f/popup-build-script into b457128 on master.

dan-f · 2017-09-13T17:46:27Z

@RubenVerborgh this is ready for review!

RubenVerborgh

Tested, works.

RubenVerborgh · 2017-09-13T19:10:46Z

README.md

+build a static popup bound to your application's name.

-Keeping this in mind, here's how to get things working.
+Keeping this in mind, it's pretty simple to build a popup for your app!


I suppose they could even just edit the HTML at this point, it has become so easy 😉

RubenVerborgh · 2017-09-13T19:12:22Z

bin/solid-auth-client.js

+
+// helpers
+
+function ns(fn) {


Maybe a comment about what this does?

RubenVerborgh · 2017-09-13T19:13:29Z

bin/solid-auth-client.js

+
+function ns(fn) {
+  return function() {
+    fn.apply(


There's no need for apply anymore in ES6, just use the spread operator.

RubenVerborgh · 2017-09-13T19:13:58Z

bin/solid-auth-client.js

+    'dist-popup/popup.html'
+  )
+  if (!fs.existsSync(templateFilename)) {
+    const issueUrl = 'https://github.com/solid/solid-auth-client/issues'


maybe hoist all the way up the file?

RubenVerborgh · 2017-09-13T19:14:54Z

bin/solid-auth-client.js

+  }
+}
+
+const log = ns(console.log.bind(console))


No need to bind; log works independently (in Node, not the browser).

RubenVerborgh · 2017-09-13T19:17:29Z

popup-app/index.js

+}
+
+function getStoredAppOrigin() {
+  return getData(sessionStorage).then(data => data.appOrigin)


I'd just use async/await consistently.

RubenVerborgh · 2017-09-13T19:18:14Z

src/ipc.js

+    if (origin !== clientOrigin) {
      console.warn(
-        `SECURITY WARNING: solid-auth-client is listening for messages from ${childOrigin},` +
+        `SECURITY WARNING: solid-auth-client is listening for messages from ${clientOrigin},` +


I'd explicitly say that the message has been rejected.

RubenVerborgh · 2017-09-13T19:18:53Z

src/popup.js


+export const appOriginHandler = (req: request): ?Promise<response> => {
+  const { id, method } = req
+  switch (method) {


Could be a simple if I suppose.

RubenVerborgh · 2017-09-13T19:27:25Z

bin/solid-auth-client.js

+
+program
+  .version(version)
+  .command('generate-popup <trusted-name> [filename]')


Why is required? Also, I found the name confusing.

RubenVerborgh · 2017-09-13T19:30:14Z

.env.demo.example

@@ -1 +0,0 @@
-POPUP_URI=http://localhost:8081/popup.html


Some mentions of the .env files are left in the README.

coveralls · 2017-09-13T20:05:37Z

Coverage increased (+0.2%) to 86.145% when pulling eb8ab9a on dan-f/popup-build-script into b457128 on master.

dan-f force-pushed the dan-f/popup-build-script branch from ce3a4e9 to 99dab3c Compare September 8, 2017 20:10

dan-f force-pushed the dan-f/popup-build-script branch from 99dab3c to 0fd6f74 Compare September 8, 2017 20:19

dan-f requested a review from RubenVerborgh September 8, 2017 20:21

dan-f force-pushed the dan-f/popup-build-script branch from 0fd6f74 to 657569b Compare September 13, 2017 17:29

dan-f changed the title ~~Simplify Popup Building~~ Easier Postmessage Origin handling & Popup Build Script Sep 13, 2017

Replace TRUSTED_APP_ORIGIN with origin discovery

aecd02e

Additionally, add `solid-auth-client` CLI for generating the popup.

dan-f force-pushed the dan-f/popup-build-script branch from 657569b to aecd02e Compare September 13, 2017 17:37

RubenVerborgh approved these changes Sep 13, 2017

View reviewed changes

Address code review feedback

eb8ab9a

dan-f merged commit 685b1ae into master Sep 13, 2017

dan-f deleted the dan-f/popup-build-script branch September 13, 2017 20:07

		@@ -1 +0,0 @@
		POPUP_URI=http://localhost:8081/popup.html

Conversation

dan-f commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 8, 2017

Uh oh!

coveralls commented Sep 8, 2017

Uh oh!

coveralls commented Sep 8, 2017

Uh oh!

coveralls commented Sep 8, 2017

Uh oh!

coveralls commented Sep 13, 2017

Uh oh!

coveralls commented Sep 13, 2017

Uh oh!

dan-f commented Sep 13, 2017

Uh oh!

RubenVerborgh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dan-f commented Sep 8, 2017 •

edited

Loading