Gitea drone #28

Dunky13 · 2021-03-30T15:30:00Z

Update tasks to allow Gitea to get client ID/Secret for Drone to work/implement with Gitea

Fixes: #26

The script checks and compares if there is a secret on the cluster, and whether is matches with the oauth in Gitea. If it maches, it checks whether authorization for that oauth key is granted, if not grant it as well. That way human interaction is minimized. redkubes/unassigned-issues#170, linode/apl-core#364, #26

Renamed class to GiteaDroneOAuth so it is more descriptive #26

Removed an uncessary else statement #26

add debug logs to find issue in compiled code #26

Added more debug logs to find the issue #26

Print error message along trace #26

for some reason the program doesn't want to eat it's cookies, trying to see what's in them that it doesn't like #26

Needed to set redirect uri to drone instead of gitea #26

Found issue (redirect uri), fixed it, now is clean up time #26

Seems like when oauth is ready, it will follow redirect. Don't want/need that. #26

values repo was by default public (whoops) - now it's private redkubes/unassigned-issues#170

package.json

src/tasks/gitea/gitea-drone-oauth.ts

Morriz

looking good....please rewrite the class to functions and wrap up minor issues...and ship!

package.json

src/tasks/gitea/gitea-drone-oauth.ts

Clarified error messages, and fixed naming #26 #28

Create a droneLoginUrl variable, as well as making the csrfToken local, and remove implicit any #26 #28

_csrf is MVC necessary for authorization & grants of oauth #26 #28

j-zimnowoda

Could you write some unit tests?

Make script functional #26 #28

Also run the main function, only when executed the script. Don't run it if it is imported. This messes up unit tests. #26 #28

Dunky13 · 2021-04-01T15:01:39Z

@Morriz and @j-zimnowoda I implemented your suggestions, rewrote part of the script to make it better testable. Hope all is well now?

Use ts-custom-error as a shim for the tests. Make DRONE_URL an env variable. And lazy load apiClient #26 #28

src/tasks/gitea/gitea-drone-oauth.test.ts

@j-zimnowoda

Based on @j-zimnowoda review, use already encoded values #26 #28

Morriz

Lets see if we can clean this up and move on

Morriz · 2021-04-06T08:01:32Z

src/tasks/gitea/gitea-drone-oauth.ts

+  } else if (oauthApps.length == 0) {
+    console.log("Gitea doesn't have any oauth application defined")


Please understand it is WE who add them, so WE are in control. WE can check if one exists and NEVER insert another.

Morriz · 2021-04-09T08:43:56Z

src/tasks/gitea/gitea-drone-oauth.test.ts

+  afterEach(() => {
+    sandbox.restore()
+  })
+  it('should successfully validate secret', () => {


The secret content is validated through typing as a contract. No logic is involved there. No tests should exist therefor.

Can we please move on?

Morriz · 2021-04-09T08:44:46Z

src/tasks/gitea/gitea-drone-oauth.test.ts

+  })
+})
+
+describe('Gitea-Drone: Parse CSRF Token', () => {


Really? After all my comments you still advocate for useless tests?

Morriz · 2021-04-09T08:45:48Z

src/tasks/gitea/gitea-drone-oauth.test.ts

+  })
+})
+
+describe('Gitea-Drone: Parse CSRF Token', () => {


when I say use the force it means you don't come up with it yourself but instead use the powers given to you through open source software. Use micro libraries to avoid inventing a cookie parser.

Morriz · 2021-04-09T08:48:02Z

src/tasks/gitea/gitea-drone-oauth.ts

+  try {
+    await axios.request(grantOptions)
+  } catch (error) {
+    // Do nothing, error code could be on the redirect


Ok, that is fine. Just put these remarks there in the comment to be precise.

Morriz · 2021-04-09T08:48:58Z

src/tasks/gitea/gitea-drone-oauth.ts

+
+export function isSecretValid(remoteSecret: DroneSecret, oauthApps: OAuth2Application[]): boolean {
+  if (!remoteSecret) {
+    console.log('Remote secret was not found')


Why is null valid?

Morriz · 2021-04-09T08:50:13Z

src/tasks/gitea/gitea-drone-oauth.ts

+  if (!remoteSecret) {
+    console.log('Remote secret was not found')
+  } else if (remoteSecret.clientId.length == 0 || remoteSecret.clientSecret.length == 0) {
+    console.log('Remote secret values were empty')


If we determine that the preconditions have not been met we should just fail.

src/tasks/gitea/gitea-drone-oauth.ts

Morriz · 2021-04-09T08:52:43Z

src/tasks/gitea/gitea-drone-oauth.ts

+    (x) => x.name === oauthOpts.name,
+  )
+  // If secret exists (correctly) and oauth apps are (still) defined, everything is good
+  if (isSecretValid(remoteSecret, oauthApps)) {


Please read my comments about why I think it should not even exist

Morriz · 2021-04-11T15:53:39Z

I would really like to see my remarks implemented and move on with the release of this. Can you please proceed and just remove the tests I deemed invalid?

j-zimnowoda

Marc, you addressed most of the remarks and proved that the code is functional.
Thank you for your hard work.

j-zimnowoda · 2021-04-12T07:56:53Z

src/tasks/gitea/gitea-drone-oauth.test.ts

+  afterEach(() => {
+    sandbox.restore()
+  })
+  it('should successfully validate secret', () => {


The typing is static and has nothing to do with code execution

Morriz

good enough for now

j-zimnowoda

@Morriz this PR is a mess that you have made.
There are way too many changes that you did not agree with the team.
This is not healthy. You also removed a lot of tests without justifying it.
The Wild West coast of yours makes the team highly inefficient.

Morriz · 2021-04-15T08:51:17Z

The changes are refactoring changes, not functional. None of those unit tests I removed tested any of the code we have. Please check your statements.

Morriz · 2021-04-15T08:54:15Z

It is far from a "mess" imo and is a better starting point to create any tests on iyam. Fortunately, the tasks are extremely robust and idempotent, and well tested manually. So I am not afraid about missing tests here. I urge any in our team to really read up on testing practices and only write tests that cover our code paths. I can present one in this repo next week as an example...

Marc Went added 15 commits March 18, 2021 21:34

fix: rename class to be more descriptive

74e6ec4

Renamed class to GiteaDroneOAuth so it is more descriptive #26

fix: remove unnecessary else statement

5933752

Removed an uncessary else statement #26

refactor: add debug logs to find issue in compiled code

d9ec732

add debug logs to find issue in compiled code #26

refactor: add more debug logs

f4982db

Added more debug logs to find the issue #26

refactor: write error message along the trace to log

0d25bcf

Print error message along trace #26

refactor: print cookies

5015b66

for some reason the program doesn't want to eat it's cookies, trying to see what's in them that it doesn't like #26

refactor: add more debug logs #26

e5455eb

fix: redirect uri seems to be set to drone

9e7b25a

Needed to set redirect uri to drone instead of gitea #26

refactor: cleanup after fixing issue

8bf5109

Found issue (redirect uri), fixed it, now is clean up time #26

refactor: something still goes wrong, re-adding logging #26

5042185

fix: prevent redirect mayhem

56619f1

Seems like when oauth is ready, it will follow redirect. Don't want/need that. #26

refactor: update logging messages #26

61e6998

refactor: show little less error data #26

a947a88

fix: values repo was not private, now it is

388ab5b

values repo was by default public (whoops) - now it's private redkubes/unassigned-issues#170

Dunky13 requested review from Morriz and j-zimnowoda March 30, 2021 15:30

j-zimnowoda requested changes Mar 30, 2021

View reviewed changes

Morriz suggested changes Mar 31, 2021

View reviewed changes

Marc Went added 3 commits March 31, 2021 10:54

fix: fix and refactor comments on the PR by @j-zimnowoda and @Morriz

6fc4e50

Clarified error messages, and fixed naming #26 #28

refactor: some more refactorings, forgot these in the previous commit

665d7bc

Create a droneLoginUrl variable, as well as making the csrfToken local, and remove implicit any #26 #28

docs: add comment to explain why _csrf cookie is necessary

05105c2

_csrf is MVC necessary for authorization & grants of oauth #26 #28

j-zimnowoda reviewed Mar 31, 2021

View reviewed changes

Marc Went added 3 commits March 31, 2021 11:37

refactor: remove class, and make it functional

4413fdc

Make script functional #26 #28

test: refactor base code and write unit tests

a1647bc

Also run the main function, only when executed the script. Don't run it if it is imported. This messes up unit tests. #26 #28

refactor: remove unused import #26 #28

2a8a39b

Dunky13 requested review from j-zimnowoda and Morriz April 1, 2021 15:01

fix: fix some reviews by @Morriz

fdba707

Use ts-custom-error as a shim for the tests. Make DRONE_URL an env variable. And lazy load apiClient #26 #28

j-zimnowoda reviewed Apr 8, 2021

View reviewed changes

src/tasks/gitea/gitea-drone-oauth.test.ts Outdated Show resolved Hide resolved

refactor: use pre-encoded values instead of encoding in every step

235b25e

Based on @j-zimnowoda review, use already encoded values #26 #28

Morriz suggested changes Apr 9, 2021

View reviewed changes

j-zimnowoda approved these changes Apr 12, 2021

View reviewed changes

Maurice Faber added 2 commits April 15, 2021 03:26

refactor: tasks using shared code, harbor upgrade

01fa413

fix: docs [ci skip]

930e340

Morriz assigned Morriz and unassigned Morriz Apr 15, 2021

Morriz requested review from j-zimnowoda and Morriz April 15, 2021 01:29

Morriz approved these changes Apr 15, 2021

View reviewed changes

j-zimnowoda requested changes Apr 15, 2021

View reviewed changes

Maurice Faber added 10 commits April 15, 2021 20:41

fix: mixed up func args order [ci skip]

05afb5a

refactor: unified tasks to use doApiCall

4457952

feat: added drone task back [ci skip]

69bd8b0

fix: vulnerability [ci skip]

c7b11e9

fix: dep version

fa7e2ec

fix: missing file [ci skip]

66c1f9d

fix: trying org repo ci skip]

cfc6341

fix: auth [ci skip]

3eb2c10

refactor: added functionality from api for future work

af4580a

fix: lint

9cd3a4b

Morriz merged commit 0e58f0c into master May 6, 2021

delete-merged-branch bot deleted the gitea-drone branch May 6, 2021 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gitea drone #28

Gitea drone #28

Dunky13 commented Mar 30, 2021

Morriz left a comment

j-zimnowoda left a comment

Dunky13 commented Apr 1, 2021

Morriz left a comment

Morriz Apr 6, 2021

Morriz Apr 9, 2021

Morriz Apr 9, 2021

Morriz Apr 9, 2021

Morriz Apr 9, 2021

Morriz Apr 9, 2021

Morriz Apr 9, 2021

Morriz Apr 9, 2021

Morriz commented Apr 11, 2021

j-zimnowoda left a comment

j-zimnowoda Apr 12, 2021

Morriz left a comment

j-zimnowoda left a comment

Morriz commented Apr 15, 2021

Morriz commented Apr 15, 2021

		} else if (oauthApps.length == 0) {
		console.log("Gitea doesn't have any oauth application defined")

Gitea drone #28

Gitea drone #28

Conversation

Dunky13 commented Mar 30, 2021

Morriz left a comment

Choose a reason for hiding this comment

j-zimnowoda left a comment

Choose a reason for hiding this comment

Dunky13 commented Apr 1, 2021

Morriz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morriz commented Apr 11, 2021

j-zimnowoda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morriz left a comment

Choose a reason for hiding this comment

j-zimnowoda left a comment

Choose a reason for hiding this comment

Morriz commented Apr 15, 2021

Morriz commented Apr 15, 2021