Add OpenID configuration in install page #2276

strk · 2017-08-08T09:16:15Z

Allows enabling OpenID signin via installation page GUI

strk · 2017-08-08T09:36:46Z

This needs be merged before 1.2.0 is out - but there's still a problem with routes depending on the configuration and the configuration being changed by install page but not being re-read upon changing it... So it takes re-starting gitea to be able to register first user via OpenID. I've to figure out how to do the settings update from form submission

strk · 2017-08-08T09:42:50Z

I think the fix here is to always define the route but then return a Forbidden status if the service is not enabled

strk · 2017-08-08T09:50:58Z

Ready for merge, tested locally.

strk · 2017-08-08T10:00:06Z

@cweiske you might be interested in testing this too. It basically adds a way to configure OpenID at installation time (was missing, so it took manual intervention to enable it)

lafriks

You have enabled all openid routes but added check that openid is enabled only in two of them

strk · 2017-08-08T10:15:27Z

@lafriks 754dd4ea7c9b8c009bf164153dedead98331b636 fixes that, thanks for spotting it !

lunny · 2017-08-08T15:28:04Z

@strk CI failed.

lafriks · 2017-08-08T15:39:33Z

routers/user/auth_openid.go

@@ -29,6 +29,11 @@ const (
 func SignInOpenID(ctx *context.Context) {
 	ctx.Data["Title"] = ctx.Tr("sign_in")

+	if ! setting.Service.EnableOpenIDSignIn {


CI is failing because of space after ! in multiple places

strk · 2017-08-08T19:46:24Z

Thanks @lafriks, I removed all those spaces and verified that `make check` passes locally

lafriks · 2017-08-09T05:06:52Z

routers/routes/routes.go

+			Post(bindIgnErr(auth.SignInOpenIDForm{}), user.SignInOpenIDPost)
+		m.Group("/openid", func() {
+			m.Combo("/connect").
+				Get(user.ConnectOpenID).


ConnectOpenID is still missing check if EnableOpenIDSignIn is enabled

Agreed, any pre-requisite settings should be checked in the route-setup, not the route-handler 🙂

lafriks · 2017-08-09T05:07:25Z

routers/routes/routes.go

-		}
-
+		m.Group("/openid", func() {
+			m.Combo("").Get(user.SettingsOpenID).


SettingsOpenID is still missing check if EnableOpenIDSignIn is enabled

lafriks · 2017-08-09T05:08:19Z

routers/user/setting_openid.go

@@ -40,6 +40,12 @@ func SettingsOpenID(ctx *context.Context) {

 // SettingsOpenIDPost response for change user's openid
 func SettingsOpenIDPost(ctx *context.Context, form auth.AddOpenIDForm) {
+
+	if !setting.Service.EnableOpenIDSignUp {


I think there should be check for EnableOpenIDSignIn not for EnableOpenIDSignUp that is for new user registration

lafriks · 2017-08-09T05:08:43Z

routers/user/setting_openid.go

@@ -132,6 +138,10 @@ func settingsOpenIDVerify(ctx *context.Context) {

 // DeleteOpenID response for delete user's openid
 func DeleteOpenID(ctx *context.Context) {
+	if !setting.Service.EnableOpenIDSignUp {


There should also be check for EnableOpenIDSignIn

lafriks · 2017-08-09T05:09:02Z

routers/user/setting_openid.go

@@ -146,6 +156,10 @@ func DeleteOpenID(ctx *context.Context) {

 // ToggleOpenIDVisibility response for toggle visibility of user's openid
 func ToggleOpenIDVisibility(ctx *context.Context) {
+	if !setting.Service.EnableOpenIDSignUp {


And there also should be check for EnableOpenIDSignIn

strk · 2017-08-09T15:40:53Z

@lafriks please see ed9ade1d
To be honest for the settings we might not even check...

bkcsoft · 2017-08-11T15:57:13Z

modules/auth/user_form.go

@@ -41,6 +41,8 @@ type InstallForm struct {
 	OfflineMode                    bool
 	DisableGravatar                bool
 	EnableFederatedAvatar          bool
+	EnableOpenIDSignIn             bool
+	DisableOpenIDRegistration      bool


I'd rather have these both as EnableFoo since it makes more sense when reading

bkcsoft · 2017-08-11T15:59:20Z

routers/routes/routes.go

+			Post(bindIgnErr(auth.SignInOpenIDForm{}), user.SignInOpenIDPost)
+		m.Group("/openid", func() {
+			m.Combo("/connect").
+				Get(user.ConnectOpenID).


Agreed, any pre-requisite settings should be checked in the route-setup, not the route-handler 🙂

strk · 2017-08-11T16:03:15Z

On Fri, Aug 11, 2017 at 08:59:31AM -0700, bkcsoft wrote: + EnableOpenIDSignIn bool + DisableOpenIDRegistration bool I'd rather have these both as `EnableFoo` since it makes more sense when reading

Please take a look at the user GUI first. There's a "DisableRegistration", so I made this "DisableOpenIDRegistration" for consistency on that side of things. The GUI makes the two go togheter (via Javascript).

strk · 2017-08-16T07:23:57Z

@bkcsoft do you really want to hold this PR back due to the enable vs. disable wording ?

strk · 2017-08-16T09:11:24Z

@bkcsoft I've renamed Disable to Enable, as you requested. And rebased to master.
@lafriks I think I've handled your requests too.

strk · 2017-08-16T09:21:30Z

actually, the change was partial, more changes were needed around, so I reverted it.

strk · 2017-08-16T14:47:57Z

latest commits fix the handling of checkboxes and turn openid sign-in on by default for new installs

strk · 2017-08-16T15:02:08Z

And for @bkcsoft now I've also turned the "OpenID Registration" form into an "enable" rather than "disable". Also added more javascript to check/uncheck dependents...

lafriks · 2017-08-17T23:52:11Z

As for consideration it would probably be better as @bkcsoft recommended to create new handler/filter to check if openid signin/signup is enabled. Something like this:
https://github.com/go-gitea/gitea/blob/master/routers/routes/routes.go#L336

strk · 2017-08-18T14:28:29Z

Can we get this merged before 1.2.0 please ? Aesthetic changes can happen in a second step, for now it's important for the functionality to be there.

lafriks · 2017-08-18T14:44:38Z

I'm not against it, @bkcsoft?

lafriks · 2017-08-18T19:04:43Z

BTW don't write LG-TM in comments for your own PR ;) otherwise I see LG-TM 1 label

strk · 2017-08-18T20:01:05Z

Sorry about that, I thought it was fixed (definitely a bug, isn't it ?) - I wasn't seeing the label when I wrote the next comment, btw. Now I see it.

lunny · 2017-08-19T01:50:56Z

@strk it was fixed I remember.

strk · 2017-08-19T07:12:54Z

@lunny so it was not deployed?
Anyway, can you review this PR or at least mark it as targeted to 1.2?

lunny · 2017-08-19T10:16:43Z

@strk I don't think it's necessary for add this config to installation on v1.2 since LDAP, OAuth and SMTP didn't add to the page. And I think a middleware to check is better than so many settings check on the route functions.

Forbid them upon usage, if needed. This allows registering first user via openid.

Also fix label for the checkbox

Does not affect existing installs (upgrades)

strk · 2017-08-19T10:44:11Z

Ok now that master branch is fixed I've rebased and also committed a change using filters as suggested in previous comment. @bkcsoft can you please review and merge or at least label as targetted at 1.2.0

lafriks · 2017-08-19T10:53:01Z

@strk please fix formatting issues (see Drone)

strk · 2017-08-19T11:07:22Z

@lunny I committed the middleware check and dropped the checks in the route functions. As per installation steps: OpenID is special in that it is a config file, not database, configuration. That's why LDAP and OAuth and SMTP do not belong in the installation page, while OpenID does.

lafriks · 2017-08-19T11:34:35Z

LG-TM

bkcsoft · 2017-08-19T15:33:57Z

LGTM

strk force-pushed the install-time-openid-config branch 2 times, most recently from d003d54 to b6ca8d9 Compare August 8, 2017 09:33

strk changed the title ~~[WIP] Add OpenID configuration in install page~~ Add OpenID configuration in install page Aug 8, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 8, 2017

lafriks requested changes Aug 8, 2017

View reviewed changes

lafriks requested changes Aug 9, 2017

View reviewed changes

bkcsoft suggested changes Aug 11, 2017

View reviewed changes

strk force-pushed the install-time-openid-config branch 2 times, most recently from bf01ca3 to e11db5b Compare August 16, 2017 09:10

strk force-pushed the install-time-openid-config branch from e11db5b to b83addc Compare August 16, 2017 09:21

lafriks approved these changes Aug 17, 2017

View reviewed changes

strk force-pushed the install-time-openid-config branch from a6336e1 to fd66feb Compare August 18, 2017 14:27

strk added 9 commits August 19, 2017 12:27

Add OpenID configuration in install page

9415e6f

Always make openid routes available

c406ef2

Forbid them upon usage, if needed. This allows registering first user via openid.

Have openid registration checkbox follow general registration one

66ad276

Also fix label for the checkbox

Forbid all openid routes when openid is disabled

2026491

lint: remove space after negation operator

b77de5b

Add more checks for EnableOpenIDSignIn

47221ad

Fix openid checkbox state handling

99a98c9

Enable OpenID sign-in by default at installation time

153b430

Does not affect existing installs (upgrades)

Use 'Enable' for OpenID registration

34b9bee

strk force-pushed the install-time-openid-config branch from fd66feb to 34b9bee Compare August 19, 2017 10:28

Use filters to determine if openid routes are available or not

3597f64

Fix formatting issues

dea492a

bkcsoft approved these changes Aug 19, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 19, 2017

bkcsoft merged commit 2c3a229 into go-gitea:master Aug 19, 2017

strk deleted the install-time-openid-config branch August 19, 2017 15:41

lunny added this to the 1.2.0 milestone Aug 20, 2017

lunny added the type/enhancement An improvement of existing functionality label Aug 25, 2017

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Add OpenID configuration in install page #2276

Add OpenID configuration in install page #2276

Conversation

strk commented Aug 8, 2017

strk commented Aug 8, 2017

strk commented Aug 8, 2017

strk commented Aug 8, 2017

strk commented Aug 8, 2017

lafriks left a comment

Choose a reason for hiding this comment

strk commented Aug 8, 2017 via email

lunny commented Aug 8, 2017

lafriks Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

strk commented Aug 8, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strk commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strk commented Aug 11, 2017 via email

strk commented Aug 16, 2017

strk commented Aug 16, 2017

strk commented Aug 16, 2017

strk commented Aug 16, 2017

strk commented Aug 16, 2017

lafriks commented Aug 17, 2017

strk commented Aug 18, 2017

lafriks commented Aug 18, 2017

lafriks commented Aug 18, 2017

strk commented Aug 18, 2017

lunny commented Aug 19, 2017

strk commented Aug 19, 2017

lunny commented Aug 19, 2017

strk commented Aug 19, 2017

lafriks commented Aug 19, 2017

strk commented Aug 19, 2017 via email

lafriks commented Aug 19, 2017

bkcsoft commented Aug 19, 2017

lafriks Aug 8, 2017 •

edited

Loading