oauth: Increase the max size of Provider to 255 #28664
Conversation
GiteaBot
added
the
lgtm/need 2
There are legit reasons why a Provider ("Authentication Name") can be
longer than 25 characters, so lets increase that limit substantially to
255.
To do so, we need a migration too, and `AuthenticationForm` needs to
have the maximum size of its `Name` field increased similarly.
Fixes #24891, at least for the Provider case.
Signed-off-by: Gergely Nagy <forgejo@gergo.csillger.hu>
GiteaBot
added
lgtm/need 1
There was a problem hiding this comment.
Edit: Following may be wrong. Checking it again. Still don't know why we need that field.
I don't understand why we even have this field. Provider is a copy of the name of the login source.
Lines 112 to 115 in cb10f27
There we don't have a length limit.
It's only used here
gitea/models/user/external_login_user.go
Lines 173 to 175 in cb10f27
to support a name search. And that should just be a sql join to the
login_source table.
Therefore I'm against this change. We should just remove that field.
| @@ -16,7 +16,7 @@ import ( | |||
| type AuthenticationForm struct { | |||
| ID int64 | |||
| Type int `binding:"Range(2,7)"` | |||
| Name string `binding:"Required;MaxSize(30)"` | |||
| Name string `binding:"Required;MaxSize(255)"` | |||
There was a problem hiding this comment.
Note: This field is not used for the Provider field extended in this PR.
|
Ok, I was wrong. The field is still used by the migration. For that you have to create a login source for GitHub as example. Then have a user link their local account with that login source. This will create an external login user entry with Lines 293 to 296 in cb10f27 That logic is fragile³ Anyway: The provider name is provided by goth and goth does not use names > 16 characters, so the current 25 characters are enough. The changes to |
|
Well, the reason this PR was made because in #24891, it was discovered and shown that there is a legit issue with the length of the field, that happens in real installations, and increasing the limit made that problem disappear. It is not the best solution, it's a band aid, based on findings in the original bug report. I do not have the time or resources to dive deeper than that, so I suppose the way forward is to close this one out. |
|
No, the comment in #24891 (comment) explains that the In #24891 (comment) you can see that the problem is solved and the provider field is still only 25 characters long. |
|
And indeed it is so. I misread the original report then! All the more reason to move this to the bin. |
There are legit reasons why a Provider ("Authentication Name") can be longer than 25 characters, so lets increase that limit substantially to 255.
To do so, we need a migration too, and
AuthenticationFormneeds to have the maximum size of itsNamefield increased similarly.Fixes #24891, at least for the Provider case.