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

[Question] Send registration email when users login using LDAP for the first time #16178

Closed
rlukata opened this issue Jun 16, 2021 · 5 comments · Fixed by #16523
Closed

[Question] Send registration email when users login using LDAP for the first time #16178

rlukata opened this issue Jun 16, 2021 · 5 comments · Fixed by #16523
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active.

Comments

@rlukata
Copy link

rlukata commented Jun 16, 2021

I have setup Gitea to only use LDAP bind as authentication store, and users only need to enter their username/email and their ldap password to login and get an account created.

Is there a way to send a registration email to users who login via LDAP for the first time?

Here's the service snippet of my app.ini:

[service]
SHOW_REGISTRATION_BUTTON	  = false
REGISTER_EMAIL_CONFIRM            = true
ENABLE_NOTIFY_MAIL                = true
DISABLE_REGISTRATION              = false
ALLOW_ONLY_EXTERNAL_REGISTRATION  = true
ENABLE_CAPTCHA                    = false
REQUIRE_SIGNIN_VIEW               = true
DEFAULT_KEEP_EMAIL_PRIVATE        = false
DEFAULT_ALLOW_CREATE_ORGANIZATION = true
DEFAULT_ENABLE_TIMETRACKING       = true
NO_REPLY_ADDRESS                  = noreply.localhost
MINIMUM_KEY_SIZE_CHECK		  = false
@zeripath
Copy link
Contributor

I'm afraid not at present - these are auto registrations.

@zeripath zeripath added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jun 16, 2021
@rlukata
Copy link
Author

rlukata commented Jun 16, 2021

I'm afraid not at present - these are auto registrations.

Can this be requested as a feature? If yes, what do I need to do?

@zeripath
Copy link
Contributor

OK... so I've just had a look at doing this.

It's a bit more involved than I would have hoped.

  • First we'd need to add SendRegisterNotifyMail to models.LoginSource
  • We'd need to refactor the actual functionality of LoginViaLDAP etc out of models likely into services
  • Once that's happened we could use mailer.SendRegisterNotifyMail
  • Then we need to adjust the UI for login sources to add this option.

The refactoring in step two is the difficult step. (It needs to be done though as this functionality doesn't really belong in models.)

@zeripath
Copy link
Contributor

OK, #16199 does the basics to allow this to be possible, however I think we need some more details about how you think this would/should work.

  1. PAM, Oauth2, SMTP and LDAP can all cause autoregistration - do you think that notification should be done for all of these - always? As a global setting?
  2. If not do you think this should be an option for each individual login source?
  3. Some sources may cause registration of a user who is prohibited from logging in. Should this notification still occur?

@rlukata
Copy link
Author

rlukata commented Jun 21, 2021

PAM, Oauth2, SMTP and LDAP can all cause autoregistration - do you think that notification should be done for all of these - always? As a global setting?

Yes, and no.

If not do you think this should be an option for each individual login source?

Yes this would be ideal. In addition, I think it should be enabled by default.

Some sources may cause registration of a user who is prohibited from logging in. Should this notification still occur?

Yes, if an account gets created in the DB I think a notification should still occur. This is advantageous especially when troubleshooting: a user can use the email to claim they've "registered".

zeripath added a commit that referenced this issue Jul 24, 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 #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>
zeripath added a commit to zeripath/gitea that referenced this issue Jul 24, 2021
When users login and are autoregistered send email notification.

Fix go-gitea#16178

Signed-off-by: Andrew Thornton <art27@cantab.net>
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this issue 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>
lafriks pushed a commit that referenced this issue Aug 12, 2021
When users login and are autoregistered send email notification.

Fix #16178

* Protect public functions within the mailer by testing if the mailer is configured

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: 6543 <6543@obermui.de>
@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
type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants