Skip to content
This repository was archived by the owner on Nov 28, 2022. It is now read-only.

2647.5 store secure template repo creds for use in project create #482

Conversation

rwalle61
Copy link
Contributor

@rwalle61 rwalle61 commented May 22, 2020

What type of PR is this ?

  • Bug fix
  • Enhancement

What does this PR do ?

For eclipse-archived/codewind#2647 (comment):

As per the design above, we want cwctl to let the user provide credentials just once (when adding the secure template repo), and not need to re-enter the credentials when creating a project from one of those secure templates.

I will make cwctl do this by storing the credentials in the keychain when the user calls cwctl templates repos add, and using those credentials when the user calls cwctl project create .

See updated README and tests for more details

Note: this allows cwctl project create to use GHE personal access tokens, but will need a minor tweak when we want to use tokens to access non-GHE URLs. Once we have an example a non-GHE URL from which we can download projects using access tokens, we can make this change (stop using a GitHub Enterprise client in these cases)

Which issue(s) does this PR fix ?

part of eclipse-archived/codewind#2647

Does this PR require a documentation change ?

Included

Any special notes for your reviewer ?

  • Most/all of the tests I've added have to be skipped in Jenkins
  • Tests use my GHE template repo. Would be good to move to a GHE repo we all control

@codecov
Copy link

codecov bot commented May 22, 2020

Codecov Report

Merging #482 into master will decrease coverage by 0.18%.
The diff coverage is 11.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #482      +/-   ##
==========================================
- Coverage   20.84%   20.66%   -0.19%     
==========================================
  Files          61       62       +1     
  Lines        5157     5236      +79     
==========================================
+ Hits         1075     1082       +7     
- Misses       3954     4026      +72     
  Partials      128      128              
Impacted Files Coverage Δ
pkg/apiroutes/templates.go 3.72% <0.00%> (ø)
pkg/templates/templates.go 0.00% <0.00%> (ø)
pkg/utils/download.go 53.07% <55.55%> (-3.49%) ⬇️
pkg/project/create.go 49.75% <100.00%> (ø)
pkg/utils/templates.go 12.28% <0.00%> (+12.28%) ⬆️

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 b872644...790f3ea. Read the comment docs.

@hhellyer hhellyer self-assigned this May 27, 2020
Copy link
Contributor

@hhellyer hhellyer left a comment

Choose a reason for hiding this comment

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

I tried testing locally creating a project from a non-authenticated repo and that failed (but worked with master):

$ ./cwctl project create -p /tmp/testproj -u https://github.com/codewind-resources/nodeExpressTemplate
ERRO[0000] {"error":"sec_keyring_secret_not_found","error_description":"Secret org.eclipse.codewind.local.gitcredentials-03c5555f-26c0-5945-96a5-b09de8f4f66f not found in keyring"} 

This needs resolving before we can merge.

@@ -80,7 +81,7 @@ func AddTemplateRepo(c *cli.Context) {
}

conID := strings.TrimSpace(strings.ToLower(c.String("conid")))
repos, err := apiroutes.AddTemplateRepo(conID, url, desc, name, gitCredentials)
repos, err := templates.AddTemplateRepo(conID, url, desc, name, gitCredentials)
Copy link
Contributor

Choose a reason for hiding this comment

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

We now have AddTemplateRepo/DeleteTemplateRepo in pkg/actions/templates.go, pkg/apiroutes/templates.go and pkg/templates/templates.go.
Should anything else be calling the code in apiroutes other than the code in pkg/templates/templates.go? Possibly that code should move to prevent confusion.

Copy link
Contributor Author

@rwalle61 rwalle61 May 27, 2020

Choose a reason for hiding this comment

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

yes I wasn't sure how to cut up our code to avoid huge files, happy to take suggestions.

The previous logic flow was pkg/actions/templates.go -> pkg/apiroutes/templates.go, and apiroutes just handled communication with PFE.

But now we need more than just communicating with PFE, because e.g. we need to save and extract git credentials from the keychain. So I thought we could use a pkg/templates/templates.go to coordinate template stuff.

So I changed the flow to pkg/actions/templates.go -> pkg/templates/templates.go -> pkg/apiroutes/templates.go.

But I'm open to ideas. I notice that our project package handles communication with PFE too, rather than calling a pkg/apiroutes/projects.go file

@rwalle61
Copy link
Contributor Author

@hhellyer thanks for the review!

65d35e7 fixes the bug you found. The bug was that we couldn't create projects from URLs matching a template already added to Codewind that has a source ID.

I fixed it by implementing the following logic: "if this URL matches a sourceID, but we can't find the keychain secret for this URL, assume the URL does not need git credentials from the keychain". The credentials should be correctly stored when we add the template repo in the first place

Other commits:

  • add tests
  • stop storing nil git credentials when adding insecure template repos
  • fix tests broken by update to PFE templates API

@hhellyer hhellyer merged commit a0997b8 into eclipse-archived:master May 27, 2020
@rwalle61 rwalle61 deleted the 2647-store-git-creds-for-use-in-proj-create branch May 27, 2020 15:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants