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

Set OpenID support on by default when installing new instance #3010

Merged
merged 1 commit into from
Nov 29, 2017

Conversation

strk
Copy link
Member

@strk strk commented Nov 28, 2017

Partially fixes #2984

@@ -1165,7 +1165,7 @@ func newService() {
Service.NoReplyAddress = sec.Key("NO_REPLY_ADDRESS").MustString("noreply.example.org")

sec = Cfg.Section("openid")
Service.EnableOpenIDSignIn = sec.Key("ENABLE_OPENID_SIGNIN").MustBool(false)
Service.EnableOpenIDSignIn = sec.Key("ENABLE_OPENID_SIGNIN").MustBool(true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think this should keep false. For a default installation, the minimal feature rule will keep system safer.

if customConfExists {
defOpenIDSignIn = true;
}
Service.EnableOpenIDSignIn = sec.Key("ENABLE_OPENID_SIGNIN").MustBool(defOpenIDSignIn)
Service.EnableOpenIDSignUp = sec.Key("ENABLE_OPENID_SIGNUP").MustBool(!Service.DisableRegistration && Service.EnableOpenIDSignIn)
Copy link
Contributor

@0rzech 0rzech Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should simply be

Service.EnableOpenIDSignIn = sec.Key("ENABLE_OPENID_SIGNIN").MustBool(!InstallLock)

because InstallLock state seems to indicate whether this is fresh install or not, while Gitea creates app.ini itself during install process.

@lafriks lafriks added the type/enhancement An improvement of existing functionality label Nov 28, 2017
@lafriks lafriks added this to the 1.4.0 milestone Nov 28, 2017
@lafriks lafriks added backport/v1.3 type/bug and removed type/enhancement An improvement of existing functionality labels Nov 28, 2017
@lunny
Copy link
Member

lunny commented Nov 29, 2017

LGTM

@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Nov 29, 2017
@lafriks
Copy link
Member

lafriks commented Nov 29, 2017

LGTM

@tboerger 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 Nov 29, 2017
@codecov-io
Copy link

Codecov Report

Merging #3010 into master will not change coverage.
The diff coverage is 33.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3010   +/-   ##
=======================================
  Coverage   32.98%   32.98%           
=======================================
  Files         270      270           
  Lines       39534    39534           
=======================================
  Hits        13042    13042           
  Misses      24637    24637           
  Partials     1855     1855
Impacted Files Coverage Δ
routers/install.go 0% <0%> (ø) ⬆️
modules/setting/setting.go 47.18% <100%> (ø) ⬆️

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 b9452c4...68fe411. Read the comment docs.

@lafriks lafriks changed the title Set OpenID support on by default, remove hard-coding from installer Set OpenID support on by default when installing new instance Nov 29, 2017
@lafriks lafriks merged commit 67b0d21 into go-gitea:master Nov 29, 2017
@lafriks
Copy link
Member

lafriks commented Nov 29, 2017

@strk please send backport to 1.3

lafriks pushed a commit to lafriks-fork/gitea that referenced this pull request Nov 29, 2017
@strk strk deleted the openid-default-on branch November 29, 2017 18:04
@lafriks lafriks added the backport/done All backports for this PR have been created label Dec 13, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web installer has some settings hardcoded
6 participants