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

Can't link Github/Gitlab to existing account #9066

Closed
2 of 7 tasks
SkYNewZ opened this issue Nov 18, 2019 · 5 comments · Fixed by #9147
Closed
2 of 7 tasks

Can't link Github/Gitlab to existing account #9066

SkYNewZ opened this issue Nov 18, 2019 · 5 comments · Fixed by #9147
Labels
Milestone

Comments

@SkYNewZ
Copy link

SkYNewZ commented Nov 18, 2019

  • Gitea version (or commit ref): 1.10.0
  • Git version: 2.20.1
  • Operating system: Debian GNU/Linux 10 (buster) amd64
  • Database (use [x]):
    • PostgreSQL
    • MySQL
    • MSSQL
    • SQLite
  • Can you reproduce the bug at https://try.gitea.io:
    • Yes (provide example URL)
    • No
    • Not relevant
  • Log gist: only SQL queries in gitea.log

Description

Hi! I enabled Gitlab and Github authentication. Its works because I can comme back on Gitea from Github/Gitlab login. Its asks me to link to existing account. I fill it, but I'm redirected to "Register New Account". I can't log in with Github and Gitlab.

Screenshots

Screen Recording 2019-11-18 at 23 51 28
Screen Recording 2019-11-18 at 23 58 06

@BaxAndrei
Copy link

same as #8923

@lunny lunny added the type/bug label Nov 21, 2019
@lunny lunny added this to the 1.10.1 milestone Nov 21, 2019
@blueworrybear
Copy link
Contributor

blueworrybear commented Nov 23, 2019

It seems to me that it's because external account sign in currently requires password.
Looking at the router:

routers.go:306

m.Post("/link_account_signin", bindIgnErr(auth.SignInForm{}), user.LinkAccountPostSignIn)

LinkAccountPostSignIn binds with auth.SignInForm, which force password must exist with binding:"Required;MaxSize(255)".

type SignInForm struct {
	UserName string `binding:"Required;MaxSize(254)"`
	// TODO remove required from password for SecondFactorAuthentication
	Password string `binding:"Required;MaxSize(255)"`
	Remember bool
}

As the result, LinkAccountPostSignIn always redirects to register new account due to the context error:

func LinkAccountPostSignIn(ctx *context.Context, signInForm auth.SignInForm) {
// ...
	if ctx.HasError() {
		ctx.HTML(200, tplLinkAccount)
		return
	}
//...

The context error flag is set in modules/auth/auth.go:112, function validate

The possible solution may be creating a new ExternalSignInForm and make Password optional. But it requires to check Password field within LinkAccountPostSignIn.

Let me know if this is an acceptable solution. I could create a PR to solve this issue.

@BaxAndrei
Copy link

But don't you think that if you eliminate the need to request the password for verification when connecting with oAuth2, users may abuse to acquire accounts that do not belong to them?

@blueworrybear
Copy link
Contributor

blueworrybear commented Nov 24, 2019

But don't you think that if you eliminate the need to request the password for verification when connecting with oAuth2, users may abuse to acquire accounts that do not belong to them?

You are right. The problem is Gitea removes Password form field if you set app.ini REQUIRE_EXTERNAL_REGISTRATION_PASSWORD = false. Both Register New Account and Link to Existing Account use REQUIRE_EXTERNAL_REGISTRATION_PASSWORD to decide whether to show Password field. It seems to me that it's confused. IMO, we should always check user's password when linking to existing account.

So the other solution would be always showing Password field on Link to Existing Account tab. However, this solution conflicts with what #6606 wants to do. I am not sure if it's acceptable. 😕

@BaxAndrei
Copy link

BaxAndrei commented Nov 24, 2019

after I changed REQUIRE_EXTERNAL_REGISTRATION_PASSWORD = true I can confirm that everything is completely normal.
image

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants