These are chat archives for go-gitea/gitea

7th
Feb 2017
Lunny Xiao
@lunny
Feb 07 2017 00:24
@strk I think yes
Willem van Dreumel
@willemvd
Feb 07 2017 07:08
are you still building OpenID as a complete new flow?
@strk ^
Sandro Santilli
@strk
Feb 07 2017 07:24
willemvd: yes
@willemvd: assuming you mean it's not a "LoginSource"
damn, the matrix/gitter bridge broke
@willemvd: I might get that back into LoginSource if your OAuth2 one is found to be clean (didn't check how you implement AutoSignIn)
I don't like the idea of making one login method "primary", except for the cascading username/password receiving login sources
actually, I don't like the cascading login sources handling at all
as it means passing your password to a number of sources before reaching the correct one
sounds like a security leak to me
say my username/password are for a LDAP account
Willem van Dreumel
@willemvd
Feb 07 2017 07:28
that’s why in that flow I ignore the OAuth2 based logins
in that loop
Sandro Santilli
@strk
Feb 07 2017 07:28
it is possible, currently, that they are passed to an SMTP server before getting sent to LDAP
Willem van Dreumel
@willemvd
Feb 07 2017 07:29
true , seen that as well
not so nice :P
Sandro Santilli
@strk
Feb 07 2017 07:29
so I like the idea of the user selecting the auth method (thus the left navbar)
I understand the "primary" login source (User.LoginType) kind of serves this purpose, although at posteriori
Willem van Dreumel
@willemvd
Feb 07 2017 07:29
looked at your comments of yesterday and I see you have trouble with that delete account part, that is one of the reasons to go for the current approach
Sandro Santilli
@strk
Feb 07 2017 07:30
ie: once you get a "primary" source registered, your creds are only sent there
@willemvd: the "delete account" is a completely different "problem", it's just the need for username/password
or what other problem are you referring to ?
Willem van Dreumel
@willemvd
Feb 07 2017 07:30
yeah that part should be refactored for sure
Sandro Santilli
@strk
Feb 07 2017 07:30
other than that, the current OpenID branch works pretty nicely for me
it could be improved in user-feedback (does a Session-stored "flash message" concept exist in the UI ?... to let the user know his OpenID URI was added to his account, when "connect"ing)
and in password management (ideally I'd have the user with NO password and UNABLE to use the passwowrd until he sets one, either via Settings.UpdatePassword OR via "Forgot Password")
also, I hand't tested email confirmation, but code to handle that does exist (the user is created as inactive and a mail is sent)
so right now, in my branch, you can connect or register_new upon entering an unknown OpenID
while you're logged in straight upon entering a known OpenID
when registering a new account (upon entering an unknown OpenID), you are NOT asked for a password, but get a random one, which is not even logged anywhere. I'd guess a "Forgot Password" call would fix that, but found out it's disabled by default, so.... you are stuck with an unknown and immutable password in those cases.
but really I don't want to force users to set a password until they find it's needed
it could never be, if ssh is used for code, openid for logging in and auth tokens for apps
Willem van Dreumel
@willemvd
Feb 07 2017 07:36
flash messages works
ctx.Flash.Success(…)
ctx.Redirect(url)
I understand , but why I appoint this is because your are facing thing I already (tried to ;) ) fixed
and maybe you will end up with a different approach and when every thing is inside the code we have stuff that looks like each other but work slightly or totally different
that could get messy ;)
Willem van Dreumel
@willemvd
Feb 07 2017 07:41
at least IMHO
Sandro Santilli
@strk
Feb 07 2017 08:00
agreed, I'm also looking at your code indeed
could also be that at the end I'll drop the OpenID branch and as you say implement it inside goth..
but I'd like to be able NOT to set a password, when logging into a new OpenID instance
I like, from your work, to not need to switch tabs to 'login-via-x'
that part is not bad :)
(the rest is not bad either, just mentioning what I noticed)
going back to using a LoginSource could be feasible, if I only have to "skip" the sign-in loop
but still, thre's an issue with pre-installation steps
I mean, say I want to only use OpenID to login
right now it takes creating a database record
(to enable more login sources)
so you still need to first create an account to be the administrator, dont you ?
Willem van Dreumel
@willemvd
Feb 07 2017 08:06
yes that is some PR I want to create later, only allow admin to login with user/pass on GUI (not all http, because that would break git over http)
strk @strk tries ctx.Flash meanwhile
Sandro Santilli
@strk
Feb 07 2017 08:08
uhm, didn't work
I did: ctx.Flash.Success(ctx.Tr("settings.add_openid_success")); handleSignIn(ctx, u, false)
Willem van Dreumel
@willemvd
Feb 07 2017 08:08
strange , we use it on more places
Sandro Santilli
@strk
Feb 07 2017 08:09
maybe not all pages handle the Flash ?
I'm redirected to /
Willem van Dreumel
@willemvd
Feb 07 2017 08:09
that could be true
Sandro Santilli
@strk
Feb 07 2017 08:09
uhm... templates/base/alert.tmpl:{{if .Flash.SuccessMsg}}
Willem van Dreumel
@willemvd
Feb 07 2017 08:09
there should be a
{{template "base/alert" .}}
indeedd
Sandro Santilli
@strk
Feb 07 2017 08:10
whta's the template for the home ?
Willem van Dreumel
@willemvd
Feb 07 2017 08:11
home.tmpl ? :p
or maybe dashboard.tmpl
Sandro Santilli
@strk
Feb 07 2017 08:11
no alert in home.tmpl indeed
nor dashboard
I'll send a PR adding it in these two places, don't see why not
yep, works, just have to find a proper way to place that alert :)
not for me, maybe better if someone with UI skills does it, any volunteer ?
Sandro Santilli
@strk
Feb 07 2017 08:19
please review go-gitea/gitea#856
only did in dashboard (small PRs are better :)
Sandro Santilli
@strk
Feb 07 2017 08:49
now I remember another thing that I should clean up is all the Session's "openid_<thing>" variables
Thomas Boerger
@tboerger
Feb 07 2017 12:41
every day again and again a single discussion between @willemvd and @strk...
Willem van Dreumel
@willemvd
Feb 07 2017 12:46
@tboerger maybe good to give your opion (as owner) on what the best way should be on implementing these new flows in gitea ...;)
btw my PR is done, so I will not be changing stuff anymore until one of you owners really refuses my PR
i only think the code might become messy when there are (kind of) simular flow implemented in 2 different ways (that’s all and the only thing)
Lunny Xiao
@lunny
Feb 07 2017 13:34
I have give @willemvd’s PR a LGTM
:smile:
Willem van Dreumel
@willemvd
Feb 07 2017 13:39
great! :)
Sandro Santilli
@strk
Feb 07 2017 16:06
it's fine for me too, not voting against, and I think our discussions are constructive, nothing to be concerned about
Sandro Santilli
@strk
Feb 07 2017 18:32
@willemvd: the only reason why I don't send my LGTM is for the LoginType thing (ie: being unable to see which "external login" association exists, when you log in via github)
Sandro Santilli
@strk
Feb 07 2017 19:40
@willemvd: while checking the "forgot_password" path (to help with the password-less accounts who want to create one, I see that the "forgot_password" route forbids to reset the password for so-called "non-local" accounts
if !u.IsLocal() {
ctx.RenderWithErr(ctx.Tr("auth.non_local_account"), tplForgotPassword, nil)
now User.IsLocal is implemented as: return u.LoginType <= LoginPlain
which means you won't be able to reset your password if you have Oauth2 as your LoginType
but you will have a password
Sandro Santilli
@strk
Feb 07 2017 20:30
please review go-gitea/gitea#862 -- will help password-less accounts to reset their passwords
Sandro Santilli
@strk
Feb 07 2017 21:16
I hope someone will do LibreJS for gitea: gogits/gogs#4092