Refactor: Maybe remove function `hasPermission` #213

New issue

Open

opened 2022-08-20 18:01:43 +00:00 by ilja · 2 comments

ilja commented

2022-08-20 18:01:43 +00:00

Member

src/permission.js has a function

function hasPermission(roles, permissionRoles) {
  if (roles.indexOf('admin') >= 0) return true // admin permission passed directly
  if (!permissionRoles) return true
  return roles.some(role => permissionRoles.indexOf(role) >= 0)
}

In practice this seems to always return true because the only place I see the function being called has to.meta.roles for permissionRoles. And none of the routes have meta.roles.

AFAICT this is used to return 401 for calls to the BE that a user shouldn't be able to do. But imo it should be up to the BE to determine what is allowed or not, not the FE.

So basically, I believe this function and everything that uses it and the 401 routes can/should all be removed.

`src/permission.js` has a function ```javascript function hasPermission(roles, permissionRoles) { if (roles.indexOf('admin') >= 0) return true // admin permission passed directly if (!permissionRoles) return true return roles.some(role => permissionRoles.indexOf(role) >= 0) } ``` In practice this seems to always return true because the only place I see the function being called has `to.meta.roles` for `permissionRoles`. And none of the routes have `meta.roles`. AFAICT this is used to return 401 for calls to the BE that a user shouldn't be able to do. But imo it should be up to the BE to determine what is allowed or not, not the FE. So basically, I believe this function and everything that uses it and the 401 routes can/should all be removed.

lanodan commented

2022-08-22 12:17:52 +00:00

Owner

But imo it should be up to the BE to determine what is allowed or not, not the FE.

I'm not sure on this, I think the user experience would be quite confusing if the FE wouldn't at the very least grey-out options that BE wouldn't allow.
Specially as the interactivity on AdminFE is horribly poor so I wouldn't be surprised to see a massive bug like "BE said no but there is no visual feedback", which would be even more massive if it's not just some elements here and there.

Of course there can be some synchronisation issues in the mapping of the permissions but maybe it's the kind of stuff where Swagger/OpenAPI could be reused (importing the spec at dev/build-time but well fetching it at runtime could be tried as well).

> But imo it should be up to the BE to determine what is allowed or not, not the FE. I'm not sure on this, I think the user experience would be quite confusing if the FE wouldn't at the very least grey-out options that BE wouldn't allow. Specially as the interactivity on AdminFE is horribly poor so I wouldn't be surprised to see a massive bug like "BE said no but there is no visual feedback", which would be even more massive if it's not just some elements here and there. Of course there can be some synchronisation issues in the mapping of the permissions but maybe it's the kind of stuff where Swagger/OpenAPI could be reused (importing the spec at dev/build-time but well fetching it at runtime could be tried as well).

ilja commented

2022-08-22 18:04:44 +00:00

Author

Member

I think the user experience would be quite confusing if the FE wouldn't at the very least grey-out options that BE wouldn't allow

Yeah I agree, but that's not what this does. Here the routes are broken, that's it. So you still see the options, but then they don't work. (Which is already the case even without this logic)

I wouldn't be surprised to see a massive bug like "BE said no but there is no visual feedback"

You do get visual feedback from that. Example:

The worst part is that the whole thing isn't even used. The function is called, but in practice it always returns true, so it's completely useless. It kinda feels like it was added "just in case we need it later". But also assuming access would always be done purely on roles, which isn't the case any more. And even if you do want to reuse the function somehow, it's too limited in what it handles (i.e. it doesn't hide or grey out options).

Idk, maybe I'll remove it later on in #441 but we'll see.

> I think the user experience would be quite confusing if the FE wouldn't at the very least grey-out options that BE wouldn't allow Yeah I agree, but that's not what this does. Here the routes are broken, that's it. So you still see the options, but then they don't work. (Which is already the case even without this logic) > I wouldn't be surprised to see a massive bug like "BE said no but there is no visual feedback" You do get visual feedback from that. Example: ![Screenshot_20220822_195519](/attachments/66f8aec2-e140-42d4-b99e-281feaa9d9c6) The worst part is that the whole thing isn't even used. The function is called, but in practice it always returns true, so it's completely useless. It kinda feels like it was added "just in case we need it later". But also assuming access would always be done purely on roles, which isn't the case any more. And even if you do want to reuse the function somehow, it's too limited in what it handles (i.e. it doesn't hide or grey out options). Idk, maybe I'll remove it later on in https://git.pleroma.social/pleroma/admin-fe/pulls/441 but we'll see.

Screenshot_20220822_195519.png

5.7 KiB