search

coreinfrastructure / best-practices-badge

🏆Open Source Security Foundation (OpenSSF) Best Practices Badge (formerly Core Infrastructure Initiative (CII) Best Practices Badge)
https://www.bestpractices.dev
MIT License
1.21k stars 201 forks source link

Unable to remove user with badge editing privileges #1413

Open dmcbride64 opened 4 years ago

dmcbride64 commented 4 years ago

https://bestpractices.coreinfrastructure.org/en/projects/1738

  1. Navigate to project
  2. Enter editing mode
  3. Move to bottom of "Basics" section
  4. Enter "-2272" and click SAVE
  5. No change in list of users with editing privileges

I tried refreshing the page. I also had one of my colleagues, who just added a user, try the same steps and was unsuccessful.

david-a-wheeler commented 4 years ago

I have no idea why that would happen. As you surmise, that shouldn't happen. I am going to have to check on that later.

Just to confirm, do you know your user number on the badge app? When you log in, and look at your profile, it will show up in the URL as the last number.

david-a-wheeler commented 4 years ago

Only the owner of a badge entry can remove a user; if anyone else with editing privileges tries to remove a user, we just ignore the request. We probably should report an error in that case, but we don't currently.

My guess is that someone other than the owner is trying to remove a user. Who is trying to remove a user?

If I don't hear in a while, I'll presume that it's "working as designed" and close this issue. If the badge entry owner is trying to remove a user, then that would be a problem! Please let me know.

dmcbride64 commented 4 years ago

The problem is that we have community members who "disappear" and don't respond to requests, so we need an administrative capability to remove badges when people leave the project.

david-a-wheeler commented 4 years ago

Can you set someone else (say you!) who will stick around as the owner of the badge entry, and then can add/remove people?

jdossett commented 4 years ago

@david-a-wheeler user 2272 is the owner. I am thinking @dmcbride64 is wanting a transfer of ownership. @dmcbride64 is that correct?

dmcbride64 commented 4 years ago

That might be correct. Basically, we have people regularly joining and leaving the project. Apparently, we have the ability to add new owners, but not the ability to remove owners. So, over time, you wind up having a large number of owners that are no longer associated with the project. Probably not that big of a deal. I can’t point to a situation where that’s ever been a problem. Yet, it seems like it would be a good idea to remove privileges from people that have left the project.

David

On Tue, Apr 21, 2020 at 2:54 PM Jason Dossett notifications@github.com wrote:

@david-a-wheeler https://github.com/david-a-wheeler user 2272 is the owner. I am thinking @dmcbride64 https://github.com/dmcbride64 is wanting a transfer of ownership. @dmcbride64 https://github.com/dmcbride64 is that correct?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/coreinfrastructure/best-practices-badge/issues/1413#issuecomment-617434063, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2QG223YXDLAVMI3VR6GXLRNYIZPANCNFSM4LDF4TVA .

jdossett commented 4 years ago

@dmcbride64 I am not sure there is much we can do beyond reach out to the original owner of the badge entry and ask about transferring the project. Do you have a long term member of the project who everyone would rather be the badge owner. We try not to step too much on the toes of badge owners or the internal workings of projects.

@david-a-wheeler what do you think?

david-a-wheeler commented 4 years ago

@dmcbride64 - in normal cases there's someone who sticks around (long-term) as leader of the project who can serve as owner, and the owner can tell us who else is allowed to edit the badge entry. Should you be that person?

dmcbride64 commented 4 years ago

David and Jason,

First of all, thanks for your patience and persistence with this issue.

It may be that I'm simply misunderstanding the tool and the process. However, from an administrative perspective, what I see is that we can give privileges to people that join the project, but we can't remove privileges from people that leave the project. I can't think of any other IT resource that's setup that way. It really seems odd, which makes me think that I may be misunderstanding something.

The result is that we have a growing collection of people with badge editing privileges who no longer work on the project. It seems that, just as with any other IT resource (network, DB, etc.), you would provide for an administrator role that could both grant and remove privileges as people join and leave the project. What am I missing?

BTW - as a program manager, I can't serve in the role as badge editor. That's for the community to determine. I simply grant privileges to the people that the community determines should work on the badging process.

Thanks.

David

On Mon, Apr 27, 2020 at 7:02 PM David A. Wheeler notifications@github.com wrote:

@dmcbride64 https://github.com/dmcbride64 - in normal cases there's someone who sticks around (long-term) as leader of the project who can serve as owner, and the owner can tell us who else is allowed to edit the badge entry. Should you be that person?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/coreinfrastructure/best-practices-badge/issues/1413#issuecomment-620332121, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2QG2ZURPLGRM32AEWXFVLROY2MNANCNFSM4LDF4TVA .

david-a-wheeler commented 4 years ago

(Sorry for the accidental close)

david-a-wheeler commented 4 years ago

We do indeed have the idea of an administrator. That is what the badger owner is. The badge owner can decide who is allowed to edit. It sounds like you should be the badge owner.

We don't really have the concept of an administrator who cannot edit. Which is what it sounds like you're expecting. In our system, the project badge entry owner can decide who else can edit the badge, and can also edit. For smaller projects we should never change, trying to have two distinct roles would be confusing. I don't think that should be an issue. You can simply be the owner of these badges, and then decide who can edit them, while refraining from editing yourself. In the end, being able to decide exactly who else can edit something is essential the same as being able to edit it yourself.

So if you want to become the project badge entry owner, please let us know, I think that's what should happen.

dmcbride64 commented 4 years ago

Thanks, David.

I think that I understand the disconnect now. We have interpreted "badge owner" as something that should go along with project leadership, i.e., the project PTL, not as a PM function. Now that I understand what you mean by "badge owner" I don't think that we need a separate administrator role.

However, it does seem that the term "badge owner" could be misleading. It's also difficult to explain to your open source community why the PM should be the badge owner and not a community member. Suggest that you consider a different term.

Let me discuss this with the other PMs and get back to you. Thanks, again.

David

On Wed, Apr 29, 2020 at 12:43 PM David A. Wheeler notifications@github.com wrote:

We do indeed have the idea of an administrator. That is what the badger owner is. The badge owner can decide who is allowed to edit. It sounds like you should be the badge owner.

We don't really have the concept of an administrator who cannot edit. Which is what it sounds like you're expecting. In our system, the project badge entry owner can decide who else can edit the badge, and can also edit. For smaller projects we should never change, trying to have two distinct roles would be confusing. I don't think that should be an issue. You can simply be the owner of these badges, and then decide who can edit them, while refraining from editing yourself. In the end, being able to decide exactly who else can edit something is essential the same as being able to edit it yourself.

So if you want to become the project badge entry owner, please let us know, I think that's what should happen.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/coreinfrastructure/best-practices-badge/issues/1413#issuecomment-621419511, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2QG27RBS44PXG7WH3GQBTRPB7NZANCNFSM4LDF4TVA .

david-a-wheeler commented 4 years ago

I'm glad we were able to clear it up!

It's hard to find the perfect term. If you have suggestions I'd love to hear it.

TonyLHansen commented 2 years ago

Perhaps the right term would be "administrator" or "super-editor"?

I'm looking at this issue again from an ONAP perspective (where David McBride is also coming from). We have about 45 CII projects that need to be managed and have various editor IDs removed.

Related question: how can we determine who the current "owner" ("super-editor") is?

There is also the bus factor issue. We should be able to designate someone to be an alternate "owner"/"super-editor".

david-a-wheeler commented 2 years ago

Related question: how can we determine who the current "owner" ("super-editor") is?

Look at the project entry. At the bottom it will say:

Project badge entry owned by: *OWNER*

We should be able to designate someone to be an alternate "owner"/"super-editor".

That's more complex. You'd then need roles associated with each co-owner. That's possible, and we did design the schema so it'd be easy to add. The real challenge is making it easy from a UI point of view (which includes making it easy to understand). It certainly can be done, and we've thought that might become useful, but that will require some thinking through.

For the moment, it's been rare enough that handling it manually is adequate.

If you think it's time, we'll need to have a discussion on what that would look like. It's not that the code is hard, it's everything else.

TonyLHansen commented 2 years ago

Yes, I think it is time. :) I have about 45 badge entries on which we would like to have the owner changed, and user ids removed. We would would like to have a co-owner added as well for the bus factor. So here is a swag design:

Every account has one or more accounts designated as the owner/administrator/super-editor. The exact name is TBD, and further discussion here will continue to use the term owner. The examples below come from badge entry 1718.)

The description of "additional_rights" changes to:

(Advanced) What users have the rights to edit this badge entry? Currently: [1597, 1992, 2630, 3607, 4469, 11077]
Most projects should ignore this field. Project badge entries can always be edited by the badge entry owner (initially, the creator), BadgeApp administrators, and anyone who can commit to the GitHub repository (if it's on GitHub). If you want someone else to be able to edit this badge entry, and you already have edit rights to this project badge entry, you can add additional users with edit rights. Just enter "+" followed by a comma-separated list of integer user ids. Those users will then also be allowed to edit this project entry. If you're the owner of the badge entry or a BadgeApp administrator, you can remove users from this list by entering "-" followed by a comma-separated list of integer user ids. We expect that normally only one person will edit a particular badge entry at a time. This application uses optimistic locking to prevent saving stale data if multiple users try to edit a badge entry simultaneously. If you have multiple editors, we recommend saving badge entry data incrementally and often (that is a wise practice anyway).

A new editable field is added immediately after:

(Advanced) What users are owners of this badge entry? Currently: [2630]
Most projects should ignore this field. The creator of the project is initially considered the owner of the badge entry, but who should be considered the owner may change over time. A badge entry owner and BadgeApp administrators may add or remove additional users from the list of owners. Just enter "+" followed by a comma-separated list of integer user ids; those users will then be considered an owner of the badge entry. Similarly, enter a "-" followed by a comma-separated list of integer user ids to remove those users from the list of owners. An owner may not remove themselves, and there must always be at least one owner of the badge entry. A user id added to the owner list will  also be automatically added to the editor list (if not already present), but removing a user id from the owner list does not also remove that user id from the editor list.

The list of owners at the bottom is changed to have a list of IDs:

Project badge entry owned by: Vijay Venkatesh Kumar, owner b name, etc.

TonyLHansen commented 2 years ago

@david-a-wheeler any comment?

david-a-wheeler commented 2 years ago

I mostly like it. I think I'd like 3 groupings though: owner, admins, and editors.

Currently we display an "owner" for each project entry in a table, and I think that's helpful. If there are multiple "owners" then it's hard to display (a table entry could suddenly have a long list). It'd be more complicated to not have an owner (since the current code all assumes one). Also, since owners get displayed in many circumstances, it'd require implementing a join against the user rights table for practically anything. Owners & admins would have the same rights, but only the "owner" shows up at the top level.

Access control rules:

What do you think?

dmcbride64 commented 2 years ago

I like it. I can see that there could be some confusion about the difference between an owner and an admin, but that seems like a relatively minor issue.

David

On Fri, Nov 19, 2021 at 9:10 AM David A. Wheeler @.***> wrote:

I mostly like it. I think I'd like 3 groupings though: owner, admins, and editors.

Currently we display an "owner" for each project entry in a table, and I think that's helpful. If there are multiple "owners" then it's hard to display (a table entry could suddenly have a long list). It'd be more complicated to not have an owner (since the current code all assumes one). Also, since owners get displayed in many circumstances, it'd require implementing a join against the user rights table for practically anything. Owners & admins would have the same rights, but only the "owner" shows up at the top level.

Access control rules:

  • An "editor" can edit the project entry.
  • An "admin" can edit the project entry like an editor, and can also add/remove editors/admins/owners.
  • An "owner" is a designated lead admin with no special rights but is shown in the UI as the owner. To turn an existing admin into an owner, add them as an admin again.

What do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/coreinfrastructure/best-practices-badge/issues/1413#issuecomment-974251821, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2QG27ECFXPQ3VCCX4MASTUM2AHLANCNFSM4LDF4TVA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

TonyLHansen commented 2 years ago

Sure, having 3 levels is fine. The only clunky thing is this:

"To turn an existing admin into an owner, add them as an admin again."

but that can be managed with appropriate verbiage in the description.

TonyLHansen commented 2 years ago

@david-a-wheeler any more thoughts on this ticket?

david-a-wheeler commented 2 years ago

I've been really overwhelmed on other tasks & just haven't had time to focus on this one. In particular the Great MFA Distribution Project, log4j, White House meeting, and getting Alpha-Omega started have taken time.

It's not that it's that issue is hard or time-consuming, it's just hard to peel away the block of time needed.

In the short term, let's get folks like @dmcbride64 set up so that if the project leads repeatedly change, then someone who usually doesn't change is the owner & can add/remove others as needed. In most projects it's unusual for the project lead to disappear from a project, which is why handling them manually "by exception" has been fine in most cases.

I still agree that we need to make the bigger change, it's just that other things are pressing at this instant :-).

TonyLHansen commented 2 years ago

Ping

david-a-wheeler commented 2 years ago

I see the ping! I'm currently focused on updating the training course, and I expect to be prepping for a WH follow-up meeting in early May (one month from now).

I think for the badges project we need to prioritize moving the site to bestpractices.dev & finally getting that logo issue resolved. Once that's done we can do other stuff, such as this automation.

Don't get me wrong, automation is the right long-term direction. But most projects don't need this, and I think we can handle the special cases manually while get some other things done.