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

Idp select UI #24

Merged
merged 24 commits into from Sep 1, 2017
Merged

Idp select UI #24

merged 24 commits into from Sep 1, 2017

Conversation

dan-f
Copy link
Contributor

@dan-f dan-f commented Aug 22, 2017

This PR introduces a popup-based workflow for logging in to solid IDPs, shielding client applications from redirects.

  • demo working
  • tests and refactor
  • create build scripts for idp select / redirect app
  • optimize the builds
  • style the popup app
  • simplify .env files
  • remove *.bundle.js from the inlined html bundle directories
  • add text box for logging in to a "custom" IDP
  • report errors to the user in the popup

@RubenVerborgh RubenVerborgh self-requested a review August 23, 2017 15:00
@dan-f dan-f changed the title [WIP] Idp select UI Idp select UI Aug 27, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.1%) to 86.681% when pulling 7952c0d on idp-select-ui into a39c081 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.1%) to 86.681% when pulling ac8a001 on idp-select-ui into a39c081 on master.

README.md Outdated
#### Install the dependencies

```sh
$ npm i
Copy link
Contributor

Choose a reason for hiding this comment

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

That should probably be Yarn.

@@ -0,0 +1,3 @@
TRUSTED_APP_NAME="Your App Here"
TRUSTED_APP_ORIGIN="http://localhost:8080"
FONT_AWESOME_URL="https://use.fontawesome.com/YOUR-CODE-HERE.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

I was confused by this. Maybe document or remove?

README.md Outdated
#### Create and edit your `.env` file

```sh
$ cp .env.example .env
Copy link
Contributor

Choose a reason for hiding this comment

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

This file doesn't exist.

@@ -0,0 +1,3 @@
TRUSTED_APP_NAME="Your App Here"
TRUSTED_APP_ORIGIN="http://localhost:8080"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe good to use the default Solid testing port 8443 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this is intended to be the origin of the trusted application.

@@ -0,0 +1,3 @@
TRUSTED_APP_NAME="Your App Here"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this file have comments, so I know why all of these are used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in the README

README.md Outdated
### Why might I need this?

If you're building a web app and want to identify users with Solid, or store
personal information on your user's solid account, you'll have to authenticate
Copy link
Contributor

Choose a reason for hiding this comment

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

*Solid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coming soon...

README.md Outdated
### How can I use this?

The simplest way to use this library is to install it via `npm` or `yarn`. You can then use the ES6 module (`import { login, currentUser, logout } from 'solid-auth-client'`), or you can grab the transpiled UMD bundle from `node_modules/solid-auth-client/dist-lib/solid-auth-client.bundle.js`.

## API
Copy link
Contributor

Choose a reason for hiding this comment

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

Takes a bit of time here until we get to the meat. Could it make sense to put the types at the end?

README.md Outdated
- CALLBACK_URI: URI for the popup-based callback app. When testing locally,
this will be something like 'http://localhost:XXXX/idp-callback.html'

### Building the popup app
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps explain why I would want to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "app" is a bit confusing, in the sense that "demo app" and "popup app" mean different things. The popup is a mandatory component, the demo is just an external app.

Copy link
Contributor

Choose a reason for hiding this comment

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

And the title is about building, but we are actually running it.

$ yarn test:dev # just run the tests in watch mode
```

### Building the demo app
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the popup come first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is addressed now, I hope.

@@ -3,17 +3,30 @@
[![Build Status](https://travis-ci.org/solid/solid-auth-client.svg?branch=master)](https://travis-ci.org/solid/solid-auth-client)
[![Coverage Status](https://coveralls.io/repos/github/solid/solid-auth-client/badge.svg?branch=master)](https://coveralls.io/github/solid/solid-auth-client?branch=master)

Opaquely authenticates solid clients
Opaquely authenticates [Solid](https://github.com/solid/) clients

## About
Copy link
Contributor

Choose a reason for hiding this comment

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

Very clear!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ty :)

@@ -0,0 +1,2 @@
IDP_SELECT_URI=http://localhost:8081/idp-select.html
CALLBACK_URI=http://localhost:8081/idp-callback.html
Copy link
Contributor

Choose a reason for hiding this comment

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

How about 8443 as default port (to align with node-solid-server)?

Copy link
Contributor

Choose a reason for hiding this comment

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

…and https:// too.


```sh
$ cp .env.popup.example .env.popup && $EDITOR .env.popup # configure the popup app
$ yarn start:popup
Copy link
Contributor

Choose a reason for hiding this comment

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

I was confused here that this didn't just give me the HTML files I need, but instead starts an app. (I mean, the "start" part is named well, but it's just not what I expect.)

How can I generate the files?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also weird that both the popup and demo are started on 8080.

README.md Outdated
- CALLBACK_URI: URI for the popup-based callback app. When testing locally,
this will be something like 'http://localhost:XXXX/idp-callback.html'

### Building the popup app
Copy link
Contributor

Choose a reason for hiding this comment

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

And the title is about building, but we are actually running it.

const query = `
@prefix foaf http://xmlns.com/foaf/0.1/
${webId} { foaf:name }
`
return fetch('https://databox.me/,query', { method: 'POST', body: query })
.then(resp => resp.json())
return fetch('https://databox.me/,query', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it going there? It should just dereference my WebID.

Copy link
Contributor Author

@dan-f dan-f Aug 29, 2017

Choose a reason for hiding this comment

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

This is using my live twinql query endpoint :) It is dereferencing your webid behind the scenes.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.8%) to 86.042% when pulling ec5bc22 on idp-select-ui into a0f242e on master.

@dan-f
Copy link
Contributor Author

dan-f commented Aug 30, 2017

@RubenVerborgh - here's the final draft. If there's anything else you'd like to see changed, let me know!

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.1%) to 85.773% when pulling c516dc6 on idp-select-ui into a0f242e on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.1%) to 85.773% when pulling c481e50 on idp-select-ui into a0f242e on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.1%) to 85.773% when pulling 8ddea37 on idp-select-ui into a0f242e on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.1%) to 85.773% when pulling 69548ca on idp-select-ui into a0f242e on master.

@dan-f dan-f merged commit 55c2693 into master Sep 1, 2017
@dan-f dan-f deleted the idp-select-ui branch September 1, 2017 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants