Disabling application elements fixed

[pboguslawski]

Gitea does not remove all elements (UI, cron tasks) of some
disabled features which makes UI unnecessarily complicated.

This mod removes these unnecessary elements from UI/cron:

• token edition in user settings is not available
  when ENABLE_SWAGGER=false,

• application tab is not available in user settings
  if swagger and oauth2 are both disabled,

• SSH keys editor is not displayed if SSH is disabled
  and tab name changed to more general "Keys",

• resync_all_sshkeys cron task is disabled if SSH
  is disabled,

• deploy keys tab is not displayed if SSH is disabled,

• git hook access option hidden in user settings when
  git hooks are disabled,

• git hook sync cron task removed when git hooks
  are disabled.

Author-Change-Id: IB#1105071

[techknowlogick]

Even with git hooks disabled and admin would still need access to the refresh all git hooks. The disabling of access to git hooks is to prevent arbitrary changes, whereas the button in dashboard sets it to what it should be

[pboguslawski]

Even with git hooks disabled and admin would still need access to the refresh all git hooks.

What for? If gitea need some internal stuff in git hooks then it should configure it/upgrade it without admin intervention probably. Any reason to have cron job available for admin even if git hooks changes are disabled with DISABLE_GIT_HOOKS=true?

Disabling git hooks should prevent any "human" changes in git hooks, even by admin. If manual repair is required - admin may temporarily set DISABLE_GIT_HOOKS=false + run cron job + disable hooks again after.

The disabling of access to git hooks is to prevent arbitrary changes, whereas the button in dashboard sets it to what it should be

There should be an option do disallow any access to git hooks for security reasons. DISABLE_GIT_HOOKS=true seems best option to archive it.

[CirnoT]

Please rebase your fork, your recent PRs are opened straight off with conflicts.

[silverwind]

There is a "API" Link in the bottom right of the footer which could also be hidden if DisableSwagger is true.

Scratch that, it's already handled.

[lafriks]

DISABLE_GIT_HOOKS does not disable Gitea git hooks as they are essential for Gitea to work but task you are disabling are actually exactly for that

[silverwind]

Should consolidate existing EnableSwagger context variable created here to either use the new DisableSwagger helper or set the same EnableSwagger context so it's consistent.

I'm generally leaning towards having Enable* context variables over Disable*.

[pboguslawski]

DISABLE_GIT_HOOKS does not disable Gitea git hooks as they are essential for Gitea to work but task you are disabling are actually exactly for that

Is there any way to change or break git hooks in gitea repos when DISABLE_GIT_HOOKS=true (other than intentional messing from shell outside of gitea - this may be repaired with tmp setting change and running task)?

Any reason for repairing repo git hooks after initial repo creation with correct gitea hooks if DISABLE_GIT_HOOKS was always true (=should forbid any changes in git hooks by gitea)?

If newer gitea need to update its hooks after upgrade to newer gitea version it should probably do migration like in DB (migration cli?) and not require additional cron tasks.

[pboguslawski]

but task you are disabling are actually exactly for that

Remote touching git hooks is risky so DISABLE_GIT_HOOKS should disable touching it permanently, even with cron tasks if not absolutely necessarry. If repairing / mirgation is required - should probably be done using cli during service downtime.

[pboguslawski]

Recreating hooks during gitea web downtime using gitea cli seems also safer solution to avoid races during short period when files are absent during recreation in https://github.com/go-gitea/gitea/blob/master/modules/repository/hooks.go#L56

[CirnoT]

Any reason for repairing repo git hooks after initial repo creation with correct gitea hooks if DISABLE_GIT_HOOKS was always true (=should forbid any changes in git hooks by gitea)?

Changing configuration, moving servers and probably few other edge cases where they might get outdated.

resync_all_sshkeys cron task is disabled if SSH
is disabled,

Might also want to hide it when SSH_CREATE_AUTHORIZED_KEYS_FILE is false

[pboguslawski]

Please rebase your fork, your recent PRs are opened straight off with conflicts.

According to The Golden Rule of Rebasing...

https://www.atlassian.com/git/tutorials/merging-vs-rebasing#the-golden-rule-of-rebasing

...we do not rebase public branches. Just merged current master to this PR and resolved conflicts.

Should consolidate existing EnableSwagger context variable created here to either use the new DisableSwagger helper or set the same EnableSwagger context so it's consistent.

Removed redundant DisableSwagger; existing EnableSwagger is used instead.

Changing configuration, moving servers and probably few other edge cases where they might get outdated.

All sounds like admin cli tasks under downtime not cron business (see comment about race above also).

Might also want to hide it when SSH_CREATE_AUTHORIZED_KEYS_FILE is false

Dependency added (also && !setting.SSH.StartBuiltinServer like in ssh_key.go > rewriteAllPublicKeys()). New task registerRewriteAllPrincipalKeys will be disabled also in similar way.

[codecov-io]

Codecov Report

Merging #13129 into master will decrease coverage by 0.05%.
The diff coverage is 48.00%.

@@            Coverage Diff             @@
##           master   #13129      +/-   ##
==========================================
- Coverage   42.08%   42.02%   -0.06%     
==========================================
  Files         681      681              
  Lines       75121    75138      +17     
==========================================
- Hits        31613    31577      -36     
- Misses      38357    38414      +57     
+ Partials     5151     5147       -4     

Impacted Files	Coverage Δ
routers/user/setting/applications.go	39.34% <30.76%> (-2.17%)	⬇️
modules/cron/tasks_extended.go	56.12% <33.33%> (-13.36%)	⬇️
modules/templates/helper.go	47.89% <100.00%> (+0.63%)	⬆️
services/pull/check.go	33.57% <0.00%> (-15.33%)	⬇️
modules/indexer/stats/db.go	52.17% <0.00%> (-8.70%)	⬇️
modules/charset/charset.go	68.53% <0.00%> (-2.25%)	⬇️
services/gitdiff/gitdiff.go	74.88% <0.00%> (-1.77%)	⬇️
modules/queue/unique_queue_disk_channel.go	53.84% <0.00%> (-1.54%)	⬇️
models/pull.go	54.54% <0.00%> (-0.63%)	⬇️
... and 5 more

---

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 25f7e1c...e3d4d70. Read the comment docs.

[pboguslawski]

Just added one more enhancement - removing external account management when openid signin/up is disabled. Not 100% sure if its done right so please verify before merging.

[pboguslawski]

One mode change: 4b867e9 introduces DISABLE_2FA parameter in [security] section
of app.ini (by default set to false). If set to true it disables access to 2FA feature in
user preferences (not required in some environments i.e. when reverse proxy auth
is used). Authentication code using 2FA and any existing 2FA configuration are
left untouched.

This mod hides also security tab in user preferences when openid is also
disabled; for this reason this mod is not separate PR but exiting PR
enhancement.

[silverwind]

Would suggest not piling too many changes into one PR as it get's harder to review every time :)

[lafriks]

Why would there be need to disable 2FA to me it seems unnecessary

[pboguslawski]

Why would there be need to disable 2FA to me it seems unnecessary

2FA in gitea does not work if header auth is enabled so its unnecessarry in this scenario; more, 2FA should be an option not requirement (i.e. reverse proxy may take care of whole auth stuff); we want to use as clean vcs UI as possible without unnecessarry stuff thats why this parameter.

BTW: please consider accepting new optional gitea features only if provided with parameter to disable it if not used.

[pboguslawski]

Would suggest not piling too many changes into one PR as it get's harder to review every time

We don't plan to extend this PR; 2FA mod was included here because this mod hides also security tab which depends on 2FA switch.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

[wxiaoguang]

Thanks for the PR. After reading the code and the comments, I feel that there are still many problems here.

1. Too many changes are mixed together, and main branch has changed a lot, many conflicts here.
2. DISABLE_2FA should never be used. If you do not need it, just leave it there, it won't cause any problem.
3. The non-English locale files should not be changed in PR, they are managed by crowdin automatically.
4. EnableSwagger is used to control the Swagger API Document, but this PR uses it for DeleteApplication / loadApplicationsData, I think it is incorrect.

I have a feeling that if some UI elements need to be hidden, we need a new and clear PR for that, instead of mixing things together. Since this PR has been stale for a long time, maybe we could close this one and use a new cherry-picked PR for a clear purpose.

[pboguslawski]

Too many changes are mixed together, and main branch has changed a lot, many conflicts here

Will spit changes to separate PR with update to current main.

DISABLE_2FA should never be used. If you do not need it, just leave it there, it won't cause any problem.

DISABLE_2FA should be used in systems where reverse proxy handles auth with strong enough solutions (2FA seems not even work when reverse proxy is enabled in gitea...) and admin don't want to leave unnecessary elements in UI. This mod was spilt to #18481.

[pboguslawski]

The non-English locale files should not be changed in PR, they are managed by crowdin automatically.

Disabling unnecessary SSH elements was split into #18482 and only the EN locale was adjusted.

[pboguslawski]

Disabling unnecessary GitHooks elements was split into #18485.

[pboguslawski]

EnableSwagger is used to control the Swagger API Document, but this PR uses it for DeleteApplication / loadApplicationsData, I think it is incorrect.

Thanks for this explanation; option to disable access tokens was split into #18488 with separate new parameter.

[pboguslawski]

Disabling unnecessary OpenID elements was split into #18491.