OAuth2 auto-register

[mgjm]

Changes

• Refactored user creation (code deduplication, see individual commit messages: a3366c4 and
  6e2ece4)
• Added auto-registration for OAuth2 users

Settings

A new settings section (oauth2_client) is added and documented in custom/conf/app.ini.sample and content/doc/advanced/config-cheat-sheet.en-us.md.

Breaking changes?

No. The default settings reflect the current behaviour. And OAuth2 auto-register needs to be manually enabled.

Referenced Issues

Implements #3520 und solves the secondary consideration in #1036.

[codecov-io]

Codecov Report

Merging #5123 (e166d49) into master (487f2ee) will increase coverage by 0.01%.
The diff coverage is 40.12%.

@@            Coverage Diff             @@
##           master    #5123      +/-   ##
==========================================
+ Coverage   42.21%   42.23%   +0.01%     
==========================================
  Files         767      771       +4     
  Lines       81624    82064     +440     
==========================================
+ Hits        34458    34657     +199     
- Misses      41531    41756     +225     
- Partials     5635     5651      +16     

Impacted Files	Coverage Δ
models/action.go	48.52% <0.00%> (-2.42%)	⬇️
models/migrations/migrations.go	2.59% <ø> (ø)
models/migrations/v166.go	0.00% <0.00%> (ø)
models/migrations/v172.go	0.00% <0.00%> (ø)
models/migrations/v173.go	0.00% <0.00%> (ø)
models/session.go	0.00% <0.00%> (ø)
models/user.go	53.05% <ø> (+0.38%)	⬆️
modules/auth/oauth2/oauth2.go	27.27% <0.00%> (ø)
modules/context/context.go	58.05% <0.00%> (-0.45%)	⬇️
modules/indexer/code/elastic_search.go	1.72% <0.00%> (-0.02%)	⬇️
... and 60 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 b56c19d...e166d49. Read the comment docs.

[techknowlogick]

There are some related PRs that might already achieve this functionality. @coolaj86 would you be able to review?

[coolaj86]

@techknowlogick I've looked over the files to understand the changes broadly, but I'd like to look more closely later this upcoming week and see if I can manually merge into my branch.

@mgjm It looks like there's a lot of value here. You've covered some cases that I haven't covered in my PRs, but the implementation is different so it'll probably take some manual labor to merge them.

1. Can you provide some screenshots or a screencast (QuickTime Player on OS X has a record option) showing the functionality and changes?

2. Would you mind going through the login process at https://git.coolaj86.com and let me know if my implementation meets your expectations and needs or not?

3. Please take a look at UX Optional Password & Captcha for External Registration #5029 and see if there are checkboxes that are missing or that seem incorrect to you.

I don't think I handled OpenID the same as OAuth2, and it looks like you do, so that sticks out the most as something where we'll probably want pieces of both.

[mgjm]

@coolaj86 Oh, i have not seen your PRs. As far as I can see they have a different approach to new external users. In your version an external user still needs to enter a new username, email und real name. But in my version this information is fetched from the identity provider and therefore a local account can be created without additional user interaction.

I think both approaches serve different needs and could (or should) exist at the same time. For example in my use case we have a central SSO identity provider and therefore login should be possible only using this provider (and the user profile can be trusted). But in other situations (e.g. login with GitHub account) a second step to enter a username, real name and so on is the desired option.

Can you provide some screenshots or a screencast (QuickTime Player on OS X has a record option) showing the functionality and changes?

This is not really interesting. The user experience is in my version the same as it is currently for users with an already linked account. As soon as they come back from the OAuth2 provider they are logged in. In my version the same applies to users without an account. They get a new one created on the fly. So the end user will not notice a difference between their first login (with account creation) and a later login (when the account already exists).

[mgjm]

@lafriks The following settings would be possible:

• OAUTH2_USE_NICKNAME Whether to use the nickname or the userID
• OAUTH2_USERID_FALLBACK Whether to use the userID as a fallback or fail
• OAUTH2_REGISTRATION_FALLBACK Whether to fall back to the registration form
• OAUTH2_PREFILL_REGISTRATION_FORM Whether to fill the registration form with the details from the OAuth2 provider

IMHO only the first and the last option should be added. The first option (OAUTH2_USE_NICKNAME) is quite useful as it allows to use different styles of OAuth2 providers and the last option (OAUTH2_PREFILL_REGISTRATION_FORM) is probably the most useful option. E.g. a GitHub login would still require the user to create the account in the way it is right now, but the registration form could be pre filled with the account details from GitHub. The validation and error handling of the existing registration form could be used without any changes. (e.g. handling of invalid or already taken usernames)

I would not recommend the fallback options. You ether have a trusted provider where it is desired to actually fail if there is a username collision or you have an untrusted provider, but the the normal registration form with pre filled details is the better option. Especially if #5029 is merged and a password is no longer required.

And the desicion whether to do auto registration (with hard failures) or pre fill the registration form would be based on ENABLE_OAUTH2_AUTO_REGISTRATION.

[lafriks]

@mgjm I agree fallback option is probably not the best one and can have undesired consequences. Now thinking hard fail seems way to go. OAUTH2_USE_NICKNAME would be good to implement in this PR. Other options would be better to be implemented in other PR to not make this one too big and hard to review.

[mgjm]

Sounds good. Maybe it is the right time now to add a new settings section [service.oauth2] or just [oauth2]?

[lafriks]

Yes [oauth2] would be good

[mgjm]

@lafriks OAUTH2_USE_NICKNAME is now implemented and ENABLE_OAUTH2_AUTO_REGISTRATION could be added in another PR under the new [oauth2] section.

Edit: And there seems to be a problem with apt in the CI system (E: Unable to locate package git-lfs). http://deb.debian.org/debian currently gives a 404 Not Found 😢

[meredrica]

any news on this?

[calind]

It would be great if this gets merged in 1.8. Anything I can do to help?

[lunny]

When the login name is not exist but the email is empty, `` is enabled, login will result in 500.

2019/02/09 15:50:42 [I] [SQL] BEGIN TRANSACTION
2019/02/09 15:50:42 [I] [SQL] SELECT  TOP 1 "id", "lower_name", "name", "full_name", "email", "keep_email_private", "passwd", "must_change_password", "login_type", "login_source", "login_name", "type", "location", "website", "rands", "salt", "language", "created_unix", "updated_unix", "last_login_unix", "last_repo_visibility", "max_repo_creation", "is_active", "is_admin", "allow_git_hook", "allow_import_local", "allow_create_organization", "prohibit_login", "avatar", "avatar_email", "use_custom_avatar", "num_followers", "num_following", "num_stars", "num_repos", "description", "num_teams", "num_members", "diff_view_style", "theme" FROM "user" WHERE (id!=?) AND "lower_name"=? []interface {}{0, "81045"}
2019/02/09 15:50:42 [I] [SQL] SELECT  TOP 1 "id", "lower_name", "name", "full_name", "email", "keep_email_private", "passwd", "must_change_password", "login_type", "login_source", "login_name", "type", "location", "website", "rands", "salt", "language", "created_unix", "updated_unix", "last_login_unix", "last_repo_visibility", "max_repo_creation", "is_active", "is_admin", "allow_git_hook", "allow_import_local", "allow_create_organization", "prohibit_login", "avatar", "avatar_email", "use_custom_avatar", "num_followers", "num_following", "num_stars", "num_repos", "description", "num_teams", "num_members", "diff_view_style", "theme" FROM "user" WHERE (email=?) []interface {}{""}
2019/02/09 15:50:42 [I] [SQL] ROLL BACK
2019/02/09 15:50:42 [...routers/user/auth.go:983 createUserInContext()] [E] CreateUser: e-mail already in use [email: ]
2019/02/09 15:50:42 [D] Template: status/500

[mgjm]

@lunny What is the expected behaviour if the login name does not exist and the email is empty? As far as I know an account cannot be created without an email, so an error is the appropriate reaction. The OAuth2 Provider should have provided the email. If your provider allows to select whether the email is provided, you should probable not use OAuth2 autologin as it is intended for trusted providers.

[lunny]

@mgjm I think we could redirect to binding page and automatically fill some fields.

[mgjm]

@lunny This could be an alternative, but I think this would be out of the scope of this PR. See this comment. Once the OAUTH2_REGISTRATION_FALLBACK and OAUTH2_PREFILL_REGISTRATION_FORM configuration is implemented (in another PR) we could redirect to the registration page and prefill the form.

But in some use cases it should be an actual error if the email is missing. The current implementation is planed to be used in environments where registration and local accounts are not wanted. (Login only via oauth2 provider), but can be extended in future PRs.

[kvaster]

@mgjm, it looks like we should set LoginName = Name in our case. I've made a patch already.

[mgjm]

@kvaster I think we should keep the user.LoginName set to gothUser.UserID. Otherwise a user would loose access to a linked Gitea account after changing the GitHub username. The UserID is the only attribute of the user that would never change.

[kvaster]

@mgjm, seems you're right. The whole user model is not well suited for such authorization model. This is why it will be not possible to link against (for example) ldap account in case user account was created with oauth2. Currently user created with oauth2 will not have linked external account - that's why we should keep LoginName = userId for such accounts.

[kvaster]

I think that the best refactoring would be to make no difference between any authorization models. I.e. local login is also linked authorization. The only problem is how to match user during initial linking. But email or/and username/nickname may be used at this time.

[kvaster]

@mgjm, we may attach external linked account to newly created user :) But it will look 'stange'...

@lunny, authentication sign-in name is LoginName and I think that it is not used to map commits to user...

[lunny]

I suspect that LoginName should not be goth.UserID but that should be another PR. I just left #5123 (comment), otherwise LGTM.

@mgjm You can find some functions here https://github.com/go-gitea/gitea/blob/master/modules/migrations/update.go .
So there is a bug if the account is a newly created one and no link account. I think we have to force every user have a link account except local user.

Could you always create a linked account at this PR and I think I will send PRs to change other places?

[mgjm]

So this PR should create a new user by only setting Name, FullName and Email (leave LoginType, LoginSource and LoginName empty). And then we can call externalaccount.LinkAccountToUser on the new user.

This would result in the same state as if there was an existing local account (with an empty password field) and an external account has been linked.

@lunny If that is the desired behaviour, this seems doable.

[kvaster]

@mgjm, I think that LoginName should be non empty... Probably we should set LoginName = Name...

[mgjm]

@kvaster I think we should just do the same as a regular sign-up via the sign-up form. And that leaves those fields empty (not set).

[lunny]

@mgjm That means, all users should have a local record on user table. and the type should always local type and will be removed in future.

@kvaster LoginName is unused if login_type is plain, it will only be used when login_type is not plain.

[lunny]

Forcing every external user has a link account could be another PR because it will change a lot.

I think the only thing should be done in this PR is to improve the documents mentioned #5123 (comment)

[kvaster]

@mgjm, sent you PR with updated descriptions :)

[6543]

@kvaster @mgjm Thank you for your patience & effort 👍 🚀