)]}'
{"glance/api/v2/image_tags.py":[{"author":{"_account_id":8122,"name":"Cyril Roelandt","email":"cyril@redhat.com","username":"cyril.roelandt.enovance"},"change_message_id":"5d04f19a7a17afaa584b0a4579a29546d2f173b4","unresolved":true,"context_lines":[{"line_number":57,"context_line":""},{"line_number":58,"context_line":"            # FIXME(abhishekk): This check is already performed at db layer"},{"line_number":59,"context_line":"            # but our simple API which is used in test does not have this"},{"line_number":60,"context_line":"            # support"},{"line_number":61,"context_line":"            try:"},{"line_number":62,"context_line":"                api_policy.check_is_image_mutable(req.context, image)"},{"line_number":63,"context_line":"            except exception.Forbidden as e:"}],"source_content_type":"text/x-python","patch_set":1,"id":"cd8ac62f_2838ad48","line":60,"range":{"start_line":60,"start_character":0,"end_line":60,"end_character":21},"updated":"2021-08-16 19:33:31.000000000","message":"How hard would it be to move this to the simple API?","commit_id":"5f5a5e8d7e330bb1b136b15fc40e92314990ec4c"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"91256bfacba00b5e76fd2571c1ecafee203ebcff","unresolved":true,"context_lines":[{"line_number":57,"context_line":""},{"line_number":58,"context_line":"            # FIXME(abhishekk): This check is already performed at db layer"},{"line_number":59,"context_line":"            # but our simple API which is used in test does not have this"},{"line_number":60,"context_line":"            # support"},{"line_number":61,"context_line":"            try:"},{"line_number":62,"context_line":"                api_policy.check_is_image_mutable(req.context, image)"},{"line_number":63,"context_line":"            except exception.Forbidden as e:"}],"source_content_type":"text/x-python","patch_set":1,"id":"f4ade0e8_684e6956","line":60,"range":{"start_line":60,"start_character":0,"end_line":60,"end_character":21},"in_reply_to":"cd8ac62f_2838ad48","updated":"2021-08-18 16:56:28.000000000","message":"I am not sure about impact on other tests, so kept it like this.\nSimple API update method is little bit difficult to understand as well","commit_id":"5f5a5e8d7e330bb1b136b15fc40e92314990ec4c"},{"author":{"_account_id":8122,"name":"Cyril Roelandt","email":"cyril@redhat.com","username":"cyril.roelandt.enovance"},"change_message_id":"5d04f19a7a17afaa584b0a4579a29546d2f173b4","unresolved":true,"context_lines":[{"line_number":99,"context_line":""},{"line_number":100,"context_line":"            # FIXME(abhishekk): This check is already performed at db layer"},{"line_number":101,"context_line":"            # but our simple API which is used in test does not have this"},{"line_number":102,"context_line":"            # support"},{"line_number":103,"context_line":"            try:"},{"line_number":104,"context_line":"                api_policy.check_is_image_mutable(req.context, image)"},{"line_number":105,"context_line":"            except exception.Forbidden as e:"}],"source_content_type":"text/x-python","patch_set":1,"id":"c5c151b1_2bc5c2f4","line":102,"range":{"start_line":102,"start_character":0,"end_line":102,"end_character":21},"updated":"2021-08-16 19:33:31.000000000","message":"Ditto","commit_id":"5f5a5e8d7e330bb1b136b15fc40e92314990ec4c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"fb02f92e0ebf8fe87f632059c0019e6a98cc28bb","unresolved":true,"context_lines":[{"line_number":57,"context_line":""},{"line_number":58,"context_line":"            # FIXME(abhishekk): This check is already performed at db layer"},{"line_number":59,"context_line":"            # but our simple API which is used in test does not have this"},{"line_number":60,"context_line":"            # support"},{"line_number":61,"context_line":"            try:"},{"line_number":62,"context_line":"                api_policy.check_is_image_mutable(req.context, image)"},{"line_number":63,"context_line":"            except exception.Forbidden as e:"}],"source_content_type":"text/x-python","patch_set":6,"id":"57e3b5b6_46f9b17b","line":60,"updated":"2021-08-24 14:06:35.000000000","message":"So, the fixme here is to remove the check from the DB layer and keep the one you\u0027re adding?","commit_id":"d7ae622853007fe1841eb4dbeebe49c162233656"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"0a2e504d2ed70fdae73047fc2cde98e6510b259a","unresolved":true,"context_lines":[{"line_number":57,"context_line":""},{"line_number":58,"context_line":"            # FIXME(abhishekk): This check is already performed at db layer"},{"line_number":59,"context_line":"            # but our simple API which is used in test does not have this"},{"line_number":60,"context_line":"            # support"},{"line_number":61,"context_line":"            try:"},{"line_number":62,"context_line":"                api_policy.check_is_image_mutable(req.context, image)"},{"line_number":63,"context_line":"            except exception.Forbidden as e:"}],"source_content_type":"text/x-python","patch_set":6,"id":"f4b0e9bf_14303802","line":60,"in_reply_to":"30a08874_2c51d718","updated":"2021-08-24 15:47:40.000000000","message":"Interesting. The check_is_image_mutable() method feels like glance business logic. I would have expected it to live in a higher layer so that multiple backends don\u0027t need to implement it.\n\nUnless the \"mutable\" part of the check is from the perspective of \"does this backend support mutable images?\"\n\nBut, the current implementation is written to be specific to context objects, not backend functionality.","commit_id":"d7ae622853007fe1841eb4dbeebe49c162233656"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"112761c4e35e1a0a15a32c747cb6f77ae197cd3b","unresolved":true,"context_lines":[{"line_number":57,"context_line":""},{"line_number":58,"context_line":"            # FIXME(abhishekk): This check is already performed at db layer"},{"line_number":59,"context_line":"            # but our simple API which is used in test does not have this"},{"line_number":60,"context_line":"            # support"},{"line_number":61,"context_line":"            try:"},{"line_number":62,"context_line":"                api_policy.check_is_image_mutable(req.context, image)"},{"line_number":63,"context_line":"            except exception.Forbidden as e:"}],"source_content_type":"text/x-python","patch_set":6,"id":"fcf1eb8f_7f6e2449","line":60,"in_reply_to":"40a85281_0a8b5270","updated":"2021-08-24 18:42:41.000000000","message":"Yeah - I noticed a pattern where we were duplicating the check_is_image_mutable() check in the API layer. I thought the plan was to remove it from the database layer entirely because we didn\u0027t want the database making authorization decisions.\n\nThis probably doesn\u0027t need to be a sticking point of this review, since it needs to happen later anyway, but now I\u0027m curious :)","commit_id":"d7ae622853007fe1841eb4dbeebe49c162233656"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"2bbd5c77fc65532577664e5d352b02978d1580e7","unresolved":true,"context_lines":[{"line_number":57,"context_line":""},{"line_number":58,"context_line":"            # FIXME(abhishekk): This check is already performed at db layer"},{"line_number":59,"context_line":"            # but our simple API which is used in test does not have this"},{"line_number":60,"context_line":"            # support"},{"line_number":61,"context_line":"            try:"},{"line_number":62,"context_line":"                api_policy.check_is_image_mutable(req.context, image)"},{"line_number":63,"context_line":"            except exception.Forbidden as e:"}],"source_content_type":"text/x-python","patch_set":6,"id":"30a08874_2c51d718","line":60,"in_reply_to":"57e3b5b6_46f9b17b","updated":"2021-08-24 14:45:44.000000000","message":"Fix me is for adding this check in simple db similar what we are doing in sqlalchemy api and remove it from here.","commit_id":"d7ae622853007fe1841eb4dbeebe49c162233656"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"feb461d30591acb1007285a1ef0b71c90bd81841","unresolved":true,"context_lines":[{"line_number":57,"context_line":""},{"line_number":58,"context_line":"            # FIXME(abhishekk): This check is already performed at db layer"},{"line_number":59,"context_line":"            # but our simple API which is used in test does not have this"},{"line_number":60,"context_line":"            # support"},{"line_number":61,"context_line":"            try:"},{"line_number":62,"context_line":"                api_policy.check_is_image_mutable(req.context, image)"},{"line_number":63,"context_line":"            except exception.Forbidden as e:"}],"source_content_type":"text/x-python","patch_set":6,"id":"40a85281_0a8b5270","line":60,"in_reply_to":"f4b0e9bf_14303802","updated":"2021-08-24 15:51:58.000000000","message":"This check is performed almost for all API operations at the moment, I think similar to policy layer refactoring we need to carry forward db layer refactoring as well.","commit_id":"d7ae622853007fe1841eb4dbeebe49c162233656"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"0ba5560362065fee76082e479929e8c40702d651","unresolved":true,"context_lines":[{"line_number":57,"context_line":""},{"line_number":58,"context_line":"            # FIXME(abhishekk): This check is already performed at db layer"},{"line_number":59,"context_line":"            # but our simple API which is used in test does not have this"},{"line_number":60,"context_line":"            # support"},{"line_number":61,"context_line":"            try:"},{"line_number":62,"context_line":"                api_policy.check_is_image_mutable(req.context, image)"},{"line_number":63,"context_line":"            except exception.Forbidden as e:"}],"source_content_type":"text/x-python","patch_set":6,"id":"3dfda1e4_a4a5b1a1","line":60,"in_reply_to":"fcf1eb8f_7f6e2449","updated":"2021-08-24 18:54:14.000000000","message":"May be instead of this I should call modify_image check which is similar to mutability check I think.","commit_id":"d7ae622853007fe1841eb4dbeebe49c162233656"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"28ca2bd50a88a16b7374339bcff5abc822cc84ff","unresolved":true,"context_lines":[{"line_number":48,"context_line":"        image_repo \u003d self.gateway.get_repo("},{"line_number":49,"context_line":"            req.context, authorization_layer\u003dFalse)"},{"line_number":50,"context_line":"        try:"},{"line_number":51,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":52,"context_line":"            # NOTE(abhishekk): get_image and modify_images are only"},{"line_number":53,"context_line":"            # policies which are enforced for Image Tag APIs"},{"line_number":54,"context_line":"            api_pol \u003d api_policy.ImageAPIPolicy(req.context, image,"}],"source_content_type":"text/x-python","patch_set":9,"id":"aea5aa8e_93378b7b","line":51,"updated":"2021-08-25 13:38:09.000000000","message":"Will this short-circuit line 56? If a user attempts to update the image tags for a private image outside their project, they are going to fail this check with a NotFound exception handled at line 61. It looks like we have that built into the get_image() policy check.\n\nIIUC, should lines 56 - 57 be moved up here before line 51?","commit_id":"7ea1427d38553e168d4cfe1ec667da1f1976482b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"2eeeddf72ee37fa6ec2333a6da3d7a3f9ff8b954","unresolved":true,"context_lines":[{"line_number":48,"context_line":"        image_repo \u003d self.gateway.get_repo("},{"line_number":49,"context_line":"            req.context, authorization_layer\u003dFalse)"},{"line_number":50,"context_line":"        try:"},{"line_number":51,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":52,"context_line":"            # NOTE(abhishekk): get_image and modify_images are only"},{"line_number":53,"context_line":"            # policies which are enforced for Image Tag APIs"},{"line_number":54,"context_line":"            api_pol \u003d api_policy.ImageAPIPolicy(req.context, image,"}],"source_content_type":"text/x-python","patch_set":9,"id":"fd3f0517_e65b1e0c","line":51,"in_reply_to":"aea5aa8e_93378b7b","updated":"2021-08-25 15:10:55.000000000","message":"I\u0027m not sure how we could do that because we need the image we grab here to pass to the enforcer.\n\nAnd yes, we have this in many other places where the DB makes a decision about whether or not you can see an image before we get a chance to enforce policy. I think fixing that comes later with the RBAC work, where we remove some of that from the DB layer so that we can actually enforce policy as written.","commit_id":"7ea1427d38553e168d4cfe1ec667da1f1976482b"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"3aedd646c5f02da2833be673bc97eafa2915f6ff","unresolved":false,"context_lines":[{"line_number":48,"context_line":"        image_repo \u003d self.gateway.get_repo("},{"line_number":49,"context_line":"            req.context, authorization_layer\u003dFalse)"},{"line_number":50,"context_line":"        try:"},{"line_number":51,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":52,"context_line":"            # NOTE(abhishekk): get_image and modify_images are only"},{"line_number":53,"context_line":"            # policies which are enforced for Image Tag APIs"},{"line_number":54,"context_line":"            api_pol \u003d api_policy.ImageAPIPolicy(req.context, image,"}],"source_content_type":"text/x-python","patch_set":9,"id":"38d1c016_1d29c572","line":51,"in_reply_to":"fd3f0517_e65b1e0c","updated":"2021-08-25 15:16:16.000000000","message":"Ack","commit_id":"7ea1427d38553e168d4cfe1ec667da1f1976482b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"e2db8cd3f855a7dc5d395b6c927f6a805d5b4355","unresolved":true,"context_lines":[{"line_number":53,"context_line":"            # policies which are enforced for Image Tag APIs"},{"line_number":54,"context_line":"            api_pol \u003d api_policy.ImageAPIPolicy(req.context, image,"},{"line_number":55,"context_line":"                                                self.policy)"},{"line_number":56,"context_line":"            api_pol.get_image()"},{"line_number":57,"context_line":"            api_pol.modify_image()"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"            image.tags.add(tag_value)"}],"source_content_type":"text/x-python","patch_set":9,"id":"aaaaa71d_cf1e215b","line":56,"updated":"2021-08-25 15:16:56.000000000","message":"I\u0027m not sure why we\u0027re checking get_image here. If modify is all that is needed for this, then checking modify will either allow it or reject with 403/404 based on get_image. What about some auditing thing that is going along and tagging images based on whether or not they\u0027ve been backed up or something. That bot doesn\u0027t need to be able to read the license keys from the image, just add a tag. Same for delete.","commit_id":"7ea1427d38553e168d4cfe1ec667da1f1976482b"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"28ca2bd50a88a16b7374339bcff5abc822cc84ff","unresolved":true,"context_lines":[{"line_number":88,"context_line":"            # policies which are enforced for Image Tag APIs"},{"line_number":89,"context_line":"            api_pol \u003d api_policy.ImageAPIPolicy(req.context, image,"},{"line_number":90,"context_line":"                                                self.policy)"},{"line_number":91,"context_line":"            api_pol.get_image()"},{"line_number":92,"context_line":"            api_pol.modify_image()"},{"line_number":93,"context_line":""},{"line_number":94,"context_line":"            if tag_value not in image.tags:"}],"source_content_type":"text/x-python","patch_set":9,"id":"c0acc120_c4dfc69d","line":91,"updated":"2021-08-25 13:38:09.000000000","message":"Ditto","commit_id":"7ea1427d38553e168d4cfe1ec667da1f1976482b"}]}