OAuth2 auto-register #5123

techknowlogick · 2018-10-19T11:09:26Z

mgjm commented

2018-10-19 11:09:26 +00:00

(Migrated from github.com)

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.

## 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.

👍 18 ❤️ 6 🎉 4

codecov-io commented

2018-10-19 11:17:02 +00:00

(Migrated from github.com)

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.

# [Codecov](https://codecov.io/gh/go-gitea/gitea/pull/5123?src=pr&el=h1) Report > Merging [#5123](https://codecov.io/gh/go-gitea/gitea/pull/5123?src=pr&el=desc) (e166d49) into [master](https://codecov.io/gh/go-gitea/gitea/commit/487f2ee41cb06f0173b8bf12bd6a78408c75fabf?el=desc) (487f2ee) will **increase** coverage by `0.01%`. > The diff coverage is `40.12%`. [![Impacted file tree graph](https://codecov.io/gh/go-gitea/gitea/pull/5123/graphs/tree.svg?width=650&height=150&src=pr&token=t1G57YGbPy)](https://codecov.io/gh/go-gitea/gitea/pull/5123?src=pr&el=tree) ```diff @@ 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](https://codecov.io/gh/go-gitea/gitea/pull/5123?src=pr&el=tree) | Coverage Δ | | |---|---|---| | [models/action.go](https://codecov.io/gh/go-gitea/gitea/pull/5123/diff?src=pr&el=tree#diff-bW9kZWxzL2FjdGlvbi5nbw==) | `48.52% <0.00%> (-2.42%)` | :arrow_down: | | [models/migrations/migrations.go](https://codecov.io/gh/go-gitea/gitea/pull/5123/diff?src=pr&el=tree#diff-bW9kZWxzL21pZ3JhdGlvbnMvbWlncmF0aW9ucy5nbw==) | `2.59% <ø> (ø)` | | | [models/migrations/v166.go](https://codecov.io/gh/go-gitea/gitea/pull/5123/diff?src=pr&el=tree#diff-bW9kZWxzL21pZ3JhdGlvbnMvdjE2Ni5nbw==) | `0.00% <0.00%> (ø)` | | | [models/migrations/v172.go](https://codecov.io/gh/go-gitea/gitea/pull/5123/diff?src=pr&el=tree#diff-bW9kZWxzL21pZ3JhdGlvbnMvdjE3Mi5nbw==) | `0.00% <0.00%> (ø)` | | | [models/migrations/v173.go](https://codecov.io/gh/go-gitea/gitea/pull/5123/diff?src=pr&el=tree#diff-bW9kZWxzL21pZ3JhdGlvbnMvdjE3My5nbw==) | `0.00% <0.00%> (ø)` | | | [models/session.go](https://codecov.io/gh/go-gitea/gitea/pull/5123/diff?src=pr&el=tree#diff-bW9kZWxzL3Nlc3Npb24uZ28=) | `0.00% <0.00%> (ø)` | | | [models/user.go](https://codecov.io/gh/go-gitea/gitea/pull/5123/diff?src=pr&el=tree#diff-bW9kZWxzL3VzZXIuZ28=) | `53.05% <ø> (+0.38%)` | :arrow_up: | | [modules/auth/oauth2/oauth2.go](https://codecov.io/gh/go-gitea/gitea/pull/5123/diff?src=pr&el=tree#diff-bW9kdWxlcy9hdXRoL29hdXRoMi9vYXV0aDIuZ28=) | `27.27% <0.00%> (ø)` | | | [modules/context/context.go](https://codecov.io/gh/go-gitea/gitea/pull/5123/diff?src=pr&el=tree#diff-bW9kdWxlcy9jb250ZXh0L2NvbnRleHQuZ28=) | `58.05% <0.00%> (-0.45%)` | :arrow_down: | | [modules/indexer/code/elastic\_search.go](https://codecov.io/gh/go-gitea/gitea/pull/5123/diff?src=pr&el=tree#diff-bW9kdWxlcy9pbmRleGVyL2NvZGUvZWxhc3RpY19zZWFyY2guZ28=) | `1.72% <0.00%> (-0.02%)` | :arrow_down: | | ... and [60 more](https://codecov.io/gh/go-gitea/gitea/pull/5123/diff?src=pr&el=tree-more) | | ------ [Continue to review full report at Codecov](https://codecov.io/gh/go-gitea/gitea/pull/5123?src=pr&el=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/go-gitea/gitea/pull/5123?src=pr&el=footer). Last update [b56c19d...e166d49](https://codecov.io/gh/go-gitea/gitea/pull/5123?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

techknowlogick commented

2018-10-20 21:27:14 +00:00

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

coolaj86 commented

2018-10-20 22:00:30 +00:00

(Migrated from github.com)

@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.

Can you provide some screenshots or a screencast (QuickTime Player on OS X has a record option) showing the functionality and changes?
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?
Please take a look at https://github.com/go-gitea/gitea/pull/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.

@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 https://github.com/go-gitea/gitea/pull/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 commented

2018-10-20 23:05:46 +00:00

(Migrated from github.com)

@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).

@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).

lafriks (Migrated from github.com) reviewed 2018-10-20 23:44:59 +00:00

routers/user/auth.go

lafriks (Migrated from github.com) commented

2018-10-20 23:44:59 +00:00

Might be good to check if gothUser.NickName contains only allowed gitea username characters and use that and only if it contains not allowed characters than fallback to UserID as in many providers userid will be cryptic uuid

Might be good to check if `gothUser.NickName` contains only allowed gitea username characters and use that and only if it contains not allowed characters than fallback to UserID as in many providers userid will be cryptic uuid

mgjm (Migrated from github.com) reviewed 2018-10-21 11:16:41 +00:00

routers/user/auth.go

mgjm (Migrated from github.com) commented

2018-10-21 11:16:41 +00:00

This sounds good. What should be the handling if two users have the same nickname attribute. I think that should be a configuration option. Depending on whiter the nickname attribute of the provider is unique or not. Or is the nickname unique in all common providers?

lafriks (Migrated from github.com) reviewed 2018-10-21 18:51:35 +00:00

routers/user/auth.go

lafriks (Migrated from github.com) commented

2018-10-21 18:51:35 +00:00

Don't know about all, might be good if it would fallback to uid also if user with such nickname exists already

mgjm commented

2018-10-21 21:47:06 +00:00

(Migrated from github.com)

@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 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 commented

2018-10-21 22:35:46 +00:00

(Migrated from github.com)

@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 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 commented

2018-10-21 22:40:38 +00:00

(Migrated from github.com)

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

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

👍 1

lafriks commented

2018-10-22 00:57:13 +00:00

(Migrated from github.com)

Yes [oauth2] would be good

Yes `[oauth2]` would be good

mgjm commented

2018-10-23 00:11:39 +00:00

(Migrated from github.com)

@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 ?

@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` ?

👍 1

meredrica commented

2019-02-05 11:52:28 +00:00

(Migrated from github.com)

any news on this?

calind commented

2019-02-07 22:28:44 +00:00

(Migrated from github.com)

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

lunny reviewed 2019-02-09 07:51:25 +00:00

routers/user/auth.go

						
				@ -863,3 +943,4 @@

					if err := ctx.Session.Set("twofaRemember", remember); err != nil {

						log.Error("Error setting twofaRemember in session: %v", err)

					}

					if err := ctx.Session.Set("linkAccount", true); err != nil {

lunny commented

2019-02-09 07:41:12 +00:00

handleUserCreated

lunny commented

2019-02-09 07:52:45 +00:00

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

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 (Migrated from github.com) reviewed 2019-02-09 17:24:50 +00:00

routers/user/auth.go

						
				@ -863,3 +943,4 @@

					if err := ctx.Session.Set("twofaRemember", remember); err != nil {

						log.Error("Error setting twofaRemember in session: %v", err)

					}

					if err := ctx.Session.Set("linkAccount", true); err != nil {

mgjm (Migrated from github.com) commented

2019-02-09 17:24:50 +00:00

I saw this in oAuth2UserLoginCallback and copied this pattern. But I can fix this ?

I saw this in [oAuth2UserLoginCallback](https://github.com/go-gitea/gitea/blob/9504a74191738e66cd00ec739ba0a397d44b8f85/routers/user/auth.go#L628-L630) and copied this pattern. But I can fix this ?

mgjm commented

2019-02-09 17:32:58 +00:00

(Migrated from github.com)

@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 commented

2019-02-10 01:36:43 +00:00

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

mgjm commented

2019-02-10 01:54:02 +00:00

(Migrated from github.com)

@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.

@lunny This could be an alternative, but I think this would be out of the scope of this PR. See this [comment](https://github.com/go-gitea/gitea/pull/5123#issuecomment-431707034). 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.

anoff commented

2019-03-17 08:18:17 +00:00

(Migrated from github.com)

Would love to see this hit master. Imho the point of using oauth providers is to reuse accounts instead of creating new ones - the current implementation does not enable this workflow.
Also I feel pretty uncomfortable about dealing with passwords so it would be great to config Gitea to not store any user passwords at all.

Any way to help move this forward?

Would love to see this hit master. Imho the point of using oauth providers is to reuse accounts instead of creating new ones - the current implementation does not enable this workflow. Also I feel pretty uncomfortable about dealing with passwords so it would be great to config Gitea to not store any user passwords at all. Any way to help move this forward?

👍 5

lafriks commented

2019-03-17 10:30:10 +00:00

(Migrated from github.com)

Section should probably be renamed to [oauth2-client] now that oauth2 is used for server

Section should probably be renamed to `[oauth2-client]` now that oauth2 is used for server

adelowo reviewed 2019-03-17 12:08:05 +00:00

docs/content/doc/advanced/config-cheat-sheet.en-us.md

adelowo commented

2019-03-17 12:07:12 +00:00

can we spell this out as email instead of mail? and it also looks like there is an unnecessary new line.

lunny commented

2019-03-17 13:05:22 +00:00

@anoff a password will be used when clone via https.

👍 1

anoff commented

2019-03-17 15:52:40 +00:00

(Migrated from github.com)

@anoff a password will be used when clone via https.

valid point. Wonder how gitlab handles this in the oauth scenario. I guess setting a separate password on demand? For the people I know SSH is the default way to clone.

> @anoff a password will be used when clone via https. valid point. Wonder how gitlab handles this in the oauth scenario. I guess setting a separate password on demand? For the people I know SSH is the default way to clone.

lunny commented

2019-03-18 13:36:57 +00:00

@anoff right, a password is not always necessary if you use SSH clone/push

stale[bot] commented

2019-05-17 13:56:11 +00:00

(Migrated from github.com)

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

👎 4

mgjm (Migrated from github.com) reviewed 2020-02-25 06:32:06 +00:00

docs/content/doc/advanced/config-cheat-sheet.en-us.md

mgjm (Migrated from github.com) commented

2020-02-25 06:32:05 +00:00

Fixed in 6a3a0e7.

Fixed in 6a3a0e7.

mgjm (Migrated from github.com) reviewed 2020-02-25 06:32:35 +00:00

routers/user/auth.go

						
				@ -863,3 +943,4 @@

					if err := ctx.Session.Set("twofaRemember", remember); err != nil {

						log.Error("Error setting twofaRemember in session: %v", err)

					}

					if err := ctx.Session.Set("linkAccount", true); err != nil {

mgjm (Migrated from github.com) commented

2020-02-25 06:32:34 +00:00

Fixed in 527c7e6.

Fixed in 527c7e6.

6543 (Migrated from github.com) requested changes 2020-03-31 19:34:18 +00:00

custom/conf/app.ini.sample

6543 (Migrated from github.com) commented

2020-03-31 19:28:48 +00:00

REGISTER_EMAIL_CONFIRM =

```suggestion REGISTER_EMAIL_CONFIRM = ```

routers/user/auth.go

6543 (Migrated from github.com) commented

2020-03-31 19:32:01 +00:00

+1

6543 (Migrated from github.com) reviewed 2020-04-01 12:25:22 +00:00

routers/user/auth.go

						
				@ -625,0 +641,4 @@

							u = &models.User{

								Name:        getUserName(&gothUser),

								FullName:    gothUser.Name,

								Email:       gothUser.Email,

6543 (Migrated from github.com) commented

2020-04-01 12:25:22 +00:00

@lunny one thing I didnt looked at: does this way also migrate migrated-contend to the user?

lafriks (Migrated from github.com) reviewed 2021-03-30 10:50:21 +00:00

custom/conf/app.example.ini

lafriks (Migrated from github.com) commented

2021-03-30 10:50:20 +00:00

do we really need to customize these? Maybe just always request "email profile" if ENABLE_AUTO_REGISTRATION is enabled?

do we really need to customize these? Maybe just always request "email profile" if `ENABLE_AUTO_REGISTRATION` is enabled?

kvaster (Migrated from github.com) reviewed 2021-03-30 12:04:31 +00:00

custom/conf/app.example.ini

kvaster (Migrated from github.com) commented

2021-03-30 12:04:31 +00:00

Profile scope may be usefull even without auto linking. This patch also provides possibility for automatic avatar update from oauth provider.

6543 (Migrated from github.com) requested changes 2021-03-31 08:56:55 +00:00

custom/conf/app.example.ini

6543 (Migrated from github.com) commented

2021-03-31 08:56:01 +00:00

USERNAME = nickname

```suggestion USERNAME = nickname ```

docs/content/doc/advanced/config-cheat-sheet.en-us.md

6543 (Migrated from github.com) commented

2021-03-31 08:56:11 +00:00

- `USERNAME`: **nickname**: The source of the username for new oauth2 accounts: userid, nickname, email (username part).

```suggestion - `USERNAME`: **nickname**: The source of the username for new oauth2 accounts: userid, nickname, email (username part). ```

modules/setting/service.go

6543 (Migrated from github.com) commented

2021-03-31 08:56:38 +00:00

	OAuth2Client.Username = OAuth2UsernameType(sec.Key("USERNAME").MustString(string(OAuth2UsernameNickname)))

```suggestion OAuth2Client.Username = OAuth2UsernameType(sec.Key("USERNAME").MustString(string(OAuth2UsernameNickname))) ```

6543 (Migrated from github.com) commented

2021-03-31 08:56:51 +00:00

		log.Warn("Username setting is not valid: '%s', will fallback to '%s'", OAuth2Client.Username, OAuth2UsernameNickname)
		OAuth2Client.Username = OAuth2UsernameNickname

```suggestion log.Warn("Username setting is not valid: '%s', will fallback to '%s'", OAuth2Client.Username, OAuth2UsernameNickname) OAuth2Client.Username = OAuth2UsernameNickname ```

6543 (Migrated from github.com) approved these changes 2021-03-31 10:31:32 +00:00

6543 (Migrated from github.com) left a comment

about scopes ... v1.15.0 development has just started ... a follow up pull is the better choice :)

lunny reviewed 2021-04-01 05:52:32 +00:00

models/migrations/v178.go

lunny commented

2021-04-01 05:52:32 +00:00

copyright header

lunny reviewed 2021-04-01 05:53:45 +00:00

modules/setting/service.go

lunny commented

2021-04-01 05:53:45 +00:00

blank line.

6543 (Migrated from github.com) approved these changes 2021-04-03 10:10:08 +00:00

6543 (Migrated from github.com) left a comment

works & tests PASS (on my pc)

lunny reviewed 2021-04-06 15:51:53 +00:00

docs/content/doc/advanced/config-cheat-sheet.en-us.md

lunny commented

2021-04-06 15:51:53 +00:00

I would like to copy the description from ini file here and add a warning for auto because it may automatically link the two accounts only because the two account have same names. It has some security risks.

I would like to copy the description from ini file here and add a warning for `auto` because it may automatically link the two accounts only because the two account have same names. It has some security risks.