)]}'
{"mistral/api/access_control.py":[{"author":{"_account_id":18238,"name":"Hardik Parekh","email":"hardik.parekh047@gmail.com","username":"hparekh"},"change_message_id":"971a8950ca5251c55cbacdee03647334ff651ca9","unresolved":false,"context_lines":[{"line_number":62,"context_line":"    :return: returns True if authorized and False if not authorized and"},{"line_number":63,"context_line":"             do_raise is False."},{"line_number":64,"context_line":"    \"\"\""},{"line_number":65,"context_line":"    target_obj \u003d {"},{"line_number":66,"context_line":"        \u0027project_id\u0027: context.project_id,"},{"line_number":67,"context_line":"        \u0027user_id\u0027: context.user_id,"},{"line_number":68,"context_line":"    }"}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_9cda5ae8","line":65,"range":{"start_line":65,"start_character":4,"end_line":65,"end_character":14},"updated":"2016-07-08 10:21:07.000000000","message":"Here We should also check if keystone is used as authentication mechanism or not. As enforce is called at api layer\nhttps://github.com/openstack/mistral/blob/master/mistral/api/controllers/v2/action.py#L122\nWe can create decorator or simple if condition. Decorator may be useful if any further authentication mechanism is used.","commit_id":"06068de08986e8ec5f10d70017cf6e38a7194872"},{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"b12349a18b71380d6135aaa813027f30642d55dd","unresolved":false,"context_lines":[{"line_number":62,"context_line":"    :return: returns True if authorized and False if not authorized and"},{"line_number":63,"context_line":"             do_raise is False."},{"line_number":64,"context_line":"    \"\"\""},{"line_number":65,"context_line":"    target_obj \u003d {"},{"line_number":66,"context_line":"        \u0027project_id\u0027: context.project_id,"},{"line_number":67,"context_line":"        \u0027user_id\u0027: context.user_id,"},{"line_number":68,"context_line":"    }"}],"source_content_type":"text/x-python","patch_set":13,"id":"1aa78d24_7ff4c086","line":65,"range":{"start_line":65,"start_character":4,"end_line":65,"end_character":14},"in_reply_to":"1aa78d24_9cda5ae8","updated":"2016-07-08 10:28:09.000000000","message":"Good catch! Thanks.","commit_id":"06068de08986e8ec5f10d70017cf6e38a7194872"}],"mistral/context.py":[{"author":{"_account_id":19206,"name":"Dawid Deja","email":"dawdej@gmail.com","username":"ddeja"},"change_message_id":"8601a73963d21f11621e02a0f84757bfdc702580","unresolved":false,"context_lines":[{"line_number":223,"context_line":"        if CONF.auth_type \u003d\u003d \u0027keystone\u0027:"},{"line_number":224,"context_line":"            error_msg \u003d authenticate_with_keystone(state.request)"},{"line_number":225,"context_line":""},{"line_number":226,"context_line":"        if CONF.auth_type \u003d\u003d \u0027keycloak-oidc\u0027:"},{"line_number":227,"context_line":"            error_msg \u003d authenticate_with_keycloak(state.request)"},{"line_number":228,"context_line":""},{"line_number":229,"context_line":"        if error_msg:"}],"source_content_type":"text/x-python","patch_set":5,"id":"1aa78d24_70369804","line":226,"range":{"start_line":226,"start_character":8,"end_line":226,"end_character":10},"updated":"2016-07-04 15:29:25.000000000","message":"Soft: elif","commit_id":"1c373836c0044fc3adcdb233a4109599f3438013"},{"author":{"_account_id":19206,"name":"Dawid Deja","email":"dawdej@gmail.com","username":"ddeja"},"change_message_id":"8601a73963d21f11621e02a0f84757bfdc702580","unresolved":false,"context_lines":[{"line_number":258,"context_line":""},{"line_number":259,"context_line":"    # NOTE(rakhmerov): There\u0027s a special endpoint for introspecting"},{"line_number":260,"context_line":"    # access tokens described in OpenID Connect specification but it\u0027s"},{"line_number":261,"context_line":"    # available in KeyCloak start only with version 1.8.Final so we have"},{"line_number":262,"context_line":"    # to use user info endpoint which also takes exactly one parameter"},{"line_number":263,"context_line":"    # (access token) and replies with error if token is invalid."},{"line_number":264,"context_line":"    user_info_endpoint \u003d ("}],"source_content_type":"text/x-python","patch_set":5,"id":"1aa78d24_36b7e83a","line":261,"range":{"start_line":261,"start_character":28,"end_line":261,"end_character":38},"updated":"2016-07-04 15:29:25.000000000","message":"Soft: starting","commit_id":"1c373836c0044fc3adcdb233a4109599f3438013"},{"author":{"_account_id":19206,"name":"Dawid Deja","email":"dawdej@gmail.com","username":"ddeja"},"change_message_id":"8601a73963d21f11621e02a0f84757bfdc702580","unresolved":false,"context_lines":[{"line_number":281,"context_line":"            \"HTTP response from OIDC provider: %s\" %"},{"line_number":282,"context_line":"            pprint.pformat(resp.json())"},{"line_number":283,"context_line":"        )"},{"line_number":284,"context_line":"    except Exception as e:"},{"line_number":285,"context_line":"        msg \u003d \"Failed to validate access token:\\n %s\" % str(e)"},{"line_number":286,"context_line":""},{"line_number":287,"context_line":"        LOG.exception(msg)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1aa78d24_a0b54dce","line":284,"range":{"start_line":284,"start_character":4,"end_line":284,"end_character":26},"updated":"2016-07-04 15:29:25.000000000","message":"I\u0027m not an expert in OpenID, but from documentation it looks like an http error 401 would be raised in case the token is invalid[1]. Therefore \u0027raise_for_status\u0027 would raise an \u0027requests.HTTPError\u0027[2] exception and such exception should be caught in the first place, before \u0027except Exception as e\u0027\n\n[1] http://openid.net/specs/openid-connect-core-1_0.html#UserInfoError\n[2] http://docs.python-requests.org/en/master/api/#requests.Response.raise_for_status","commit_id":"1c373836c0044fc3adcdb233a4109599f3438013"},{"author":{"_account_id":19206,"name":"Dawid Deja","email":"dawdej@gmail.com","username":"ddeja"},"change_message_id":"6732747e79bedf2661c9af55c8a159aa20f33630","unresolved":false,"context_lines":[{"line_number":281,"context_line":"            \"HTTP response from OIDC provider: %s\" %"},{"line_number":282,"context_line":"            pprint.pformat(resp.json())"},{"line_number":283,"context_line":"        )"},{"line_number":284,"context_line":"    except Exception as e:"},{"line_number":285,"context_line":"        msg \u003d \"Failed to validate access token:\\n %s\" % str(e)"},{"line_number":286,"context_line":""},{"line_number":287,"context_line":"        LOG.exception(msg)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1aa78d24_d1bad722","line":284,"range":{"start_line":284,"start_character":4,"end_line":284,"end_character":26},"in_reply_to":"1aa78d24_255b1d30","updated":"2016-07-05 06:50:48.000000000","message":"What I mean is that caughting all exceptions when we excepting just one (in this case, HTTPError) is a python antipattern[1].\n\n[1] https://realpython.com/blog/python/the-most-diabolical-python-antipattern/","commit_id":"1c373836c0044fc3adcdb233a4109599f3438013"},{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"ad670707b0b8fd0ba93abca9b9d00f43771c61f1","unresolved":false,"context_lines":[{"line_number":281,"context_line":"            \"HTTP response from OIDC provider: %s\" %"},{"line_number":282,"context_line":"            pprint.pformat(resp.json())"},{"line_number":283,"context_line":"        )"},{"line_number":284,"context_line":"    except Exception as e:"},{"line_number":285,"context_line":"        msg \u003d \"Failed to validate access token:\\n %s\" % str(e)"},{"line_number":286,"context_line":""},{"line_number":287,"context_line":"        LOG.exception(msg)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1aa78d24_255b1d30","line":284,"range":{"start_line":284,"start_character":4,"end_line":284,"end_character":26},"in_reply_to":"1aa78d24_a0b54dce","updated":"2016-07-05 04:15:59.000000000","message":"Maybe I don\u0027t see your point. HTTPError inherits from Exception so it will be caught by this clause. After all, it works, I checked it.","commit_id":"1c373836c0044fc3adcdb233a4109599f3438013"},{"author":{"_account_id":9029,"name":"Moshe Elisha","email":"moshe.elisha@nokia.com","username":"MosheElisha"},"change_message_id":"a808a3b59be4589267e01f0c375ccc5bbe884b56","unresolved":false,"context_lines":[{"line_number":225,"context_line":"        elif CONF.auth_type \u003d\u003d \u0027keycloak-oidc\u0027:"},{"line_number":226,"context_line":"            error_msg \u003d authenticate_with_keycloak(state.request)"},{"line_number":227,"context_line":""},{"line_number":228,"context_line":"        if error_msg:"},{"line_number":229,"context_line":"            pecan.abort("},{"line_number":230,"context_line":"                status_code\u003d401,"},{"line_number":231,"context_line":"                detail\u003derror_msg,"}],"source_content_type":"text/x-python","patch_set":7,"id":"1aa78d24_bb88c523","line":228,"updated":"2016-07-05 08:03:33.000000000","message":"IMO the authenticate_with_XXX methods should throw an exception and here should be try-except that logs and inits the \"error_msg\"","commit_id":"21222e9a0e35c93f4bc8e36d964d4b44d0072994"},{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"f1eea1c3165af3484201d418a13b963b48878cac","unresolved":false,"context_lines":[{"line_number":225,"context_line":"        elif CONF.auth_type \u003d\u003d \u0027keycloak-oidc\u0027:"},{"line_number":226,"context_line":"            error_msg \u003d authenticate_with_keycloak(state.request)"},{"line_number":227,"context_line":""},{"line_number":228,"context_line":"        if error_msg:"},{"line_number":229,"context_line":"            pecan.abort("},{"line_number":230,"context_line":"                status_code\u003d401,"},{"line_number":231,"context_line":"                detail\u003derror_msg,"}],"source_content_type":"text/x-python","patch_set":7,"id":"1aa78d24_fb3b6de9","line":228,"in_reply_to":"1aa78d24_bb88c523","updated":"2016-07-05 08:10:27.000000000","message":"Ok, yes. That way we don\u0027t have to assign error_msg many times.","commit_id":"21222e9a0e35c93f4bc8e36d964d4b44d0072994"},{"author":{"_account_id":9029,"name":"Moshe Elisha","email":"moshe.elisha@nokia.com","username":"MosheElisha"},"change_message_id":"a808a3b59be4589267e01f0c375ccc5bbe884b56","unresolved":false,"context_lines":[{"line_number":271,"context_line":"        resp \u003d requests.get("},{"line_number":272,"context_line":"            user_info_endpoint,"},{"line_number":273,"context_line":"            headers\u003d{\"Authorization\": \"Bearer %s\" % access_token},"},{"line_number":274,"context_line":"            verify\u003dFalse"},{"line_number":275,"context_line":"        )"},{"line_number":276,"context_line":""},{"line_number":277,"context_line":"        resp.raise_for_status()"}],"source_content_type":"text/x-python","patch_set":7,"id":"1aa78d24_3bcfb580","line":274,"updated":"2016-07-05 08:03:33.000000000","message":"verify should not always be False - it should depend on configuration","commit_id":"21222e9a0e35c93f4bc8e36d964d4b44d0072994"},{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"f1eea1c3165af3484201d418a13b963b48878cac","unresolved":false,"context_lines":[{"line_number":271,"context_line":"        resp \u003d requests.get("},{"line_number":272,"context_line":"            user_info_endpoint,"},{"line_number":273,"context_line":"            headers\u003d{\"Authorization\": \"Bearer %s\" % access_token},"},{"line_number":274,"context_line":"            verify\u003dFalse"},{"line_number":275,"context_line":"        )"},{"line_number":276,"context_line":""},{"line_number":277,"context_line":"        resp.raise_for_status()"}],"source_content_type":"text/x-python","patch_set":7,"id":"1aa78d24_3b5fb542","line":274,"in_reply_to":"1aa78d24_3bcfb580","updated":"2016-07-05 08:10:27.000000000","message":"True. I\u0027m now working on tests and will address this comment too in the next patchset.","commit_id":"21222e9a0e35c93f4bc8e36d964d4b44d0072994"},{"author":{"_account_id":9712,"name":"Dougal Matthews","email":"dougal.matthews@canonical.com","username":"dougal"},"change_message_id":"4a824eb197ca07ba10983f68a7ef287671b314ff","unresolved":false,"context_lines":[{"line_number":287,"context_line":""},{"line_number":288,"context_line":"        return msg"},{"line_number":289,"context_line":""},{"line_number":290,"context_line":"    return None"},{"line_number":291,"context_line":""},{"line_number":292,"context_line":""},{"line_number":293,"context_line":"class ContextHook(hooks.PecanHook):"}],"source_content_type":"text/x-python","patch_set":7,"id":"1aa78d24_9b66a927","line":290,"updated":"2016-07-05 08:07:50.000000000","message":"This isn\u0027t needed, with no return statement None is always returned. It doesn\u0027t do any harm though so I don\u0027t have a strong opinion :)","commit_id":"21222e9a0e35c93f4bc8e36d964d4b44d0072994"},{"author":{"_account_id":19206,"name":"Dawid Deja","email":"dawdej@gmail.com","username":"ddeja"},"change_message_id":"d2dff35a09520c78ff59f1aa0d27b4a35cc023fa","unresolved":false,"context_lines":[{"line_number":219,"context_line":"            return"},{"line_number":220,"context_line":""},{"line_number":221,"context_line":"        try:"},{"line_number":222,"context_line":"            if CONF.auth_type \u003d\u003d \u0027keystone\u0027:"},{"line_number":223,"context_line":"                authenticate_with_keystone(state.request)"},{"line_number":224,"context_line":"            elif CONF.auth_type \u003d\u003d \u0027keycloak-oidc\u0027:"},{"line_number":225,"context_line":"                authenticate_with_keycloak(state.request)"}],"source_content_type":"text/x-python","patch_set":12,"id":"1aa78d24_9329373a","line":222,"range":{"start_line":222,"start_character":34,"end_line":222,"end_character":42},"updated":"2016-07-06 07:28:17.000000000","message":"Soft: Usually string values are stored in variables and then only variable name is used","commit_id":"28c7d07474bcaacc738733d096cd6aada83aa755"},{"author":{"_account_id":19206,"name":"Dawid Deja","email":"dawdej@gmail.com","username":"ddeja"},"change_message_id":"d2dff35a09520c78ff59f1aa0d27b4a35cc023fa","unresolved":false,"context_lines":[{"line_number":221,"context_line":"        try:"},{"line_number":222,"context_line":"            if CONF.auth_type \u003d\u003d \u0027keystone\u0027:"},{"line_number":223,"context_line":"                authenticate_with_keystone(state.request)"},{"line_number":224,"context_line":"            elif CONF.auth_type \u003d\u003d \u0027keycloak-oidc\u0027:"},{"line_number":225,"context_line":"                authenticate_with_keycloak(state.request)"},{"line_number":226,"context_line":"        except Exception as e:"},{"line_number":227,"context_line":"            msg \u003d \"Failed to validate access token: %s\" % str(e)"}],"source_content_type":"text/x-python","patch_set":12,"id":"1aa78d24_33196323","line":224,"range":{"start_line":224,"start_character":36,"end_line":224,"end_character":49},"updated":"2016-07-06 07:28:17.000000000","message":"ditto","commit_id":"28c7d07474bcaacc738733d096cd6aada83aa755"},{"author":{"_account_id":19206,"name":"Dawid Deja","email":"dawdej@gmail.com","username":"ddeja"},"change_message_id":"d2dff35a09520c78ff59f1aa0d27b4a35cc023fa","unresolved":false,"context_lines":[{"line_number":273,"context_line":"        verify\u003dnot CONF.keycloak_oidc.insecure"},{"line_number":274,"context_line":"    )"},{"line_number":275,"context_line":""},{"line_number":276,"context_line":"    resp.raise_for_status()"},{"line_number":277,"context_line":""},{"line_number":278,"context_line":"    LOG.debug("},{"line_number":279,"context_line":"        \"HTTP response from OIDC provider: %s\" %"}],"source_content_type":"text/x-python","patch_set":12,"id":"1aa78d24_d3257f09","line":276,"range":{"start_line":276,"start_character":4,"end_line":276,"end_character":27},"updated":"2016-07-06 07:28:17.000000000","message":"resp.status_code \u003d\u003d 401 can be used to raise \u0027exc.UnauthorizedException()\u0027 so that both keystone and keycloack authentication would raise same exception in case user is unauthorized. But this is not mandatory","commit_id":"28c7d07474bcaacc738733d096cd6aada83aa755"}]}