archimatetool / archi-modelrepository-plugin

coArchi - a plug-in to share and collaborate on Archi models.
153 stars 52 forks source link

Add git ssh support #78

Closed potto007 closed 5 years ago

potto007 commented 5 years ago

I've done my best to retrofit in ssh support into the workflow and preferences without causing any abrupt changes. The new preference pane gives the user the ability to:

screen shot 2019-02-01 at 2 40 45 pm
lindison commented 5 years ago

This is awesome!

jbsarrodie commented 5 years ago

Hi,

This seems great (not tested on my side at the moment though).

My only remark would be related to the Preference pane: I think we should now revamp it a bit:

@Phillipus what do you think?

Phillipus commented 5 years ago

Hi,

thanks for contributing this, we appreciate it!

I haven't looked at the code in depth, nor tested it yet but I will do so over the coming days. Presumably I can just set things up from the Preferences page?

Leave it with me and I'll get back to you when I've had a chance to investigate in depth.

@Phillipus what do you think?

Nice! ! Phil

Phillipus commented 5 years ago

@potto007

I need you to do an "ELI5" on this! ("Explain Like I'm Five") ;-)

First problem I have is I don't have an "id_rsa" file so I get an error message. Can you talk me through how to get this going. The test URL is:

git@github.com:archimate-models/archisurance.git

(I'm putting myself in the position of the first time user who may not know about SSH storage files.)

Phillipus commented 5 years ago

getSSHIdentityPassword() can return null. When it does return null the Collaboration Workspace window fails to load. So needs some null checking there.

Overall, I think the code needs to be quite defensive in ensuring that everything is in place when using SSH option - null checking, check if ssh files exist, etc.

Phillipus commented 5 years ago

I followed the online instructions to create SHH keys for GitHub but when trying to add a new Repo in Archi I get "invalid private key". Any advice?

potto007 commented 5 years ago

Hi @Phillipus - you may need to ensure that ssh-keygen is outputting a keyfile with the PEM standard and not the newer RFC 4716 standard. You can tell by running file on the contents of your ~/.ssh/id_rsa file; ie: file ~/.ssh/id_rsa. *nix will respond with PEM RSA private key if it's PEM, and OpenSSH private key if it's RFC 4716.

Unfortunately, jgit does not support the newer format. For what it's worth, egit added the support a couple of months ago.

If you need to generate a new keyfile that is PEM format, use ssh-keygen -m PEM.

Phillipus commented 5 years ago

@potto007 OK, it's RFC 4716 OpenSSH. So as I'm testing with GitHub will that support the PEM standard? I'm still not clear how to set this up to test with GitHub.

potto007 commented 5 years ago

With regards to more defensive code, I ran into trouble having great exception / error handling from the place in the class hierarchy org.archicontribs.modelrepository.grafico.ArchiRepository sits in, which is why the silent fails end up happening in a couple places (it's logging errors in the background). That abstract class doesn't have access to the workbench window handles - any suggestions?

That said, things like calls to getSSHIdentityPassword() do need to be more defensive. I'll do that.

potto007 commented 5 years ago

@potto007 OK, it's RFC 4716 OpenSSH. So as I'm testing with GitHub will that support the PEM standard? I'm still not clear how to set this up to test with GitHub.

Yes, GitHub supports PEM - I actually only learned about the newer standard when I recently created a new keyfile after upgrading my workstation to macOS Mojave. Everything supports PEM, not everything supports RFC 4716.

Phillipus commented 5 years ago

~Best thing to do is when I get it working I'll create a new branch, refactor the changes commit and then we can continue on that branch.~ I've made some recommendations, below.

At the end you then submit a new PR.

Phillipus commented 5 years ago

One thing that's needed is to try and ease the pain for users to get this working. Either through clear instructions on generating the keyfile or some other means.

potto007 commented 5 years ago

One thing that's needed is to try and ease the pain for users to get this working. Either through clear instructions on generating the keyfile or some other means.

Okay, yeah. I could create documentation for that. I approached this from the angle of people who already uses SSH git, and need that in Archimate Tool.

lindison commented 5 years ago

I find that most of the request for this feature were from users already using ssh to authenticate into github, gitlab, etc. The feature has always been requested with a user who presently uses only ssh keys to interact with their git server. Also, the plugin should support both users of https and ssh and allow those who use only an ssh key for auth to git to use Archimate (as is my case).

lindison commented 5 years ago

The ssh features with this plugin also work with the following: Windows 10 Professional 10.0.17134.0

SSH key generated using this process: https://help.github.com/articles/generating-a-new-ssh-key-and-adding-it-to-the-ssh-agent/#platform-windows

SSH id_rsa.pub added to my profiles SSH and GPG keys section of GitHub.

Like the MacOS version, works right out of the box.

Unchecked ssh and used https and was able to import a model from github.

Phillipus commented 5 years ago

If the global setting for using SSH is either on or off then that means that a user can only use SSH or HTTPS but not both. This would affect syncing of repos, as HTTPS would fail to authenticate.

Would it be possible to identify which to use by the repo URL so that users can continue to use their HTTPS repos and also SSH?

@jbsarrodie What do you think?

Phillipus commented 5 years ago

For others testing, here's how I created the keyfiles and added it to GitHub.

I used Windows and a Git Bash terminal.

Create the files:

  1. ssh-keygen -t rsa -b 4096 -m PEM -C "yourGitHubEmailAddress"
  2. Enter a pass-phrase when prompted
  3. eval $(ssh-agent -s)
  4. ssh-add ~/.ssh/id_rsa

Add to GitHub:

  1. Copy the public key to clipboard with clip < ~/.ssh/id_rsa.pub
  2. At GitHub create a new key at https://github.com/settings/keys and paste the contents of the clipboard

For reference:

https://help.github.com/articles/generating-a-new-ssh-key-and-adding-it-to-the-ssh-agent/#platform-windows

https://help.github.com/articles/adding-a-new-ssh-key-to-your-github-account/

jbsarrodie commented 5 years ago

What do you think?

I think that asof today settings are already either on or off (either proxy is used for all models synced or for none), so a first step in which users can decide to use SSH for all or none of their model makes sens.

Of course, in the future, Preferences should be only used as defaults by synced models, but user should be able to override these setting model per model... but this is not a quick and simple change.

Phillipus commented 5 years ago

If it's possible to know what protocol each repo uses it would be simple just to use the appropriate one for that repo.

jbsarrodie commented 5 years ago

If it's possible to know what protocol each repo uses it would be simple just to use the appropriate one for that repo.

Yes, but at the end user could well want to use SSH for several models but with different ID. This is the same as user/password: these are in fact per-model settings.

I would go for the simplest solution today (central configuration) and think more about it later (taking all preferences into account). I guest more users requesting this feature are using a single Git server in their company and not consultants (like me) working for 10+ companies, each using different solution ;-)

Phillipus commented 5 years ago

@potto007 OK here are the things that need to be addressed if you could take a look at these, please.

  1. Remove the following entries from the .classpath file, as they are causing log problems: <classpathentry exported="true" kind="lib" path="lib/slf4j-api-1.7.25.jar"/> <classpathentry exported="true" kind="lib" path="lib/slf4j-simple-1.7.25.jar"/>
  2. Refactor the SshSessionFactory code that's currently in the ArchiRepository class into its own helper class, so that it doesn't mix concerns. (Much like UsernamePasswordCredentialsProvider is a separate class). This can also check for user prefs whether to use https or ssh in ArchiRepository class.
  3. getSSHIdentityPassword() can return null. This needs to be checked when called.
  4. Plan for when there is no id_rsa file or when it is not in correct format so that the user knows (perhaps throw an exception)
  5. You've commented out the line testConnection(repositoryURL, null); in ProxyAuthenticater so if this is causing a problem because of using ssh then a check needs to be made at this point so it is still called if https is used.

BTW - I remember I did get proof of concept ssh working last July (commit 38f1e25dbb19619478545f02377a7d87c1c657c4 in branch "ssh") but I don't think I had to use PEM when generating the key.

Phillipus commented 5 years ago

There hasn't been any progress on this, so I've addressed the issues above.

@potto007 Thanks for this PR, but as I can't do an actual code merge on the existing PR I have to close this. This is not a rejection!

To be continued in Issue #81...

Phillipus commented 5 years ago

The new code has been pushed to the "ssh2" branch.

potto007 commented 5 years ago

Fair enough. FWIW, I wasn't ignoring your PR comment, I have just had too much going on over the weekends and other things have taken priority at work. I agree that it's probably best to approach this as an Issue anyways, since a full and proper implementation requires some refactoring within the workflow.

Phillipus commented 5 years ago

@potto007 No problem.

The thing with PRs is that you're only offered the choice of merge or reject when actually you want to "negotiate and continue".

When a user submits a PR ideally we want the git commits to carry the user name so they can take the credit (or blame ;-)) for the work. But if the PR is kind of more of a proof of concept and needs more work that's not always possible.

However...GitHub has just introduced the option of Draft Pull requests for this exact use case.

See https://github.blog/2019-02-14-introducing-draft-pull-requests/

So I think draft PRs are the way to go for all PRs.

potto007 commented 5 years ago

I saw your update to the contributor section this morning and just finished reading GitHub's blog entry! :) I agree, these are a significant improvement to the process on GitHub.

Phillipus commented 5 years ago

Ideally I think the PR process should also be branch based. Something like:

  1. Contributor opens an issue for discussion.
  2. After agreement a new branch is created and the contributor works in that branch.
  3. A draft PR is issued and work and refactoring continues between the contributor and the owner in that branch
  4. When agreed and ready, the PR is made "ready for review" and merged into the branch
potto007 commented 5 years ago

For what it's worth, that is how PR's have worked for me in the past.

  1. I fork a project
  2. Create a feature branch in that my forked version
  3. Commit code changes to feature branch
  4. Push feature branch to my forked version on GitHub
  5. Create a PR from my feature branch against the project
  6. Await PR code review comments
  7. If necessary, make changes
  8. Commit changes to feature branch
  9. Push feature branch to my forked version on GitHub -- GitHub automatically updates the PR with the changes

Some projects I've contributed to, the maintainers ask if I can kindly amend my commits or squash merge my commits into one, then force push my changes to rewrite history. Other project maintainers accept all commits atomically, change the merge branch from master to a WIP branch, accept and merge the PR, then squash merge their WIP branch into master.