feat(authz): authorization webhook to check team existence#1894
feat(authz): authorization webhook to check team existence#1894
Conversation
…stence On-behalf-of: @SAP krzysztof.zagorski@sap.com
On-behalf-of: @SAP krzysztof.zagorski@sap.com
…stence On-behalf-of: @SAP krzysztof.zagorski@sap.com
There was a problem hiding this comment.
Pull request overview
This PR implements validation of Team existence and support-group status in the authorization webhook, addressing a security gap where Teams referenced in resource ownership labels were not validated. The changes refactor the authorization logic to use a unified flow for both ServiceAccount and user authentication, with new functions that cleanly separate concerns: getSupportGroups to extract support groups from identity, authorizeAccess to perform unified authorization, and validateTeam to verify Team validity and support-group status.
Changes:
- Refactor authorization flow to separate support group extraction, resource authorization, and Team validation into dedicated functions
- Add
validateTeamfunction to verify that Teams referenced in owned-by labels exist and are marked as support-groups - Update test coverage to use new functions and add testTeam fixture with support-group label
- Update REST mapper to handle Team resources in addition to Plugins
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/authz/authorization.go | Refactors authorization logic into getSupportGroups, authorizeAccess, and validateTeam functions; adds Team validation for existence and support-group status |
| cmd/authz/authorization_test.go | Updates test cases to use new function signatures and adds testTeam fixture with support-group label; updates REST mapper to include Team GVK |
</return_format>
Comments suppressed due to low confidence (1)
cmd/authz/authorization_test.go:90
- The PR adds Team validation to check both existence and support-group status, but there is no test coverage for scenarios where: (1) the Team referenced in the owned-by label doesn't exist, or (2) the Team exists but is not marked as a support-group. These are critical validation paths that should have explicit test coverage to ensure the authorization webhook correctly denies access in these cases.
var (
testTeam = test.NewTeam(test.Ctx, "demo", "my-org",
test.WithTeamLabel(greenhouseapis.LabelKeySupportGroup, "true"))
)
var _ = Describe("extractServiceAccountName", func() {
DescribeTable("extracting service account name from username",
func(username string, namespace string, expected string) {
result := extractServiceAccountName(username, namespace)
Expect(result).To(Equal(expected), "should correctly extract SA name from username")
},
Entry("valid SA returns name",
"system:serviceaccount:my-org:demo-sa", "my-org", "demo-sa"),
Entry("SA with any name returns the name",
"system:serviceaccount:my-org:demo", "my-org", "demo"),
Entry("SA in a different namespace returns empty string",
"system:serviceaccount:other-ns:demo-sa", "my-org", ""),
Entry("regular user (not a service account) returns empty string",
"demo-user", "my-org", ""),
Entry("SA with hyphenated name returns full name",
"system:serviceaccount:my-org:my-team-name-sa", "my-org", "my-team-name-sa"),
Entry("empty username returns empty string",
"", "my-org", ""),
)
})
var _ = Describe("handleAuthorize", func() {
Context("HTTP method validation", func() {
It("should reject non-POST methods", func() {
h := makeHandler(nil)
req := httptest.NewRequest(http.MethodGet, "/authorize", http.NoBody)
rec := httptest.NewRecorder()
h.ServeHTTP(rec, req)
Expect(rec.Code).To(Equal(http.StatusMethodNotAllowed), "GET requests should be rejected with 405 status")
})
})
Context("request validation", func() {
It("should deny requests with missing resource attributes", func() {
h := makeHandler(nil)
review := authv1.SubjectAccessReview{
Spec: authv1.SubjectAccessReviewSpec{
User: "demo-user",
Groups: []string{"support-group:demo"},
// ResourceAttributes intentionally nil
},
}
resp := postReview(h, review)
Expect(resp.Status.Allowed).To(BeFalse(), "requests without resource attributes should be denied")
Expect(resp.Status.Reason).To(ContainSubstring("missing resource attributes"), "denial reason should mention missing attributes")
})
})
Context("user authorization with support groups", func() {
It("should allow user with matching support-group", func() {
plugin := test.NewPlugin(test.Ctx, "plugin-demo", "my-org",
test.WithPluginLabel(greenhouseapis.LabelKeyOwnedBy, "demo"))
h := makeHandler(plugin, testTeam)
review := authv1.SubjectAccessReview{
Spec: authv1.SubjectAccessReviewSpec{
User: "demo-user",
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
On-behalf-of: @SAP krzysztof.zagorski@sap.com
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…stence On-behalf-of: @SAP krzysztof.zagorski@sap.com
Description
This PR adds a check in authorization webhook if the requested resource's owner Team exists and is a support group. Also it unifies the authorization flow for both User and ServiceAccount cases.
What type of PR is this? (check all applicable)
Related Tickets & Documents
Added tests?
Added to documentation?
Checklist