Unsanitize user and org names in DB

[pat-s]

fix #3614

As discussed in chat, preferred to use non-sanitized values everywhere.

• Store user and org names using the casing provided by the forge
• Allow to search for users / orgs using case insensitive names
• Removed all sanitation in the code
• AFAICS only the org names were stored sanitized in the DB. User names and repo names are not affected

[codecov]

Codecov Report

Attention: Patch coverage is 40.00000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 28.28%. Comparing base (fac744d) to head (e638c62).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
...ore/migration/024_unsanitize_org_and_user_names.go	39.28%	14 Missing and 3 partials ⚠️
server/store/datastore/migration/common.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4762      +/-   ##
==========================================
- Coverage   28.29%   28.28%   -0.01%     
==========================================
  Files         398      399       +1     
  Lines       28295    28318      +23     
==========================================
+ Hits         8005     8011       +6     
- Misses      19580    19594      +14     
- Partials      710      713       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[xoxys]

Tests are failing.

[xoxys]

After thinking of it again, I'm not sure if thats the best approach. Cant really say why but I see a lot of potential for issues and corner cases. Looks like this issue only occurs with forgejo/gitea and the api returns mixed capitalization while the forge only supports case-insensitive values. What do you think?

Edit: Tested it. You can create an org "Foo" in gitea and in that case its even displayed as "Foo" in the url.... But you cant create "foo" with the error "The organization name is already taken." This is a somewhat inconsistent behavior upstream, however I think we should switch back to the initial approach from @pat-s

[pat-s]

If all users are sanitized, the user list in the UI would also reflect a non-optimal state.

The simplest fix which also wouldn't require a migration would probably be to just sanitize the comparison call of user.Login and org.Name?

[qwerty287]

Why does the license header of the migration start with // Excerpt from:?

[xoxys]

This is worrisome. Can you check which forge ID got assigned to the user org? Actually, tests should catch this.

woodpecker_server=# select * from orgs;
 id | forge_id |    name    | is_user | private 
----+----------+------------+---------+---------
  1 |        0 | woodpecker | t       | f

forge_id is 0 while only one forge exists with id 1:

woodpecker_server=# select * from forges;
 id | type  |             url              |                client                |                      client_secret                       | skip_verify | oauth_host | additional_options 
----+-------+------------------------------+--------------------------------------+----------------------------------------------------------+-------------+------------+--------------------
  1 | gitea | http://woodpecker-gitea:3000 | **** | **** | f           |            | {}
(1 row)

[anbraten]

Strange, I started to add a method to the storage interface to directly get an org by name and forge-id, that might prevent such cases. Forge-id 0 sounds like not set at all (gos default for not setting a number).

[xoxys]

Yeah maybe we mixed in too much different fixes into this PR? Can we revert it to focus on the sanitizing issue and move other improvements to another PR?

I have a backup of the DB before the migration from this PR so I can also re-test this PR.

[anbraten]

Yeah maybe we mixed in too much different fixes into this PR? Can we revert it to focus on the sanitizing issue and move other improvements to another PR?

Extracted most other changes to #4817

[pat-s]

I don't know where this PR stands now and what there is do to - appreciated if somebody could clarify or apply the necessary actions.

[xoxys]

As more changes are required for #4817, can we revert those changes here so we can merge it?

[pat-s]

@xoxys Will you test this again after merge? Is a patch release planned after merge?

[xoxys]

Tested the PR already. Can login again but had no time to test fresh install and migration again.

[qwerty287]

I don't think we need a patch, we can just release 3.1.0. This bug is present in most older versions anyways...

[pat-s]

Appreciated if somebody could release 3.1.0 when it fits. Thanks for getting this merged.