-
Notifications
You must be signed in to change notification settings - Fork 76
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
Prevent user without permissions from seeing service as a clickable in context selector #304
Conversation
@josemigallas screen shot? |
} | ||
|
||
.unauthorized { | ||
pointer-events: none; |
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.
Why not cursor: not-allowed
? It should be clear you can't interact
You can have cursor: not-allowed
in the <li>
, and pointer-events: none
in the<a>
.
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 idea, not exactly the implementation I did but I added a Gif showing this alternative.
|
||
if (menu === 'dashboard' && activeMenu === 'dashboard' || | ||
menu === 'audience' && (['buyers', 'finance', 'cms', 'site'].indexOf(activeMenu) !== -1) || | ||
api && (['serviceadmin', 'monitoring'].indexOf(activeMenu) !== -1) && api === currentApi.service.id) { | ||
menu === 'audience' && (['buyers', 'finance', 'cms', 'site'].indexOf(activeMenu) !== -1)) { |
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.
Maybe extract to an array
const menuNames = ['buyers', 'finance', 'cms', 'site'];
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 idea! This actually gives me some ideas for a better refactoring, I will address it in a different PR.
const { activeMenu, currentApi } = this.props | ||
let classNames = 'PopNavigation-link' | ||
|
||
if (['serviceadmin', 'monitoring'].indexOf(activeMenu) !== -1 && |
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.
Same here:
const servicesNames = ['serviceadmin', 'monitoring'];
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.
Read comment above.
What this PR does / why we need it:
Includes a new css class
unauthorized
that cancels all pointer events (disabling the hover effect and possible click events).This class is added to any service item in
ContextSelector
that doesn't have a defined fieldlink
(which means the user it's not authorized).Which issue(s) this PR fixes
THREESCALE-1502 Member provider without permissions to access the services can see them clickable in the top selector
Verification steps
Hard way: Having signed in with user that has limited access to some service, open ContextSelector in the top navigation bar and hover the mouse over that service. The item should neither change nor be clickable.
Easy way: Using Chrome's React extension (or similar) you can edit the props of ContextSelector and delete some link (i.e.
ContextSelector.apis[0].service.link = undefined
). This way you can emulate your user doesn't have enough permission.Alternatives
There's also
cursor: not allowed
property that gives more feedback: