Only show SSH clone URL if signed in (#2169) #2170

stklcode · 2017-07-15T08:06:01Z

This PR targets issue #2169

For cloning via SSH a user needs to provide a public key. If this is the case, this user has an account or is added by a member. Anonymous visitors hence do not need to see the SSH URL.

~~This is a hard-coded soliton (as voted by @lafriks), equivalent to GitHub's behavior. If a configurable solution is desired instead, I got another diofferent branch (stklcode/gitea@d7cbf5d) ready.~~

This solution introduces a config flag SSH_EXPOSE_ANONYMOUS (default false) to hide by default and opt-in to the old behavior.

lafriks · 2017-07-15T08:16:33Z

If you have configurable solution than submit that

If this flag (default True) is set to false, the SSH clone URL will only be exposed if the current user is signed in.

stklcode · 2017-07-15T08:30:12Z

Done. Updated my branch and the above description.

lafriks · 2017-07-15T08:48:06Z

conf/app.ini

@@ -154,7 +156,7 @@ LFS_START_SERVER = false
 ; Where your lfs files put on, default is data/lfs.
 LFS_CONTENT_PATH = data/lfs
 ; LFS authentication secret, changed this to yourself.
-LFS_JWT_SECRET   = 
+LFS_JWT_SECRET   =


Do not change unneeded spaces

lafriks · 2017-07-15T08:49:03Z

conf/app.ini

@@ -126,6 +126,8 @@ SSH_KEY_TEST_PATH =
 SSH_KEYGEN_PATH = ssh-keygen
 ; Enable SSH Authorized Key Backup when rewriting all keys, default is true
 SSH_BACKUP_AUTHORIZED_KEYS = true
+; Enable exposure of SSH clone URL to anonymous visitors, default is true
+SSH_EXPOSE_ANONYMOUS = true


I think default should be false to match GitHub and also for security reason not to expose unneeded URL's

lafriks · 2017-07-15T08:57:42Z

modules/setting/setting.go

 	}{
 		Disabled:           false,
 		StartBuiltinServer: false,
 		Domain:             "",
 		Port:               22,
 		KeygenPath:         "ssh-keygen",
+		ExposeAnonymous:    true,


Default value should be better set lower like AuthorizedKeysBackup:
https://github.com/stklcode/gitea/blob/24109f4093603a6b4dbcf0735a7506e24ef1e289/modules/setting/setting.go#L711

lafriks · 2017-07-15T09:04:02Z

Also integration test would be nice to check that

To match GitHub and for security reasons, SSH URL exposure is disabled by default. In addition to that. minor code changes have been applied. Signed-off-by: Stefan Kalscheuer <[email protected]>

lafriks · 2017-07-15T11:15:56Z

I hope you don't mind that I added integration tests, thanks for your first PR 👍

lafriks · 2017-07-15T11:21:41Z

conf/app.ini

@@ -126,6 +126,8 @@ SSH_KEY_TEST_PATH =
 SSH_KEYGEN_PATH = ssh-keygen
 ; Enable SSH Authorized Key Backup when rewriting all keys, default is true
 SSH_BACKUP_AUTHORIZED_KEYS = true
+; Enable exposure of SSH clone URL to anonymous visitors, default is true


Please change also comment that default is false

lafriks · 2017-07-15T11:24:12Z

templates/repo/home.tmpl

 							<button class="ui basic clone button" id="repo-clone-ssh" data-link="{{.CloneLink.SSH}}">
 								SSH
 							</button>
 						{{end}}
 						{{if not $.DisableHTTP}}
 							<input id="repo-clone-url" value="{{$.CloneLink.HTTPS}}" readonly>
-						{{else}}
+						{{else if or $.IsSigned $.ExposeAnonSSH}}


Please also change condition to be same as for button to take into account that ssh is not disabled

lafriks · 2017-07-15T11:30:52Z

templates/repo/home.tmpl

 							<button class="ui basic clone button" id="repo-clone-ssh" data-link="{{.CloneLink.SSH}}">
 								SSH
 							</button>
 						{{end}}
 						{{if not $.DisableHTTP}}
 							<input id="repo-clone-url" value="{{$.CloneLink.HTTPS}}" readonly>
-						{{else}}
+						{{else if or $.IsSigned $.ExposeAnonSSH}}
 							<input id="repo-clone-url" value="{{$.CloneLink.SSH}}" readonly>
 						{{end}}
 						<button class="ui basic icon button poping up clipboard" id="clipboard-btn" data-original="{{.i18n.Tr "repo.copy_link"}}" data-success="{{.i18n.Tr "repo.copy_link_success"}}" data-error="{{.i18n.Tr "repo.copy_link_error"}}" data-content="{{.i18n.Tr "repo.copy_link"}}" data-variation="inverted tiny" data-clipboard-target="#repo-clone-url">


Also do not show copy button if both http and ssh clone are disabled or is not signed in and because of that clone url is not visible

The button has been shown before, if both were disabled. But you're right, definitely a reasonable change.

lafriks · 2017-07-15T11:48:42Z

templates/repo/bare.tmpl

@@ -18,14 +18,14 @@
 										{{if UseHTTPS}}HTTPS{{else}}HTTP{{end}}
 									</button>
 								{{end}}
-								{{if not $.DisableSSH}}
+								{{if and (not $.DisableSSH) (or $.IsSigned $.ExposeAnonSSH)}}


For bare repository check is not needed as there is already check to show clone url only if user is IsRepositoryAdmin

Signed-off-by: Stefan Kalscheuer <[email protected]>

lafriks · 2017-07-15T12:37:52Z

Great job 👍 LGTM

Bwko · 2017-07-15T14:18:58Z

LGTM

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

stklcode force-pushed the master branch from ef4fc28 to 3030142 Compare July 15, 2017 08:18

Add configuration flag SSH_EXPOSE_ANONYMOUS

24109f4

If this flag (default True) is set to false, the SSH clone URL will only be exposed if the current user is signed in.

stklcode force-pushed the master branch from 3030142 to 24109f4 Compare July 15, 2017 08:24

lafriks requested changes Jul 15, 2017

View reviewed changes

Default SSH exposure set to false

3d08a4e

To match GitHub and for security reasons, SSH URL exposure is disabled by default. In addition to that. minor code changes have been applied. Signed-off-by: Stefan Kalscheuer <[email protected]>

lafriks approved these changes Jul 15, 2017

View reviewed changes

Add integration tests

37ab58f

lafriks added this to the 1.2.0 milestone Jul 15, 2017

lafriks added the topic/ui Change the appearance of the Gitea UI label Jul 15, 2017

lafriks reviewed Jul 15, 2017

View reviewed changes

lafriks requested changes Jul 15, 2017

View reviewed changes

lafriks reviewed Jul 15, 2017

View reviewed changes

Hide clone button neither HTTP and SSH is enabled

a49ac74

Signed-off-by: Stefan Kalscheuer <[email protected]>

lafriks approved these changes Jul 15, 2017

View reviewed changes

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 Jul 15, 2017

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 Jul 15, 2017

lunny merged commit 0b17757 into go-gitea:master Jul 15, 2017

lafriks mentioned this pull request Jul 15, 2017

Option to not expose SSH to anonymous visitors #2169

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Only show SSH clone URL if signed in (#2169) #2170

Only show SSH clone URL if signed in (#2169) #2170

Uh oh!

stklcode commented Jul 15, 2017 •

edited

Loading

Uh oh!

lafriks commented Jul 15, 2017

Uh oh!

stklcode commented Jul 15, 2017

Uh oh!

lafriks Jul 15, 2017

Uh oh!

lafriks Jul 15, 2017

Uh oh!

lafriks Jul 15, 2017

Uh oh!

lafriks commented Jul 15, 2017

Uh oh!

lafriks commented Jul 15, 2017

Uh oh!

lafriks Jul 15, 2017

Uh oh!

lafriks Jul 15, 2017

Uh oh!

lafriks Jul 15, 2017

Uh oh!

stklcode Jul 15, 2017

Uh oh!

lafriks Jul 15, 2017

Uh oh!

lafriks commented Jul 15, 2017

Uh oh!

Bwko commented Jul 15, 2017 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Only show SSH clone URL if signed in (#2169) #2170

Only show SSH clone URL if signed in (#2169) #2170

Uh oh!

Conversation

stklcode commented Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lafriks commented Jul 15, 2017

Uh oh!

stklcode commented Jul 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lafriks commented Jul 15, 2017

Uh oh!

lafriks commented Jul 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lafriks commented Jul 15, 2017

Uh oh!

Bwko commented Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

stklcode commented Jul 15, 2017 •

edited

Loading

Bwko commented Jul 15, 2017 •

edited

Loading