[fixes #4495] - Quarkus Keycloak Authorization Extension #4776

pedroigor · 2019-10-22T22:10:29Z

Quickstart: quarkusio/quarkus-quickstarts#331

...rization/runtime/src/main/java/io/quarkus/keycloak/pep/KeycloakPolicyEnforcerAuthorizer.java

...uthorization/runtime/src/main/java/io/quarkus/keycloak/pep/KeycloakPolicyEnforcerConfig.java

sberyozkin · 2019-10-23T09:46:10Z

@pedroigor thanks, I'll be commenting as well during the next few days. One thing is that we indeed should try to reuse as much as possible from the lower levels like the core security and oidc, and if it is not possible, push down some changes. (re the comment from @stuartwdouglas about the paths for ex)

docs/src/main/asciidoc/keycloak-authorization-guide.adoc

extensions/keycloak-authorization/deployment/pom.xml

...rization/runtime/src/main/java/io/quarkus/keycloak/pep/KeycloakPolicyEnforcerAuthorizer.java

gsmet

I reviewed the doc and added some suggestions.

Question is: should this extension go in if it doesn't support native? Until now, it was a strict condition.

docs/src/main/asciidoc/keycloak-authorization-guide.adoc

docs/src/main/asciidoc/security-guide.adoc

extensions/keycloak-authorization/runtime/src/main/resources/META-INF/quarkus-extension.json

sberyozkin · 2019-10-23T10:07:59Z

...uthorization/runtime/src/main/java/io/quarkus/keycloak/pep/KeycloakPolicyEnforcerConfig.java

+         * By default, the adapter responds with a 403 HTTP status code
+         */
+        @ConfigItem
+        Optional<String> onDenyRedirectTo;


I believe this has to be managed by the quarkus-oidc - this is exactly what it will already do as part of your authorization code flow :-) or the existing bearer-only check. I mean it will do 401 if the token is invalid (signature for ex), but if the token has not enough permissions, it is equally invalid. And if we let the user be redirected to something local then we'll lose the flow control, what will happen after the user lands on some local page, as opposed to be redirected to IDP ?

This is not really related to code flow. This option, in particular, is just providing an option to customize requests denied by the server. The user is already authenticated and if not, this page is not meant to start any flow.

For instance, instead of returning a 403 response the application may want to show a more fancy page.

@pedroigor It looks like the error can only occur in the process of the user authenticating (for the bearer only flow it is only 403 anyway). So if it is SPA then it is not really an adapter level issue what happens to a user view - let the script running in the browser decide. And if it is a server web app code flow then the user has signed in at the IDP site, is being redirected now to the adapter, the token fails a check, and with custom page the user lands there seeing that the access is denied, But what is next ? The user stays authenticated and is advised to try to access some resource again ? Does not seem like the right idea.

The other thing, I doubt that in production people will want to display the details outside of its log :-)

Perhaps we should have an option for OIDC adapter itself return a page like "Your access to this resource has been denied, please contact the administrator, you will be redirected to IDP in 5 sec..." (and this page can be customized).

But whatever we do here, it feels like it is not a KC specific extension concern since 403 will be possible at the quarkus-oidc level as well :-)

That is the idea. This requirement came from the community a long time ago.

The user would just see this custom page with whatever the application wants to show. Either a simple and fancy "Access Denied" or something else.

If we want to have that on quarkus-oidc, that is fine and I can remove this.

Btw, given that KC runs on different containers, this option was also important to support this requirement across all of them.

@pedroigor yes please lets remove if for the moment and then figure out where to have this property - the same concept should work across the OIDC bits in Quarkus

sberyozkin · 2019-10-23T10:09:37Z

...uthorization/runtime/src/main/java/io/quarkus/keycloak/pep/KeycloakPolicyEnforcerConfig.java

+         * Specifies that the adapter uses the UMA protocol.
+         */
+        @ConfigItem
+        boolean userManagedAccess;


IMHO everything not related to the Keycloak specific policy-enforcer configuration should not be here at all; we've promised quarkus-oidc will work with not only KC :-), and UMA is supported not only by Keycloak

I see. So we would be providing a separated extension for UMA support if someone demands it ?

Sounds good for me. I can remove UMA support from this extension.

@pedroigor hey Pedro, exactly, lets have it done either directly as part of quarkus-oidc (it is nor really that fat so may be it should be there) or a new extension like quarkus-oidc-uma - so that we can say Quarkus can be UMA protected with any IDP which does it, which will be massive. Let me open an issue to track it, it would probably be a copy and paste for you :-), may be the only minor effort would be required to replace the KC adapter client code with some basic HTTP client....

Client-side, UMA is quite straight forward to implement. I agree that a separated extension is the best way.

I won't hold up this PR for this, but we should create an issue to track it.

@stuartwdouglas we get it in too fast and the users will see the extra properties in the wrong namespace and a new migration headache is on the way

...uthorization/runtime/src/main/java/io/quarkus/keycloak/pep/KeycloakPolicyEnforcerConfig.java

integration-tests/keycloak-authorization/src/main/resources/keycloak.json

pedroigor · 2019-10-23T12:58:30Z

I reviewed the doc and added some suggestions.

Question is: should this extension go in if it doesn't support native? Until now, it was a strict condition.

I can work on native and it should not take too much time. We had native work for the old Keycloak extension before so is just a matter of doing the same changes.

@sberyozkin suggested leaving native for now though. But I can deliver it too.

pedroigor · 2019-10-23T13:54:51Z

@gsmet Native support added.

sberyozkin · 2019-10-23T19:32:37Z

@pedroigor @gsmet yes, since Pedro was having some issues I suggested leaving it for now in case if would block the PR, the user who can't migrate may not be using the native mode...Nice to see the native is supported now after all :-)

pedroigor · 2019-10-23T21:54:20Z

@sberyozkin Yeah. I was missing something when facing that error. Sorry for the false alarm :)

stuartwdouglas · 2019-10-24T00:54:59Z

I have pushed an extra commit onto this that will run the native tests in CI. If this passes then I think we are good to go.

sberyozkin · 2019-10-24T07:43:56Z

@stuartwdouglas we don't have to rush. It is nearly ready but UMa does not belong at the KC level, similarly to the redirect page property - these are the extra bits which are not required for the KC policy-enforcer migration support. IMHO the consolidation around the quarkus-oidc is the key

sberyozkin

Hi Pedro, Stuart, @pedroigor @stuartwdouglas I'm only asking here not for some extra improvements, but only to limit what is currently exposed at the quarkus-keycloak-authorization configuration level, specifically UMA-related and redirect related configs.
My point really is we've gone for a major upgrade of the OIDC code, dropped the original keycloak extension, hence IMHO we should be trying to keep all what can be generic at the oidc level to avoid people migrating again when we will support oidc-uma, etc. If we get these properties going into 0.27.0 then it is a new concern. Hope you are all OK with it.

pedroigor · 2019-10-24T12:04:58Z

@sberyozkin Sure. Both options are not available anymore.

sberyozkin · 2019-10-24T13:34:50Z

@pedroigor thanks :-) and we'll get them reborn pretty soon for sure :-)

sberyozkin · 2019-10-24T13:39:30Z

...ion-tests/keycloak-authorization/src/main/java/io/quarkus/it/keycloak/ProtectedResource.java

+    @GET
+    @Produces(MediaType.APPLICATION_JSON)
+    public List<Permission> permissions() {
+        if (identity.checkPermissionBlocking(new AuthPermission("Permission Resource"))) {


@pedroigor I'll add a follow up issue for us to come up with Quarkus permission class so that the users can write a portable code with or without KC policy enforcer, not a prob for this PR
CC @stuartwdouglas

pedroigor force-pushed the issue-4495 branch 2 times, most recently from b334668 to da3b5b4 Compare October 22, 2019 22:33

pedroigor requested review from stuartwdouglas and sberyozkin October 22, 2019 22:36

stuartwdouglas requested changes Oct 23, 2019

View reviewed changes