PR#3328: Only display SSH URL for repo if SSH key is uploaded and gitolite config is compiled - pagure

pagure

#3328 Only display SSH URL for repo if SSH key is uploaded and gitolite config is compiled

Merged 6 years ago by pingou. Opened 6 years ago by bkabrda.

bkabrda/pagure clone-url-display into master

Inject git_url_ssh to all templates rendered, since a lot templates are using it

Slavek Kabrda • 6 years ago

4194aa6

Only display SSH URL for repo if SSH key is uploaded and gitolite config is compiled

Slavek Kabrda • 6 years ago

9e14c88

pagure/default_config.py

file modified

		`@@ -89,6 +89,10 @@`
		`GIT_URL_SSH = 'ssh://git@localhost.localdomain/'`
		`GIT_URL_GIT = 'git://localhost.localdomain/'`

		`+ # Set to True if git ssh URLs should be displayed even if user`
		`+ # doesn't have SSH key uploaded`
		`+ ALWAYS_RENDER_SSH_CLONE_URL = False`
		`+`
		`# Default queue names for the different services`
		`WEBHOOK_CELERY_QUEUE = 'pagure_webhook'`
		`LOGCOM_CELERY_QUEUE = 'pagure_logcom'`

pagure/flask_app.py

file modified

+30

		`@@ -111,6 +111,36 @@`
		`os.path.join(app.root_path, 'static'),`
		`]`

		`+ @app.context_processor`
		`+ def context_processor():`
		`+ def user_can_clone_ssh(username):`
		`+ ssh_keys = ''`
		`+ if flask.g.authenticated:`
		`+ ssh_keys = pagure.lib.search_user(`
		`+ flask.g.session,`
		`+ username=flask.g.fas_user.username`
		`+ ).public_ssh_key or ''`
		`+ if not (pagure_config.get('ALWAYS_RENDER_SSH_CLONE_URL')`
		`+ or ssh_keys.strip()):`
		`+ return False`
		`+ return True`
		`+`
		`+ def get_git_url_ssh():`
		`+ """ Return the GIT SSH URL to be displayed in the UI based on the`
		`+ content of the configuration file.`
		`+ """`
		`+ git_url_ssh = pagure_config.get('GIT_URL_SSH')`
		`+ if flask.g.authenticated and git_url_ssh:`
		`+ try:`
		`+ git_url_ssh = git_url_ssh.format(`
		`+ username=flask.g.fas_user.username)`
		`+ except (KeyError, IndexError):`
		`+ pass`
		`+ return git_url_ssh`
		`+`
		`+ return {'user_can_clone_ssh': user_can_clone_ssh,`
		`+ 'git_url_ssh': get_git_url_ssh()}`
		`+`
		`auth = pagure_config.get('PAGURE_AUTH', None)`
		`if auth in ['fas', 'openid']:`
		`# Only import and set flask_fas_openid if it is needed`

pagure/templates/repo_master.html

file modified

+23 -28

		`@@ -1,5 +1,24 @@`
		`{% extends "master.html" %}`

		`+ {% macro print_ssh_url(repo, git_url_ssh, current_user) %}`
		`+ {% if not user_can_clone_ssh(current_user) %}`
		`+ <a href="{{ url_for('ui_ns.user_settings') + '#nav-ssh-tab' }}">`
		`+ You need to upload SSH key to be able to clone over SSH`
		`+ </a>`
		`+ {% elif repo.read_only %}`
		`+ The permissions on this repository are being updated.`
		`+ Cloning over SSH is disabled.`
		`+ {% else %}`
		`+ <div class="form-group">`
		`+ <div class="input-group input-group-sm">`
		`+ <div class="input-group-addon">SSH</div>`
		`+ <input class="form-control bg-white" type="text" value="{{`
		`+ git_url_ssh }}{{ repo.fullname }}.git" readonly>`
		`+ </div>`
		`+ </div>`
		`+ {% endif %}`
		`+ {% endmacro %}`
		`+`
		`{% block title %}{{`
		`repo.namespace + '/' if repo.namespace }}{{ repo.name }}{% endblock %}`
		`{% set tag = "home" %}`
		`@@ -154,13 +173,7 @@`
		`<h5><strong>Source Code</strong></h5>`
		`<div>`
		`{% if g.authenticated and g.repo_committer %}`
		`- <div class="form-group">`
		`- <div class="input-group input-group-sm">`
		`- <div class="input-group-addon">SSH</div>`
		`- <input class="form-control bg-white" type="text" value="{{`
		`- git_url_ssh }}{{ repo.fullname }}.git" readonly>`
		`- </div>`
		`- </div>`
		`+ {{ print_ssh_url(repo, git_url_ssh, g.fas_user.username) }}`
		`{% endif %}`
		`<div class="form-group">`
		`<div class="input-group input-group-sm">`
		`@@ -175,13 +188,7 @@`
		`and repo.settings.get('project_documentation', True) %}`
		`<h5><strong>Documentation</strong></h5>`
		`{% if g.authenticated and g.repo_committer %}`
		`- <div class="form-group">`
		`- <div class="input-group input-group-sm">`
		`- <div class="input-group-addon">SSH</div>`
		`- <input class="form-control bg-white" type="text" value="{{`
		`- git_url_ssh }}docs/{{ repo.fullname }}.git" readonly>`
		`- </div>`
		`- </div>`
		`+ {{ print_ssh_url(repo, git_url_ssh + "docs/", g.fas_user.username) }}`
		`{% endif %}`
		`<div class="form-group">`
		`<div class="input-group input-group-sm">`
		`@@ -195,22 +202,10 @@`
		`{% if config.get('ENABLE_TICKETS', True)`
		`and repo.settings.get('issue_tracker', True) %}`
		`<h5><strong>Issues</strong></h5>`
		`- <div class="form-group">`
		`- <div class="input-group input-group-sm">`
		`- <div class="input-group-addon">SSH</div>`
		`- <input class="form-control bg-white" type="text" value="{{`
		`- git_url_ssh }}tickets/{{ repo.fullname }}.git" readonly>`
		`- </div>`
		`- </div>`
		`+ {{ print_ssh_url(repo, git_url_ssh + "tickets/", g.fas_user.username) }}`
		`{% endif %}`
		`<h5><strong>Pull Requests</strong></h5>`
		`- <div class="form-group">`
		`- <div class="input-group input-group-sm">`
		`- <div class="input-group-addon">SSH</div>`
		`- <input class="form-control bg-white" type="text" value="{{`
		`- git_url_ssh }}requests/{{ repo.fullname }}.git" readonly>`
		`- </div>`
		`- </div>`
		`+ {{ print_ssh_url(repo, git_url_ssh + "requests/", g.fas_user.username) }}`
		`{% endif %}`
		`</div>`
		`</div>`

pagure/ui/repo.py

file modified

-17

		`@@ -57,7 +57,6 @@`
		`from pagure.ui import UI_NS`
		`from pagure.utils import (`
		`__get_file_in_tree,`
		`- authenticated,`
		`login_required,`
		`is_true,`
		`stream_template,`
		`@@ -70,20 +69,6 @@`
		`_log = logging.getLogger(__name__)`


		`- def get_git_url_ssh():`
		`- """ Return the GIT SSH URL to be displayed in the UI based on the`
		`- content of the configuration file.`
		`- """`
		`- git_url_ssh = pagure_config.get('GIT_URL_SSH')`
		`- if authenticated() and git_url_ssh:`
		`- try:`
		`- git_url_ssh = git_url_ssh.format(`
		`- username=flask.g.fas_user.username)`
		`- except (KeyError, IndexError):`
		`- pass`
		`- return git_url_ssh`
		`-`
		`-`
		`def get_preferred_readme(tree):`
		`""" Establish some order about which README gets displayed`
		`if there are several in the repository. If none of the listed`
		`@@ -180,7 +165,6 @@`
		`branchname=branchname,`
		`last_commits=last_commits,`
		`tree=tree,`
		`- git_url_ssh=get_git_url_ssh(),`
		`)`


		`@@ -276,7 +260,6 @@`
		`safe=safe,`
		`readme=readme,`
		`diff_commits=diff_commits,`
		`- git_url_ssh=get_git_url_ssh(),`
		`)`
		`"""`

tests/test_pagure_flask_ui_issues.py

file modified

		`@@ -3294,6 +3294,11 @@`

		`user = tests.FakeUser()`
		`user.username = 'pingou'`
		`+`
		`+ repo = pagure.lib._get_project(self.session, 'test')`
		`+ pagure.lib.update_read_only_mode(self.session, repo, read_only=False)`
		`+ pagure.lib.get_user(self.session, 'pingou').public_ssh_key = 'foo'`
		`+ self.session.commit()`
		`with tests.user_set(self.app.application, user):`
		`# Check that the git issue URL is present`
		`output = self.app.get('/test')`

tests/test_pagure_flask_ui_repo.py

file modified

+38

		`@@ -1519,6 +1519,10 @@`
		`tests.create_projects(self.session)`
		`tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True)`

		`+ pagure.lib.get_user(self.session, 'pingou').public_ssh_key = 'foo'`
		`+ repo = pagure.lib._get_project(self.session, 'test')`
		`+ pagure.lib.update_read_only_mode(self.session, repo, read_only=False)`
		`+ self.session.commit()`
		`user = tests.FakeUser(username='pingou')`
		`with tests.user_set(self.app.application, user):`
		`output = self.app.get('/test')`
		`@@ -1562,6 +1566,40 @@`
		`self.perfMaxWalks(0, 0)`
		`self.perfReset()`

		`+ def test_view_repo_ssh_key_not_uploaded_no_ssh_url(self):`
		`+ """ Test viewing repo when user hasn't uploaded SSH key yet`
		`+ and thus should see a message instead of url for SSH cloning. """`
		`+ tests.create_projects(self.session)`
		`+ tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True)`
		`+ user = tests.FakeUser(username='pingou')`
		`+`
		`+ with tests.user_set(self.app.application, user):`
		`+ output = self.app.get('/test')`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output_text = output.get_data(as_text=True)`
		`+ self.assertIn(`
		`+ 'You need to upload SSH key to be able to clone over SSH',`
		`+ output_text)`
		`+`
		`+ def test_view_repo_read_only_no_ssh_url(self):`
		`+ """ Test viewing repo that is still readonly and thus user`
		`+ should see a message instead of url for SSH cloning. """`
		`+ tests.create_projects(self.session)`
		`+ tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True)`
		`+ repo = pagure.lib._get_project(self.session, 'test')`
		`+ pagure.lib.update_read_only_mode(self.session, repo, read_only=True)`
		`+ pagure.lib.get_user(self.session, 'pingou').public_ssh_key = 'foo'`
		`+ self.session.commit()`
		`+ user = tests.FakeUser(username='pingou')`
		`+`
		`+ with tests.user_set(self.app.application, user):`
		`+ output = self.app.get('/test')`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output_text = output.get_data(as_text=True)`
		`+ self.assertIn(`
		`+ 'Cloning over SSH is disabled.',`
		`+ output_text)`
		`+`
		`def test_view_repo(self):`
		`""" Test the view_repo endpoint. """`

bkabrda commented 6 years ago

As a usability improvement, I'd like to propose that url for SSH cloning is only displayed when:

Gitolite config for the repo is compiled (== repo doesn't have read_only set to true)
User has an ssh key uploaded

This is based on feedback from several users who had problems with cloning as one of the above conditions wasn't met and it wasn't obvious for them.

The two different messages that may show up instead of the SSH url deserve some UI love, I'm open to suggestions here.

ngompa commented on line 7 of pagure/default_config.py 6 years ago

This is a poorly named setting.

I'd probably suggest ALWAYS_RENDER_SSH_CLONE_URL or something similar.

ngompa commented on line 7 of pagure/default_config.py 6 years ago

And depending on the name, obviously adjust the default setting.

pingou commented 6 years ago

I think @ngompa has a point regarding the variable name, and we'll need tests :)

bkabrda commented 6 years ago

Agreed and thanks for the suggestion. I'll also work on tests.

How about some CSS, any suggestions there? I don't feel like this is the best way to present this information, but don't have a good idea on improving it.

ryanlerch commented 6 years ago

@bkabrda do you have a screenshot of how you are presenting the information at the moment?

bkabrda commented 6 years ago

@ryanlerch here are the two variants:

No ssh key uploaded for user: https://bkabrda.fedorapeople.org/pagure-no-ssh-key.png
Project is readonly: https://bkabrda.fedorapeople.org/pagure-readonly.png

rebased onto 0d55a6f9e4d4db6356b1d344d7adf47499142001

6 years ago

1 new commit added

Inject git_url_ssh to all templates rendered, since a lot templates are using it

6 years ago

2 new commits added

Inject git_url_ssh to all templates rendered, since a lot templates are using it
Only display SSH URL for repo if SSH key is uploaded and gitolite config is compiled

6 years ago

rebased onto db523180467c191c79d63b4c4d3dccd4842a06b9

6 years ago

rebased onto 9e14c88

6 years ago

pingou commented 6 years ago

running the tests locally :)

pingou commented 6 years ago

Tests passed :)