minder icon indicating copy to clipboard operation
minder copied to clipboard

Add owner flag validation during provider enrollment

Open psekar opened this issue 1 year ago • 4 comments
trafficstars

Summary

***Provide a brief overview of the changes and the issue being addressed. This PR addresses the issue #2723 . When enrolling a provider, the owner flag is not validated. Incase the owner flag is invalid or the token has no access to the owner/organization during provider enrollment, the repo registration will fail which is a later step in the process. The change includes checking for a valid GH organization (passed as owner flag) that matches the owner flag and ensure the token has access to the GH organization.

Explain the rationale and any background necessary for understanding the changes. List dependencies required by this change, if any.*** I created a type struct to make Git API calls prior to actual provider enrollment flow, also created an interface to be able to mock the tests. The intent of the existing types seems to be useful post provider enrollment so decided to create a new type.

Fixes #(related issue) #2723

Change Type

Mark the type of change your PR introduces:

  • [x] Bug fix (resolves an issue without affecting existing features)
  • [ ] Feature (adds new functionality without breaking changes)
  • [ ] Breaking change (may impact existing functionalities or require documentation updates)
  • [ ] Documentation (updates or additions to documentation)
  • [ ] Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations. Attach screenshots if helpful. 👉 Existing tests will validate the valid owner path. 👉 Added the unit tests to validate the invalid owner flag code path. 👉 Provide a valid owner flag (a GH org) and provider enrollment is successful 👉 Provide an invalid owner flag and provider enrollment fails during the oauth flow with clear error message about the owner flag.

Review Checklist:

  • [x] Reviewed my own code for quality and clarity.
  • [ ] Added comments to complex or tricky code sections.
  • [ ] Updated any affected documentation.
  • [x] Included tests that validate the fix or feature.
  • [ ] Checked that related changes are merged.

psekar avatar Aug 09 '24 00:08 psekar

Thanks for the updates @psekar!

I've added a few follow-up comments. You can also run make lint before committing to makes sure the code passes the lint rules.

I am still very new to Go and your review has been super useful 🙏

psekar avatar Aug 13 '24 04:08 psekar

Coverage Status

coverage: 53.817% (+0.07%) from 53.752% when pulling f1c23506d2fd4127a959f4a9f34260ce513e65c6 on tinytrail:validate-owner-filter into 9a3fc9462768b610feb174ea94011bcfef39c9e4 on stacklok:main.

coveralls avatar Aug 13 '24 07:08 coveralls

Coverage Status

coverage: 53.872% (-0.03%) from 53.899% when pulling 94472cb on tinytrail:validate-owner-filter into e438848 on stacklok:main.

Will add more test coverage and send it back for review tomorrow.

psekar avatar Aug 15 '24 05:08 psekar

Coverage Status coverage: 53.872% (-0.03%) from 53.899% when pulling 94472cb on tinytrail:validate-owner-filter into e438848 on stacklok:main.

Will add more test coverage and send it back for review tomorrow.

Fixed the linter issues and added more test coverage now. @eleftherias Appreciate your review 👍

psekar avatar Aug 17 '24 15:08 psekar