views:

41

answers:

1

I have users that fall into the following

  • Not logged in
  • Not Verified
  • Verified
  • Moderator
  • Admin

All code that only admin and moderators can access (like banning) is in ModeratorUser which inherits from verified which inherits from BaseUser. Some pages are accessible to all users such as public profiles. If a user is logged in he can leave a comment. To check this i use if (IsVerifiedUser). Now here is the problem. To avoid problems if a user is banned he is not recognized as a verified user. However in the rare case i need to know if he is verified i can use usertype & Verified.

Should i not be doing this? I have a bunch of code in my VerifiedUser class and find i am moving tons of it to BaseUser. Is this something i help because a not logged in user can access the page? Should i handle the ban user in a different way and allow IsVerifiedUser to be true even if the user is banned?

+2  A: 

At least in my opinion, most situations like this should be handled in data, not code. Hard-coding the fact that (for example) operation X can only be done by an administrator tends to be relatively brittle. Right now, you have five classes of users, but (just for example) you'll almost inevitably (somewhere along the line) end up inventing some other class of user, and have to re-organize quite a bit of code to fit (e.g. a new step halfway between moderator and admin, or perhaps a "restricted user" that's a step below a normal verified user, etc.) In fact, you're already basically running into that with your "banned" user who's mostly like an unverified user, but in a few ways like a verified user.

Having to rewrite the code every time you decide on a change like this is a poor idea. Instead, you should (probably) pre-define your five (or maybe six) user groups, and (for example) assign a bit to each. Likewise, assign a bit-mask to each function. To verify whether a given user can execute a given function, you AND those bitmasks together, and see whether the user has the appropriate bits set in their mask.

This makes it considerably easier to create new groups as needed, and/or change the assignments of rights to execute specific functions to groups of users. In particular, it allows changes in such rights to be done administratively rather than requiring a code rewrite.

Jerry Coffin
sounds interesting. Most of this is not 'hard' coded. As an example i have a send email function. I know only verified users and higher can do this so i prefer not to be able to call the function (compile time error) if i am a unverified user. In the case of banning the verified class throws a banned exception which is fine because calling anything in there is something i woulnt want to do if the user is banned.You just gave me a solution which i cant beleive i didnt think of nor would have thought of. Having banned user in between unverified and verified. (New comment formatting reasons)
acidzombie24
How would program 'send email' based on bit flags? I use to have every public backend function check the permission to see if the user qualifies. A friend suggested it may be better to have classes that thrown an exception and functions not visible through inheritance (Base cannot call verified function == less error prone). Would i be going back to the old way except checking bits instead of usertype? Would i still have inheritance or one big class like before? (with a few classes within).
acidzombie24
Great answer btw
acidzombie24
By "hard coded" I mean that (for example) the connection between "verified user" and "send email" is directly expressed in the code itself. If you decided to change the rules so somebody has to been a verified user for 30 days before they can send email, you'd have to modify the code and create a new class and move the "send email" member from the verified user class to the new class.
Jerry Coffin
Yes, what I'm suggesting sounds fairly close to how you did things before. I don't agree that trying to use types to represent privileges/rights is a good idea. As to how to organize the code, I'd probably think in terms of classes whose members are truly related, instead of being in the same class just because the same level of user happens to be able to do both. You'd then do your bitmask checking on a class-by-class basis, not a function by function basis.
Jerry Coffin
As an example (i dont have real numbers) I have 3 pages only mods can access and each page has 3 functions for it (so thats 9). I have 5 pages only verified users can access and average of 4 func per page (20). You are saying i should use a bitmask to check the permission but are you saying i should have these 29 functions in the same class? Lets say these functions are strictly for html like getListOfUsers(pageNumber), printReplySectionType1 (which has type1 divs arouind it calling comment.printReplySection) etc.
acidzombie24
These belong to the specific page and require many functions found i baseUser like useridToName. These 29 functions should go in the same class? I asked a question in the past about functions scoped at a file level but that doesnt exist. The next best thing it seems is to split it to (userType) permissions. I still have PrivateMessage pm; declared in verified user. So its still organized by class. How do i split these page specific functions to classes? Have a large one instead of split to (userType) permissions?
acidzombie24
No, I'm saying the functions should be in classes, but the classes should only include related functions, not just everything a particular class of user can do. For example, let's assume one of the mods-only pages has three functions dealing with list, adding, banning, etc. users. In that case, I'd have one class dealing with listing, adding, banning, etc., users, and one bit in the bitmask saying who can use that class. You might need (or want) finer granularity, such as allowing some people to read but not modify that data though, in which case you need more than one bit.
Jerry Coffin
I pretty much have that except taking PM as an example. The PM doesnt check if the user has permission, it just does it. It doesnt care or have knowledge of any users. It accepts a userid for author and recipient and knows how to insert or pull data to/from the database. I dont like the idea of having it check permissions. I have the callee class do that for it (in this case verified user) -edit- i do want my banned users to be able to -read- the pms and read only. But still have write access to delete pms but not send. ok new question. Using PMs as an example.
acidzombie24
new question. Using PMs as an example. How/Where do i check the permissions? Do i do it inside of that PM class or always outside? I still have 29 page. Should i get rid of all class types (VerifiedUser,BanedUser) and if so i should merge the 29+ (page specific) functions from those class into BaseUser class? (theres a userdata class which holds actual user information and permissions).
acidzombie24
This has been helpful enough to mark as accept ;)
acidzombie24