repo group: introduce editing of owner

The repo group owner concept was only partially implemented. Owners were shown
in the repo group listing, but couldn't be changed. Users owning repo groups
couldn't be deleted, with no other solution than deleting owned repo groups.

This also fixes the existing broken update_repo_group API, which tried to use
unimplemented functionality.

repo group: fix API for updating parent

With other bugs out of the way, the API implementation just has to pass the new
group id correctly.

Note that - as in many other places - moving things to the top level doesn't
work.

Original patch has been modified by Mads Kiilerich.

tests: introduce test_api_update_repo_group covering update_repo_group api

This shows that some of the API is broken. Failing test cases are disabled and
will be enabled when the problems are fixed.

repo: introduce enable_downloads and enable_statistics when creating repos

These booleans were not shown in the normal repo creation form, so the form
validation applied the "default" values of False. These values were however not
used by the model when creating repos - it just unconditionally used the real
global defaults.

The API already exposed some of this, but it wasn't implemented.

The web form for creating repos lacked these fields, but it was present in the
repo edit form. Just make these fields mandatory. There will thus not be any
defaults to apply in the model for creating repos.

tests: update test_api_create_repo with better coverage of create_repo api

Based on test_api_update_repo.

This shows that some of the API is broken. Failing test cases are disabled and
will be enabled when the problems are fixed.

tests: update_repo without begging the question

Don't verify changes by comparing the API result from the update with data
directly from the API that contain the same update.

Instead, set the expectations based on the previous API data before applying
the change.

tests: address pedantic pytype 2021.4.1 warning

File "kallithea/tests/api/api_base.py", line 1315, in test_api_fork_repo_non_admin_no_permission_to_fork: Name 'fork_name' is not defined [name-error]

pytype: add Python type annotations where necessary to guide pytype

Mute pytype warnings:

File "kallithea/lib/auth.py", line 142, in _cached_perms_data: No attribute 'DEFAULT_USER_ID' on module 'kallithea' [module-attr]
File "kallithea/lib/vcs/backends/base.py", line 73, in ...: No attribute '...' on BaseRepository [attribute-error]
File "kallithea/lib/vcs/backends/base.py", line 405, in ...: No attribute '...' on BaseChangeset [attribute-error]
File "kallithea/tests/api/api_base.py", line 2397, in test_api_get_changeset: No attribute 'TEST_REVISION' on _BaseTestApi [attribute-error]
File "kallithea/tests/api/api_base.py", line 2445, in test_api_get_pullrequest: No attribute 'TEST_PR_DST' on _BaseTestApi [attribute-error]
File "kallithea/tests/api/api_base.py", line 2445, in test_api_get_pullrequest: No attribute 'TEST_PR_SRC' on _BaseTestApi [attribute-error]
File "kallithea/tests/api/api_base.py", line 2467, in test_api_get_pullrequest: No attribute 'TEST_PR_REVISIONS' on _BaseTestApi [attribute-error]
File "kallithea/tests/api/api_base.py", line 67, in api_call: No attribute 'app' on _BaseTestApi [attribute-error]
File "kallithea/tests/base.py", line 154, in log_user: No attribute 'app' on TestController [attribute-error]
File "kallithea/tests/base.py", line 169, in _get_logged_user: No attribute '_logged_username' on TestController [attribute-error]

celery: drop tracking of task_id - we use ignore_result=True and will never get anything back

There is thus no need for configuration of celery.result_backend .

The alternative would be to fix it. That could give better error reporting from
failing repo creations, but would require quite a bit of additional changes
before it actually works reliably.

api: fix update_repo check for permission to create top level repos

The hg.create.repository permission only apply to creation (or renaming) of top
level repos - it is not relevant for other kinds of renaming.

Moving or renaming repos in other locations is now covered by other checks.

api: fix repo group permission check for repo creation

hg.create.repository is only controlling whether all users have write access at
top level. For other repo locations, check for write permission to the repo
group.

Note: This also covers "repo creation" by forking or by moving another repo.

imports: try to use global imports unless it is a layering violation

To minimize the impact, do imports that violate the layering at runtime instead
of at import time.

lib: remove now unused class Optional

Last usage was removed in previous commit.

lib: remove now unused class Oattr/OptionalAttr

Last usage was removed in previous commit.

api: new method: edit_reviewers

Allow adding and removing reviewers of a pull request with the API call
'edit_reviewers', taking a pull request ID and a list of usernames or
userids to add and remove. For single-user operation, a string can be used
instead of a list.

Note that the 'kallithea-api' tool only accepts strings, so can only perform
single-user operations. Use python 'requests', 'curl' or similar instead if
you need to operate on multiple users at a time.

api: extend get_pullrequest data with created_on and updated_on fields

The get_pullrequest API call returned most attributes of a pullrequest, but
not created_on and updated_on.

Set microseconds to 0, as done for modified_at in ChangesetStatus. See
commit 71033bd37b4c.

isort: upgrade to 5.1.2

The changes to non-top-level imports seem nice.

tests: consistent use of more helpful raise_exception "fixture"

"Total Crash" was too misleading when debugging problems.

db: drop Repository CacheInvalidation

The benefit of this functionality is questionable. Especially in bigger setups
with multiple front-end instances all serving the same multitude of
repositories, making the hit rate very low. And the overhead of storing cache
invalidation data *in* the database is non-trivial.

We preserve a small cache in Repository SA records, but should probably just in
general know what we are doing and not ask for the same information multiple
times in each request.

lib: clean up ext_json and how it is used - avoid monkey patching

Note that py3 json.dumps will return ASCII (with all unicode escaped) encoded
as str. But we generally want JSON as bytes (which json.loads also can read),
so also wrap the result with ascii_bytes in many places.

cleanup: get rid of most "import *"

Apply script generated with the following hack:
(
hg loc '*.py'|xargs pyflakes-2 | sed -rn "s/([^:]*):.*'(.*)' may be undefined, or defined from star imports.*/sed -ri 's,\\\\<\2\\\\>([^=]|$),XXXX.\2\\\\1,g' \1/gp" | sort -u
hg loc '*.py'|xargs pyflakes-2 | sed -rn "s/([^:]*):.* undefined name '(.*)'$/sed -ri 's,\\\\<\2\\\\>([^=]|$),XXXX.\2\\\\1,g' \1/gp" | sort -u
hg loc '*.py'|xargs pyflakes-2 | sed -rn "s/([^:]*):.*'(from .*)\.([^.]*) import \*' used.*/sed -ri 's,\\\\<XXXX\\\\.,\3.,g' \1/gp" | sort -u
hg loc '*.py'|xargs pyflakes-2 | sed -rn "s/([^:]*):.*'(from .*)\.([^.]*) import \*' used.*/sed -ri 's,\2\\\\.\3 .*,\2 import \3,g' \1/gp" | sort -u
) | grep -v kallithea/bin/kallithea_cli_ishell.py > fix2.sh

py3: add b'' annotations in some places where they will be needed later

Mostly entirely trivial adding of b prefix that is a ignored for py2 ... and
also a bit of related trivial reformatting/refactorings.

tests: fix ordering of options of "ret_type must be one of ..." returned from api

Py3 would fail becauase it use another ordering.

auth: explicit user permission should not blindly overrule permissions through user groups

Before, explicit permissions of a user could shadow higher permissions that
would otherwise be obtained through a group the user is member of.
That was confusing and fragile: *removing* a permission could then suddenly
give a user *more* permissions.

Instead, change the flag for controlling internal permission computation to
*not* use "explicit". Permissions will then add up, no matter if they are
explicit or through groups.

The change in auth.py is small, but read the body of __get_perms to see the
actual impact ... and also the clean-up changeset that will come next.

This might in some cases be a behaviour change and give users more access ...
but it will probably only give the user that was intended. This change can thus
be seen as a bugfix.

Some tests assumed the old behaviour. Not for good reasons, but just because
that is how they were written. These tests are updated to expect the new
behaviour, and it has been reviewed that it makes sense.

Note that this 'explicit' flag mostly is for repo permissions and independent
of the 'user_inherit_default_permissions' that just was removed and is about
global permissions.

locking: drop the pull-to-lock / push-to-unlock functionality

The feature is not worth the maintenance cost. The locking is too coarse and
unflexible with insufficient UI and UX. The implementation is also quite
invasive in tricky areas of the code, and thus high maintenance.
Dropping this will enable other cleanup ... or at least make it easier.

tests: make lock tests stable against running 'py.test -k user'

Add some cleanup, and merge some tests to make sure they really have a
well-known starting point.

repos: introduce low level slug check of repo and group names

The high level web forms already slug-ify repo and repo group names. It might
thus not create the exact repo that was created, but the name will be "safe".

For API, we would rather have it fail than not doing exactly what was requested.

Thus, always verify at low level that the provided name wouldn't be modified by
slugification. This makes sure the API provide allow the same actual names as
the web UI.

This will only influence creation and renaming of repositories and repo groups.
Existing repositories will continue working as before.

This is a slight API change, but it makes the system more stable and can
prevent some security issues - especially XSS attacks.

This issue was found and reported by
Kacper Szurek
https://security.szurek.pl/

tests: clarify that fixture.create_repo_group takes a name, not a path

Don't pass a path on to the model layer - it can't and won't handle it correctly.

repos: only allow api repo creation in existing groups

Fix problem with '../something' paths being allowed; '..' will always exist and
can't be created.

This also introduce a small API change: Repository groups must now exist before
repositories can be created. This makes the API more explicit and simpler.

This issue was found and reported by
Kacper Szurek
https://security.szurek.pl/

repos: introduce low level check of clone URIs to prevent direct file system access to local repos

This is already checked in web form validation, but also check at low level to
make sure API access enforce the same invariants.

This issue was found and reported by
Kacper Szurek
https://security.szurek.pl/

repos: introduce low level slug check of repo and group names

The high level web forms already slug-ify repo and repo group names. It might
thus not create the exact repo that was created, but the name will be "safe".

For API, we would rather have it fail than not doing exactly what was requested.

Thus, always verify at low level that the provided name wouldn't be modified by
slugification. This makes sure the API provide allow the same actual names as
the web UI.

This will only influence creation and renaming of repositories and repo groups.
Existing repositories will continue working as before.

This is a slight API change, but it makes the system more stable and can
prevent some security issues - especially XSS attacks.

This issue was found and reported by
Kacper Szurek
https://security.szurek.pl/

tests: clarify that fixture.create_repo_group takes a name, not a path

Don't pass a path on to the model layer - it can't and won't handle it correctly.

repos: only allow api repo creation in existing groups

Fix problem with '../something' paths being allowed; '..' will always exist and
can't be created.

This also introduce a small API change: Repository groups must now exist before
repositories can be created. This makes the API more explicit and simpler.

This issue was found and reported by
Kacper Szurek
https://security.szurek.pl/

repos: introduce low level check of clone URIs to prevent direct file system access to local repos

This is already checked in web form validation, but also check at low level to
make sure API access enforce the same invariants.

This issue was found and reported by
Kacper Szurek
https://security.szurek.pl/

api: add max_revisions option to get_changesets

The returning JSON can become pretty big and hard to parse.
Therefore add an option that allows a client to request the changesets in
smaller chuncks.

vcs: fix get_changesets filtering on hg repo to AND the criteria instead of OR

Mercurial scmutil.revrange takes a list of filters ... and OR them.
But when for example a user uses the api and sets branch name and date, he would
expect to only get revisions from the provided branch.
So we need to use AND when filtering.

When using AND, the special handling of start_date and end_date is no longer necessary.

Also add a test to check for this use case.

tests: api: add test for pulling from a custom remote

This test accompanies commit 85d812ab4c64.

tests: api: cover original behavior for 'pull'

Up to commit 85d812ab4c64, the api 'pull' method can either pull from the
repository's specified remote (clone_uri) or, if the repository is a fork,
from the fork origin.

The existing test case only covered pulling from a remote, and pulling from
a repo (non-fork) that does not have a remote specified.

Update the test method naming to clarify this, and add a test to pull from a
fork.

codingstyle: replace ".has_key(...)" with "... is in ..."

Reported by flake8.

codingstyle: replace comparison for equality against None with "is" expression

Both style and correctness.

Reported by flake8.

codingstyle: trivial whitespace fixes

Reported by flake8.

api: change precision of ChangesetStatus.modified_at to seconds

Per default MySQL only uses seconds. So in order to be consistent on all
databases, this is the easiest solution. I don't think the microseconds are
necessary. And AFAICS mercurial only uses seconds for the changeset
modification time as well. So why should we use microseconds for ChangesetStatus.

Without this, for example test_api_get_changeset_with_reviews fails, because of
datetime mismatch if MySQL is used.

sqlalchemy: fix warnings from running the test suite

Mainly warnings about strings being passed where unicode was expected.

api: add get_pullrequest and comment_pullrequest methods

Modified by Mads Kiilerich, mainly to let the test helper function
create_pullrequest use model directly.

api: add optional branches, tags and bookmarks information to get_repo API data

Modified by Mads Kiilerich to make these revision names optional - there can be
a lot of them.

Add JSON-RPC API for reading changeset status

Allows reading a changeset status as json. This is useful for e.g. reporting,
extracting review status for all changesets between two releases.

cleanup: refer less to User.DEFAULT_USER

Down the road we might want to identify the default user in another
way than by username.

cleanup: use list comprehensions

It's often the same number of lines, but avoids introducing a needless
"result" variable, and makes the item expression stand out more clearly.

It's also a tiny bit faster, but the readability gains is what matters.

db: remove superfluous Session.add calls

Don't re-add objects to the SQLAlchemy Session just because they were
modified. Session.add is only for freshly constructed objects that
SQLAlchemy doesn't know about yet.

The rules are quite simple:

When creating a database object by calling the constructor directly, it
must explicitly be added to the session.

When creating an object using a factory function (like "create_repo"),
the returned object has already (by convention) been added to the
session, and should not be added again.

When getting an object from the session (via Session.query or any of the
utility functions that look up objects in the database), it's already
added, and should not be added again. SQLAlchemy notices attribute
modifications automatically for all objects it knows about.

db: inline calls to get_all

This method saves basically no typing, compared to "query().all()".
Additionally, "all()" returns a list, forcing all records to be loaded
into a memory at the same time, but some callers just need to iterate
over the objects one at a time, in which case "query()" alone is more
efficient. In one case, the caller can even use "count()" and avoid
loading any objects from the database at all.

tests: add as little code as possible in __init__.py

kallithea/tests/__init__.py contained quite a lot of code, including the test
base class TestController. This in itself may be considered bad practice.

Specifically, this poses a problem when using pytest 3.0+, in which asserts
in some files are not automatically rewritten to give improved assert
output. That problem can be fixed by explicitly registering such files for
assertion rewriting, but that register call should be executed _before_ said
files are imported. I.e. if the register call is in
kallithea/tests/__init__.py, assert calls in __init__.py itself can not be
rewritten.

Since the TestController base class does effectively contain asserts, and we
do not want to execute the register call from somewhere outside the
kallithea/tests directory, we need to move the TestController class to
another file (kallithea/tests/base.py) so we can have a register call in
__init__.py before loading base.py.

While not strictly necessary to fix the mentioned pytest problem, we take
the opportunity to fully clean __init__.py and move everything to
the new kallithea/tests/base.py. While doing so, unnecessary imports are
removed, and imports are ordered alphabetically. Explicit imports of symbols
from modules that were already imported as a whole, are removed in favor of
fully qualifying the references (e.g. tempfile._RandomNameSequence).

api: fail when given unknown arguments

This make the API more type safe and can help developers fix issues early.

model: inline BaseModel.get_all

This is just needless indirection (and doesn't actually add any
abstraction on top of the database object), so inline all calls to it.

Don't touch PullRequestModel.get_all, though, since it is an entirely
unrelated method that just shadows the inherited get_all.

api: fix forking of repos in repo groups (Issue #210)

Patch by Alexandre Beaulieu.

api: avoid duplicating group name when updating repo (Issue #37)

The api incorrectly passed repo.repo_name as repo_name, and the model update
function incorrectly always required repo_name.

pytest migration: api: switch to standard assert statements

Use unittest2pytest to replace unittest-style assert statements (e.g.
assertEqual) with standard Python assert statements to benefit from pytest's
improved reporting on assert failures.

The conversion by unittest2pytest was correct.

tests: api: fix intertest dependency on repository locking

In test classes based on unittest, tests are executed in alphabetical order.
In test classes based on pytest, tests are executed in the order they are
specified. This difference revealed a problem in the API tests:

- test_api_lock_repo_lock_optional_locked locks the test repository
- test_api_get_locks_regular_user gets the current locks and expects it to
be empty

With unittest as base class, this worked fine because the 'get_locks' group
of tests are executed before the 'lock_repo' group (alphabetical order).
Using a real pytest-based test class, the order is swapped and the locked
repository from the first test invalidates the preconditions of the second
test.

Fix this specific problem by releasing the lock from
test_api_lock_repo_lock_optional_locked.

This commit does not fix other interdependencies between tests. For example,
test_api_lock_repo_lock_optional_locked expects the existing lock state to
be 'locked' but did not lock the repo itself; instead it expects a previous
test to have locked. In practice, this is
test_api_lock_repo_lock_aquire_optional_userid.
A full solution would make each test fully self contained so that tests can
be executed in random order. The pytest extension pytest-random can help
detecting these problems.

tests: finally block also needs to be executed if api_call itself fails

The cleanup code (finally block) should be executed as soon as something what
changes the db fails.

I don't remember which one but one of these tests failed and caused all the
others to fail as well because the database wasn't cleaned up after the first
failure. That made finding the root cause more difficult then it should have
been.

tests: don't use the TESTS_TMP_PATH string

it differs each time you run the test
and therefore doesn't work if you use a separat kallithea test instance
(KALLITHEA_NO_TMP_PATH=1)

instead get the 'real' path from the DB

FIXME: breaks kallithea/tests/functional/test_admin_repos.py

tests: more consistently use unicode where unicode is expected

Nothing but extra u annotation to turn str constants into unicode.

This has been verified by hacking sqlalchemy to fail if wrong string types are passed.

cleanup: use example.com for tests and examples

example.com is explicitly reserved for this purpose. Using that means
we won't accidentally hammer a real server or real email address if an
example value escapes into the wild, e.g. in an automated test.

The domain "kallithea.example.com" has been used throughout to refer to the
example Kallithea server.

remove vestiges of Python 2.5 support

We only support Python 2.6 and 2.7; hence we do not need to import
with-statement support from __future__.

cleanup: fix some risky variable redefinitions

Found with pyflakes.

auth: construct AuthUser from either user_id or db.User object

If the caller already has the database User object, there's no reason
for AuthUser to look it up again.

The `api_key` lookup functionality is dropped, because 1) it's only
used in one place, and 2) it's simple enough for the caller to do the
lookup itself.

The `user_id` lookup functionality is kept, because 1) it's frequently
used, and 2) far from a simple `User.get(id)` lookup, it has a complex
interaction with UserModel. (That cleanup will have to wait for another
day.)

All calls of the form `AuthUser(user_id=x.user_id)` can be replaced with
`AuthUser(dbuser=x)`, assuming `x` is a db.User. However, verifying that
assumption requires a manual audit of every call site, since `x` might
also be another `AuthUser` object, for instance. Therefore, only the
most obvious call sites have been fixed here.

UserModel: remove methods that are redundant with db.User

UserModel().get_by_api_key is exactly equivalent to User.get_by_api_key.

UserModel's get_by_username and get_by_email are not exactly identical
to their db.User counterparts, due to a difference in the order of
optional arguments. Fortunately, these optional arguments are never
used.