This reverts commit 4ed372af13.
This change from Gitea was not considered by the Forgejo UI team and there is a consensus that it feels like a regression.
The test which was added in that commit is kept and modified to test that reviews can successfully be submitted on closed and merged PRs.
Closes forgejo/design#11
---
Conflict resolution: trivial
Things done differently: Improve localization message, use the paragraph
element instead of the div element, fix passing this variable to the
template and add a integration test
(cherry picked from commit 9633f336c87947dc7d2a5e76077a10699ba5e50d)
ForkRepository performs two different functions:
* The fork itself, if it does not already exist
* Updates and notifications after the fork is performed
The function is split to reflect that and otherwise unmodified.
The two function are given different names to:
* clarify which integration tests provides coverage
* distinguish it from the notification method by the same name
Previous arch package grouping was not well-suited for complex or multi-architecture environments. It now supports the following content:
- Support grouping by any path.
- New support for packages in `xz` format.
- Fix clean up rules
<!--start release-notes-assistant-->
## Draft release notes
<!--URL:https://codeberg.org/forgejo/forgejo-->
- Features
- [PR](https://codeberg.org/forgejo/forgejo/pulls/4903): <!--number 4903 --><!--line 0 --><!--description c3VwcG9ydCBncm91cGluZyBieSBhbnkgcGF0aCBmb3IgYXJjaCBwYWNrYWdl-->support grouping by any path for arch package<!--description-->
<!--end release-notes-assistant-->
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4903
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: Exploding Dragon <explodingfkl@gmail.com>
Co-committed-by: Exploding Dragon <explodingfkl@gmail.com>
`BranchName` provides the nearest branch of the requested `:commit`.
It's plenty fast on smaller repositories.
On larger repositories like nixpkgs, however, this can easily take 2-3
seconds on a modern machine on a NVMe.
For context, at the time of writing, nixpkgs has over 650k commits and
roughly 250 branches.
`BranchName` is used once in the whole view:
The cherry-pick target branch default selection.
And I believe that's a logic error, which is why this patch is so small.
The nearest branch of a given commit will always be a branch the commit
is already part of. The branch you most likely *don't* want to
cherry-pick to.
Sure, one can technically cherry-pick a commit onto the same branch, but
that simply results in an empty commit.
I don't believe this is intended and even less so worth the compute.
Instead, the cherry-pick branch selection suggestion now always uses
the default branch, which used to be the fallback.
If a user wants to know which branches contain the given commit,
`load-branches-and-tags` exists and should be used instead.
Also, to add insult to injury, `BranchName` was calculated for both
logged-in and not logged-in users, despite its only consumer, the
cherry-pick operation, only being rendered when a given user has
write/commit permissions.
But this isn't particularly surprising, given this happens a lot in
Forgejo's codebase.
Now that my colleague just posted a wonderful blog post https://blog.datalad.org/posts/forgejo-runner-podman-deployment/ on forgejo runner, some time I will try to add that damn codespell action to work on CI here ;) meanwhile some typos managed to sneak in and this PR should address them (one change might be functional in a test -- not sure if would cause a fail or not)
### Release notes
- [ ] I do not want this change to show in the release notes.
- [ ] I want the title to show in the release notes with a link to this pull request.
- [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title.
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4857
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Co-committed-by: Yaroslav Halchenko <debian@onerussian.com>
These are the three conflicted changes from #4716:
* https://github.com/go-gitea/gitea/pull/31632
* https://github.com/go-gitea/gitea/pull/31688
* https://github.com/go-gitea/gitea/pull/31706
cc @earl-warren; as per discussion on https://github.com/go-gitea/gitea/pull/31632 this involves a small compatibility break (OIDC introspection requests now require a valid client ID and secret, instead of a valid OIDC token)
## Checklist
The [developer guide](https://forgejo.org/docs/next/developer/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).
### Tests
- I added test coverage for Go changes...
- [ ] in their respective `*_test.go` for unit tests.
- [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
### Documentation
- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [ ] I did not document these changes and I do not expect someone else to do it.
### Release notes
- [ ] I do not want this change to show in the release notes.
- [ ] I want the title to show in the release notes with a link to this pull request.
- [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title.
<!--start release-notes-assistant-->
## Draft release notes
<!--URL:https://codeberg.org/forgejo/forgejo-->
- Breaking features
- [PR](https://codeberg.org/forgejo/forgejo/pulls/4724): <!--number 4724 --><!--line 0 --><!--description T0lEQyBpbnRlZ3JhdGlvbnMgdGhhdCBQT1NUIHRvIGAvbG9naW4vb2F1dGgvaW50cm9zcGVjdGAgd2l0aG91dCBzZW5kaW5nIEhUVFAgYmFzaWMgYXV0aGVudGljYXRpb24gd2lsbCBub3cgZmFpbCB3aXRoIGEgNDAxIEhUVFAgVW5hdXRob3JpemVkIGVycm9yLiBUbyBmaXggdGhlIGVycm9yLCB0aGUgY2xpZW50IG11c3QgYmVnaW4gc2VuZGluZyBIVFRQIGJhc2ljIGF1dGhlbnRpY2F0aW9uIHdpdGggYSB2YWxpZCBjbGllbnQgSUQgYW5kIHNlY3JldC4gVGhpcyBlbmRwb2ludCB3YXMgcHJldmlvdXNseSBhdXRoZW50aWNhdGVkIHZpYSB0aGUgaW50cm9zcGVjdGlvbiB0b2tlbiBpdHNlbGYsIHdoaWNoIGlzIGxlc3Mgc2VjdXJlLg==-->OIDC integrations that POST to `/login/oauth/introspect` without sending HTTP basic authentication will now fail with a 401 HTTP Unauthorized error. To fix the error, the client must begin sending HTTP basic authentication with a valid client ID and secret. This endpoint was previously authenticated via the introspection token itself, which is less secure.<!--description-->
<!--end release-notes-assistant-->
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4724
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: Shivaram Lingamneni <slingamn@cs.stanford.edu>
Co-committed-by: Shivaram Lingamneni <slingamn@cs.stanford.edu>
- Adjust the counting of the number of lines of a file to match the
amount of rendered lines. This simply means that a file with the content
of `a\n` will be shown as having `1 line` rather than `2 lines`. This
matches with the amount of lines that are being rendered (the last empty
line is never rendered) and matches more with the expecation of the
user (a trailing EOL is a technical detail).
- In the case there's no EOL, the reason why it was counting
'incorrectly' was to show if there was a trailing EOL or not, but now
text is shown to tell the user this.
- Integration test added.
- Resolves Codeberg/Community#1612
An instance-wide actor is required for outgoing signed requests that are
done on behalf of the instance, rather than on behalf of other actors.
Such things include updating profile information, or fetching public
keys.
Signed-off-by: Gergely Nagy <forgejo@gergo.csillger.hu>
Part of #24256.
Clear up old action logs to free up storage space.
Users will see a message indicating that the log has been cleared if
they view old tasks.
<img width="1361" alt="image"
src="https://github.com/user-attachments/assets/9f0f3a3a-bc5a-402f-90ca-49282d196c22">
Docs: https://gitea.com/gitea/docs/pulls/40
---------
Co-authored-by: silverwind <me@silverwind.io>
(cherry picked from commit 687c1182482ad9443a5911c068b317a91c91d586)
Conflicts:
custom/conf/app.example.ini
routers/web/repo/actions/view.go
trivial context conflict
Fix #31707.
It's split from #31724.
Although #31724 could also fix #31707, it has change a lot so it's not a
good idea to backport it.
(cherry picked from commit 81fa471119a6733d257f63f8c2c1f4acc583d21b)
Fix #26685
If a commit status comes from Gitea Actions and the user cannot access
the repo's actions unit (the user does not have the permission or the
actions unit is disabled), a 404 page will occur after clicking the
"Details" link. We should hide the "Details" link in this case.
<img
src="https://github.com/go-gitea/gitea/assets/15528715/68361714-b784-4bb5-baab-efde4221f466"
width="400px" />
(cherry picked from commit 7dec8de9147b20c014d68bb1020afe28a263b95a)
Conflicts:
routers/web/repo/commit.go
trivial context commit
The previous commit laid out the foundation of the quota engine, this
one builds on top of it, and implements the actual enforcement.
Enforcement happens at the route decoration level, whenever possible. In
case of the API, when over quota, a 413 error is returned, with an
appropriate JSON payload. In case of web routes, a 413 HTML page is
rendered with similar information.
This implementation is for a **soft quota**: quota usage is checked
before an operation is to be performed, and the operation is *only*
denied if the user is already over quota. This makes it possible to go
over quota, but has the significant advantage of being practically
implementable within the current Forgejo architecture.
The goal of enforcement is to deny actions that can make the user go
over quota, and allow the rest. As such, deleting things should - in
almost all cases - be possible. A prime exemption is deleting files via
the web ui: that creates a new commit, which in turn increases repo
size, thus, is denied if the user is over quota.
Limitations
-----------
Because we generally work at a route decorator level, and rarely
look *into* the operation itself, `size:repos:public` and
`size:repos:private` are not enforced at this level, the engine enforces
against `size:repos:all`. This will be improved in the future.
AGit does not play very well with this system, because AGit PRs count
toward the repo they're opened against, while in the GitHub-style fork +
pull model, it counts against the fork. This too, can be improved in the
future.
There's very little done on the UI side to guard against going over
quota. What this patch implements, is enforcement, not prevention. The
UI will still let you *try* operations that *will* result in a denial.
Signed-off-by: Gergely Nagy <forgejo@gergo.csillger.hu>
This is an implementation of a quota engine, and the API routes to
manage its settings. This does *not* contain any enforcement code: this
is just the bedrock, the engine itself.
The goal of the engine is to be flexible and future proof: to be nimble
enough to build on it further, without having to rewrite large parts of
it.
It might feel a little more complicated than necessary, because the goal
was to be able to support scenarios only very few Forgejo instances
need, scenarios the vast majority of mostly smaller instances simply do
not care about. The goal is to support both big and small, and for that,
we need a solid, flexible foundation.
There are thee big parts to the engine: counting quota use, setting
limits, and evaluating whether the usage is within the limits. Sounds
simple on paper, less so in practice!
Quota counting
==============
Quota is counted based on repo ownership, whenever possible, because
repo owners are in ultimate control over the resources they use: they
can delete repos, attachments, everything, even if they don't *own*
those themselves. They can clean up, and will always have the permission
and access required to do so. Would we count quota based on the owning
user, that could lead to situations where a user is unable to free up
space, because they uploaded a big attachment to a repo that has been
taken private since. It's both more fair, and much safer to count quota
against repo owners.
This means that if user A uploads an attachment to an issue opened
against organization O, that will count towards the quota of
organization O, rather than user A.
One's quota usage stats can be queried using the `/user/quota` API
endpoint. To figure out what's eating into it, the
`/user/repos?order_by=size`, `/user/quota/attachments`,
`/user/quota/artifacts`, and `/user/quota/packages` endpoints should be
consulted. There's also `/user/quota/check?subject=<...>` to check
whether the signed-in user is within a particular quota limit.
Quotas are counted based on sizes stored in the database.
Setting quota limits
====================
There are different "subjects" one can limit usage for. At this time,
only size-based limits are implemented, which are:
- `size:all`: As the name would imply, the total size of everything
Forgejo tracks.
- `size:repos:all`: The total size of all repositories (not including
LFS).
- `size:repos:public`: The total size of all public repositories (not
including LFS).
- `size:repos:private`: The total size of all private repositories (not
including LFS).
- `sizeall`: The total size of all git data (including all
repositories, and LFS).
- `sizelfs`: The size of all git LFS data (either in private or
public repos).
- `size:assets:all`: The size of all assets tracked by Forgejo.
- `size:assets:attachments:all`: The size of all kinds of attachments
tracked by Forgejo.
- `size:assets:attachments:issues`: Size of all attachments attached to
issues, including issue comments.
- `size:assets:attachments:releases`: Size of all attachments attached
to releases. This does *not* include automatically generated archives.
- `size:assets:artifacts`: Size of all Action artifacts.
- `size:assets:packages:all`: Size of all Packages.
- `size:wiki`: Wiki size
Wiki size is currently not tracked, and the engine will always deem it
within quota.
These subjects are built into Rules, which set a limit on *all* subjects
within a rule. Thus, we can create a rule that says: "1Gb limit on all
release assets, all packages, and git LFS, combined". For a rule to
stand, the total sum of all subjects must be below the rule's limit.
Rules are in turn collected into groups. A group is just a name, and a
list of rules. For a group to stand, all of its rules must stand. Thus,
if we have a group with two rules, one that sets a combined 1Gb limit on
release assets, all packages, and git LFS, and another rule that sets a
256Mb limit on packages, if the user has 512Mb of packages, the group
will not stand, because the second rule deems it over quota. Similarly,
if the user has only 128Mb of packages, but 900Mb of release assets, the
group will not stand, because the combined size of packages and release
assets is over the 1Gb limit of the first rule.
Groups themselves are collected into Group Lists. A group list stands
when *any* of the groups within stand. This allows an administrator to
set conservative defaults, but then place select users into additional
groups that increase some aspect of their limits.
To top it off, it is possible to set the default quota groups a user
belongs to in `app.ini`. If there's no explicit assignment, the engine
will use the default groups. This makes it possible to avoid having to
assign each and every user a list of quota groups, and only those need
to be explicitly assigned who need a different set of groups than the
defaults.
If a user has any quota groups assigned to them, the default list will
not be considered for them.
The management APIs
===================
This commit contains the engine itself, its unit tests, and the quota
management APIs. It does not contain any enforcement.
The APIs are documented in-code, and in the swagger docs, and the
integration tests can serve as an example on how to use them.
Signed-off-by: Gergely Nagy <forgejo@gergo.csillger.hu>
Add an optional `order_by` parameter to the `user.ListMyRepos`
handler (which handles the `/api/v1/user/repos` route), allowing a user
to sort repos by name (the default), id, or size.
The latter will be useful later for figuring out which repos use most
space, which repos eat most into a user's quota.
Signed-off-by: Gergely Nagy <forgejo@gergo.csillger.hu>
- add package counter to repo/user/org overview pages
- add go unit tests for repo/user has/count packages
- add many more unit tests for packages model
- fix error for non-existing packages in DeletePackageByID and SetRepositoryLink
Document return type for the endpoints that fetch specific files from a
repository. This allows the swagger generated code to read the returned
data.
Co-authored-by: Giteabot <teabot@gitea.io>
(cherry picked from commit bae87dfb0958e6a2920c905e51c2a026b7b71ca6)
- In the spirit of #4635
- Notify the owner when their account is getting enrolled into TOTP. The
message is changed according if they have security keys or not.
- Integration test added.
The PATCH if issue & pull request switched to use the service
functions instead. However, the service function changing the state is
not idempotent. Instead of doing nothing which changing from open to
open or close to close, it will fail with an error like:
Issue [2472] 0 was already closed
Regression of: 6a4bc0289d
Fixes: https://codeberg.org/forgejo/forgejo/issues/4686
- Currently if the password, primary mail, TOTP or security keys are
changed, no notification is made of that and makes compromising an
account a bit easier as it's essentially undetectable until the original
person tries to log in. Although other changes should be made as
well (re-authing before allowing a password change), this should go a
long way of improving the account security in Forgejo.
- Adds a mail notification for password and primary mail changes. For
the primary mail change, a mail notification is sent to the old primary
mail.
- Add a mail notification when TOTP or a security keys is removed, if no
other 2FA method is configured the mail will also contain that 2FA is
no longer needed to log into their account.
- `MakeEmailAddressPrimary` is refactored to the user service package,
as it now involves calling the mailer service.
- Unit tests added.
- Integration tests added.
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.
