• Qwerty (unregistered)

    That programmer will get no defense from me! Clearly he or she should have written it this way:

    GetUserPrivileges = UBound(Split(UserPriv, ","))+1

  • Barry Etter (unregistered)

    Ha, ha. Maybe the orginial design meetings with the users went like this:

    DEV: How many security roles do you need?
    USER: Three.
    DEV: Will you ever add roles?
    USER: No.
    DEV: Never ever?
    USER: No, just write the damn thing.
    DEV: You got it.

    I know because I've been the DEV in such a meeting!

  • TheF0o1 (unregistered)

    Now, why would you assume that commas always mean "comma-delimited"? Sometimes, commas want to fit in just like any other character,digit,punctuation. They're tired of getting preferential,special,unusual treatment.

    Besides, they could've just used "MANAGER", but that's not nearly as descriptive,informative,clear as "USER,SUPERVISOR,MANAGER", is it? How else do you know that a MANAGER is also a USER and a SUPERVISOR? However, maybe they should've used "USER, SUPERVISOR, AND MANAGER", just to be sure,certain,safe.

  • Ron (unregistered)

    That's awesome. I totally wish every website trusted the cookies I supply as much as this one does.

  • Mathew Nolton (unregistered)

    ahhhhh......too funny.

    it's amazing. i work as a consultant for a living and all too often high-level managers thinks you can swap consultants in and out without regard for quality and also try to get people to compete on price alone....ohhh how wrong.

  • blameMike (unregistered)

    That's Hot! LMAO.

  • Mark Mullin (unregistered)

    Oh, I just can't resist - note that my defense is not an endorsement, just for amusement :-)

    OK, there's a user

    Bit 0 = user bit

    Then there's the user who's a supervisor - that is, they're a supervisory user :-)

    Bit 0 = false, they are not a user
    Bit 1 = true, they are a supervisor

    Then there's the phb (manager), who's merely blundering around the scene, but gets hurt and refuses to sign PO's if he doesn't feel important enough

    Bit 0 = true so they can play solitaire
    Bit 1 = true so they can edit the timeline in the project schedule to satisfy their phb

    :-)

  • Patrick Sullivan (unregistered)

    i'm at a loss for words..really I am


    no really.. how could....what


    aww forget it!!

  • Patrick Sullivan (unregistered)

    Oh I found the words!

    WTF!!!!!!!!

  • Siva Mateti (unregistered)

    Amazing!

  • Peter Ibbotson (unregistered)

    You've got me stumped on this one. Is this the same codebase that had the yes/no/maybe function?
    I do have a defence for why the code does this (Barry Etter kinda covers it), but I can't actually defend the function itself.

  • Greg (unregistered)

    This made me laugh so hard I snorted... (which made people around me give me that "look" :)

  • Jason Mauss (unregistered)

    wow, insane.

    I think the first comment mentioned this but dang... use the split function with the comma delimiter and throw the results into an array and just loop from 0 to ubound - 1 to find all roles.

    so, sooo sad.

  • mumbo jumbo (unregistered)

    Well, at least he used UCASE rather than having addition if / then conditions for all cases.

  • Patrick Sullivan (unregistered)

    I also love the cookie key name cur_user_role

    I know this is a bit nit picky but wouldn't the cookie always be for the current user?

  • Corprew Reed (unregistered)

    Wow, I don't really give a rat's ass about how the code is written, but did the person in question seriously add new roles to the database just assuming he knew how they were used and didn't actually look?

    Irrespective of whether the code is good or bad, that's a pretty serious failure in practice.

  • Tony (unregistered)

    Why is anyone even mentioning the split function.

    This code is wrong on so many levels it is pathetic.

    If you are using a combined security role methodology use Bit Wise Logic.
    Bit1: USER

    Bit2: SUPERVISOR
    Bit3: MANAGER

    If this approach is too complicated (your in the wrong profession) for you use a Cross Reference table with a type table of rows.

    Finally, FOR GOD SAKE DON’T HARD CODE DATABASE TEXT VALUES IN YOUR CODE!!!

    This code made me nauseous.

  • Jason Mauss (unregistered)

    Tony - even if you use the "Bit Wise Logic" you're speaking of - you're still hard coding the roles you're looking for, are you not? And, it's not a very good solution for adding roles later.

    For the overall "solution", IMO, it should be based on priviledges, not roles. Allow users/admins/programmers to create as many roles as they'd like and just assign certain permissions/priviledges to the role. Then you can add roles ad nauseum.

  • [email protected] (Hassan Voyeau) (unregistered)

    I always look forward to defending some poor soul but this time I just can't. The only I would say is that Mr. Cartwright should not have assumed that he could just go in and change the roles without understanding how they are used.

  • That Guy (unregistered)

    I don't see what's wrong with this code! Do any of you bitches have the spec?
    /sarcasm

  • sproc (unregistered)

    The worst thing is not the code, but the fact that the new developer didn't know he broke everything before releasing it to QA?

    Test your code yourself before making QA deal with it!

    Have you no professional pride?!

    BTW, yes the code sucks, too. But that can be fixed more easily, and at much less cost to the company, than fixing a developer who doesn't test his own code.

  • Tim Cartwright (unregistered)

    sproc, if you read the post carefully, the new code was in a NEW application, so I did not regression test the legacy application, I did NOT have time. I always test my code sproc. I tested the new application and it worked fine.

  • Miles Archer (unregistered)

    What about the poor DBA who puts in roles like:
    USER,MANAGER,SUPERVISOR

    or puts a space between the commas. Even if, as Barry Etter suggests, the user forced the developer to onlyh support 3 roles, it's not particularly robust.

    (but I agree you should have tested your fix...)

  • Tim Cartwright (unregistered)

    "I always look forward to defending some poor soul but this time I just can't.

    the only I would say is that Mr. Cartwright should not have assumed that he

    could just go in and change the roles without understanding how they are used. "


    Hassan, In my defense I made the horrible assumption that the code that parsed

    the users permissions in the legacy application would have been written in such

    a manner that it would handle the roles in ANY order, and with new roles added.

  • Tim Cartwright (unregistered) 
    One more comment about me not testing. I did test the legacy application,


    but the legacy application seemed to work fine to me. I was new to working on
    the legacy application, having inherited it. I did not know that the role based
    security was solely for hiding / showing certain features. With my lack of
    experience with the application I did not see any failure(s). The application
    performed. It just did not show any of the special capabilities that managers
    and supervisors can do. When I released to QA, they inquired where the other
    menu items had disappeared to, hence my finding the WTF.

    The legacy application was written in classic ASP, and the new application
    was written in C# (winform), so they shared no code, just the database.

  • Shambo (unregistered)

    The developer doth protest too much, methinks.

  • Tim Cartwright (unregistered)

    LMAOROTF @ Shambo

    Shambo == co-worker == SMARTA55

  • Tim Cartwright (unregistered)

    "Is this the same codebase that had the yes/no/maybe function?"

    YES

  • I am not gonna ruin my family name (unregistered)

    Whats wrong with the code? I mean, it works, its "fast", no spliting or anything, just flat out binary comparison of strings/ if idiot wants more roles, just hardcode them into the if, and let some other fool that gets the project fix it later.

    Just get your bucks out of it. Lol

  • [email protected] (FYK) (unregistered)

    Argggh. Irremissible. ... Lovely many2many relationship solution ;)

  • t-man (unregistered)

    The logic here is obvious.

    Its based on the number word thingys i.e. if the ROLE is USER - there is 1 word thingy, if its USER,SUPERVISOR there are 2 word thingys, and (obviously)if its USER,SUPERVISOR,MANAGER then there are 3 word thingys.

    It was only when the new developer tried to introduce the illogic code when things broke!

  • Steve (unregistered)

    Doesn't the fact that the role is being read from a clear text cookie bother anyone?
    The rest just makes it stupid, but that makes it insecure.

  • Jason Hasner (unregistered)

    Tim, I feel your pain.

  • John Carter (unregistered)

    Crisp, clean, elegant. I think I will copy and paste the code for my future reference.

  • Tim Smith (unregistered)

    I have to agree. As much as I like pointing out how some insane code is actually reasonable in the context, this code is just stupid.

  • R.S.S. (unregistered)

    The code could well be a good solution to the needs at the time of it's development. It is reasonable to assume that it:
    (a) met the immediate needs of 3 levels of security, and
    (b) used a syntax for the data which could be naturally extended in various ways, and
    (c) did not expend resources developing mechanisms that were not, and might never be, needed.

    The only thing indefensible seems to be changing the range of values (eg "IT") without looking to see what the effect of such values might be.

  • simon (unregistered)

    this code is already wonderful, but it could be optimised to:

    getUserPrivileges=1 + number of commas

  • Centaur (unregistered)

    Observe how the original developer used the LeastPrivilegeByDefault pattern:

    > GetUserPrivileges = 0 'init the return

    That’s pretty considerate, if you ask me :) Of course, everything else is a mess, and if the “orginial design meetings” went like Barry Etter described, the guy should have just hardcoded 0/1/2/3 into the database, doing the simplest possible thing, or alternatively he should have made it right up front, with proper many-to-many-to-many user/group/privilege relationships.

  • Simon 'You Bitches' (unregistered)

    It does exactly what was asked. Really badly.


    More than the dreadful text descriptions of the permissions, what I loved most was storing the values in cookies, please tell me this is an intranet application or winforms and not public facing. If it is whats the URL ?


    BTW.

    If any of my colleagues had written code like the yes/no/maybe routines from the other day, they would have got a severe kicking, and had to have the "plastic cabbage" displayed on their monitors to let everyone know how stupid they are.



  • RJ (unregistered)

    And where can these plastic cabbages be obtained? ;)

  • riviera (unregistered)

    There's nothing wrong with this -- you just have to be very careful what you write in the actual table fields ;;;)))))

    (sorry, couldn't resist to say)

  • Alex Papadimoulis (unregistered)

    This comment may just qualify for a post of it's own :-D

    "The only thing indefensible seems to be changing the range of values (eg "IT") without looking to see what the effect of such values might be."

    That has to be sarcasm ... it just has to be.

  • User("Reagan","USER,SUPERVISOR,MANA (unregistered)

    What a great site - you rock Alex!

  • Eric Hodel (unregistered)

    Bitwise logic doesn't work with the legacy app at all:

    USER == 0b1
    USER, SUPERVISOR == 0b10
    USER, SUPERVISOR, MANAGER == 0b11

    You need three bits to have three independent roles.

  • DrFooMod2 (unregistered)

    I have some code that stores user roles in the DB the same way. However, this is how I read them back out:

    usr.Role = (UserRoles)Enum.Parse(typeof(UserRoles), r["Role"].ToString());

    and UserRoles is an enum with the [Flags] attribute. Works well for me.

  • sproc (unregistered)

    I understand you tested the new app, but if the legacy app is still being supported, you must test it, too. From your later comment, it sounds like you did some testing, but without knowing exact how it was supposed to work, you didn't catch the fact that you introduced a regression defect.

    Since the chances are nil that the previous developer created a good set of automated unit tests for you to run, I guess you did what you could.

    Please don't read too much into my choice of phrases in my posts. There is an implied ";-)" at the end of all of them, especially things like "Have you no professional pride?!" Just mild ribbing, one developer to another. No intention to pillory anybody.

    You are obviously a decent programmer, and I certainly didn't mean to offend or discourage you from posting more things here. Your contributions (or should I say the contributions of your predecessors) are hilarious!

    But it wouldn't hurt to go write a nice suite of automated unit tests anyway. ;-) <-- smiley added explicitly, just this once.

    Carry on.

  • Tim Cartwright (unregistered)

    I like Nunit... :) Test before code!, or death before dishonor!
    http://nunit.org/ Live it, Love it. And I have taken no offense to anything
    anyone said here. I was just trying to explain the situation better. The legacy
    app has over 100 asp pages, and is pure spaghetti, so regression testing from my
    point of view was overly time consuming, and was not budgeted. Yea, Yea, I know,
    excuses are like......... ;-D So, no sweat sproc.....

  • Tim Cartwright (unregistered)

    DrFooMod2, in my C# app, I extend Iprincipal to encapsulate the roles, take a gander at this :

    http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dnnetsec/html/SecNetHT06.asp

  • Peter Ibbotson (unregistered)

    Thanks for confirming it's the same code base as yes/no/maybe.

Leave a comment on “GetUserPrivileges()”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article