Enable redirection to previous page after login #487

igorauad · 2015-03-22T14:55:36Z

In response to #185

Two different strategies are adopted, one for when the user authenticates locally and the other through providers. When authenticating locally, the signin function in the client controller redirects to the previous state (storing and using a state name) after successful login. When authenticating through a provider, the first call to provider stores the previous URL (not state, URL) in the session. Then, when provider actually calls the authentication callback, session redirect_to path is used for redirecting user.

Note: I originally did this to master, so there is one extra thing to correct. First, note on branch master sign-in and signup are not nested states. Given they are nested in 0.4.0, the controller instantiated in the nested state does not capture the previous state properly. Therefore, this commit is not redirecting when authenticating locally yet. Suggestions are welcome.

For example, it seems an ugly solution to instantiate AuthenticationController both at parent and nested states. What would be better?

ilanbiala · 2015-03-22T15:05:25Z

modules/users/server/controllers/users/users.authentication.server.controller.js

 * OAuth callback
 */
 exports.oauthCallback = function(strategy) {
 	return function(req, res, next) {
+		// Pop redirect URL from session
+		var sessionRedirectURL = req.session.redirect_to;
+		delete req.session.redirect_to;


Why are we temporarily setting a property on the req session?

@ilanbiala thanks for looking into this. I am setting a property in the first call, oauthCall. In the callback I retrieve it. Do you think there would be a better way to implement this?

I think I'm missing something, but what about the successRedirect property?
http://passportjs.org/guide/google/

@ilanbiala , the callback is called by the provider. As I understand, it is not possible to pass the previous URL to the callback. This is why a oauthCall was introduced, so we have a way of storing the previous state. If we use successRedirect, then we would again need to pass the value for successRedirect somehow, perhaps through a property on session.

Here is where the idea came from http://stackoverflow.com/a/13336055/2859410

What is req.url in oauthCall? What about in oauthCallback?

For example:

oauthCall: /api/auth/twitter
oauthCallback: /api/auth/twitter/callback?oauth_token=xxx&oauth_verifier=xxx

So this PR uses a setter on the URL in Angular to tell the server which URL to redirect to? Is there no way for us to know what page the user was previously on before the going to the authentication route?

Not that I could think of. As my understanding, this Information does not come in req in the oauthCallback

ilanbiala · 2015-03-22T15:48:29Z

modules/users/server/controllers/users/users.authentication.server.controller.js

+	return function(req, res, next) {
+		// Set redirection path on session.
+		// Do not redirect to a signin or signup page
+		if (['/authentication/signin', '/authentication/signup'].indexOf(req.query.redirect_to) === -1) {


Instantiating this array every time will be costly, let's store a constant reference somewhere, no?

lirantal · 2015-07-16T21:01:26Z

@igorauad what's the status here? we can't merge it to 0.4.0 due to conflicts.

igorauad · 2015-07-17T17:44:30Z

Hi @lirantal , thanks for asking. There was something missing, which I just added. Have a look, if possible.

I think it is possible to merge now.

lirantal · 2015-07-18T09:25:56Z

thanks @igorauad, I assume you tested it? as we don't have any frontend tests yet to confirm.
@rhutchison @ilanbiala @simison can you review?

ilanbiala · 2015-07-18T13:27:36Z

modules/users/client/views/authentication/authentication.client.view.html

-		<a href="/api/auth/paypal" target="_self" class="undecorated-link">
-			<img src="/modules/users/img/buttons/paypal.png">
-		</a>
+		<img ng-click="callOauthProvider('/api/auth/facebook')" src="/modules/users/img/buttons/facebook.png">


Shouldn't these use ng-src even though we know the resource?

@ilanbiala let me know once you think the modifications are ready, so that I can squash the commits

rhutchison · 2015-07-21T04:43:48Z

@lirantal Need more time to review. I think this should wait for post-0.4.0

simison · 2015-07-21T08:50:10Z

I blew up my test setup for oauth stuff, gotta wait a bit before I can test this stuff again.

PS. https://pagekite.net/ is pretty neat for testing this.

lirantal · 2015-07-22T19:50:16Z

sure, I'm ok with it. @ilanbiala what do you think?

ilanbiala · 2015-07-22T21:08:31Z

Looks good, but I haven't tested it.

igorauad · 2015-07-22T22:58:45Z

I've just realized something specifically for Paypal authentication was missing, since it didn't exist when I first wrote this PR. I have included it and re-tested the modifications to double check. It is working for both local and external authentication. I think it is ready for a merge. Anybody can confirm?

ilanbiala · 2015-07-23T14:22:17Z

@igorauad are you familiar with adding tests for strategies? @lirantal I think we should push for that in the next few versions, because OAuth strategies can change quite a bit.

igorauad · 2015-07-23T18:51:12Z

@ilanbiala tests would be very useful, but I haven't seen an approach for this. Do you want to share anything that I could take a look?

ilanbiala · 2015-07-24T01:57:16Z

@igorauad I did it with a local strategy https://github.com/HTHS/hths-lunch/blob/master/test/server/mock-strategy.js, but maybe we can adapt it to use e2e tests to do OAuth?

lirantal · 2015-07-24T06:30:27Z

@ilanbiala yeah I agree on the tests. I'm also working on adding some coverage to the code base, and I'd love it if you can help out let's take it in another thread.

About this PR - need angular people to review. @rhutchison WDYT?

pgrodrigues · 2015-07-24T11:35:45Z

modules/users/client/views/authentication/authentication.client.view.html

-		<a href="/api/auth/paypal" target="_self" class="undecorated-link">
-			<img src="/modules/users/img/buttons/paypal.png">
-		</a>
+		<img ng-click="callOauthProvider('/api/auth/facebook')" ng-src="/modules/users/img/buttons/facebook.png">


There are some data- prefix missing both in ng-click and ng-src in this line and in the ones below

data- isn't necessary, I'm not sure why we even started using it.

That was when meanjs was in xhtml era: https://github.com/meanjs/mean/blob/master/app/views/layout.server.view.html#L2

Now with html5 it's perfectly fine not to have them: https://github.com/meanjs/mean/blob/0.4.0/modules/core/server/views/layout.server.view.html#L1-L2

pgrodrigues · 2015-07-24T18:07:19Z

@ilanbiala probably it was most likely to pass in w3c html validation tool

ilanbiala · 2015-07-29T01:53:12Z

@igorauad is this ready to review?

igorauad · 2015-07-29T02:01:43Z

Yes, @ilanbiala. This PR works. Is there anything I should change?

ilanbiala · 2015-07-29T12:13:39Z

@lirantal LGTM.

lirantal · 2015-07-29T13:53:34Z

Sure.
@igorauad can you please rebase to squash your commits?

Two different strategies are adopted, one for when the user authenticates locally and the other through providers. When authenticating locally, the signin function in the client controller redirects to the previous state (storing and using a state name) after successful login. When authenticating through a provider, the first call to provider stores the previous URL (not state, URL) in the session. Then, when provider actually calls the authentication callback, session redirect_to path is used for redirecting user.

igorauad · 2015-07-29T13:56:39Z

Done, @lirantal

lirantal · 2015-07-29T13:57:28Z

Thanks @igorauad

Enable redirection to previous page after login

ilanbiala reviewed Mar 22, 2015
View reviewed changes

igorauad force-pushed the 0.4.0 branch 2 times, most recently from 3a84f3d to 5a7d093 Compare March 22, 2015 15:46

ilanbiala reviewed Mar 22, 2015
View reviewed changes

igorauad force-pushed the 0.4.0 branch from 5a7d093 to a305141 Compare March 22, 2015 15:53

igorauad force-pushed the 0.4.0 branch from a305141 to 358188b Compare July 17, 2015 17:42

ilanbiala added this to the 0.4.0 milestone Jul 17, 2015

ilanbiala added the enhance:feature label Jul 17, 2015

ilanbiala assigned lirantal Jul 17, 2015

ilanbiala reviewed Jul 18, 2015
View reviewed changes

igorauad force-pushed the 0.4.0 branch from 358188b to 197d52b Compare July 19, 2015 22:16

lirantal mentioned this pull request Jul 20, 2015

0.4.0 Release #669

Closed

14 tasks

igorauad mentioned this pull request Jul 23, 2015

Filter states for which login is required during state change #686

Closed

pgrodrigues reviewed Jul 24, 2015
View reviewed changes

igorauad force-pushed the 0.4.0 branch from 0a05aab to 56a5de3 Compare July 24, 2015 15:08

igorauad force-pushed the 0.4.0 branch from 56a5de3 to ab76f00 Compare July 24, 2015 23:35

igorauad force-pushed the 0.4.0 branch from ab76f00 to e6a35a7 Compare July 29, 2015 13:56

lirantal added a commit that referenced this pull request Jul 29, 2015

Merge pull request #487 from igorauad/0.4.0

99a8168

Enable redirection to previous page after login

lirantal merged commit 99a8168 into meanjs:0.4.0 Jul 29, 2015

codydaig mentioned this pull request Aug 2, 2015

Implement a returnUrl service to allow users to return to pages that require login #308

Closed

lirantal mentioned this pull request Aug 7, 2015

Has anybody implemented signin redirection to "previous page"? #185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable redirection to previous page after login #487

Enable redirection to previous page after login #487

igorauad commented Mar 22, 2015

ilanbiala Mar 22, 2015

igorauad Mar 22, 2015

ilanbiala Mar 22, 2015

igorauad Mar 22, 2015

ilanbiala Mar 22, 2015

igorauad Mar 22, 2015

ilanbiala Mar 22, 2015

igorauad Mar 22, 2015

ilanbiala Mar 22, 2015

igorauad Mar 22, 2015

lirantal commented Jul 16, 2015

igorauad commented Jul 17, 2015

lirantal commented Jul 18, 2015

ilanbiala Jul 18, 2015

igorauad Jul 19, 2015

rhutchison commented Jul 21, 2015

simison commented Jul 21, 2015

lirantal commented Jul 22, 2015

ilanbiala commented Jul 22, 2015

igorauad commented Jul 22, 2015

ilanbiala commented Jul 23, 2015

igorauad commented Jul 23, 2015

ilanbiala commented Jul 24, 2015

lirantal commented Jul 24, 2015

pgrodrigues Jul 24, 2015

ilanbiala Jul 24, 2015

simison Jul 24, 2015

pgrodrigues commented Jul 24, 2015

ilanbiala commented Jul 29, 2015

igorauad commented Jul 29, 2015

ilanbiala commented Jul 29, 2015

lirantal commented Jul 29, 2015

igorauad commented Jul 29, 2015

lirantal commented Jul 29, 2015

Enable redirection to previous page after login #487

Enable redirection to previous page after login #487

Conversation

igorauad commented Mar 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lirantal commented Jul 16, 2015

igorauad commented Jul 17, 2015

lirantal commented Jul 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhutchison commented Jul 21, 2015

simison commented Jul 21, 2015

lirantal commented Jul 22, 2015

ilanbiala commented Jul 22, 2015

igorauad commented Jul 22, 2015

ilanbiala commented Jul 23, 2015

igorauad commented Jul 23, 2015

ilanbiala commented Jul 24, 2015

lirantal commented Jul 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrodrigues commented Jul 24, 2015

ilanbiala commented Jul 29, 2015

igorauad commented Jul 29, 2015

ilanbiala commented Jul 29, 2015

lirantal commented Jul 29, 2015

igorauad commented Jul 29, 2015

lirantal commented Jul 29, 2015