Unauthorized client side routing #796

trainerbill · 2015-08-13T17:46:45Z

Reintroducing to master branch. closing #727

@ilanbiala @lirantal @rhutchison @codydaig

ilanbiala · 2015-08-16T13:09:45Z

modules/core/client/config/core.client.routes.js

+        templateUrl: 'modules/core/views/400.client.view.html'
+      })
+      .state('unauthorized', {
+				url: '/unauthorized',


indentation

trainerbill · 2015-08-17T14:18:59Z

@ilanbiala Not that I am looking at this, I don't think we should do interceptors for bad request on the client side.... Server side is throwing them quite a bit, even for things like article is not found. Guessing we don't want a redirect on these errors... Or do we?

lirantal · 2015-08-17T21:23:43Z

@trainerbill can you elaborate more on the requirement/purpose of this PR? I'm not really following what's it about.

rhutchison · 2015-08-18T05:52:20Z

Server side is throwing them quite a bit, even for things like article is not found. Guessing we don't want a redirect on these errors... Or do we?

@trainerbill yes, this is the intention. 404 should go use $state.transitionTo on the client to go to the 404 page. This will all make better sense when resolves are used properly.

I am not a fan of having the pub/sub implementation here, I don't see the value of doing it this way. The only thing I can think of is you tried implementing state.go in the http interceptor and ran in to the circular reference issue. You need to use $injector like so, $injector.get('$state').transitionTo('bad-request');

trainerbill · 2015-08-18T15:17:54Z

@lirantal HTTP interceptors will do certain actions if the server responds with certain error codes. So for like an unauthorized 401 response I created a route so that the client will redirect to the route so the user knows they are unauthorized. This will come in handy when a user views an article then adds /edit to the end of the URL of an article that they don't own. I suggest pulling the branch and checking it out. Login with a user account and try to go to /admin/users. View an article the user doesn't own and add a /edit to the URL and try and edit the article. You will go to the unauthorized route.

@rhutchison The example you gave me was based on pub/sub. I actually like the idea. So then in the module controllers we can broadcast the unauth event and it does the same thing every time for certain events. $state couldn't be injected into config so that is why I went with pub/sub but after doing it I kinda like it. As a framework/middleware we should really have events like this to make it easier on the modules. Just do a $rootScope.broadcast(events) and we handle everything for the modules.

As for 404, the only reason I don't like it is because the backend throws them a lot. I don't think that redirecting to a bad-request route is a better option then just displaying the error on the client since the bad request error message is variable. Then the user has to hit the back button to get back to their previous state.

trainerbill · 2015-08-18T22:09:27Z

@rhutchison Removed Modal, Removed Pub/Sub, changed unauthorized to forbidded to align with the http status code. 401 goes to authentication 403 goes to forbidden.

rhutchison · 2015-08-18T23:30:45Z

LGTM - thank you.

trainerbill · 2015-08-19T00:21:09Z

@lirantal. This is ready to merge

lirantal · 2015-08-20T06:16:52Z

@ilanbiala ?

mleanos · 2015-08-20T12:05:37Z

@trainerbill I pulled this branch and tested. A few things I noticed... Forgive me if you're aware of these issues..

When not signed in, I go to an invalid route. I'm redirected to the Bad Request state. Now if sign in successfully, I'm redirected back to the Bad Request state.

If a 400 status is returned from the server, it redirects to the Bad Request, even if the 400 was caused by an invalid form submission; for instance, incorrect credentials at login.

I saw mention of the issue where the user can add /edit to an article they are viewing & get to that state. However, they are correctly sent to the Forbidden state if they attempt to submit the form. What is the plan with addressing that? Wouldn't we want to just avoid allowing that state transition in the first place? Perhaps, that's outside the scope of this PR.

@trainerbill Overall this looks really good. Thanks.

trainerbill · 2015-08-20T15:06:53Z

@mleanos Thanks for taking it for a test drive. I was actually worried on the bad request 400 interceptor as the backend seems to throw them quite a bit. I am going to remove the 400 interceptor just to get the PR in. The forbidden route and interceptor is what I really want in. @rhutchison any issue with this?

The /edit route will be taken care of by the article refactor PR #800

rhutchison · 2015-08-20T22:54:06Z

modules/core/client/services/interceptors/auth.interceptor.client.service.js

@@ -0,0 +1,23 @@
+'use strict';
+
+//Articles service used for communicating with the articles REST endpoints


remove unrelated comment

rhutchison · 2015-08-20T23:17:00Z

modules/core/client/app/init.js

-    $locationProvider.html5Mode(true).hashPrefix('!');
-  }
+angular.module(ApplicationConfiguration.applicationModuleName).config(['$locationProvider', '$httpProvider',
+function ($locationProvider, $httpProvider) {


fix indentation

rhutchison · 2015-08-20T23:37:41Z

LGTM thanks @trainerbill

mleanos · 2015-08-21T00:16:01Z

@trainerbill I see the 400 is removed, so the issue with the invalid form submission is resolved. Thanks.

I'm still seeing that when I get sent to the not-found state, and then login I'm getting redirected back to the not-found state. Were you waiting to handle that in a separate PR?

Other than that everything looks good to me.

~~removed part of comment as it was unrelated to this PR~~

This LGTM, and appears to be ready for merging.

codydaig · 2015-08-21T22:18:17Z

LGTM

rhutchison · 2015-08-23T07:21:00Z

@mleanos @trainerbill @codydaig Please review #843

I'm still seeing that when I get sent to the not-found state, and then login I'm getting redirected back to the not-found state. Were you waiting to handle that in a separate PR?

trainerbill · 2015-08-24T18:22:31Z

Don't merge yet. Writing tests

trainerbill · 2015-08-24T20:13:34Z

@lirantal @rhutchison Ready to merge

Added Auth Interceptor tests cleaned up test Update routes

rhutchison · 2015-08-25T01:46:44Z

LGTM

Unauthorized client side routing

trainerbill mentioned this pull request Aug 13, 2015

Unauthorized route #727

Closed

lirantal assigned ilanbiala Aug 13, 2015

lirantal added this to the 0.4.x milestone Aug 13, 2015

lirantal added the enhance:feature label Aug 13, 2015

ilanbiala reviewed Aug 16, 2015
View reviewed changes

trainerbill force-pushed the UnauthorizedRoute2 branch from 4b1eac7 to d8eec6f Compare August 18, 2015 22:25

rhutchison mentioned this pull request Aug 20, 2015

0.4.1 Release #833

Closed

15 tasks

trainerbill force-pushed the UnauthorizedRoute2 branch from 55164b3 to a873414 Compare August 20, 2015 22:48

rhutchison reviewed Aug 20, 2015
View reviewed changes

trainerbill force-pushed the UnauthorizedRoute2 branch from 2f59044 to 19c1c59 Compare August 20, 2015 22:57

rhutchison reviewed Aug 20, 2015
View reviewed changes

trainerbill force-pushed the UnauthorizedRoute2 branch from 28ec9a7 to 3258163 Compare August 20, 2015 23:35

trainerbill closed this Aug 24, 2015

trainerbill force-pushed the UnauthorizedRoute2 branch from d3c3f30 to 979c4e5 Compare August 24, 2015 17:31

trainerbill reopened this Aug 24, 2015

trainerbill force-pushed the UnauthorizedRoute2 branch 2 times, most recently from 58572ce to 3bd74cd Compare August 24, 2015 20:04

Unauthorized client routing

ed89f9e

Added Auth Interceptor tests cleaned up test Update routes

trainerbill force-pushed the UnauthorizedRoute2 branch from 1ff8089 to ed89f9e Compare August 24, 2015 22:05

lirantal assigned lirantal and unassigned ilanbiala Aug 25, 2015

lirantal added a commit that referenced this pull request Aug 25, 2015

Merge pull request #796 from trainerbill/UnauthorizedRoute2

2b015b0

Unauthorized client side routing

lirantal merged commit 2b015b0 into meanjs:master Aug 25, 2015

ilanbiala modified the milestones: 0.4.1, 0.4.x Aug 31, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unauthorized client side routing #796

Unauthorized client side routing #796

trainerbill commented Aug 13, 2015

ilanbiala Aug 16, 2015

trainerbill commented Aug 17, 2015

lirantal commented Aug 17, 2015

rhutchison commented Aug 18, 2015

trainerbill commented Aug 18, 2015

trainerbill commented Aug 18, 2015

rhutchison commented Aug 18, 2015

trainerbill commented Aug 19, 2015

lirantal commented Aug 20, 2015

mleanos commented Aug 20, 2015

trainerbill commented Aug 20, 2015

rhutchison Aug 20, 2015

rhutchison Aug 20, 2015

rhutchison commented Aug 20, 2015

mleanos commented Aug 21, 2015

codydaig commented Aug 21, 2015

rhutchison commented Aug 23, 2015

trainerbill commented Aug 24, 2015

trainerbill commented Aug 24, 2015

rhutchison commented Aug 25, 2015

		@@ -0,0 +1,23 @@
		'use strict';

		//Articles service used for communicating with the articles REST endpoints

Unauthorized client side routing #796

Unauthorized client side routing #796

Conversation

trainerbill commented Aug 13, 2015

ilanbiala Aug 16, 2015

Choose a reason for hiding this comment

trainerbill commented Aug 17, 2015

lirantal commented Aug 17, 2015

rhutchison commented Aug 18, 2015

trainerbill commented Aug 18, 2015

trainerbill commented Aug 18, 2015

rhutchison commented Aug 18, 2015

trainerbill commented Aug 19, 2015

lirantal commented Aug 20, 2015

mleanos commented Aug 20, 2015

trainerbill commented Aug 20, 2015

rhutchison Aug 20, 2015

Choose a reason for hiding this comment

rhutchison Aug 20, 2015

Choose a reason for hiding this comment

rhutchison commented Aug 20, 2015

mleanos commented Aug 21, 2015

codydaig commented Aug 21, 2015

rhutchison commented Aug 23, 2015

trainerbill commented Aug 24, 2015

trainerbill commented Aug 24, 2015

rhutchison commented Aug 25, 2015