-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add owner flag validation during provider enrollment #4107
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @psekar!
This approach looks good to me! I've added a few comments inline with suggestions that will simplify the code.
Feel free to reach out if you have any questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick updates again @psekar!
I've left some final comments about cleanup / linting changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also noticed that the mocks need to be updated. You can run make gen
in the root directory before committing your code and that will update them.
1a1cc4a
to
155261a
Compare
a8a1e9d
to
25dbdac
Compare
Fixed the linter issues and added more test coverage now. @eleftherias Appreciate your review 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience on this @psekar!
The cyclomatic complexity of processOAuthCallback
is still high, I've proposed a change inline that will fix it. After this change, we should be ready to merge.
25dbdac
to
f1c2350
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @psekar!
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:
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: