-
Notifications
You must be signed in to change notification settings - Fork 41
Fix entitlement messages #130
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
Conversation
tbouron
left a comment
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.
Thanks @jcabrerizo, just have one small comment but LGTM otherwise.
Can be merged after that.
| vm.entity.name = vm.name; | ||
| }).catch((error)=> { | ||
| brSnackbar.create('Cannot update entity name: the entity [' + entityId + '] is undefined'); | ||
| if(error.data && error.data.error === 403){ |
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.
I don't think you need this change as 403 errors are now handled by the Java securityproviser you did @jcabrerizo ;)
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.
The security provider manages the authentication, but an authenticated user can try execute actions that need an entitlement that s/he hasn't. In this case, the exception thrown send an 403 as status. I tried to left the previous behavior and also manage the unauthorized action. If it doesn't make sense for you @tbouron i can change it.
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.
Good point about the REST API. But using your argument, it should then be applied to all endpoints, not just this one. I think it can be done globally though, like an interceptor of some kind, but it is outside on the scope of this PR IMO.
So I would remove it for now, but create a JIRA ticket to track this task. How does it sound @jcabrerizo ?
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.
I've changed the get the message from the error object
tbouron
left a comment
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.
Awesome, thanks @jcabrerizo 🎉
Now shows the messages from the entitlement control system