fix(core): Client routes guest access bug #1100
Conversation
| @@ -16,7 +16,7 @@ angular.module(ApplicationConfiguration.applicationModuleName).run(function ($ro | |||
|
|
|||
| // Check authentication before changing state | |||
| $rootScope.$on('$stateChangeStart', function (event, toState, toParams, fromState, fromParams) { | |||
| if (toState.data && toState.data.roles && toState.data.roles.length > 0) { | |||
| if (toState.data && toState.data.roles && toState.data.roles.length > 0 && toState.data.roles.indexOf('guest') === -1) { | |||
There was a problem hiding this comment.
Can this be moved to inside the forEach loop instead of the if statement?
I'm not a fan of iterating over an array to check if we need to iterate over an array.
There was a problem hiding this comment.
@codydaig Sure. I originally had this check in the forEach loop. Then thought it would be more appropriate here; since it doesn't really make sense to go any further, if the state allows guest access.
I'll move it back down though. Just wanted to point out my reasoning here. Thanks for the comment.
|
@codydaig I've addressed your line comment. |
mleanos
force-pushed
the
bugfix/ClientRoutes-Guest-Access
branch
from
f63e9e7 to
a560351
Compare
mleanos
changed the title
|
@mleanos Just to clarify: If the state it is transitioning too has the role 'guest' it doesn't check authentication anymore. Correct? |
|
Well, it does check since the auth check is on the left-side of the In its its current state, this doesn`t enforce the authentication. |
|
@codydaig Did my last comment address what you were after? |
|
LGTM |
| @@ -19,7 +19,7 @@ angular.module(ApplicationConfiguration.applicationModuleName).run(function ($ro | |||
| if (toState.data && toState.data.roles && toState.data.roles.length > 0) { | |||
| var allowed = false; | |||
| toState.data.roles.forEach(function (role) { | |||
| if (Authentication.user.roles !== undefined && Authentication.user.roles.indexOf(role) !== -1) { | |||
| if ((Authentication.user && Authentication.user.roles !== undefined && Authentication.user.roles.indexOf(role) !== -1) || (role === 'guest')) { | |||
There was a problem hiding this comment.
if this needs to be added, I would swap your logic, putting the role === 'guest' check first.
There was a problem hiding this comment.
I was waiting for clarification on that. It's probably more intuitive to have the guest role check first.
There was a problem hiding this comment.
What do you mean by "if this needs to be added"? Do you have another option, or suggestion?
There was a problem hiding this comment.
I was waiting for clarification on that. It's probably more intuitive to have the guest role check first.
and probably more optimized.
What do you mean by "if this needs to be added"? Do you have another option, or suggestion?
I haven't tested or know what issue you're fixing - just added my comment.
There was a problem hiding this comment.
Ah ok. Just making sure ;) And yea, def more optimized. I'll go ahead amd get that changed.
Adds a check for the existence of the "guest" role in the state configuration that we're transitioning to, in the core $stateChangeStart event handler. If it exists, then we allow access. Also, added validation of Authentication.user object. While writing tests, I ran into an issue here when the Authentication service wasn't injected into a controller. Probably best to have this check in place. Fixes meanjs#1098
mleanos
force-pushed
the
bugfix/ClientRoutes-Guest-Access
branch
from
a560351 to
bfcfb55
Compare
|
LGTM |
fix(core): Client routes guest access bug
|
Thanks guys! |
Adds a check for the existence of the "guest" role in the client route
state configuration of
data.rolesin the $stateChangeStart of coreclient app init configuration.
If this role is present then it doesn't attempt to check authentication
on the route that is being transitioned to.
Fixes #1098