Allow custom SSH user in UI for built-in SSH server (#2617) #2678

pgaskin · 2017-10-08T18:23:06Z

This allows a custom username to be set for the built-in SSH server.

It adds the config option BUILTIN_SSH_USER in the [server] section. If this option is missing or empty, it defaults to the RUN_USER (which was the default before), and it it is present and the built-in SSH server is enabled, then it uses the custom user for the built-in SSH server.

This does not introduce any compatibility issues with old versions.

I have tested this feature.

See issue #2617

codecov-io · 2017-10-08T18:30:31Z

Codecov Report

Merging #2678 into master will decrease coverage by <.01%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##           master    #2678      +/-   ##
==========================================
- Coverage   27.38%   27.38%   -0.01%     
==========================================
  Files          86       86              
  Lines       17010    17014       +4     
==========================================
+ Hits         4658     4659       +1     
- Misses      11673    11675       +2     
- Partials      679      680       +1

Impacted Files	Coverage Δ
models/repo.go	`13.17% <28.57%> (+0.02%)`	⬆️

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 514006a...9706d5b. Read the comment docs.

lafriks · 2017-10-08T18:33:08Z

LGTM

lunny · 2017-10-09T00:57:46Z

It's not format well

pgaskin · 2017-10-09T11:39:46Z

@lunny You mean I need to run gofmt?

daviian · 2017-10-09T11:50:01Z

@geek1011 You can use make fmt too

bkcsoft · 2017-10-09T21:36:07Z

Shouldn't that user be verified in the builtin ssh-server as well?

pgaskin · 2017-10-09T21:51:19Z

@bkcsoft no, that would introduce compatibility issues and an unnecessary check. The SSH user for the built-in SSH server has no real meaning.

pgaskin · 2017-10-11T12:11:14Z

I did make fmt.

pgaskin · 2017-10-11T12:12:21Z

Can this be merged now?

Morlinest

Some suggestions

Morlinest · 2017-10-11T12:19:13Z

modules/setting/setting.go

@@ -720,6 +722,8 @@ func NewContext() {
 		SSH.StartBuiltinServer = false
 	}

+	SSH.BuiltinServerUser = sec.Key("BUILTIN_SSH_USER").MustString("")


Can this be changed to SSH.BuiltinServerUser = sec.Key("BUILTIN_SSH_USER").MustString(RunUser)?

It is set to the runuser if blank a bit further down. I cannot combine them because this one requires the server sec variable, and the runuser is set further down. (Line 922)

Yes, I saw that line. Can you move this setup line down as BuiltinSSHServerUser is not used between lines 725 and 922?

Morlinest · 2017-10-11T12:20:28Z

modules/setting/setting.go

@@ -104,6 +105,7 @@ var (
 	}{
 		Disabled:           false,
 		StartBuiltinServer: false,
+		BuiltinServerUser:  "",


Why setting default string value?

I'll remove that.

Morlinest · 2017-10-11T12:24:30Z

conf/app.ini

@@ -113,6 +113,8 @@ LOCAL_ROOT_URL = %(PROTOCOL)s://%(HTTP_ADDR)s:%(HTTP_PORT)s/
 DISABLE_SSH = false
 ; Whether use builtin SSH server or not.
 START_SSH_SERVER = false
+; Username to use for builtin SSH server. If blank, then it is the value of RUN_USER.
+BUILTIN_SSH_USER =


IMO is better to use name like BUILTIN_SSH_SERVER_USER.

Should I change it?

I have changed it.

- Renamed config to BUILTIN_SSH_SERVER_USER - Removed unnecessary default string value for config item

sapk · 2017-10-11T14:16:57Z

modules/setting/setting.go

@@ -915,6 +918,10 @@ func NewContext() {
 		}
 	}

+	if SSH.BuiltinSSHServerUser == "" {
+		SSH.BuiltinSSHServerUser = RunUser


Couldn't we do simply SSH.BuiltinSSHServerUser = sec.Key("BUILTIN_SSH_USER").MustString(RunUser) here ?

@sapk That's my question :D

@Morlinest sorry I haven't look at others reviews. just quickly look at it ^^

So my code is fine?

What we suggest is that you move (and add the RunUser as default) L724 SSH.BuiltinSSHServerUser = sec.Key("BUILTIN_SSH_USER").MustString(RunUser) in place of L921-924

@sapk It cannot happen because sec is redefined in between those spots. That's why I have the two lines.

Cfg.Section("server").Key("BUILTIN_SSH_SERVER_USER").MustString(RunUser)?

@Morlinest I didn't know about that one. Thanks. I'll try it.

pgaskin · 2017-10-12T13:23:28Z

@sapk @Morlinest I have updated according to your review.

Morlinest · 2017-10-12T16:55:17Z

modules/setting/setting.go

@@ -915,6 +916,8 @@ func NewContext() {
 		}
 	}

+	SSH.BuiltinSSHServerUser = Cfg.Section("server").Key("BUILTIN_SSH_USER").MustString(RunUser)


BUILTIN_SSH_SERVER_USER

Morlinest · 2017-10-12T17:02:33Z

modules/setting/setting.go

@@ -90,6 +90,7 @@ var (
 	SSH = struct {
 		Disabled             bool           `ini:"DISABLE_SSH"`
 		StartBuiltinServer   bool           `ini:"START_SSH_SERVER"`
+		BuiltinSSHServerUser string         `ini:"BUILTIN_SSH_SERVER_USER"`


Idea: What about changing BuiltinSSHServerUser to BuiltinServerUser. Then reading config inside go setting.SSH.BuiltinServerUser. Also "builtin" word can be removed so it would be more consistent with other ssh settings. What do you think?

I'll remove the SSH from the var name. Since this user only applies to the internal server, I'll keep the Builtin

Good point. The last thing: SSH_BUILTIN_SERVER_USER instead of BUILTIN_SSH_SERVER_USER?

pgaskin · 2017-10-12T19:01:55Z

@Morlinest should be fixed now.

lafriks · 2017-10-14T11:01:47Z

@Morlinest need your approval

Morlinest · 2017-10-14T11:08:00Z

@lafriks Waiting for response (last thing), I think SSH_BUILTIN_SERVER_USER would be better than BUILTIN_SSH_SERVER_USER.

pgaskin · 2017-10-14T12:07:52Z

@Morlinest how many times do you have to change your mind?

lafriks · 2017-10-14T12:14:30Z

@Morlinest I think current is better as SSH_BUILTIN_SERVER_USER does not sound correct in English

Morlinest · 2017-10-14T12:21:31Z

@lafriks I meant SSH_ as prefix in config. In code it translates to SSH.BuiltinServerUser.

Morlinest · 2017-10-14T12:34:29Z

@geek1011 I was asking about another idea after accepting your opinion. I don't see anything bad on it.

Morlinest · 2017-10-14T12:40:33Z

@lafriks Same as SSH_BACKUP_AUTHORIZED_KEYS or SSH_EXPOSE_ANONYMOUS

lafriks · 2017-10-14T15:06:17Z

@Morlinest not really:
SSH_BUILTIN_SERVER_USER -> built-in server in ssh
BUILTIN_SSH_SERVER_USER -> built-in ssh server

Morlinest · 2017-10-14T15:10:46Z

@lafriks OK, I accept your interpretation.

LGTM

lafriks · 2017-10-14T15:26:14Z

Make LG-TM work again

Allow custom SSH user in UI for built-in SSH server (#2617)

2607da5

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

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 8, 2017

Some fixes

2d20cd1

lafriks added the type/enhancement An improvement of existing functionality label Oct 8, 2017

lafriks added this to the 1.3.0 milestone Oct 8, 2017

Did make fmt

d9d4791

Morlinest suggested changes Oct 11, 2017

View reviewed changes

Updated according to review

ccdac95

- Renamed config to BUILTIN_SSH_SERVER_USER - Removed unnecessary default string value for config item

sapk reviewed Oct 11, 2017

View reviewed changes

Updated according to review

3e86b1a

lafriks approved these changes Oct 12, 2017

View reviewed changes

Morlinest suggested changes Oct 12, 2017

View reviewed changes

Morlinest reviewed Oct 12, 2017

View reviewed changes

Fixed some minor issues

248c0e6

Morlinest approved these changes Oct 14, 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 Oct 14, 2017

Merge branch 'master' into custom-builtin-ssh-user

9706d5b

lafriks merged commit e89bb7e into go-gitea:master Oct 14, 2017

techknowlogick mentioned this pull request Apr 24, 2018

BUILTIN_SSH_SERVER_USER config variable not acknowledged by gitea #3847

Closed

2 tasks

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

Allow custom SSH user in UI for built-in SSH server (#2617) #2678

Allow custom SSH user in UI for built-in SSH server (#2617) #2678

Conversation

pgaskin commented Oct 8, 2017 • edited Loading

codecov-io commented Oct 8, 2017 • edited Loading

Codecov Report

lafriks commented Oct 8, 2017

lunny commented Oct 9, 2017

pgaskin commented Oct 9, 2017

daviian commented Oct 9, 2017

bkcsoft commented Oct 9, 2017

pgaskin commented Oct 9, 2017

pgaskin commented Oct 11, 2017

pgaskin commented Oct 11, 2017

Morlinest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgaskin Oct 11, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Morlinest Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgaskin commented Oct 12, 2017

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

pgaskin commented Oct 12, 2017

lafriks commented Oct 14, 2017

Morlinest commented Oct 14, 2017

pgaskin commented Oct 14, 2017

lafriks commented Oct 14, 2017

Morlinest commented Oct 14, 2017 • edited Loading

Morlinest commented Oct 14, 2017

Morlinest commented Oct 14, 2017

lafriks commented Oct 14, 2017

Morlinest commented Oct 14, 2017

lafriks commented Oct 14, 2017

pgaskin commented Oct 8, 2017 •

edited

Loading

codecov-io commented Oct 8, 2017 •

edited

Loading

pgaskin Oct 11, 2017 •

edited

Loading

Morlinest Oct 12, 2017 •

edited

Loading

Morlinest commented Oct 14, 2017 •

edited

Loading