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

Rework SSH key management UI to add GPG #1293

Merged
merged 9 commits into from
Apr 26, 2017

Conversation

sapk
Copy link
Member

@sapk sapk commented Mar 17, 2017

Complete GPG implementation #710 and #1150

image

TODO :

  • Rename SSH keys to keys
  • List GPG Key
  • Display key validity in time
  • Add GPG Key
  • Add translatation

@lunny lunny added this to the 1.2.0 milestone Mar 17, 2017
@lunny lunny added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Mar 17, 2017
@sapk sapk mentioned this pull request Mar 19, 2017
6 tasks
@Fastidious
Copy link

An early, and minimal, suggestion: change the key icon to a lock icon for the GPG/PGP portion. Really love, and appreciate the work you are doing to get this in!

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 20, 2017
@drsect0r
Copy link
Contributor

Awesome work! 👍

@sapk
Copy link
Member Author

sapk commented Mar 23, 2017

@Fastidious I was thinking of making it like github that use also a key for both but add a label under. But it would be better to just have a different icon like you propose.
For the next 5 days, I will not have much time but I will finish it just after.

@sapk
Copy link
Member Author

sapk commented Mar 23, 2017

@pree
Copy link

pree commented Mar 24, 2017

Hey, I just tested the gpg Validation and really like it!
Is it intended, that the Validation (green box + icons) are not shown in the Code-Overview ?
This is something that GitHub doesn't have, because they are not showing the commit-ref in the Code-Overview, but since Gitea shows that, it would be really awesome to see the verified "Checkmarks" there too

@sapk
Copy link
Member Author

sapk commented Mar 24, 2017

@Schizopriest It could be included in most part easily now with just passing commit list to ParseCommitsWithSignature like https://github.com/go-gitea/gitea/pull/1150/files#diff-da47b1b0b02ed3ac1a84db3edc178279R71 this will add information about validity of signature in template and use it like https://github.com/go-gitea/gitea/pull/1150/files#diff-7a50b55ffb3c01e4e467c74b23e0863bR44). I simply wanted to make it work simple at first and not change everywhere for base and not change a lot a pages. But now it could be done everywhere

@sapk sapk force-pushed the gpg-key-management branch 3 times, most recently from 9c9a60e to b185d55 Compare March 25, 2017 16:49
@bkcsoft
Copy link
Member

bkcsoft commented Apr 18, 2017

Woooh! Awesome 😄 Few conflicts though.

Some notes:

  • Maybe break the template down, see Refactor large templates #1275
  • Not a fan of any of the suggested alternative icons. TBH just go with the same key for both, potential icon-changes can be discussed in a later issue 🙂

@sapk
Copy link
Member Author

sapk commented Apr 18, 2017

I broke myself this PR with a other PR removing extra formatting in settings pages. ^^

I will put time on this one soon since it is the last part of sign commit with GPG key.

@sapk sapk force-pushed the gpg-key-management branch 2 times, most recently from 629e2ba to 2a3947e Compare April 19, 2017 02:46
@sapk
Copy link
Member Author

sapk commented Apr 19, 2017

This PR work now, I will look into optimization of verification since I found that hash class are consume and not re-usable so I fix it by regenerating hash before verifying against commiter keys but this is not really efficient way. I think, I will put it under a other PR since this only a overhead for signed commit and relative to the number of key of the signing user (and stop at first validation).

@sapk sapk changed the title [WIP] Rework SSH key management UI to add GPG Rework SSH key management UI to add GPG Apr 19, 2017
@lunny lunny removed the pr/wip This PR is not ready for review label Apr 19, 2017
@lunny
Copy link
Member

lunny commented Apr 19, 2017

@sapk great job! I will test this.

if ($this.attr("id")) {
filter += "#"+$this.attr("id")
}
$('.delete.modal'+filter).modal({
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure that nothing else on the site uses this?

Copy link
Member Author

@sapk sapk Apr 20, 2017

Choose a reason for hiding this comment

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

I have check that all matching .delete-button don't have id so that this change don't interact with it. I will double check and confirm it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I confirm !

@@ -0,0 +1,144 @@
{{template "base/head" .}}
Copy link
Member

Choose a reason for hiding this comment

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

Could we break this file into 3? Having settings/keys.tmpl, settings/keys_ssh.tmpl and settings/keys_gpg.tmpl? Would make it easier to copy-paste a file to possibly add more key-types in the future. Also makes the files smaller ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I will do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done !

@lunny
Copy link
Member

lunny commented Apr 20, 2017

500 when I add a GPG key with a non-exist email address.

@sapk
Copy link
Member Author

sapk commented Apr 20, 2017

Thanks for the feedback.

@lunny I added a error type to catch to be display.

I think that when integration test are ready, this functionality will need some testing scenario.

@lunny
Copy link
Member

lunny commented Apr 21, 2017

@sapk another consideration. After I added a gpg key, then I remove the email address. What will happen?

@sapk
Copy link
Member Author

sapk commented Apr 21, 2017

Good question ^^

@sapk
Copy link
Member Author

sapk commented Apr 21, 2017

For validation of commit, those will failed because of 14fe901#diff-212b9fa4c927665ecbc5a5a36382ecedR385

For display of GPG keys this need to be checked. At first view, I think that there will still be display and not failed because we grab them by OwnerID. But I don't know how xorm will handle Emails fields leading to nothing. ca1c3f1#diff-212b9fa4c927665ecbc5a5a36382ecedR33

@lunny
Copy link
Member

lunny commented Apr 21, 2017

But anyhow I will give this LGTM.

@tboerger 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 Apr 21, 2017
@bkcsoft
Copy link
Member

bkcsoft commented Apr 25, 2017

Yeah, appart from the conflict this is LGTM :D

@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 Apr 25, 2017
@sapk
Copy link
Member Author

sapk commented Apr 25, 2017

OK I will rebase

@sapk
Copy link
Member Author

sapk commented Apr 26, 2017

Done, conflict was from #1290 because it move route from cmd to a package (for a good reason ^^).

@lunny lunny merged commit 8371f94 into go-gitea:master Apr 26, 2017
@sapk sapk deleted the gpg-key-management branch April 28, 2017 10:04
@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
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants