[WIP] Enforce usertype via GetIndividualUserByName #2118
Conversation
Use GetOrgByName in place of GetUserByName
sapk
changed the title
|
PR rebased after following PR. Tests are passing so should be good ^^ |
tboerger
added
the
lgtm/need 2
lunny
added
the
type/refactoring
|
LGTM |
tboerger
added
lgtm/need 1
There was a problem hiding this comment.
This won't work. In RepoAssignment() (and probably a lot of other places as well), we use GetUserByName(..) for both individuals and organizations.
|
Also, if we're going to change the behavior of |
|
@ethantkoenig I will create GetIndividualUserByName and used it as base to remove duplicate code. I will update code where GetIndividualUserByName need to be used in place of GetUserByName (that will stay generic). |
sapk
changed the title
|
I have two questions for this implementation. Does I add a new ErrIndividualUserNotExist or keep using ErrUserNotExist ? In the last case, if we consider User as a complete generic then ErrOrgNotExist could be also removed ? |
|
I am going for adding a field |
| @@ -413,7 +413,7 @@ func LoginViaLDAP(user *User, login, password string, source *LoginSource, autoR | |||
| sr := source.Cfg.(*LDAPConfig).SearchEntry(login, password, source.Type == LoginDLDAP) | |||
| if sr == nil { | |||
| // User not in LDAP, do nothing | |||
| return nil, ErrUserNotExist{0, login, 0} | |||
| return nil, ErrUserNotExist{0, login, 0, -1} | |||
| } | |||
There was a problem hiding this comment.
Why -1? I think it is better user type individual as you can't login with organization. Same in most other places.
There was a problem hiding this comment.
A default value ^^ correspoding to any value.
I will review each case for determining.
I was planning on keep -1 when we don't check the type of the user but for external sources this could be set to IndividualUser as it is supposed.
There was a problem hiding this comment.
Please don't introduce (more) magic numbers, if you want -1 to be "I don't care" then const UserTypeIDoNotCare -1 🙂
|
Closing as outdated. @sapk please re-open if needed. |
and switch to GetOrgByName when we want org.
As it modify the comportement of GetUserByName this need more testing (so WIP).
Maybe we will need a generic GetUserOrOrgByName method for some cases.
Follow #2117