Add DevUI permissions editor #37303

gbourant · 2023-11-24T16:59:18Z

I have created a DevUI permissions editor (policy editor is coming soon too)

The Quarkus internals gave me a headache so i decided to push this so i can get some feedback.

At the moment everything is working great but when i save/update/delete/add a permission the backend still sees the old config so the front end is not updated correctly.

How i can make AuthProcessor.java at line 94 current.getConfigSources() see the new config file when it's edited?

sberyozkin · 2023-11-24T19:59:04Z

@gbourant Interesting idea, and perhaps a start for adding more security related Dev UI support. If it were to be added, I'd suggest renaming Auth to Security and may be have a HttpSecurityPolicy editor sub option, as I can imagine other pieces of somewhat unrelated content can be provided under Security.

This specific feature also has to be very thoroughy tested in a real CORS setup to avoid unexpected modifications with 3rd party redirects

michalvavrik · 2023-11-24T20:46:50Z

As for review, I read it for couple of minutes and:

you put a lot of great work into it, it's clear this is result of a lot of work, thanks
I can only do review week after next one, this needs to be reviewed very much in detail and I have crazy next week
path patterns you shown in the screenshot of this PR description are invalid, it seems like you do not validate inputs?

I'll respect if @sberyozkin think this is a way to go and I'll review it, but personally I am convinced you are heading in a wrong direction.

I think this should be part of the http://localhost:8080/q/dev-ui/configuration-form-editor, you could find a way how to enhance adding of configuration properties so that it is pleasant and user-friendly.

Maybe other extensions could profit from your PR as well. That said, @phillip-kruger can tell if my suggestion is even possible.

I was wondering if by enhancing configuration-form-editor we could:

avoid hardcoding property names in AuthFieldType as UI already have a way how to display existing property names
avoid Java code at all, these runtime classes will be there even in production and I wonder how big would Quarkus be (how many classes) if every extension did this for their own configuration properties
what your PR does is not specific for security, it is specific for configuration, you can abstract it for all extensions; does it really brings value to create policies and permissions on dedicated page?
configuration editor already have a way how to show / persist configuration properties changes, so only difference to your PR is UI portion
there is a lot of code, I'm not looking forward to maintaining something that we can avoid in favor of joint Config editor page. I can imagine similar customization would be useful for creating datasources and so on.

michalvavrik · 2023-11-24T21:02:27Z

...ons/vertx-http/runtime/src/main/java/io/quarkus/devui/runtime/config/AuthJsonRPCService.java

+@ApplicationScoped
+public class AuthJsonRPCService {
+    private static final Logger LOG = Logger.getLogger(AuthJsonRPCService.class.getName());
+    private final List<String> METHODS = List.of("GET", "POST", "PUT", "DELETE", "PATCH", "OPTIONS", "HEAD", "CONNECT",


This is not full list of io.vertx.core.http.HttpMethods.

I wonder if we could make it enum in auth config so that config form editor provides values, but it would be really long list...

michalvavrik · 2023-11-24T21:03:39Z

...ons/vertx-http/runtime/src/main/java/io/quarkus/devui/runtime/config/AuthJsonRPCService.java

+    private final List<String> METHODS = List.of("GET", "POST", "PUT", "DELETE", "PATCH", "OPTIONS", "HEAD", "CONNECT",
+            "TRACE"); // do not change order
+    private final List<String> AUTH_MECHANISM = List.of(QUARKUS_NULL_VALUE, "basic", "bearer", "form"); // do not change order
+    private final List<String> POLICIES = List.of("permit", "deny", "authenticated"); // do not change order


Note // do not change order without explanation is not as useful as if you added bit more. btw fact that it is order sensitive seems strange.

I concur; I'll enhance the comment for clarity. It's worth noting that the sequence is non-critical and remains functional in any order. The arrangement mirrors the frontend presentation, specifically within the context of Vaadin components such as vaadin-list-box and vaadin-select.

michalvavrik · 2023-11-24T21:06:27Z

...ons/vertx-http/runtime/src/main/java/io/quarkus/devui/runtime/config/AuthJsonRPCService.java

+    private static final Logger LOG = Logger.getLogger(AuthJsonRPCService.class.getName());
+    private final List<String> METHODS = List.of("GET", "POST", "PUT", "DELETE", "PATCH", "OPTIONS", "HEAD", "CONNECT",
+            "TRACE"); // do not change order
+    private final List<String> AUTH_MECHANISM = List.of(QUARKUS_NULL_VALUE, "basic", "bearer", "form"); // do not change order


there is much more of auth mechanisms and having hardcoded string list of 4 is too little, I wonder if it would be possible to collect these names from io.quarkus.smallrye.jwt.runtime.auth.JWTAuthMechanism#getCredentialTransport => io.quarkus.vertx.http.runtime.security.HttpCredentialTransport#getAuthenticationScheme that of all Instance<HttpAuthenticationMechanism> httpAuthenticationMechanism

sberyozkin · 2023-11-24T22:16:30Z

@michalvavrik It can work in the configuration properties sure - but my only reservation will be a config centric view. IMHO having a security centric view is interesting, but I don't know right now what else can be added there. I guess if can imagine a few more security related views than Security can work well, otherwise may be it is indeed just about making it easy to configure these properties in the config editor...

michalvavrik · 2023-11-24T23:15:46Z

@michalvavrik It can work in the configuration properties sure - but my only reservation will be a config centric view. IMHO having a security centric view is interesting, but I don't know right now what else can be added there. I guess if can imagine a few more security related views than Security can work well, otherwise may be it is indeed just about making it easy to configure these properties in the config editor...

But I can't see these security related views. Sure, I didn't do proper review yet. But it seems to me like this is config editor customized for security, in which case I'd prefer option for other extensions (I've mentioned creating of datasources) that will allow to auto-suggest values.

If there are security views that should be implemented, then it is something that needs to be discussed and defined in Quarkus issue so that @gbourant put a lot of his valuable time in effectively.

michalvavrik · 2023-11-24T23:19:15Z

Anyway, let's hear from @phillip-kruger what is really possible, I'll read it comment again in 2 weeks. Thanks

phillip-kruger

I probably also lean towards just improving the config screen, but I can also see that making security setting easy does make sense from a user point of view, even if it's just changing config. I would suggest if we do want this separate from config that this work moves to the appropriate security extension and probably not copy from config, but design the screen from a security p.o.w and then we can see how the backend can update the config.

phillip-kruger · 2023-11-27T06:26:53Z

...ions/vertx-http/deployment/src/main/java/io/quarkus/devui/deployment/menu/AuthProcessor.java

@@ -0,0 +1,274 @@
+package io.quarkus.devui.deployment.menu;


This should probably live in the security extension and not in vertx. Config lives in vertx as it's core to quarkus (there is not extension, everybody has it) but not everyone will add security.

This must live in vertx-http extension because that's where HTTP permissions are.

However it can be in different package if you deem it more suitable.

ok, then it's fine. So are you saying that without adding any security extensions, I can secure a HTTP Endpoint ?

ok, then it's fine. So are you saying that without adding any security extensions, I can secure a HTTP Endpoint ?

no, I don't say that :-D I realize this is not straight forward, but there are some parts of Vert.x HTTP extension only added (registered as CDI beans etc.) when Security extension is present. However HTTP Security policy configuration belongs to Vert.x HTTP extension because it is HTTP specific and there is config root.

We don't want the menu item there if there will be no effect if you configure it, that is why I thought it should live in security somewhere.

Understood. I don't have answer now, let's wait for @gbourant to answer. I think it is still a lot of code that could be avoided if form config editor was enhanced instead. There are things we can make to autosuggest configuration there better:

HTTP method can be enum, though there is downside of really extensive list

permission policy is half-closed world, most of the time they will be either builtin policies (deny, authenticated, permit) or role-based policy that can be suggested based on existing configuration properties (quarkus.http.auth.policy."my-name".* tells you you can suggest my-name).

phillip-kruger · 2023-11-27T06:27:19Z

...ions/vertx-http/deployment/src/main/java/io/quarkus/devui/deployment/menu/AuthProcessor.java

+ */
+public class AuthProcessor {
+
+    private static final String NAME_SPACE = "devui-auth";


Namespace would not be needed if this moves to an extension

phillip-kruger · 2023-11-27T06:28:04Z

...ions/vertx-http/deployment/src/main/java/io/quarkus/devui/deployment/menu/AuthProcessor.java

+    @BuildStep(onlyIf = IsDevelopment.class)
+    InternalPageBuildItem createAuthPages() {
+
+        InternalPageBuildItem configurationPages = new InternalPageBuildItem("Auth", 21);


You will not be using InternalPageBuildItem when this moves to the extension

michalvavrik · 2023-12-04T07:38:34Z

@gbourant sorry not to hear back from you, you did a lot of work here, I just think these kind of PRs are better to discuss in GH issue, so that there is some compromise between our views.

At the moment everything is working great but when i save/update/delete/add a permission the backend still sees the old config so the front end is not updated correctly.

Couldn't this be avoided if you relied on existing functionality that is already implemented by config form editor? I hope you could avoid methods like cleanUpAsciiDocIfNecessary, formatJavadoc and many others because working persisting config yourself has disadvantage that whoever enhance / fix config form editor might need to remember to also update this new code.

michalvavrik · 2023-12-04T07:46:20Z

...ions/vertx-http/deployment/src/main/java/io/quarkus/devui/deployment/menu/AuthProcessor.java

+        AuthProcessor.configDescriptionBuildItems = configDescriptionBuildItems;
+        AuthProcessor.devServicesLauncherConfig = devServicesLauncherConfig;
+        AuthProcessor.recorder = recorder;
+        AuthProcessor.current = (SmallRyeConfig) ConfigProvider.getConfig();


Maybe this hack could work as we are talking DEV mode, but if it works it's IMO bug.
You can only work with runtime configuration when io.quarkus.deployment.builditem.RuntimeConfigSetupCompleteBuildItem is produced and you should do this inside recorder because not all config sources will be available during build time. I remember there is missing feature in SR Config that classic config sources in META-INF are also loaded at build time, but Quarkus also provides runtime-only config sources defined by io.quarkus.deployment.builditem.RunTimeConfigBuilderBuildItem.

Correction, I have seen using ConfigProvider.getConfig() since then many times and used it as well in the deployment module. I think there are occasional related issues (Yoann mentioned some), let's keep it. Please add the @Consume(RuntimeConfigSetupCompleteBuildItem.class) that I have mentioned. Thanks

michalvavrik · 2023-12-04T07:51:14Z

extensions/vertx-http/runtime/src/main/java/io/quarkus/devui/runtime/config/AuthFieldType.java

+    PERMISSION_PATHS(AuthConfigType.PERMISSION, "paths"),
+    PERMISSION_AUTH_MECHANISM(AuthConfigType.PERMISSION, "auth-mechanism"),
+    POLICY_ROLES_ALLOWED(AuthConfigType.POLICY, "roles-allowed"),
+    POLICY_PERMISSIONS(AuthConfigType.POLICY, "permissions"),


There is new property roles on the role-based permissions that is missing here.

This sort of consistency is one of reasons why I hoped only config form editor was enhanced instead of new AuthProcessor. We don't have tests for UI as it's difficult and they are generally slower (we Quarkus QE have some and it's generally slow to download and start playwright comparing to QuarkusTest without container) and there is no way to ensure someone will not forget to update AuthFieldType.

gbourant · 2023-12-04T12:07:07Z

I've thoroughly reviewed the entire discussion, but I haven't had the opportunity to respond yet due to work commitments.

As of my understanding there are two valid but diverge opinions. The first one is that we should update the configuration editor and the second one is to create a dedicated view (this PR).

By updating the current configuration editor, we mitigate the maintenance effort, though complete elimination is not achieved. Additionally, adopting the updating the configuration editor might not evoke the same level of "Developer Joy" as the second option.

So which option are we adopting?

michalvavrik · 2023-12-04T12:08:45Z

I've thoroughly reviewed the entire discussion, but I haven't had the opportunity to respond yet due to work commitments.

As of my understanding there are two valid but diverge opinions. The first one is that we should update the configuration editor and the second one is to create a dedicated view (this PR).

By updating the current configuration editor, we mitigate the maintenance effort, though complete elimination is not achieved. Additionally, adopting the updating the configuration editor might not evoke the same level of "Developer Joy" as the second option.

So which option are we adopting?

that is absolutely precise summary, since I already provided my view on this, I'll let @sberyozkin to have his say

gsmet · 2024-06-07T15:55:12Z

This looks interesting and it's a bit of a shame we let it rot.

My personal opinion is that permissions are a complex enough beast that having a dedicated UI could make sense.

@phillip-kruger it would be great if you could give some clear guidelines to the OP if he's willing to pursue this.

gbourant · 2024-10-21T08:22:47Z

I'm still interested in this pull request, if this is something that we want i will start working on this pull request, otherwise i will build it outside of the Quarkus repo.

michalvavrik · 2024-10-21T09:26:06Z

...ions/vertx-http/deployment/src/main/java/io/quarkus/devui/deployment/menu/AuthProcessor.java

+        AuthProcessor.configDescriptionBuildItems = configDescriptionBuildItems;
+        AuthProcessor.devServicesLauncherConfig = devServicesLauncherConfig;
+        AuthProcessor.recorder = recorder;
+        AuthProcessor.current = (SmallRyeConfig) ConfigProvider.getConfig();


you don't know whether config is ready, you need something like @Consume(RuntimeConfigSetupCompleteBuildItem.class).

phillip-kruger · 2024-10-21T09:37:17Z

@phillip-kruger it would be great if you could give some clear guidelines to the OP if he's willing to pursue this.

OP ?

michalvavrik · 2024-10-21T09:56:30Z

...ions/vertx-http/deployment/src/main/java/io/quarkus/devui/deployment/menu/AuthProcessor.java

+        DevConsoleManager.register("update-permission-set", map -> {
+            updateConfig(map);
+            register(AuthProcessor.configDescriptionBuildItems, AuthProcessor.devServicesLauncherConfig,
+                    AuthProcessor.recorder);


you have recorder as a build step parameter and but you pass down the one stored on processor; these build steps can run in parallel. there can be race, please don't store the recorder on the processor

michalvavrik · 2024-10-21T10:00:30Z

I'm still interested in this pull request, if this is something that we want i will start working on this pull request, otherwise i will build it outside of the Quarkus repo.

I don't have anything to add to my previous comments. I'd wait till users tell us setting these properties in config editor is hard for them, we have to maintain this (and possibly fix if ever problem occur), because now that I re-read your PR it's not trivial. However IIUC @gsmet and @sberyozkin (and possibly @gastaldi based on his thumb up) likes this feature, for that reason we should proceed.

Regarding current state of your PR:

please make sure you cover changes on HTTP Permissions like shared permissions
everything is hardcoded:
- UI suggests HTTP auth mechanisms that are not available (like if you don't have OIDC extension, you cannot suggest bearer) and doesn't provide others that are available
you manipulate application.properties manually even though Quarkus already have mechanism to manipulate with it, because there is DEV UI config property editor. Reusing its mechanism could makes thing more reliable

Most importantly @gbourant whatever I say are just my personal opinions, do not get discouraged by me, they are probably not shared by others. Also, if you want to improve security DEV UI, please talk to @sberyozkin , I am sure he can suggest other issues as well. Thank you

michalvavrik · 2024-10-21T10:17:39Z

@gsmet

This looks interesting and it's a bit of a shame we let it rot.

I am not aware we did, we comment on specific things in code and our comments were not addressed. Also I spend decent time putting answering any questions and reasons why we should/should not do this. So we were active.

My personal opinion is that permissions are a complex enough beast that having a dedicated UI could make sense.

That could be true if this PR make things easier to understand, if it validated inputs, we it checked that configured properties are securing paths or whatsoever. My understanding of this PR is that it allows you to set path, method, mechanism, policy in UI instead of write it down. However complexity you talk about does not come from setting configuration properties, but from:

if you set this property, what is authorized and to which extent (like authenticated, roles, permissions), check with actual call and correct authorization that what you expect is secured
which HTTP permission is applied for concrete path (aka winning permission path match)
how do I secure path if other permissions are already in place (how do I write path pattern)

None of these things are addressed by this PR (and frankly cannot be), you need to read https://quarkus.io/guides/security-authorize-web-endpoints-reference (maybe someone would suggest to give AI that text and ask it :-( ).

Again, I never tried to hinder this PR, I tried to help with review.

michalvavrik · 2024-10-28T17:18:54Z

Hope this is won't confuse things, but today I stumble on #43229 and linked PR (which provides a lot of information in comments) and I think it shows sort of issue we could run into if this PR will manually edit application properties and try to synchronize it. I am happy to be proved wrong. My point is, it can get complex or we will need to say "this supports scenarios XYZ" and limit what is supported. Which is fine as well.

cescoffier · 2025-01-24T18:59:47Z

@sberyozkin @gbourant @michalvavrik What's the status on this?

michalvavrik · 2025-01-24T19:23:13Z

@sberyozkin @gbourant @michalvavrik What's the status on this?

In short: I wrote down my arguments against this many times, see above. There is support for this from several people and I respect that. If this is going in, let's make sure it is easy to maintain. There are comments from me and Phillip that were not addressed, I could provide more if this was active PR.

add dev ui permissions editor

1cb8629

quarkus-bot bot added the area/vertx label Nov 24, 2023

sberyozkin requested review from phillip-kruger and michalvavrik November 24, 2023 20:00

michalvavrik reviewed Nov 24, 2023

View reviewed changes

phillip-kruger reviewed Nov 27, 2023

View reviewed changes

michalvavrik reviewed Dec 4, 2023

View reviewed changes

michalvavrik reviewed Oct 21, 2024

View reviewed changes

Add DevUI permissions editor #37303

Are you sure you want to change the base?

Add DevUI permissions editor #37303

Conversation

gbourant commented Nov 24, 2023

sberyozkin commented Nov 24, 2023

michalvavrik commented Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sberyozkin commented Nov 24, 2023

michalvavrik commented Nov 24, 2023 • edited Loading

michalvavrik commented Nov 24, 2023

phillip-kruger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalvavrik commented Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

michalvavrik Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbourant commented Dec 4, 2023

michalvavrik commented Dec 4, 2023

gsmet commented Jun 7, 2024

gbourant commented Oct 21, 2024

michalvavrik Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

phillip-kruger commented Oct 21, 2024

Choose a reason for hiding this comment

michalvavrik commented Oct 21, 2024

michalvavrik commented Oct 21, 2024 • edited Loading

michalvavrik commented Oct 28, 2024

cescoffier commented Jan 24, 2025

michalvavrik commented Jan 24, 2025

michalvavrik commented Nov 24, 2023 •

edited

Loading

michalvavrik commented Nov 24, 2023 •

edited

Loading

michalvavrik commented Dec 4, 2023 •

edited

Loading

michalvavrik Jan 24, 2025 •

edited

Loading

michalvavrik Oct 21, 2024 •

edited

Loading

michalvavrik commented Oct 21, 2024 •

edited

Loading