GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Send full returnToUrl to oidc-auth-manager by RubenVerborgh · Pull Request #648 · nodeSolidServer/node-solid-server

Conversation

RubenVerborgh commented Mar 23, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Addresses the node-solid-server part of #571.

oidc-auth-manager still loses the URL we send, so fixes on that side are needed to fully solve #571. Tracking that in nodeSolidServer/oidc-auth-manager#17.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

RubenVerborgh added bug ready for review on-hold blocker labels

Mar 23, 2018

RubenVerborgh requested a review from dmitrizagidulin

March 23, 2018 15:06

RubenVerborgh mentioned this pull request

Mar 23, 2018

Fragment identifier on main page URI is stripped in login process #571

Closed

RubenVerborgh commented

Mar 23, 2018

View reviewed changes

Comment thread

default-views/auth/select-provider.hbs

		</div>
		<div class="container">
		<form method="post" action="/api/auth/select-provider">
		<form method="post">

RubenVerborgh Mar 23, 2018

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Always POST to self, so we don't lose query string.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

RubenVerborgh commented

Mar 23, 2018

View reviewed changes

Comment thread

default-views/auth/select-provider.hbs

		<label for="webid">WebID or server URL:</label>
		<input type="text" class="form-control" name="webid" id="webid"
		value="{{serverUri}}" />
		<input type="hidden" name="returnToUrl" id="returnToUrl" value="" />

RubenVerborgh Mar 23, 2018

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This one was always empty, so rather pointless. Instead, the query string should be used.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

RubenVerborgh commented

Mar 23, 2018

View reviewed changes

Comment thread

lib/handlers/error-pages.js

		'/api/auth/select-provider?returnToUrl=' + currentUrl
		const locals = req.app.locals
		const currentUrl = util.fullUrlForReq(req)
		const loginUrl = `${locals.host.serverUri}/api/auth/select-provider?returnToUrl=${encodeURIComponent(currentUrl)}`

RubenVerborgh Mar 23, 2018

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Escaping was definitely important here (for query strings and such).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

RubenVerborgh commented

Mar 23, 2018

View reviewed changes

Comment thread

lib/handlers/error-pages.js

		<meta charset="UTF-8">
		<script>
		window.location.href = "${url}" + window.location.hash
		window.location.href = "${url}" + encodeURIComponent(window.location.hash)

RubenVerborgh Mar 23, 2018

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

So it is the client who appends the hash (and only the client can do that).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

RubenVerborgh commented

Mar 23, 2018

View reviewed changes

Comment thread

lib/handlers/error-pages.js

		res.header('Content-Type', 'text/html')

		let currentUrl = util.fullUrlForReq(req)
		req.session.returnToUrl = currentUrl

RubenVerborgh Mar 23, 2018

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We can't rely on session, because the server cannot see the hash.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dmitrizagidulin Mar 23, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This fixed mechanism (not relying on session) will still work on direct requests by the browser (like, requesting an image or a pdf file or whatever), not mediated by a client?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

RubenVerborgh Mar 23, 2018

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If we end up in this code, we are in the process of rendering an HTML error page. So if that's the behavior we exhibit in response to a browser directly requesting an image (I hope not), it's broken for another reason already 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

dmitrizagidulin Mar 23, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ahh, I see. Makes sense.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

RubenVerborgh mentioned this pull request

Mar 23, 2018

Respect incoming returnToUrl from query string nodeSolidServer/oidc-auth-manager#17

Closed

dmitrizagidulin previously approved these changes

Mar 23, 2018

View reviewed changes

dmitrizagidulin left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

RubenVerborgh mentioned this pull request

Apr 19, 2018

Restore WebID-TLS login in OIDC mode #650

Merged


          Send full returnToUrl.

299f5d6

Addresses the node-solid-server part of #571.

RubenVerborgh dismissed dmitrizagidulin’s stale review via

299f5d6

May 4, 2018 19:49

RubenVerborgh force-pushed the fix/redirect-hash branch from 8e3c6d0 to 299f5d6 Compare

May 4, 2018 19:49

RubenVerborgh changed the base branch from master to develop

May 4, 2018 19:49

RubenVerborgh force-pushed the fix/redirect-hash branch from b349038 to e7be5dd Compare

May 4, 2018 20:00

RubenVerborgh commented May 4, 2018

Copy link

Copy Markdown

Contributor Author

@timbl e7be5dd adds a temporary solution to preserve the full return URL (with hash) when logging in with WebID-TLS. Could you test fix/redirect-hash branch on timbl.com?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Preserve return URL for WebID-TLS.

3da620e

RubenVerborgh force-pushed the fix/redirect-hash branch from e7be5dd to 3da620e Compare

May 4, 2018 21:55

RubenVerborgh removed the on-hold label

May 4, 2018

RubenVerborgh mentioned this pull request

May 4, 2018

Make OIDC returnToUrl consistent for password and client certificate #665

Closed

3 tasks done

timbl approved these changes

May 4, 2018

View reviewed changes

timbl left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Works for me on timbl.com
Looks reasonable
An important fix!!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

RubenVerborgh merged commit 4cfd5a3 into develop

May 4, 2018

RubenVerborgh deleted the fix/redirect-hash branch

May 4, 2018 22:43

dmitrizagidulin mentioned this pull request

Jul 2, 2018

My WebID isn't working on foreign site #720

Closed

Reviewers

2 more reviewers

timbl

timbl approved these changes

dmitrizagidulin

dmitrizagidulin left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

blocker bug ready for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

RubenVerborgh commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

dmitrizagidulin left a comment

Choose a reason for hiding this comment

Uh oh!

RubenVerborgh commented May 4, 2018

Uh oh!

timbl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RubenVerborgh commented Mar 23, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.