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

Check that the jwt token is valid when running fly targets #4181

Merged
merged 2 commits into from Jul 30, 2019

Conversation

nazrhom
Copy link

@nazrhom nazrhom commented Jul 29, 2019

I don't know go at all, but this seemed like a pretty straightforward change. Let me know if there is anything else to add (e.g. tests) but please keep in mind I might need some guidance.
Thanks.

Fixes: #4107
Change-type: patch
Signed-off-by: Giovanni Garufi giovanni@balena.io

Change-type: patch
Signed-off-by: Giovanni Garufi <giovanni@balena.io>
Copy link
Member

@jamieklassen jamieklassen left a comment

Choose a reason for hiding this comment

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

This feels like a very sensible change. While trying to advise you, @jomsie and I kinda wrote an integration test on your behalf - if you're feeling ambitious you could try to break this logic down into finer-grained unit tests, but they do seem to be lacking in the fly codebase. So take this example and do with it as you wish, but let's have some kind of test!

diff --git a/fly/integration/fixtures/flyrc-badtoken.yml b/fly/integration/fixtures/flyrc-badtoken.yml
new file mode 100644
index 000000000..4238f7770
--- /dev/null
+++ b/fly/integration/fixtures/flyrc-badtoken.yml
@@ -0,0 +1,7 @@
+targets:
+  test:
+    api: https://example.com/test
+    team: test
+    token:
+      type: Bearer
+      value: banana
diff --git a/fly/integration/targets_test.go b/fly/integration/targets_test.go
index 49a9b457b..70f5960d5 100644
--- a/fly/integration/targets_test.go
+++ b/fly/integration/targets_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/concourse/concourse/fly/ui"
 	"github.com/fatih/color"
 	"github.com/onsi/gomega/gexec"
+	"github.com/onsi/gomega/gbytes"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -16,12 +17,13 @@ import (
 
 var _ = Describe("Fly CLI", func() {
 	var (
-		flyCmd *exec.Cmd
-		flyrc  string
-		tmpDir string
+		flyCmd       *exec.Cmd
+		flyrc        string
+		tmpDir       string
+		flyrcFixture string
 	)
 
-	BeforeEach(func() {
+	JustBeforeEach(func() {
 		var err error
 		tmpDir, err = ioutil.TempDir("", "fly-test")
 		Expect(err).NotTo(HaveOccurred())
@@ -29,7 +31,7 @@ var _ = Describe("Fly CLI", func() {
 		os.Setenv("HOME", tmpDir)
 		flyrc = filepath.Join(userHomeDir(), ".flyrc")
 
-		flyFixtureFile, err := os.OpenFile("./fixtures/flyrc.yml", os.O_RDONLY, 0600)
+		flyFixtureFile, err := os.OpenFile(flyrcFixture, os.O_RDONLY, 0600)
 		Expect(err).NotTo(HaveOccurred())
 
 		flyFixtureData, err := ioutil.ReadAll(flyFixtureFile)
@@ -41,6 +43,10 @@ var _ = Describe("Fly CLI", func() {
 		flyCmd = exec.Command(flyPath, "targets")
 	})
 
+	BeforeEach(func() {
+		flyrcFixture = "./fixtures/flyrc.yml"
+	})
+
 	AfterEach(func() {
 		os.RemoveAll(tmpDir)
 	})
@@ -70,6 +76,21 @@ var _ = Describe("Fly CLI", func() {
 			})
 		})
 
+		Context("when the .flyrc contains a target with an invalid token", func() {
+			BeforeEach(func() {
+				flyrcFixture = "./fixtures/flyrc-badtoken.yml"
+			})
+
+			It("shows an error message", func() {
+				sess, err := gexec.Start(flyCmd, GinkgoWriter, GinkgoWriter)
+				Expect(err).NotTo(HaveOccurred())
+
+				Eventually(sess).Should(gexec.Exit(1))
+
+				Expect(sess.Err).To(gbytes.Say("token validation failed"))
+			})
+		})
+
 		Context("when no targets are available", func() {
 			BeforeEach(func() {
 				os.RemoveAll(flyrc)

Change-type: patch
Signed-off-by: Giovanni Garufi <giovanni@balena.io>
@jamieklassen jamieklassen merged commit 671e8f0 into concourse:master Jul 30, 2019
deniseyu pushed a commit that referenced this pull request Aug 6, 2019
* #4181 fixed the segfault, but introduced exiting 1
  if any targets had an expired or invalid token in .flyrc
* Append error message inline to display after "n/a" for each target

Signed-off-by: Denise Yu <dyu@pivotal.io>
deniseyu pushed a commit that referenced this pull request Aug 6, 2019
* #4181 fixed the segfault, but introduced exiting 1
  if any targets had an expired or invalid token in .flyrc
* Append error message inline to display after "n/a" for each target

Signed-off-by: Sameer Vohra <svohra@pivotal.io>
Co-authored-by: Sameer Vohra <svohra@pivotal.io>
@jamieklassen jamieklassen added this to the v5.5.0 milestone Aug 29, 2019
@jamieklassen jamieklassen added the release/documented Documentation and release notes have been updated. label Aug 29, 2019
jamieklassen pushed a commit that referenced this pull request Aug 29, 2019
#4181
#4228

Signed-off-by: Jamie Klassen <cklassen@pivotal.io>
Co-authored-by: James Thomson <jthomson@pivotal.io>
matthewpereira pushed a commit that referenced this pull request Sep 5, 2019
#4181
#4228

Signed-off-by: Jamie Klassen <cklassen@pivotal.io>
Co-authored-by: James Thomson <jthomson@pivotal.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/documented Documentation and release notes have been updated.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SIGSEGV on startup
2 participants