BE: RBAC: Subject type/value is unintended to be optional #719
Conversation
kapybro
bot
added
status/triage
|
@Haarolean I checked locally and with these changes RBAC with AD works as expected for both types ( Parameter value If need I can add integration tests with Active Directory. |
# Conflicts: # api/src/test/java/io/kafbat/ui/AbstractIntegrationTest.java
minor test refactoring
|
@Haarolean I've added integration tests with Active Directory. |
| @@ -63,24 +62,39 @@ public ReactiveAuthenticationManager authenticationManager(LdapContextSource lda | |||
| ba.setUserSearch(userSearch); | |||
| } | |||
|
|
|||
| AuthenticationManager manager = new ProviderManager(List.of( | |||
There was a problem hiding this comment.
Let's not mix the distinct issues in one PR, please. #54 is to be solved within #717 (which was planned as more of a refactoring PR first, but, welp, it's another story).
#716 has nothing to do with LDAP in particular, we'd need RBAC subject validation. In io.kafbat.ui.model.rbac.Permission and Role classes there's a validate method, perhaps implementing one for Subject as well is the way here.
| @ActiveProfiles("rbac-ad") | ||
| @AutoConfigureWebTestClient(timeout = "60000") | ||
| @ContextConfiguration(initializers = {ActiveDirectoryIntegrationTest.Initializer.class}) | ||
| public class ActiveDirectoryIntegrationTest { |
There was a problem hiding this comment.
This one is really cool tho, can you raise a separate PR with these tests so we can merge this?
Btw, we don't quite need kafka container here, the app can perfectly start with no defined clusters.
There was a problem hiding this comment.
Btw, we don't quite need kafka container here, the app can perfectly start with no defined clusters.
Yes, but there's a test that checks the creation of a topic on a cluster.
There was a problem hiding this comment.
Yes, but there's a test that checks the creation of a topic on a cluster.
this sounds redundant. Authenticating successfully should suffice
There was a problem hiding this comment.
can you raise a separate PR with these tests so we can merge this?
Opened the PR with the tests #726
What changes did you make? (Give an overview)
Is there anything you'd like reviewers to focus on?
How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)
Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)
Check out Contributing and Code of Conduct
A picture of a cute animal (not mandatory but encouraged)