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

Refactor: Move login out of models #16199

Merged
merged 63 commits into from
Jul 24, 2021

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Jun 18, 2021

models does far too much. In particular it handles all UserSignin.

It shouldn't be responsible for calling LDAP, SMTP or PAM for signing in.

Therefore we should move this code out of models.

This code has to depend on models - therefore it belongs in services.

There is a package in services called auth and clearly this functionality belongs in there.

Plan:

  • Change auth.Auth to auth.Method - as they represent methods of authentication.
  • Move models.UserSignIn into auth
  • Move models.ExternalUserLogin
  • Move most of the LoginVia* methods to auth or subpackages
  • Move Resynchronize functionality to auth
    • Involved some restructuring of models/ssh_key.go to reduce the size of this massive file and simplify its files.
  • Move the rest of the LDAP functionality in to the ldap subpackage
  • Re-factor the login sources to express an interfaces auth.Source?
    • I've done this through some smaller interfaces Authenticator and Synchronizable - which would allow us to extend things in future
  • Now LDAP is out of models - need to think about modules/auth/ldap and I think all of that functionality might just be moveable
  • Similarly a lot Oauth2 functionality need not be in models too and should be moved to services/auth/source/oauth2
    • modules/auth/oauth2/oauth2.go uses xorm... This is naughty - probably need to move this into models.
    • models/oauth2.go - mostly should be in modules/auth/oauth2 or services/auth/source/oauth2
  • More simplifications of login_source.go may need to be done
  • Allow wiring in of notify registration - this can now easily be done - but I think we should do it in another PR - see [Question] Send registration email when users login using LDAP for the first time #16178
  • More refactors...?
    • OpenID should probably become an auth Method but I think that can be left for another PR
    • Methods should also probably be cleaned up - again another PR I think.
    • SSPI still needs more refactors.

@zeripath zeripath added pr/wip This PR is not ready for review type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Jun 18, 2021
@zeripath zeripath added this to the 1.16.0 milestone Jun 18, 2021
@zeripath zeripath force-pushed the move-login-out-of-models branch 2 times, most recently from a6ab7f3 to 5716bc1 Compare June 19, 2021 07:21
@codecov-commenter
Copy link

codecov-commenter commented Jun 19, 2021

Codecov Report

Merging #16199 (11c5a8a) into main (81091c4) will decrease coverage by 0.01%.
The diff coverage is 44.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16199      +/-   ##
==========================================
- Coverage   45.49%   45.48%   -0.02%     
==========================================
  Files         718      746      +28     
  Lines       84317    84315       -2     
==========================================
- Hits        38363    38348      -15     
- Misses      39791    39798       +7     
- Partials     6163     6169       +6     
Impacted Files Coverage Δ
cmd/admin.go 0.00% <0.00%> (ø)
models/oauth2.go 27.27% <0.00%> (-0.73%) ⬇️
models/oauth2_application.go 69.11% <ø> (-0.29%) ⬇️
modules/cron/tasks_basic.go 85.43% <0.00%> (ø)
routers/init.go 57.14% <0.00%> (ø)
routers/web/user/auth_openid.go 0.00% <0.00%> (ø)
routers/web/user/setting/account.go 25.00% <0.00%> (ø)
routers/web/user/setting/security.go 32.78% <0.00%> (ø)
services/auth/oauth2.go 49.18% <0.00%> (-0.06%) ⬇️
services/auth/reverseproxy.go 0.00% <ø> (ø)
... and 81 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 81091c4...11c5a8a. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 19, 2021
@zeripath zeripath force-pushed the move-login-out-of-models branch 2 times, most recently from 8f9cafb to 9f0c8a5 Compare June 19, 2021 20:57
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
- move functions from models/user.go that relate to ssh_key to ssh_key
- split ssh_key.go to try create clearer function domains for allow for
future refactors here.

Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

updated to include #16465

…in-out-of-models

Signed-off-by: Andrew Thornton <art27@cantab.net>
@6543 6543 self-requested a review July 17, 2021 12:58
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 17, 2021
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 24, 2021
@zeripath
Copy link
Contributor Author

make lgtm work

@zeripath zeripath merged commit 5d2e11e into go-gitea:main Jul 24, 2021
@zeripath zeripath deleted the move-login-out-of-models branch July 24, 2021 10:16
@zeripath zeripath mentioned this pull request Jul 24, 2021
2 tasks
zeripath added a commit to zeripath/gitea that referenced this pull request Jul 24, 2021
There is a regression in go-gitea#16199 whereby the add authentication page
fails to react to the change in selected type.

This is due to the String() method on the LoginSourceType which is ameliorated
with an Int() function being added.

Following on from this there are a few other related bugs.

Fix go-gitea#16541

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit that referenced this pull request Jul 25, 2021
* Fix add authentication page

There is a regression in #16199 whereby the add authentication page
fails to react to the change in selected type.

This is due to the String() method on the LoginSourceType which is ameliorated
with an Int() function being added.

Following on from this there are a few other related bugs.

Fix #16541

Signed-off-by: Andrew Thornton <art27@cantab.net>
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 10, 2021
`models` does far too much. In particular it handles all `UserSignin`.

It shouldn't be responsible for calling LDAP, SMTP or PAM for signing in.

Therefore we should move this code out of `models`.

This code has to depend on `models` - therefore it belongs in `services`.

There is a package in `services` called `auth` and clearly this functionality belongs in there.

Plan:

- [x] Change `auth.Auth` to `auth.Method` - as they represent methods of authentication.
- [x] Move `models.UserSignIn` into `auth`
- [x] Move `models.ExternalUserLogin`
- [x] Move most of the `LoginVia*` methods to `auth` or subpackages
- [x] Move Resynchronize functionality to `auth`
  - Involved some restructuring of `models/ssh_key.go` to reduce the size of this massive file and simplify its files.
- [x] Move the rest of the LDAP functionality in to the ldap subpackage
- [x] Re-factor the login sources to express an interfaces `auth.Source`?
  - I've done this through some smaller interfaces Authenticator and Synchronizable - which would allow us to extend things in future
- [x] Now LDAP is out of models - need to think about modules/auth/ldap and I think all of that functionality might just be moveable
- [x] Similarly a lot Oauth2 functionality need not be in models too and should be moved to services/auth/source/oauth2
  - [x] modules/auth/oauth2/oauth2.go uses xorm... This is naughty - probably need to move this into models.
  - [x] models/oauth2.go - mostly should be in modules/auth/oauth2 or services/auth/source/oauth2 
- [x] More simplifications of login_source.go may need to be done
- Allow wiring in of notify registration -  *this can now easily be done - but I think we should do it in another PR*  - see go-gitea#16178 
- More refactors...?
  - OpenID should probably become an auth Method but I think that can be left for another PR
  - Methods should also probably be cleaned up  - again another PR I think.
  - SSPI still needs more refactors.* Rename auth.Auth auth.Method
* Restructure ssh_key.go

- move functions from models/user.go that relate to ssh_key to ssh_key
- split ssh_key.go to try create clearer function domains for allow for
future refactors here.

Signed-off-by: Andrew Thornton <art27@cantab.net>
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 10, 2021
* Fix add authentication page

There is a regression in go-gitea#16199 whereby the add authentication page
fails to react to the change in selected type.

This is due to the String() method on the LoginSourceType which is ameliorated
with an Int() function being added.

Following on from this there are a few other related bugs.

Fix go-gitea#16541

Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/authentication type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants