This commit has a fundamental flaw, in order to syncronize if external
users are still active the commit checks if the refresh token is
accepted by the OAuth provider, if that is not the case it sees that as
the user is disabled and sets the is active field to `false` to signal
that. Because it might be possible (this commit makes this a highly
likelyhood) that the OAuth provider still recognizes this user the
commit introduces code to allow users to re-active themselves via the
oauth flow if they were disabled because of this. However this code
makes no distinction in why the user was disabled and always re-actives
the user.
Thus the reactivation via the OAuth flow allows users to bypass the
manually activation setting (`[service].REGISTER_MANUAL_CONFIRM`) or if
the admin for other reasons disabled the user.
This reverts commit 21fdd28f08.
This leverages the existing `sync_external_users` cron job to
synchronize the `IsActive` flag on users who use an OAuth2 provider set
to synchronize. This synchronization is done by checking for expired
access tokens, and using the stored refresh token to request a new
access token. If the response back from the OAuth2 provider is the
`invalid_grant` error code, the user is marked as inactive. However, the
user is able to reactivate their account by logging in the web browser
through their OAuth2 flow.
Also changed to support this is that a linked `ExternalLoginUser` is
always created upon a login or signup via OAuth2.
Ideally, we would also refresh permissions from the configured OAuth
provider (e.g., admin, restricted and group mappings) to match the
implementation of LDAP. However, the OAuth library used for this `goth`,
doesn't seem to support issuing a session via refresh tokens. The
interface provides a [`RefreshToken`
method](https://github.com/markbates/goth/blob/master/provider.go#L20),
but the returned `oauth.Token` doesn't implement the `goth.Session` we
would need to call `FetchUser`. Due to specific implementations, we
would need to build a compatibility function for every provider, since
they cast to concrete types (e.g.
[Azure](https://github.com/markbates/goth/blob/master/providers/azureadv2/azureadv2.go#L132))
---------
Co-authored-by: Kyle D <kdumontnu@gmail.com>
(cherry picked from commit 416c36f3034e228a27258b5a8a15eec4e5e426ba)
Conflicts:
- tests/integration/auth_ldap_test.go
Trivial conflict resolved by manually applying the change.
- routers/web/auth/oauth.go
Technically not a conflict, but the original PR removed the
modules/util import, which in our version, is still in use. Added it
back.
A remote user (UserTypeRemoteUser) is a placeholder that can be
promoted to a regular user (UserTypeIndividual). It represents users
that exist somewhere else. Although the UserTypeRemoteUser already
exists in Forgejo, it is neither used or documented.
A new login type / source (Remote) is introduced and set to be the login type
of remote users.
Type UserTypeRemoteUser
LogingType Remote
The association between a remote user and its counterpart in another
environment (for instance another forge) is via the OAuth2 login
source:
LoginName set to the unique identifier relative to the login source
LoginSource set to the identifier of the remote source
For instance when migrating from GitLab.com, a user can be created as
if it was authenticated using GitLab.com as an OAuth2 authentication
source.
When a user authenticates to Forejo from the same authentication
source and the identifier match, the remote user is promoted to a
regular user. For instance if 43 is the ID of the GitLab.com OAuth2
login source, 88 is the ID of the Remote loging source, and 48323
is the identifier of the foo user:
Type UserTypeRemoteUser
LogingType Remote
LoginName 48323
LoginSource 88
Email (empty)
Name foo
Will be promoted to the following when the user foo authenticates to
the Forgejo instance using GitLab.com as an OAuth2 provider. All users
with a LoginType of Remote and a LoginName of 48323 are examined. If
the LoginSource has a provider name that matches the provider name of
GitLab.com (usually just "gitlab"), it is a match and can be promoted.
The email is obtained via the OAuth2 provider and the user set to:
Type UserTypeIndividual
LogingType OAuth2
LoginName 48323
LoginSource 43
Email foo@example.com
Name foo
Note: the Remote login source is an indirection to the actual login
source, i.e. the provider string my be set to a login source that does
not exist yet.
(cherry picked from commit e11dcc60f2)
use backticks to avoid backslash
(cherry picked from commit 34212791ee)
(cherry picked from commit bde9473c69)
(cherry picked from commit d4deb43084)
(cherry picked from commit 08e91649b0)
(cherry picked from commit 2b988e5415)
[TESTS] auth LinkAccount test coverage (squash)
(cherry picked from commit a2b2e3066b)
(cherry picked from commit 841d1b5073)
(cherry picked from commit 35da630ad8)
(cherry picked from commit caf2dc4fa7)
(cherry picked from commit 6eb81e67ba)
(cherry picked from commit d59757239f)
(cherry picked from commit 38a121b688)
(cherry picked from commit 20613874ee)
(cherry picked from commit 6d2705e108)
(cherry picked from commit f177b72814)
(cherry picked from commit 75e1fc4c83)
(cherry picked from commit ba64fa9867)
(cherry picked from commit 0b8ab0893e)
(cherry picked from commit 1419d11435)
(cherry picked from commit 38766847e0)
(cherry picked from commit 6f23426a6a)
(cherry picked from commit 9e0ff9ca54)
(cherry picked from commit 353f3601c3)
(cherry picked from commit 6e4ae401d8)
(cherry picked from commit 1a7afe4153)
(cherry picked from commit f9f3e0cc02)
(cherry picked from commit 22fd0337f3)
(cherry picked from commit ee57e138d1)
(cherry picked from commit 21f9b7e73d)
(cherry picked from commit 17c548c092)
(cherry picked from commit 02d3186517)
(cherry picked from commit f02a040fa2)
(cherry picked from commit 3cf9f82b28)
(cherry picked from commit aa9d06dbac)
(cherry picked from commit 6894213154)
The function `GetByBean` has an obvious defect that when the fields are
empty values, it will be ignored. Then users will get a wrong result
which is possibly used to make a security problem.
To avoid the possibility, this PR removed function `GetByBean` and all
references.
And some new generic functions have been introduced to be used.
The recommand usage like below.
```go
// if query an object according id
obj, err := db.GetByID[Object](ctx, id)
// query with other conditions
obj, err := db.Get[Object](ctx, builder.Eq{"a": a, "b":b})
```
## Bug in Gitea
I ran into this bug when I accidentally used the wrong redirect URL for
the oauth2 provider when using mssql. But the oauth2 provider still got
added.
Most of the time, we use `Delete(&some{id: some.id})` or
`In(condition).Delete(&some{})`, which specify the conditions. But the
function uses `Delete(source)` when `source.Cfg` is a `TEXT` field and
not empty. This will cause xorm `Delete` function not working in mssql.
61ff91f960/models/auth/source.go (L234-L240)
## Reason
Because the `TEXT` field can not be compared in mssql, xorm doesn't
support it according to [this
PR](https://gitea.com/xorm/xorm/pulls/2062)
[related
code](b23798dc98/internal/statements/statement.go (L552-L558))
in xorm
```go
if statement.dialect.URI().DBType == schemas.MSSQL && (col.SQLType.Name == schemas.Text ||
col.SQLType.IsBlob() || col.SQLType.Name == schemas.TimeStampz) {
if utils.IsValueZero(fieldValue) {
continue
}
return nil, fmt.Errorf("column %s is a TEXT type with data %#v which cannot be as compare condition", col.Name, fieldValue.Interface())
}
}
```
When using the `Delete` function in xorm, the non-empty fields will
auto-set as conditions(perhaps some special fields are not?). If `TEXT`
field is not empty, xorm will return an error. I only found this usage
after searching, but maybe there is something I missing.
---------
Co-authored-by: delvh <dev.lh@web.de>
The steps to reproduce it.
First, create a new oauth2 source.
Then, a user login with this oauth2 source.
Disable the oauth2 source.
Visit users -> settings -> security, 500 will be displayed.
This is because this page only load active Oauth2 sources but not all
Oauth2 sources.
Change all license headers to comply with REUSE specification.
Fix #16132
Co-authored-by: flynnnnnnnnnn <flynnnnnnnnnn@github>
Co-authored-by: John Olheiser <john.olheiser@gmail.com>
A lot of our code is repeatedly testing if individual errors are
specific types of Not Exist errors. This is repetitative and unnecesary.
`Unwrap() error` provides a common way of labelling an error as a
NotExist error and we can/should use this.
This PR has chosen to use the common `io/fs` errors e.g.
`fs.ErrNotExist` for our errors. This is in some ways not completely
correct as these are not filesystem errors but it seems like a
reasonable thing to do and would allow us to simplify a lot of our code
to `errors.Is(err, fs.ErrNotExist)` instead of
`package.IsErr...NotExist(err)`
I am open to suggestions to use a different base error - perhaps
`models/db.ErrNotExist` if that would be felt to be better.
Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: delvh <dev.lh@web.de>
