-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a6ab7f3
to
5716bc1
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
8f9cafb
to
9f0c8a5
Compare
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>
9f0c8a5
to
bb1ff63
Compare
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>
updated to include #16465 |
…in-out-of-models Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
6543
approved these changes
Jul 17, 2021
khmarbaise
approved these changes
Jul 24, 2021
make lgtm work |
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>
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.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
models
does far too much. In particular it handles allUserSignin
.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 inservices
.There is a package in
services
calledauth
and clearly this functionality belongs in there.Plan:
auth.Auth
toauth.Method
- as they represent methods of authentication.models.UserSignIn
intoauth
models.ExternalUserLogin
LoginVia*
methods toauth
or subpackagesauth
models/ssh_key.go
to reduce the size of this massive file and simplify its files.auth.Source
?