Throwing ForbiddenException if OidcUtils.findRoles throws an exception #5815

sberyozkin · 2019-11-27T18:07:39Z

This PR simply completes the future with ForbiddenException, instead of propagating OIDCException. Note OidcUtils.findRoles does not itself throw ForbiddenException because it is not guaranteed to be called during the authentication, but it logs a warning now.
OidcUtilsTest has a test confirming OidcUtils.findRoles throws an exception with the invalid path

sberyozkin · 2019-11-27T18:11:12Z

@gsmet Hi, if it is approved and looks good to you too then please consider it for 1.0.1.Final.
I need to go offline now so may not have time to react to the review comments today, but it is not a blocker issue, thanks

gsmet · 2019-11-27T18:17:50Z

extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcIdentityProvider.java

-                } catch (Exception e) {
-                    result.completeExceptionally(e);
+                } catch (OIDCException e) {
+                    result.completeExceptionally(new ForbiddenException());


What bugs me a bit is that we don't handle the other exceptions if they happen?

This probably needs to be discussed so will be a bit late for 1.0.1.

@gsmet Hi Guillaume, yes, sure. FYI, inside OidcUtils.findRoles there is only unchecked exception which can happen which is OIDCException when some paths can not be resolved (all that code does is it iterates over already prepared JSONObject). If something else escapes then I'd consider enhancing OidcUtils further.
However I can also add a RuntimeException catch to catch everything else ? Let me know what you think please, but yes, it can wait

I would add the OIDCException as the cause of the ForbuddenException to not loose the exception context

I'm OK with all other exceptions being not handled, they will all result to a ServerError.
We only want authentication exceptions being transformed to ForbiddenException.

Can we keep the previous branch too?

I mean having something like:

} catch (OIDCException e) { result.completeExceptionally(new ForbiddenException()); } catch (Exception e) { result.completeExceptionally(e); }

That would have my preference.

@gsmet sure

@gsmet done

loicmathieu · 2019-11-28T08:43:13Z

extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcIdentityProvider.java

-                } catch (Exception e) {
-                    result.completeExceptionally(e);
+                } catch (OIDCException e) {
+                    result.completeExceptionally(new ForbiddenException());


I would add the OIDCException as the cause of the ForbuddenException to not loose the exception context

loicmathieu · 2019-11-28T08:45:54Z

extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcIdentityProvider.java

-                } catch (Exception e) {
-                    result.completeExceptionally(e);
+                } catch (OIDCException e) {
+                    result.completeExceptionally(new ForbiddenException());


I'm OK with all other exceptions being not handled, they will all result to a ServerError.
We only want authentication exceptions being transformed to ForbiddenException.

gsmet · 2019-11-28T08:49:44Z

extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcUtils.java

            }
        } else if (step + 1 < pathArray.length) {
            if (claimValue instanceof JsonObject) {
                int nextStep = step + 1;
                return findClaimValue(claimPath, (JsonObject) claimValue, pathArray, nextStep, mustExist);
            } else {
-                throw new OIDCException("Claim value at the path " + claimPath + " is not a json object");
+                LOG.warn("Claim value at the path " + claimPath + " is not a json object");
+                throw new OIDCException();


Could you explain why you chose to do that this way? I.e. going from an exception message to a warn?

@gsmet Please see my response to Loic, at the moment it is not possible to wrap the OIDC exception. I'll make it the error message again once ForbiddenException/etc are updated, does it sound OK ?

I'll open an issue to track it so as I don't forget

I'm going to do a small PR to the quarkus-security and will follow up with the updates to this code but also to other Quarkus code where the causes are not included for those exceptions

sberyozkin · 2019-11-28T10:13:35Z

@loicmathieu

I would add the OIDCException as the cause of the ForbuddenException to not loose the exception context

Not possible at the moment, since ForbiddenException only has a default constructor and it is in a separate quarkus-security repositoryright now.

I'm OK with all other exceptions being not handled...

Restoring that block would only make sure the future is cleanly completed exceptionally.

loicmathieu · 2019-11-28T10:37:57Z

@sberyozkin I tought it was the JAX-RS ForbiddenException ...
As soon as you create an issue to update the code later I'm OK with it.

sberyozkin · 2019-11-28T10:42:21Z

@loicmathieu @gsmet FYI, #5826
I'll do a quick PR for the quarkus-security as well
Thanks

sberyozkin · 2019-11-29T10:41:29Z

@gsmet PR to quarkus-security has been submitted so if you prefer we can wait till it is released so that I update this PR accordingly. Or I can follow up with the #5626 fix (have assigned that issue to me), either way works for me, thanks

sberyozkin · 2019-12-05T11:02:19Z

@gsmet, @loicmathieu Hi, FYI, I've now updated this PR to have the cause exception included across the Quarkus code :-), after Stuart has released the latest quarkus-security

sberyozkin requested a review from loicmathieu November 27, 2019 18:08

sberyozkin added this to the 1.1.0 milestone Nov 27, 2019

sberyozkin added the backport? label Nov 27, 2019

gsmet reviewed Nov 27, 2019

View reviewed changes

loicmathieu requested changes Nov 28, 2019

View reviewed changes

gsmet reviewed Nov 28, 2019

View reviewed changes

loicmathieu approved these changes Nov 28, 2019

View reviewed changes

sberyozkin self-assigned this Nov 29, 2019

sberyozkin added the area/oidc label Nov 29, 2019

Throwing ForbiddenException if OidcUtils.findRoles throws an exception

617ff6c

gsmet approved these changes Dec 5, 2019

View reviewed changes

gsmet merged commit 32bb1bd into quarkusio:master Dec 5, 2019

gsmet removed the backport? label Dec 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throwing ForbiddenException if OidcUtils.findRoles throws an exception #5815

Throwing ForbiddenException if OidcUtils.findRoles throws an exception #5815

sberyozkin commented Nov 27, 2019 •

edited

Loading

sberyozkin commented Nov 27, 2019

gsmet Nov 27, 2019

sberyozkin Nov 28, 2019

loicmathieu Nov 28, 2019

loicmathieu Nov 28, 2019

gsmet Nov 28, 2019

sberyozkin Nov 28, 2019

sberyozkin Nov 28, 2019

loicmathieu Nov 28, 2019

loicmathieu Nov 28, 2019

gsmet Nov 28, 2019

sberyozkin Nov 28, 2019

sberyozkin Nov 28, 2019

sberyozkin Nov 28, 2019

stuartwdouglas Dec 5, 2019

sberyozkin commented Nov 28, 2019 •

edited

Loading

loicmathieu commented Nov 28, 2019

sberyozkin commented Nov 28, 2019

sberyozkin commented Nov 29, 2019

sberyozkin commented Dec 5, 2019

Throwing ForbiddenException if OidcUtils.findRoles throws an exception #5815

Throwing ForbiddenException if OidcUtils.findRoles throws an exception #5815

Conversation

sberyozkin commented Nov 27, 2019 • edited Loading

sberyozkin commented Nov 27, 2019

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

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 28, 2019 • edited Loading

loicmathieu commented Nov 28, 2019

sberyozkin commented Nov 28, 2019

sberyozkin commented Nov 29, 2019

sberyozkin commented Dec 5, 2019

sberyozkin commented Nov 27, 2019 •

edited

Loading

sberyozkin commented Nov 28, 2019 •

edited

Loading