Admin Checks

[osieorb18]

I've had a few add-ons in which there's been checking for admin permissions to decide if a user can see/access something, by just checking if "is_admin" is true with an if statement, or something to that effect. Given that on some sites, mine included, there are sometimes staff who have admin permissions for the partial sake of Reactions, Trophies, etc, this is a remarkably inconvenient trend. Most of the time, it feels like a way to get around coding in an actual group permissions check. I have yet to delve into exactly what the code looks like for adjusting of group permissions, but is it a lot of work to include that by default? (My guess would be no based on the prevalence of apps that do this correctly, but I'm more than open to being proven wrong.) It would certainly make things a lot easier in several circumstances for site owners such as myself.

 

[Paul B]

Using a basic if check is lazy and inaccurate coding.

If there is an actual permission involved then a full permission check should be done.

I don't know which add-ons you are referring to, but you should feed back to the author in the first instance.

 

[osieorb18]

Using a basic if check is lazy and inaccurate coding.

If there is an actual permission involved then a full permission check should be done.

I son't know which add-ons you are referring to, but you should feed back to the author in the first instance.

How would the full permission check usually be done?

 

[osieorb18]

Not to call out a specific person, but I spent several hours messing with the code for Snog's Private Threads, as the first of several that comes to mind that use this sort of permission system.

 

[Ozzy47]

You can post your findings in the addon thread and I will look into it.

 

[osieorb18]

Just to be clear, this isn't only the Private Threads app. This has happened in the past with 1-2 add-ons, and recently with another one.

 

[Paul B]

You should always feed back to developers in the respective add-on threads.

That's the only way your issues will get addressed.

 

[Ozzy47]

As far as the Private Threads addon, it’s expected and intended behavior to check for “is_admin”

 

[Lawrence]

I've had a few add-ons in which there's been checking for admin permissions to decide if a user can see/access something, by just checking if "is_admin" is true with an if statement, or something to that effect. Given that on some sites, mine included, there are sometimes staff who have admin permissions for the partial sake of Reactions, Trophies, etc, this is a remarkably inconvenient trend. Most of the time, it feels like a way to get around coding in an actual group permissions check. I have yet to delve into exactly what the code looks like for adjusting of group permissions, but is it a lot of work to include that by default? (My guess would be no based on the prevalence of apps that do this correctly, but I'm more than open to being proven wrong.) It would certainly make things a lot easier in several circumstances for site owners such as myself.

XF developers put that field in for a reason (and as well is_moderator). Some of my add-ons uses both, just makes sense to do so. For example, my Conversation Tools add-on adds a blind carbon copy feature. Who can be BCC'd is set by an option: Admins, Admins & Moderators, or any valid member. Those two fields are used in the query to retrieve a list of Admins and Moderators (if required), and also for a sanity check (permission based).

If you feel that is_admin and is_moderator should be removed from the user table to force hardened permission checks and isMemberOf usergroup checks you should create a suggestion. But before you do, note that XF uses is_admin in areas as well, for example: admin controller, upgrades, imports, user criteria, and in various entities (user, user ban, etc.).

In my add-ons, and I am sure in other developers add-ons as well, is_admin is used as it is intended to be used, and no changes to my add-ons that use that check will be made.

edit: damn typos.

 

[Ozzy47]

XF developers put that field in for a reason (and as well is_moderator). Some of my add-ons uses both, just makes since to do so. For example, my Conversation Tools add-on adds a blind carbon copy feature. Who can be BCC'd is set by an option: Admins, Admins & Moderators, or any valid member. Those two fields are used in the query to retrieve a list of Admins and Moderators (if required), and also for a a sanity check (permission based).

If you feel that is_admin and is_moderator should be removed from the user table to force hardened permission checks and isMemberOf usergroup checks you should create a suggestion. But before you do, note that XF uses is_admin in areas as well, for example: admin controller, upgrades, imports, user criteria, and in various entities (user, user ban, etc.).

In my add-ons, and I am sure in other developers add-ons as well, is_admin is used as it is intended to be used, and no changes to my add-ons that use that check will be made.

I agree wholeheartedly. I don’t find it “lazy and inaccurate coding“ as previously suggested.

 

[Paul B]

An if check for a general admin check is fine.

If it relates to a specific permission which not all admins should have access to just because they have admin status, then yes, it's inaccurate.

The same applies to moderators - not all moderators have access to all moderation functions just because they have moderator status.

 

[Ozzy47]

For security purposes the addon needs all admins to be able to view the private threads.

But sure, I can see an addon using is_admin instead of proper permissions check and that being wrong.

 

[Paul B]

I'm not commenting on any particular add-on - the OP didn't mention it in their original post.

As has been posted, using an if check is perfectly valid in some situations.

I was just pointing out that it's not the correct application in all cases.

 

[osieorb18]

For security purposes the addon needs all admins to be able to view the private threads.

Just to use this as an example... For security purposes, do all admins need to be able to look at any Conversation, too? Because as far as I'm concerned, Private Threads and Conversations are roughly the same concept in terms of usage.

 

[Ozzy47]

That’s your call, not mine.