)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"c77f83f2c44f98cb8014ac1daed495db5b1813e4","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Dan Smith \u003cdansmith@redhat.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2020-07-08 10:16:43 -0700"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"WIP Add a policy knob for allowing non-owned image copying"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This adds a copy_image policy knob which can be used to grant users"},{"line_number":10,"context_line":"the ability to copy images other than their own using the new"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"bf51134e_6ee37b35","line":7,"range":{"start_line":7,"start_character":0,"end_line":7,"end_character":4},"updated":"2020-07-08 17:30:57.000000000","message":"Meant to remove this, but will re-spin after the current test run completes.","commit_id":"2e67cbfc6128f6ae2b2466cefceaa0bb4c05572f"}],"glance/api/v2/images.py":[{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"0e6ed765994e806a0ca03cefe3ec6346975217dc","unresolved":false,"context_lines":[{"line_number":133,"context_line":"                raise exception.Conflict(msg)"},{"line_number":134,"context_line":"            if import_method \u003d\u003d \u0027copy-image\u0027:"},{"line_number":135,"context_line":"                self.policy.enforce(req.context, \u0027copy_image\u0027,"},{"line_number":136,"context_line":"                                    dict(policy.ImageTarget(image)))"},{"line_number":137,"context_line":"            else:"},{"line_number":138,"context_line":"                if not authorization.is_image_mutable(req.context, image):"},{"line_number":139,"context_line":"                    raise webob.exc.HTTPForbidden("}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_f711c85e","line":136,"range":{"start_line":136,"start_character":36,"end_line":136,"end_character":67},"updated":"2020-07-14 23:44:59.000000000","message":"I saw in the log that somehow passed target data is not formatted properly in oslo policy[1]. Not sure why.\n\n\"target\u003dcannot format data, exception: You are not permitted to modify \u0027hw_rng_model\u0027 on this image.\"\n\nError is coming from here\n- https://github.com/openstack/oslo.policy/blob/cab28649c689067970a51a2f9b329bdd6a0f0501/oslo_policy/policy.py#L930\n\n[1]https://zuul.opendev.org/t/openstack/build/aa1a5cd62aaa4210b19a5877e77b87db/log/controller/logs/screen-g-api.txt#600","commit_id":"d3c17feb00c865b80ebf49294f4ef9c917f4c8e1"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"fce34842dcc29c3dde404abe965b9d9935d6977d","unresolved":false,"context_lines":[{"line_number":133,"context_line":"                raise exception.Conflict(msg)"},{"line_number":134,"context_line":"            if import_method \u003d\u003d \u0027copy-image\u0027:"},{"line_number":135,"context_line":"                self.policy.enforce(req.context, \u0027copy_image\u0027,"},{"line_number":136,"context_line":"                                    dict(policy.ImageTarget(image)))"},{"line_number":137,"context_line":"            else:"},{"line_number":138,"context_line":"                if not authorization.is_image_mutable(req.context, image):"},{"line_number":139,"context_line":"                    raise webob.exc.HTTPForbidden("}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_41f6e45e","line":136,"range":{"start_line":136,"start_character":36,"end_line":136,"end_character":67},"in_reply_to":"bf51134e_b5dd7e42","updated":"2020-07-15 17:21:43.000000000","message":"i see, thanks for updates. its working fine with master oslo policy with 740068.\n\nYeah it is debug log issue, not functionality.","commit_id":"d3c17feb00c865b80ebf49294f4ef9c917f4c8e1"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f688458bd37781a4dd9e1393c1d87540b2971790","unresolved":false,"context_lines":[{"line_number":133,"context_line":"                raise exception.Conflict(msg)"},{"line_number":134,"context_line":"            if import_method \u003d\u003d \u0027copy-image\u0027:"},{"line_number":135,"context_line":"                self.policy.enforce(req.context, \u0027copy_image\u0027,"},{"line_number":136,"context_line":"                                    dict(policy.ImageTarget(image)))"},{"line_number":137,"context_line":"            else:"},{"line_number":138,"context_line":"                if not authorization.is_image_mutable(req.context, image):"},{"line_number":139,"context_line":"                    raise webob.exc.HTTPForbidden("}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_b5dd7e42","line":136,"range":{"start_line":136,"start_character":36,"end_line":136,"end_character":67},"in_reply_to":"bf51134e_f711c85e","updated":"2020-07-15 14:19:32.000000000","message":"This is due to a deepcopy in the debug paths of oslo.policy, and is really a bug in glance\u0027s ImmutableImage and ImmutableProperty implementation. There is an oslo.policy patch merged to stop deepcopying the objects which will help alleviate the issue, but it\u0027s just debug output and does not affect the functioning of the code at all.","commit_id":"d3c17feb00c865b80ebf49294f4ef9c917f4c8e1"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"4ee0cf7c5ddf86908406fd4c15cd58f1adc75e9b","unresolved":false,"context_lines":[{"line_number":131,"context_line":"            if not getattr(image, \u0027disk_format\u0027, None):"},{"line_number":132,"context_line":"                msg \u003d _(\"\u0027disk_format\u0027 needs to be set before import\")"},{"line_number":133,"context_line":"                raise exception.Conflict(msg)"},{"line_number":134,"context_line":"            if import_method \u003d\u003d \u0027copy-image\u0027:"},{"line_number":135,"context_line":"                self.policy.enforce(req.context, \u0027copy_image\u0027,"},{"line_number":136,"context_line":"                                    dict(policy.ImageTarget(image)))"},{"line_number":137,"context_line":"            else:"},{"line_number":138,"context_line":"                if not authorization.is_image_mutable(req.context, image):"},{"line_number":139,"context_line":"                    raise webob.exc.HTTPForbidden("},{"line_number":140,"context_line":"                        explanation\u003d_(\"Operation not permitted\"))"},{"line_number":141,"context_line":""},{"line_number":142,"context_line":"            stores \u003d [None]"},{"line_number":143,"context_line":"            if CONF.enabled_backends:"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_7cb63f40","line":140,"range":{"start_line":134,"start_character":0,"end_line":140,"end_character":65},"updated":"2020-07-14 20:57:12.000000000","message":"After all those other conflict checks have happened at lines 112-133, what we\u0027re really interested in at this point is whether the image can be mutated in this context.  I think it would be more clear to write the condition like this:\n\n    if not authorization.is_image_mutable(req.context, image):\n        if import_method \u003d\u003d \u0027copy-image\u0027:\n            # if the policy allows us to do this, the context will be\n            # escalated to allow mutation at the appropriate time\n            self.policy.enforce(req.context, \u0027copy_image\u0027,\n                                dict(policy.ImageTarget(image)))\n        else:\n            raise webob.exc.HTTPForbidden(\n                explanation\u003d_(\"Operation not permitted\"))\n\n(What I\u0027m afraid of is that someone will add another check in lines 112-133 that will somehow inappropriately miss this mutability check if it\u0027s hidden in the else.)","commit_id":"d3c17feb00c865b80ebf49294f4ef9c917f4c8e1"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"33f2b32c1bff71e92932cd606006a14fa7012d3a","unresolved":false,"context_lines":[{"line_number":131,"context_line":"            if not getattr(image, \u0027disk_format\u0027, None):"},{"line_number":132,"context_line":"                msg \u003d _(\"\u0027disk_format\u0027 needs to be set before import\")"},{"line_number":133,"context_line":"                raise exception.Conflict(msg)"},{"line_number":134,"context_line":"            if import_method \u003d\u003d \u0027copy-image\u0027:"},{"line_number":135,"context_line":"                self.policy.enforce(req.context, \u0027copy_image\u0027,"},{"line_number":136,"context_line":"                                    dict(policy.ImageTarget(image)))"},{"line_number":137,"context_line":"            else:"},{"line_number":138,"context_line":"                if not authorization.is_image_mutable(req.context, image):"},{"line_number":139,"context_line":"                    raise webob.exc.HTTPForbidden("},{"line_number":140,"context_line":"                        explanation\u003d_(\"Operation not permitted\"))"},{"line_number":141,"context_line":""},{"line_number":142,"context_line":"            stores \u003d [None]"},{"line_number":143,"context_line":"            if CONF.enabled_backends:"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_c96fba8f","line":140,"range":{"start_line":134,"start_character":0,"end_line":140,"end_character":65},"in_reply_to":"bf51134e_752f66f5","updated":"2020-07-15 19:44:45.000000000","message":"I agree that the policy knob should actually work, so let\u0027s keep the code the way you have it.  I will settle for a blank line before line 134 to signal that this if/else block is a bit different from the others.","commit_id":"d3c17feb00c865b80ebf49294f4ef9c917f4c8e1"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f688458bd37781a4dd9e1393c1d87540b2971790","unresolved":false,"context_lines":[{"line_number":131,"context_line":"            if not getattr(image, \u0027disk_format\u0027, None):"},{"line_number":132,"context_line":"                msg \u003d _(\"\u0027disk_format\u0027 needs to be set before import\")"},{"line_number":133,"context_line":"                raise exception.Conflict(msg)"},{"line_number":134,"context_line":"            if import_method \u003d\u003d \u0027copy-image\u0027:"},{"line_number":135,"context_line":"                self.policy.enforce(req.context, \u0027copy_image\u0027,"},{"line_number":136,"context_line":"                                    dict(policy.ImageTarget(image)))"},{"line_number":137,"context_line":"            else:"},{"line_number":138,"context_line":"                if not authorization.is_image_mutable(req.context, image):"},{"line_number":139,"context_line":"                    raise webob.exc.HTTPForbidden("},{"line_number":140,"context_line":"                        explanation\u003d_(\"Operation not permitted\"))"},{"line_number":141,"context_line":""},{"line_number":142,"context_line":"            stores \u003d [None]"},{"line_number":143,"context_line":"            if CONF.enabled_backends:"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_752f66f5","line":140,"range":{"start_line":134,"start_character":0,"end_line":140,"end_character":65},"in_reply_to":"bf51134e_7cb63f40","updated":"2020-07-15 14:19:32.000000000","message":"I think your code here would *not* allow the admin to use the policy knob to deny copying images to users when they already own it right? Meaning, if the operator wanted to say \"even if you own the image, you can\u0027t copy it if the container_format is foobar\", the policy check would be skipped in the case where they own the image (i.e. it is mutable). I think it\u0027s important for the policy check to be a first-class citizen in order to behave the way the operator would expect (i.e. that if I disable copy-image, it\u0027s disabled).\n\nWhat if I changed the:\n\n  else:\n      if not authorization...\n\nto:\n\n  elif not authorization...\n\n?\n\nThat would elevate it to the elif level at least, although I\u0027ll be honest and say I\u0027m not quite sure I get the concern. Any other checks that abort the request added above this will be fine, assuming they\u0027re of the same type as those we have now, which are all state/workflow-related.","commit_id":"d3c17feb00c865b80ebf49294f4ef9c917f4c8e1"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"0ba26fdba6c55751dcb2802118d3c026433c839a","unresolved":false,"context_lines":[{"line_number":131,"context_line":"            if not getattr(image, \u0027disk_format\u0027, None):"},{"line_number":132,"context_line":"                msg \u003d _(\"\u0027disk_format\u0027 needs to be set before import\")"},{"line_number":133,"context_line":"                raise exception.Conflict(msg)"},{"line_number":134,"context_line":"            if import_method \u003d\u003d \u0027copy-image\u0027:"},{"line_number":135,"context_line":"                self.policy.enforce(req.context, \u0027copy_image\u0027,"},{"line_number":136,"context_line":"                                    dict(policy.ImageTarget(image)))"},{"line_number":137,"context_line":"            else:"},{"line_number":138,"context_line":"                if not authorization.is_image_mutable(req.context, image):"},{"line_number":139,"context_line":"                    raise webob.exc.HTTPForbidden("},{"line_number":140,"context_line":"                        explanation\u003d_(\"Operation not permitted\"))"},{"line_number":141,"context_line":""},{"line_number":142,"context_line":"            stores \u003d [None]"},{"line_number":143,"context_line":"            if CONF.enabled_backends:"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_c9ccda62","line":140,"range":{"start_line":134,"start_character":0,"end_line":140,"end_character":65},"in_reply_to":"bf51134e_c96fba8f","updated":"2020-07-15 19:58:31.000000000","message":"Okay, I\u0027mma convert to elif because there\u0027s no reason not to and add a blank line plus a comment about the reasoning which should help set this off from the checks above.","commit_id":"d3c17feb00c865b80ebf49294f4ef9c917f4c8e1"}],"glance/policies/image.py":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"4ee0cf7c5ddf86908406fd4c15cd58f1adc75e9b","unresolved":false,"context_lines":[{"line_number":21,"context_line":"    policy.RuleDefault(name\u003d\"modify_image\", check_str\u003d\"rule:default\"),"},{"line_number":22,"context_line":"    policy.RuleDefault(name\u003d\"publicize_image\", check_str\u003d\"role:admin\"),"},{"line_number":23,"context_line":"    policy.RuleDefault(name\u003d\"communitize_image\", check_str\u003d\"rule:default\"),"},{"line_number":24,"context_line":"    policy.RuleDefault(name\u003d\"copy_from\", check_str\u003d\"rule:default\"),"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"    policy.RuleDefault(name\u003d\"download_image\", check_str\u003d\"rule:default\"),"},{"line_number":27,"context_line":"    policy.RuleDefault(name\u003d\"upload_image\", check_str\u003d\"rule:default\"),"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_714dc8c2","line":24,"range":{"start_line":24,"start_character":30,"end_line":24,"end_character":38},"updated":"2020-07-14 20:57:12.000000000","message":"I think this was used only by v1, so it can be removed.  Just pointing it out because it could easily be confused with the new policy.  But I don\u0027t think that\u0027s an issue.","commit_id":"d3c17feb00c865b80ebf49294f4ef9c917f4c8e1"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f688458bd37781a4dd9e1393c1d87540b2971790","unresolved":false,"context_lines":[{"line_number":21,"context_line":"    policy.RuleDefault(name\u003d\"modify_image\", check_str\u003d\"rule:default\"),"},{"line_number":22,"context_line":"    policy.RuleDefault(name\u003d\"publicize_image\", check_str\u003d\"role:admin\"),"},{"line_number":23,"context_line":"    policy.RuleDefault(name\u003d\"communitize_image\", check_str\u003d\"rule:default\"),"},{"line_number":24,"context_line":"    policy.RuleDefault(name\u003d\"copy_from\", check_str\u003d\"rule:default\"),"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"    policy.RuleDefault(name\u003d\"download_image\", check_str\u003d\"rule:default\"),"},{"line_number":27,"context_line":"    policy.RuleDefault(name\u003d\"upload_image\", check_str\u003d\"rule:default\"),"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_751806d2","line":24,"range":{"start_line":24,"start_character":30,"end_line":24,"end_character":38},"in_reply_to":"bf51134e_714dc8c2","updated":"2020-07-15 14:19:32.000000000","message":"Ack, I was wondering. I can put a patch after this to remove it and see if anything breaks if you want.","commit_id":"d3c17feb00c865b80ebf49294f4ef9c917f4c8e1"}]}