Alon Bar-Lev has posted comments on this change. Change subject: aaa: Add Bearer and Negotiate auth filters ......................................................................
Patch Set 4: (3 comments) https://gerrit.ovirt.org/#/c/42292/4/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/SSOOAuthServiceUtils.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/SSOOAuthServiceUtils.java: Line 150: return String.format("https://%s:%s%s%s";, Line 151: EngineLocalConfig.getInstance().getProperty("ENGINE_FQDN"), Line 152: httpsPort, Line 153: EngineLocalConfig.getInstance().getProperty("SSO_URL"), Line 154: uri); why not build this in the config by setup? less logic at runtime the better. also easier to configure. Line 155: } Line 156: Line 157: private static Map getData(HttpURLConnection connection) throws Exception { Line 158: try (ByteArrayOutputStream os = new ByteArrayOutputStream()) { https://gerrit.ovirt.org/#/c/42292/4/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSORestApiAuthFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSORestApiAuthFilter.java: Line 51: token = (String) response.get("access_token"); Line 52: break; Line 53: case "Bearer": Line 54: token = headerValue.substring("Bearer".length()); Line 55: break; not sure I understand... if you have explicit usage of Basic/Bearer, why not check both here and drop the parameter of filter? also, the "authHeaderName" is quite confusing... it actually the type/method not header. but again, I do think you can parse the authorization headers, extract Bearer and Basic, and handle this dropping the semi abstraction. Line 56: default: Line 57: throw new RuntimeException(String.format("Unsupported authentication header: %s", authHeaderName)); Line 58: } Line 59: https://gerrit.ovirt.org/#/c/42292/4/backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSORestApiNegotiationFilter.java File backend/manager/modules/aaa/src/main/java/org/ovirt/engine/core/aaa/filters/SSORestApiNegotiationFilter.java: Line 69: HttpServletRequest req = (HttpServletRequest) request; Line 70: if (!FiltersHelper.isAuthenticated(req) || !FiltersHelper.isSessionValid((HttpServletRequest) request, (HttpServletResponse) response)) { Line 71: authenticateWithSSO(req, (HttpServletResponse) response); Line 72: } Line 73: chain.doFilter(request, response); again, this cannot be, there cannot be unconditional chaining... the nego should be able to stop the chaining, please see current implementation of nego. Line 74: } Line 75: Line 76: private synchronized void cacheNegotiatingProfiles() { Line 77: schemes = new ArrayList<>(); -- To view, visit https://gerrit.ovirt.org/42292 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idee5137430cefa7ca99c047cfd2d550222e5809a Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
