proposal: add banneduser middleware #455

filariow · 2024-08-28T17:50:21Z

proposal for the BannedUser middleware as discussed in #443's review

linked to codeready-toolchain/host-operator#1079 and codeready-toolchain/toolchain-e2e#1045

Signed-off-by: Francesco Ilario <[email protected]>

filariow · 2024-08-28T20:11:19Z

/retest

alexeykazakov

Looks good overall. I left a few comments though.

pkg/informers/informers.go

pkg/informers/service/informer_service.go

pkg/proxy/proxy.go

Co-authored-by: Alexey Kazakov <[email protected]>

Signed-off-by: Francesco Ilario <[email protected]>

Co-authored-by: Alexey Kazakov <[email protected]>

Signed-off-by: Francesco Ilario <[email protected]>

filariow · 2024-08-29T11:55:50Z

pkg/proxy/proxy_test.go

@@ -1168,7 +1245,7 @@ func (s *TestProxySuite) token(userID uuid.UUID, extraClaims ...authsupport.Extr
 		Username: "username-" + userID.String(),
 	}

-	extra := append(extraClaims, authsupport.WithEmailClaim("[email protected]"))
+	extra := append([]authsupport.ExtraClaim{authsupport.WithEmailClaim("[email protected]")}, extraClaims...)


for context: this change allows us to override the default email set by this auxiliary function. As a result if you don't define an email, you'll have [email protected]. Otherwise you'll have the one you are explicitly setting

Worth moving this comment to the code.

done in d81542b

alexeykazakov

Runtime code looks good! Thank you for addressing my comments. I'm going to review the test code next.

alexeykazakov

Looks good! Just one question.
And how are we doing in terms of e2e-tests? We already have some tests which check banned users in proxy, right? Let's make sure that we add some e2e-tests for banned users for the public-viewer proxy PR.

pkg/proxy/proxy_test.go

Signed-off-by: Francesco Ilario <[email protected]>

filariow · 2024-09-05T18:08:32Z

And how are we doing in terms of e2e-tests? We already have some tests which check banned users in proxy, right?

It seems to me the only test we have is https://github.com/codeready-toolchain/toolchain-e2e/blob/1753fd31f70edc8b1645461a70580488157cf45d/test/e2e/parallel/proxy_test.go#L1036 and I don't know why but it fails with BannedUser middleware: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/codeready-toolchain_registration-service/455/pull-ci-codeready-toolchain-registration-service-master-e2e/1831624866889994240. Do you have any idea?

I'm working at some more tests here: codeready-toolchain/toolchain-e2e@master...filariow:toolchain-e2e:banneduser-middleware

filariow · 2024-09-05T19:39:19Z

/retest

filariow · 2024-09-09T11:56:17Z

Looks good! Just one question. And how are we doing in terms of e2e-tests? We already have some tests which check banned users in proxy, right? Let's make sure that we add some e2e-tests for banned users for the public-viewer proxy PR.

I opened a PR for improving banned users test cases: codeready-toolchain/toolchain-e2e#1045

filariow · 2024-09-09T16:00:51Z

/retest

filariow · 2024-09-09T17:40:43Z

/retest

deploy/registration-service.yaml

mfrancisc

Great Job 🚀

openshift-ci · 2024-09-10T11:22:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, filariow, mfrancisc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alexeykazakov,mfrancisc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ware

codecov · 2024-09-11T13:17:34Z

Codecov Report

Attention: Patch coverage is 79.06977% with 9 lines in your changes missing coverage. Please review.

Project coverage is 78.69%. Comparing base (c7890b8) to head (b465d18).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/informers/service/informer_service.go	63.15%	4 Missing and 3 partials ⚠️
pkg/proxy/proxy.go	91.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
- Coverage   78.75%   78.69%   -0.06%     
==========================================
  Files          42       42              
  Lines        2739     2774      +35     
==========================================
+ Hits         2157     2183      +26     
- Misses        489      494       +5     
- Partials       93       97       +4

Flag	Coverage Δ
unittests	`78.69% <79.06%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rsoaresd

Great job 🚀

Just a minor question:

rsoaresd · 2024-09-11T14:55:17Z

pkg/informers/service/informer_service.go

+
+	bb := []toolchainv1alpha1.BannedUser{}
+	for _, obj := range objs {
+		unobj := obj.(*unstructured.Unstructured)


Should we check here the type assertion?

unobj, ok := obj.(*unstructured.Unstructured) if !ok { .... }

I'm not sure this can happen. If I'm not mistaken it's stored internally as *unstructured.Unstructured (cf. https://github.com/kubernetes/client-go/blob/2666bd29867c96168c5e9429fd74fd9bfbedac8b/dynamic/dynamicinformer/informer.go#L138). Also I can not immagine how to write a test for covering this check.

The other functions in this file are using an unchecked casting like this too. I'd say that -if we find out it to be a problem- we can create a PR for fixing this in all the functions. WDYT?

Understood! Thank you :)

sonarqubecloud · 2024-09-12T09:00:27Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
11.7% Duplication on New Code

See analysis details on SonarCloud

add banneduser middleware

017f50c

Signed-off-by: Francesco Ilario <[email protected]>

filariow requested review from MatousJobanek, xcoulon, alexeykazakov, rajivnathan, ranakan19, sbryzak and mfrancisc as code owners August 28, 2024 17:50

This was referenced Aug 28, 2024

proposal: add banneduser middleware codeready-toolchain/host-operator#1079

Closed

feat: add Public-Viewer support #443

Merged

fix unit tests

49effa5

Signed-off-by: Francesco Ilario <[email protected]>

alexeykazakov reviewed Aug 28, 2024

View reviewed changes

filariow and others added 13 commits August 29, 2024 10:12

Update pkg/informers/informers.go

50348ed

Co-authored-by: Alexey Kazakov <[email protected]>

rename InformerService method to ListBannedUsersByEmail

726b42e

Signed-off-by: Francesco Ilario <[email protected]>

use unsecured in ensureUserIsNotBanned Middleware

73039cc

Signed-off-by: Francesco Ilario <[email protected]>

Update pkg/proxy/proxy.go

1ed6846

Co-authored-by: Alexey Kazakov <[email protected]>

return error from Middleware

d7393c5

Signed-off-by: Francesco Ilario <[email protected]>

hide ban details

6c12ec8

Signed-off-by: Francesco Ilario <[email protected]>

improve ensureUserIsNotBanned comment

aa1697e

Signed-off-by: Francesco Ilario <[email protected]>

logging error on ListBannedUsersByEmail invocation

2061e88

Signed-off-by: Francesco Ilario <[email protected]>

add InformerService unit tests

e262500

Signed-off-by: Francesco Ilario <[email protected]>

fix typo

db4ae5e

Signed-off-by: Francesco Ilario <[email protected]>

fix linter complaints

c069031

Signed-off-by: Francesco Ilario <[email protected]>

add integration tests

b822ccf

Signed-off-by: Francesco Ilario <[email protected]>

fix linter complaints

62726bb

Signed-off-by: Francesco Ilario <[email protected]>

filariow commented Aug 29, 2024

View reviewed changes

alexeykazakov reviewed Aug 29, 2024

View reviewed changes

Merge branch 'master' into banneduser-middleware

ce2dc49

alexeykazakov approved these changes Sep 4, 2024

View reviewed changes

pkg/proxy/proxy_test.go Show resolved Hide resolved

openshift-ci bot added the approved label Sep 4, 2024

filariow added 2 commits September 5, 2024 11:25

add comment

d81542b

Signed-off-by: Francesco Ilario <[email protected]>

Merge branch 'master' into banneduser-middleware

6609036

This was referenced Sep 9, 2024

add e2e tests for banneduser codeready-toolchain/toolchain-e2e#1045

Merged

add banneduser middleware codeready-toolchain/host-operator#1083

Closed

mfrancisc reviewed Sep 9, 2024

View reviewed changes

deploy/registration-service.yaml Outdated Show resolved Hide resolved

Merge branch 'master' into banneduser-middleware

95ce224

mfrancisc approved these changes Sep 10, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase label Sep 11, 2024

Merge remote-tracking branch 'upstream/master' into banneduser-middle…

8588445

…ware

openshift-merge-robot removed the needs-rebase label Sep 11, 2024

rsoaresd reviewed Sep 11, 2024

View reviewed changes

filariow requested a review from rsoaresd September 11, 2024 18:10

Merge branch 'master' into banneduser-middleware

b465d18

filariow merged commit 92998a6 into codeready-toolchain:master Sep 12, 2024
10 of 11 checks passed

filariow deleted the banneduser-middleware branch September 12, 2024 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proposal: add banneduser middleware #455

proposal: add banneduser middleware #455

filariow commented Aug 28, 2024 •

edited

Loading

filariow commented Aug 28, 2024

alexeykazakov left a comment

filariow Aug 29, 2024

alexeykazakov Sep 4, 2024

filariow Sep 5, 2024

alexeykazakov left a comment

alexeykazakov left a comment

filariow commented Sep 5, 2024 •

edited

Loading

filariow commented Sep 5, 2024

filariow commented Sep 9, 2024 •

edited

Loading

filariow commented Sep 9, 2024

filariow commented Sep 9, 2024

mfrancisc left a comment

openshift-ci bot commented Sep 10, 2024

codecov bot commented Sep 11, 2024 •

edited

Loading

rsoaresd left a comment

rsoaresd Sep 11, 2024 •

edited

Loading

filariow Sep 11, 2024

rsoaresd Sep 13, 2024

sonarqubecloud bot commented Sep 12, 2024

proposal: add banneduser middleware #455

proposal: add banneduser middleware #455

Conversation

filariow commented Aug 28, 2024 • edited Loading

filariow commented Aug 28, 2024

alexeykazakov left a comment

Choose a reason for hiding this comment

filariow Aug 29, 2024

Choose a reason for hiding this comment

alexeykazakov Sep 4, 2024

Choose a reason for hiding this comment

filariow Sep 5, 2024

Choose a reason for hiding this comment

alexeykazakov left a comment

Choose a reason for hiding this comment

alexeykazakov left a comment

Choose a reason for hiding this comment

filariow commented Sep 5, 2024 • edited Loading

filariow commented Sep 5, 2024

filariow commented Sep 9, 2024 • edited Loading

filariow commented Sep 9, 2024

filariow commented Sep 9, 2024

mfrancisc left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 10, 2024

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

rsoaresd left a comment

Choose a reason for hiding this comment

rsoaresd Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

filariow Sep 11, 2024

Choose a reason for hiding this comment

rsoaresd Sep 13, 2024

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 12, 2024

Quality Gate passed

filariow commented Aug 28, 2024 •

edited

Loading

filariow commented Sep 5, 2024 •

edited

Loading

filariow commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

rsoaresd Sep 11, 2024 •

edited

Loading