fix(api): properly authorize GET workflow fallback to archive #13957
Conversation
- the authorization was accidentally removed in f1ab5aa - also if no archived workflow is found, properly return the original error as per ac9e2de Signed-off-by: Anton Gilgur <[email protected]>
- the permission suite seemed to not have previously tested this - add good, bad, and fake token checks - where "bad" is valid, but unauthorized, while "fake" is valid in format, but not corresponding to a real token Signed-off-by: Anton Gilgur <[email protected]>
- these tests were relying on ordering of assignments before, i.e. 1. good 2. bad 3. bad - should just assign before each test instead for simplicity / less complexity / less mistakes Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Alan Clucas <[email protected]>
Joibel
force-pushed
the
fix-get-workflow-archived-auth
branch
from
ca32f0c to
d2859eb
Compare
| // Test get wf w/ archive fallback with good token | ||
| s.bearerToken = goodToken | ||
| s.Run("GetWFsFallbackArchivedGoodToken", func() { | ||
| s.e().GET("/api/v1/workflows/"+nsName). |
There was a problem hiding this comment.
nit: so the spacing around the + is different on this line than on line 641 and 649 below. I would think gofmt would make these consistent though -- it didn't make changes in #12979 (comment) either so I was thinking it's possibly not running on test files? Afaik gofmt usually does x + y with a space on both sides of the +
See also golang/go#12105 -- only one argument here
There was a problem hiding this comment.
Yeah, this is pretty bonkers, and I don't understand why. I haven't analysed it in any way because I was concentrating on getting the release out.
CI does pick it up, as you can see here when I pushed it up initially with the fixed formatting. The original test has the same thing, so the linter had 'formatted' it there too when you wrote it.
I'm most thrown by the fact the other two lines you pointed out are formatted correctly, it's just this one that's "wrong", yet they are all so similar.
There was a problem hiding this comment.
I haven't analysed it in any way because I was concentrating on getting the release out.
For sure 👍
I just noticed it incidentally
CI does pick it up, as you can see here when I pushed it up initially with the fixed formatting. The original test has the same thing, so the linter had 'formatted' it there too when you wrote it.
Well that's more bizarre than I thought 👀 Thanks for checking that!
In this case I'd hazard a guess this might be a gofmt bug or some fairly odd quirk explained by some other line widths; potentially worth filing a bug report in either scenario 😅
|
@Joibel could you update GHSA-h36c-m3rf-34h9 to mention that it is resolved by this PR? Usually we've had it written at the bottom of the "Summary" section (for example, see GHSA-ghjw-32xw-ffwr) Just to be able to follow the fix accordingly and posterity/historical lineage. |
Cherry pick of patch for GHSA-h36c-m3rf-34h9