Fix minimum team access mode #24647
Fix minimum team access mode #24647
Conversation
GiteaBot
added
the
lgtm/need 2
| func MinUnitAccessMode(unitsMap map[Type]perm.AccessMode) perm.AccessMode { | ||
| res := perm.AccessModeNone | ||
| res := perm.AccessModeWrite |
There was a problem hiding this comment.
Shouldn't it be
| res := perm.AccessModeWrite | |
| res := perm.AccessModeOwner |
and letting the for loop scale the variable down?
Or do we pass an empty map as parameter at any point?
There was a problem hiding this comment.
Can we have units with perm.AccessModeOwner?
I see here we return MaxPerm=perm.AccessModeAdmin:
Line 212 in 23ae939
But I don't know of any units that can have admin/owner access in practice (the UI does not allow it that I'm aware of).
So I can set the default to perm.AccessModeAdmin, but I didn't want to go higher than needed just for additional security.
There was a problem hiding this comment.
TBH, I have difficulty to understand the logic if I didn't read the chat history & PR description carefully.
And the new logic seems counterintuitive , could there be some detailed comments or some tests for this problem?
There was a problem hiding this comment.
Yeah, comment and tests will be helpful regardless. I’ll work on adding those this weekend unless someone beats me to it.
Part of my issue is that I think this whole function should be deprecated. I’ll see how painful that will be.
I think I will just always set team.AccessMode=NoAccess whenever team.AccessMode < AdminAccess.
Essentially, every auth middleware should be checking unit permissions for anything less than admin.
GiteaBot
added
lgtm/need 1
kdumontnu
added
the
outdated/backport/v1.19
|
Marking WIP as per #24647 (comment). |
|
A new fix: Fix team permission #34128 new screenshot
|
Team access mode appears to be broken, leading to some suspect behavior. Specifically, we currently ignore "NoAccess" when getting the min access for a team in
MinUnitAccessMode.This means that if I set a team to only have write access for wiki page:
I will see the team has
team.AccessMode = write:But adding any read access:
Moves the
team.AccessMode = read:I ultimately suggest that we completely deprecate
team.AccessMode, since it's not clear what that variable is used for now that team units exist. We should addteam.IsAdminandteam.IsOwnerflags instead, but I don't have the availability to take that on.