Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Discontinued) Enforce two-factor authentication #16880

Closed
wants to merge 20 commits into from

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Aug 30, 2021

Disclaimer: Since this PR doesn't seem to be merged easily (#13606 (comment)), and much more work need to do.

I just keep this PR for my personal usage, and update it with main branch regularly.

Anyone who want to continue the work for Enforced 2FA can pick this PR or start a new one.


Design:

  1. A global setting security.ENFORCE_TWO_FACTOR_AUTH.
  2. A user without 2FA can login and may explore, but can NOT read or write to any repositories.
  3. Keep things as simple as possible.

@wxiaoguang wxiaoguang mentioned this pull request Aug 30, 2021
@techknowlogick techknowlogick added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Aug 30, 2021
@techknowlogick techknowlogick added this to the 1.16.0 milestone Aug 30, 2021
@lunny lunny added the pr/wip This PR is not ready for review label Aug 31, 2021
@lunny
Copy link
Member

lunny commented Aug 31, 2021

How did you control the permssion?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 31, 2021
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Aug 31, 2021

How did you control the permssion?

I just studied the logic of IsRestricted. For my understanding, the key points of permission control are accessLevel and HasAccess->getUserRepoPermission. In these two functions, if GetTwoFactorByUID fails, the return value is AccessModeNone to reject user access.

And unit tests are also updated with 2FA cases.

More thoughts: why are the RepoList related functions not changed: as long as a user without 2FA will setup 2FA soon and then can access to the repositories, listing repositories before 2FA setup can not be a security problem. So I think it's good to keep these logics simple.

@wxiaoguang wxiaoguang changed the title Enforce two-factor authentication WIP: Enforce two-factor authentication Aug 31, 2021
@codecov-commenter

This comment has been minimized.

@wxiaoguang wxiaoguang mentioned this pull request Oct 20, 2021
3 tasks
@lunny
Copy link
Member

lunny commented Oct 21, 2021

Maybe we could split it as two PRs, one is always requiring login with 2FA, another is changing the runtime permissions when login with 2fa or password only.

@wxiaoguang wxiaoguang marked this pull request as draft November 1, 2021 15:58
@lunny lunny removed this from the 1.16.0 milestone Nov 9, 2021
@wxiaoguang wxiaoguang changed the title WIP: Enforce two-factor authentication (Discontinued) Enforce two-factor authentication Nov 26, 2021
@techknowlogick
Copy link
Member

techknowlogick commented Dec 6, 2021

There are new commits to this, is it still discontinued, or is it now a WIP?

Edit: nvm. I should've read the description

@lunny
Copy link
Member

lunny commented Aug 10, 2022

Does this affect the first admin user registry?

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Aug 10, 2022

Discontinued (due to #13606 (comment)), I won't spend time on it.


I am still keeping updating my 2FA branch with main branch, and I am still using it in my instance.

@wxiaoguang wxiaoguang closed this Oct 30, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. pr/wip This PR is not ready for review type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants