In basic auth check for tokens before call UserSignIn

[manuelluis]

With this change we fix #5701

[codecov-io]

Codecov Report

Merging #5725 into master will increase coverage by 0.01%.
The diff coverage is 34.24%.

@@            Coverage Diff             @@
##           master    #5725      +/-   ##
==========================================
+ Coverage   37.76%   37.77%   +0.01%     
==========================================
  Files         325      325              
  Lines       47650    47686      +36     
==========================================
+ Hits        17994    18014      +20     
- Misses      27062    27074      +12     
- Partials     2594     2598       +4

Impacted Files	Coverage Δ
modules/auth/auth.go	50.67% <34.14%> (-2.81%)	⬇️
routers/repo/http.go	43.39% <34.37%> (+2.58%)	⬆️
models/repo_list.go	63.29% <0%> (-1.27%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6868378...ace7b0a. Read the comment docs.

[zeripath]

Heya, thanks for your PR.

Changing login code is always a bit worrisome as it's an area fraught with consequences.

Your proposed login scheme is somewhat concerning to me. Is this used elsewhere? (either in Gitea or elsewhere?) I really don't like the idea of overloading the password field in this way, or of putting tokens in to the username. (Too many places will log usernames.)

If we have to overload fields - which I guess we have to because git won't let us add other fields. My suggestion would be to instead overload the username field with the type of login plus a separator not allowed in usernames on Gitea (likely +) followed by the username. The password field could then be kept for secrets.

If I'm being stupid and this is already in Gitea and you're just fixing a bug please tell me and I'll look again.

I do think this is a good idea though.

[manuelluis]

@zeripath, I think there is a bug. #5701

The {USER}:{TOKEN} and {TOKEN}:x-oauth-basic are already using for cloning repos, I'm not adding new authentication schemes.

When you use the token to clone a repo, actually gitea check first the user, twice, with UserSignIn. If the user is a gitea user (not an external user) this is not a problem, but if you are validating user to LDAP, for example, you are validating the tokens to LDAP: you can block your user since you are sending the username and the token as the password (not your user passwd) or you can ex-filtrate the token if you use the token as the username and "x-oauth-basic" as the password.

For every clone you are doing 4 user validations, it's very easy to block or disable the user in the LDAP.

In routers/repo/http.go I only change the order of the validation, first I check if is a token and then the password of the user.

In modules/auth/auth.go, SignedInUser is call in the middelware Contexter for every request. I didn't find an easy way to know if we are cloning a repo. There is already a check for the api that is very simple (checks if the path begin with "/api/") but detect if the path is a route for Git smart HTTP will be more complicated.
I added the check for the token in the basic auth. This is new since that type of auth is not permitted before: use {USER}:{TOKEN} or {TOKEN}:x-oauth-basic as basic auth.

With the Git smart HTTP requests there is a problem of double validation that is not solve in this pull.

[lafriks]

Thanks for PR! This has been troubling me for some time on one instance with LDAP :)

[lafriks]

Make lgtm work

[lafriks]

Please backport this to release/v1.7 branch