ABTech / tracker

Carnegie Mellon Activities Board Technical Committee Tracker
abtech.org
22 stars 29 forks source link

Meta-ticket for roles rewriting #123

Closed tedgarb closed 10 years ago

tedgarb commented 10 years ago

This is a tracking ticket to list all other tickets that are dependent on roles/permissions being overhauled. This ticket should also serve as the official discussion thread for the roles/permissions rewrite.

Dependent Tickets:

122

tedgarb commented 10 years ago

The change to role-based access brings up a fundamental question: Who is allowed to assign a TiC (or any other run position) for an event? If anyone can make themselves a run position, then role-based control is effectively useless, as any motivated user will temporarily take a role for as long as it takes to edit the event.

I am not sure how to resolve this, but I propose the following rules as a starting point: The HoT (and his/her designates (exec? personnel manager?) can assign/remove run positions for an event. The TiC may add/remove people to run positions for a specific event. Someone with a run position can remove him/herself from a run position. A member who does not have permissions must get someone who does to give them a run position.

mmackie commented 10 years ago

HoT/Exec should be able to assign/remove run positions for an event. You could maybe make an argument for just the HoT/personnel manager having that privilege but--who cares? If you're on exec, we trust you to obey tech's 'rules'. If you can't, then we have bigger issues than you vandalizing an event (whatever that even means). Plus theoretically super TiCs are responsible for finding TiCs for a specific day and unless Ed/Kelly want to implement permissions based on what day of the week it is....

The TiC can then also add/remove run positions for a given event and run positions can remove themselves from events, but not change what run position they hold. Something I'd like to see but isn't really necessary is people can add their own assistants.

(something I wouldn't mind in general since I don't actually go on tracker all that much are email notifications--i.e. a notification to a specific subset of people if part of an event is updated. Hopefully with a very specific subject line so it's easy to filter)

tedgarb commented 10 years ago

WRT email notifications: To avoid ticket creep, please keep that discussion in its ticket, #3

hatkirby commented 10 years ago

With regards to per-user permissions, I was thinking of assigning a single role to each member from the following list, where the following roles build upon each other (each role has all of the permissions of the previous roles):

These can be combined/rearranged as necessary.

Event-based permissions are where it get more complicated. Personally, I think it makes sense to allow exec and all people with run positions to edit events, but I am not deeply familiar with the workflow for creating and editing events.

tedgarb commented 10 years ago

There should be one lower role: Suspended. This is for people who, for some reason, are no longer allowed to see any information or even log in to tracker, but their accounts are kept for the purpose of maintaining records like event roles.

mmackie commented 10 years ago

generally speaking, the HoT has been the one creating events, but for the sake of flexibility event creation should probably be an exec level permission since that will probably vary a little year to year. Ideally at this point the info we do know about the event (time, location, riders, etc) is added, but not always.

Run positions are generally figured out next. We've been doing a better job this year about picking TiCs first, but not always. Once the TiC is picked, they're responsible for finding run positions. For bigger events, things like call time (and obviously equipment) will change based off of light plot / sound complexity. Hopefully run positions will be uploading things like light plots or me paperwork.

So basically the roles would be:

personally, I'd be fine with combining run positions and TiC. In the end, tracker is limited to a small amount of people and we trust each other--the permissions levels don't need to be nearly as limiting as if tracker was open to the general public.

tedgarb commented 10 years ago

In the interest of flexibility/anticipating future growth, I would like to keep TiC/aTiC separate for permissions.

ghost commented 10 years ago

Event creation should definitely be an exec level permission. ==mmackie except for Hot/Exec--they should be able to add/remove anyone, not just TiC/aTiC.

One note about finances: Only "finance management" should be able to edit anything relating to finance. I would like to not give all of exec write permission by default--the repercussions for accidentally marking a JE as paid are way too high, and unless I'm missing something obvious, there is never any reason why someone who is not HoT or treasurer would create, edit, or delete a JE. Here is what I suggest:

Remove "finance management" from exec permissions; this is reserved for HoT and treasurer Add "finance view" to exec permissions:

hatkirby commented 10 years ago

I've created a role called "Treasurer" above "Membership Management" but below "Tracker Dev." To prevent role elevation, I'm only going to allow people to set peoples' roles to equal or lower than their own role--i.e. only Tracker Devs can make other people Tracker Devs, only Treasurers and above can make people Treasurers, and so on. Exec and lower have no access to modifying roles.

Also, default invoice status is Quote. Is "New" used at all or can it be removed?

(just to clarify, here is the current list of roles from most permissive to least permissive):

ghost commented 10 years ago

Here is my suggestion for an overall implementation based on the current/future structure of the exec board. Input requested from other exec.

Tier 1: Head of Tech, Tracker Dev

Tier 2: Treasurer (write access to finance (but not timecards/payroll)) Membership management (write access to members) Equipment management (write access to equipment) Secretary (write access to email (i.e. deleting email, if that could ever become a thing), locations, global attachments page)

Tier 3: Exec: Read-only access to all of the above (including pulling/filing email, but not deleting), global event editing permissions (excluding finance - discussed in my last comment)

Tier 4: General Member

Tier 5: Alumni Advisor (theoretically this has the same perms as alumni, but maybe it'd be good to keep them separate?) (AB Main chair might also go here?)

Tier 6: Suspended

hatkirby commented 10 years ago

How is this for the top three tiers?

mmackie commented 10 years ago

The thing I like about the way Kelly suggested is that it's a little more flexible if roles in exec are changed from year to year

ihartwig commented 10 years ago

Personally, I think that the currently model of all members being able to edit events is fine. I think we are all respectful in this organization and don't need to complicate the computer system. This would reduce the complexity of the system, and likely the frustration of the end user with not having the correct permissions to do something. What I would suggest instead is a changelog with user names attached that way we can hold people accountable if there is an issue.

I would apply the same system to exec/hot/tracker manager, except these accounts get pretty much full permissions. i. e. change roles of other users, invoices, setup timecards, delete/create events, etc.

I agree with Ed that a suspended status is important, especially if we start logging interactions with events and such.

The reason I'm suggesting not baking too many permissions restrictions into the system is that we generally don't have a great tracker dev working on the system. If exec decides to change up roles - which does happen - but they don't fit what is baked into the system, we end up in the situation we've been in for the last year of assigning pseudo-roles on tracker.

ghost commented 10 years ago

Payroll HAS to be separate. That's HoT-only no matter what. And billing/JEs need to be HoT/Treasurer only. The fallout from mistakes in those items is far too great. Aside from that, I don't disagree.

On Mon, Jan 27, 2014 at 12:41 AM, ihartwig notifications@github.com wrote:

Personally, I think that the currently model of all members being able to edit events is fine. I think we are all respectful in this organization and don't need to complicate the computer system. This would reduce the complexity of the system, and likely the frustration of the end user with not having the correct permissions to do something. What I would suggest instead is a changelog with user names attached that way we can hold people accountable if there is an issue.

I would apply the same system to exec/hot/tracker manager, except these accounts get pretty much full permissions. i. e. change roles of other users, invoices, setup timecards, delete/create events, etc.

I agree with Ed that a suspended status is important, especially if we start logging interactions with events and such.

The reason I'm suggesting not baking too many permissions restrictions into the system is that we generally don't have a great tracker dev working on the system. If exec decides to change up roles - which does happen - but they don't fit what is baked into the system, we end up in the situation we've been in for the last year of assigning pseudo-roles on tracker.

— Reply to this email directly or view it on GitHubhttps://github.com/ABTech/abtt/issues/123#issuecomment-33343239 .

Tim Reid SDG

tedgarb commented 10 years ago

I am not going to condone, support, or allow an access model based on the idea that people are inherently trustworthy. Under the current system, anyone with 6 events has the ability to do major damage to tracker before anyone can notice. While I have implemented a bit of a fallback by doing nightly database backups, this still relies on us discovering the attack within a set amount of time, which may not happen if the user is doing things like deleting old events. While I accept that the set of people who can be trusted with permissions like editing events may be larger (or just different) than the set of people who are on exec, I refuse to believe that we should trust anyone with 6 events with those powers. I accept that this will sometimes be frustrating. Not having the correct permissions to do things can be. However, that is a necessary evil in order to keep a more clear separation of roles and protect the data that tracker contains. The reason tracker has not had a consistent dev in recent years is because everyone talked a big game about rewriting the whole thing and making it sleek and sexy and now, while ignoring the fact that the existing system was falling apart and desperately in need of repair. Now that @hatkirby and I actually did the un-sexy work of making the system function again, and have put the system into a place where it can be easily updated, and has a regular schedule to do so, it is possible to make small changes like changing roles and permissions effectively on the fly by deploying a new version of the app, instead of being stuck waiting for one of the big talkers to actually do some work.

mmackie commented 10 years ago

Ed, I think you're over simplifying what Ian's saying. Allowing people to do things like edit events doesn't break tracker, but it does possibly expose us to data loss--all of which is recoverable (short of finance stuff, which they can't touch anyways). If somebody deletes a rider it takes about five seconds to re-upload it. Also there's a big difference between "can edit notes" and "can delete event". Regardless of what we end up with, a changelog is a really good idea--since it also helps with noticing accidental data loss, which is what we're most at risk for.

I still think that all of exec should have the same permissions levels for simplicity's sake, since exec is competent enough to know "don't touch that thing" and, again, if there's a changelog / email notifications of things it would be really easy to see 'hey ____ touched a thing they aren't supposed to be touching". But I also don't object to what Tim was suggesting as far as

Remove "finance management" from exec permissions; this is reserved for HoT and treasurer Add "finance view" to exec permissions:

This ties into an overall discussion about the structure of HoT / exec that's not appropriate to be having here, but briefly I think this year we're seeing the HoT doing too much finance work (and work overall) and not enough by exec which works this year, but isn't a sustainable model for future years. I'm not sure if there's anything that needs to change on tracker as far as this goes (other than documentation on how everything works) but I wanted to bring this up.

Permissions elevation, i.e. moving between the tiers, kinda ties into this. Somebody besides the HoT needs to be able to elevate people to other positions for the case when we have an inactive HoT. While obviously tracker devs could do this, that's a bad power structure because I don't want Kelly having to make that judgement call. Again, this is probably a better discussion for an exec meeting so I'm going to leave it there.

General Membership: While these people have less potential to break things, I think ironing out permissions for these tiers is important because they're less likely to be (or to want to be) familiar with the quirks of tracker.

except for Hot/Exec--they should be able to add/remove anyone, not just TiC/aTiC. I was assuming we'd build permissions so obviously HoT/Exec would be able to do anything the TiCs could do.

Thinking about this a little more I'd keep TiC/run positions really close in what they can / can't edit for an event. This is partially for events when we don't have a TiC, but I think it's easier if the TiC can tell the MR to set his own call or the LD can edit the equipment they're pulling. Ditto for add / removing run positions. As TiC it's a lot easier for me to tell my ME to add their own assistant than to remember to do it myself. How hard is it to implement near identical permissions levels for TiC / run positions then change it later on if there's an issue?

hatkirby commented 10 years ago

RE the claim that editing hardcoded permissions will be difficult: I'd like to note for the benefit of future tracker devs that under the RBAC implementation that I'm working on, it will be very simple for anyone who has a basic level of Ruby competence and who has read https://github.com/ryanb/cancan/wiki/Defining-Abilities to be able to define permissions. All permissions are defined in https://github.com/ABTech/abtt/blob/rbac/app/models/ability.rb, and while at this point permissions are based off of a strictly linear set of roles enumerated in https://github.com/ABTech/abtt/blob/rbac/app/models/member.rb (see line 27), permissions can be easily rearranged to be defined along different information in a Member record. cancan makes it very easy to have flexible permissions, and as for any permissions that deviate from the simple CRUD model, I would be happy to add more documentation to ability.rb about what each permission does.

As for editing events, this is what I have implemented:

Let me know if this is acceptable.

ihartwig commented 10 years ago

Tim’s comments are good. My main point was that I don’t think we need so many general member levels. What is more important is logging and accountability after the fact in our environment.

Also, my issue with hard coding is that we don’t always have basic ruby competence. I have shyed away from tracker dev personally because I don’t have the time to commit to becoming basically competent in ruby/our dev. environment.

Ian Hartwig [spacepenguine@gmail.com]

hatkirby commented 10 years ago

A bit of clarification is requested: when Tim says that "timecards/payroll" is HoT only, do exec/management have a read-only view of payroll via the events payroll page, the member pages (there's currently a list of billed hours on a member's profile page that anyone with Membership Management permissions can see), and the actual Timecard pages themselves? Or can only HoT access these pages at all?

Also, who is allowed to edit a user's payrate? Tracker Management and up, or just HoT?

ghost commented 10 years ago

Exec can view all payroll data. Only HoT can edit payrate.

Sent from my pocket Atari On Feb 1, 2014 11:50 PM, "Kelly Rauchenberger" notifications@github.com wrote:

A bit of clarification is requested: when Tim says that "timecards/payroll" is HoT only, do exec/management have a read-only view of payroll via the events payroll page, the member pages (there's currently a list of billed hours on a member's profile page that anyone with Membership Management permissions can see), and the actual Timecard pages themselves? Or can only HoT access these pages at all?

Also, who is allowed to edit a user's payrate? Tracker Management and up, or just HoT?

Reply to this email directly or view it on GitHubhttps://github.com/ABTech/abtt/issues/123#issuecomment-33892084 .

pchiappacmu commented 10 years ago

Tim, that's not true. I cannot go to a random event and view the payroll for that event, is what Mackie means. On production tracker, I see http://i.imgur.com/84x2aN7.png On dev, I see http://i.imgur.com/Rp5p43Q.png

ghost commented 10 years ago

Kelly was asking me what it should be. What I said is what it should be.

Sent from my pocket Atari On Feb 4, 2014 6:49 PM, "pchiappacmu" notifications@github.com wrote:

Tim, that's not true. I cannot go to a random event and view the payroll for that event, is what Mackie means. On production tracker, I see http://i.imgur.com/84x2aN7.png On dev, I see http://i.imgur.com/Rp5p43Q.png

Reply to this email directly or view it on GitHubhttps://github.com/ABTech/abtt/issues/123#issuecomment-34122416 .

mmackie commented 10 years ago

does view payroll data also include viewing #hours billed by a member on their members page? Because I can currently see that on tracker but not on tracker-dev

ghost commented 10 years ago

"All payroll data"

Sent from my pocket Atari On Feb 4, 2014 6:55 PM, "Michelle Mackie" notifications@github.com wrote:

does view payroll data also include viewing #hours billed by a member on their members page? Because I can currently see that on tracker but not on tracker-dev

Reply to this email directly or view it on GitHubhttps://github.com/ABTech/abtt/issues/123#issuecomment-34122915 .