)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":8122,"name":"Cyril Roelandt","email":"cyril@redhat.com","username":"cyril.roelandt.enovance"},"change_message_id":"df4cc852e63e6971c3c1a06975535c9525d8c678","unresolved":true,"context_lines":[{"line_number":18,"context_line":"updated tests in this patch might be obsolete once we have formal"},{"line_number":19,"context_line":"protection testing implemented in either glance or tempest that"},{"line_number":20,"context_line":"exercise the policy defaults, instead of an empty policy file."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Change-Id: If0c456617a9e17c006a6ffe2a83f4a73b53da3d0"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"e4341ffc_5a48ec2b","line":21,"range":{"start_line":21,"start_character":0,"end_line":21,"end_character":0},"updated":"2021-02-17 19:44:02.000000000","message":"Are we still properly testing both old policies (deprecated) and new policies (RBAC-compliant), though? It is my understanding that users may be able to keep using the old policies for a little while, so ensuring they still work probably matters.","commit_id":"b5f4209f1c7484da38ab492320edef8724113c46"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"db362af081de7403f5924a3f033bd0d20f958ca1","unresolved":true,"context_lines":[{"line_number":18,"context_line":"updated tests in this patch might be obsolete once we have formal"},{"line_number":19,"context_line":"protection testing implemented in either glance or tempest that"},{"line_number":20,"context_line":"exercise the policy defaults, instead of an empty policy file."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Change-Id: If0c456617a9e17c006a6ffe2a83f4a73b53da3d0"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"840b1ed9_2ee923f2","line":21,"range":{"start_line":21,"start_character":0,"end_line":21,"end_character":0},"in_reply_to":"e4341ffc_5a48ec2b","updated":"2021-02-17 20:43:24.000000000","message":"Yes, you\u0027re right.\n\nBy default, the old policies are used unless the operator configures glance otherwise by setting:\n\n  glance-api.conf\n    [oslo_policy]\n    enforce_new_defaults\u003dTrue","commit_id":"b5f4209f1c7484da38ab492320edef8724113c46"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"805e8deea27d8437758ec77c56f77269d13e8373","unresolved":true,"context_lines":[{"line_number":14,"context_line":"refactor portions of glance to make it easier to implement system-scope."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"NOTE:"},{"line_number":17,"context_line":"Glance is implementing Secure RBAC as EXPERIMENTAL in Wallaby, so to"},{"line_number":18,"context_line":"enable it operator needs to set ``glance-api.conf [oslo_policy]"},{"line_number":19,"context_line":"enforce_new_defaults`` to True."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Implements: blueprint secure-rbac"},{"line_number":22,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":14,"id":"bf1344a8_4fb0ee47","line":19,"range":{"start_line":17,"start_character":0,"end_line":19,"end_character":31},"updated":"2021-03-01 06:47:39.000000000","message":"Need to modify NOTE to add glance specific config option as decided in weekly meeting.","commit_id":"e14b0cde25ab721605f222d0dacfa119f02c5056"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"3bb75def5f5e4714196917f4b03c65dbc3b6466e","unresolved":false,"context_lines":[{"line_number":14,"context_line":"refactor portions of glance to make it easier to implement system-scope."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"NOTE:"},{"line_number":17,"context_line":"Glance is implementing Secure RBAC as EXPERIMENTAL in Wallaby, so to"},{"line_number":18,"context_line":"enable it operator needs to set ``glance-api.conf [oslo_policy]"},{"line_number":19,"context_line":"enforce_new_defaults`` to True."},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"Implements: blueprint secure-rbac"},{"line_number":22,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":14,"id":"d3d17870_e0e550dd","line":19,"range":{"start_line":17,"start_character":0,"end_line":19,"end_character":31},"in_reply_to":"bf1344a8_4fb0ee47","updated":"2021-03-01 20:12:05.000000000","message":"Done","commit_id":"e14b0cde25ab721605f222d0dacfa119f02c5056"}],"glance/api/middleware/cache.py":[{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"9916c3d7cc4a61461cc4dc470ca7103da0cef90a","unresolved":true,"context_lines":[{"line_number":98,"context_line":"    def _enforce(self, req, action, target\u003dNone):"},{"line_number":99,"context_line":"        \"\"\"Authorize an action against our policies\"\"\""},{"line_number":100,"context_line":"        if target is None:"},{"line_number":101,"context_line":"            target \u003d {}"},{"line_number":102,"context_line":"        try:"},{"line_number":103,"context_line":"            self.policy.enforce(req.context, action, target)"},{"line_number":104,"context_line":"        except exception.Forbidden as e:"}],"source_content_type":"text/x-python","patch_set":28,"id":"61d134d5_5bd7e0dc","line":101,"updated":"2021-03-05 17:52:45.000000000","message":"Should we add project_id here only?","commit_id":"c693ec8430a79745117b60ea27c95b56034ab383"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"8aad07f59abc330452daba2a0522be0209221a77","unresolved":false,"context_lines":[{"line_number":98,"context_line":"    def _enforce(self, req, action, target\u003dNone):"},{"line_number":99,"context_line":"        \"\"\"Authorize an action against our policies\"\"\""},{"line_number":100,"context_line":"        if target is None:"},{"line_number":101,"context_line":"            target \u003d {}"},{"line_number":102,"context_line":"        try:"},{"line_number":103,"context_line":"            self.policy.enforce(req.context, action, target)"},{"line_number":104,"context_line":"        except exception.Forbidden as e:"}],"source_content_type":"text/x-python","patch_set":28,"id":"6bdf50ef_6bad27a2","line":101,"in_reply_to":"61d134d5_5bd7e0dc","updated":"2021-03-05 18:06:34.000000000","message":"Done","commit_id":"c693ec8430a79745117b60ea27c95b56034ab383"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"0b7cf52b4a4039dbe36d8ed867d4fab9424bcdad","unresolved":true,"context_lines":[{"line_number":105,"context_line":"            # update the project_id attribute of the target to match the owner."},{"line_number":106,"context_line":"            # If the target isn\u0027t of dict() type already, we should explicitly"},{"line_number":107,"context_line":"            # handle that here. We take a similar approach in the policy layer"},{"line_number":108,"context_line":"            # (glance.api.policy) to make sure we\u0027re build accurate image"},{"line_number":109,"context_line":"            # targets."},{"line_number":110,"context_line":"            target \u003d dict(target)"},{"line_number":111,"context_line":"        target[\u0027project_id\u0027] \u003d target.get(\u0027owner\u0027, None)"}],"source_content_type":"text/x-python","patch_set":35,"id":"f4b09130_4f4f223b","line":108,"range":{"start_line":108,"start_character":47,"end_line":108,"end_character":52},"updated":"2021-03-08 13:46:37.000000000","message":"Gah - nit: we build an* or we\u0027re building an accurate. I can propose a follow up to fix this.","commit_id":"31414b9f61ac4c923ede30a3c7e32662e9c14e1a"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2e1fd5448e9c507204f4b499ed45baba56db4e7d","unresolved":false,"context_lines":[{"line_number":105,"context_line":"            # update the project_id attribute of the target to match the owner."},{"line_number":106,"context_line":"            # If the target isn\u0027t of dict() type already, we should explicitly"},{"line_number":107,"context_line":"            # handle that here. We take a similar approach in the policy layer"},{"line_number":108,"context_line":"            # (glance.api.policy) to make sure we\u0027re build accurate image"},{"line_number":109,"context_line":"            # targets."},{"line_number":110,"context_line":"            target \u003d dict(target)"},{"line_number":111,"context_line":"        target[\u0027project_id\u0027] \u003d target.get(\u0027owner\u0027, None)"}],"source_content_type":"text/x-python","patch_set":35,"id":"dc974cc6_40ced7ae","line":108,"range":{"start_line":108,"start_character":47,"end_line":108,"end_character":52},"in_reply_to":"f4b09130_4f4f223b","updated":"2021-03-08 13:49:36.000000000","message":"Done\n\nhttps://review.opendev.org/c/openstack/glance/+/779260","commit_id":"31414b9f61ac4c923ede30a3c7e32662e9c14e1a"}],"glance/api/policy.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"fbfcda42222c384a41dd94b5a0d195f583f9a4c9","unresolved":true,"context_lines":[{"line_number":129,"context_line":"        target \u003d self._build_image_target(image)"},{"line_number":130,"context_line":"        try:"},{"line_number":131,"context_line":"            self.policy.enforce(self.context, \u0027get_image\u0027,"},{"line_number":132,"context_line":"                                target)"},{"line_number":133,"context_line":"        except exception.Forbidden:"},{"line_number":134,"context_line":"            # NOTE (abhishekk): Returning 404 Not Found as the"},{"line_number":135,"context_line":"            # image is outside of this user\u0027s project"}],"source_content_type":"text/x-python","patch_set":15,"id":"10948893_86d9a774","line":132,"range":{"start_line":132,"start_character":32,"end_line":132,"end_character":39},"updated":"2021-03-01 20:02:22.000000000","message":"nit, but I think this\u0027ll fit on the line above :)","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2c2e19626a2ee1057b69e984e39f0958e6318e50","unresolved":false,"context_lines":[{"line_number":129,"context_line":"        target \u003d self._build_image_target(image)"},{"line_number":130,"context_line":"        try:"},{"line_number":131,"context_line":"            self.policy.enforce(self.context, \u0027get_image\u0027,"},{"line_number":132,"context_line":"                                target)"},{"line_number":133,"context_line":"        except exception.Forbidden:"},{"line_number":134,"context_line":"            # NOTE (abhishekk): Returning 404 Not Found as the"},{"line_number":135,"context_line":"            # image is outside of this user\u0027s project"}],"source_content_type":"text/x-python","patch_set":15,"id":"7509a4dd_eea86762","line":132,"range":{"start_line":132,"start_character":32,"end_line":132,"end_character":39},"in_reply_to":"10948893_86d9a774","updated":"2021-03-01 21:13:35.000000000","message":"Done","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"fbfcda42222c384a41dd94b5a0d195f583f9a4c9","unresolved":true,"context_lines":[{"line_number":156,"context_line":"        # check strings. Using this approach forces the check_str to be more"},{"line_number":157,"context_line":"        # descriptive."},{"line_number":158,"context_line":"        members \u003d self.image_repo.db_api.image_member_find("},{"line_number":159,"context_line":"            self.context, image_id\u003dimage.image_id)"},{"line_number":160,"context_line":"        # FIXME(lbragstad): Remove this if statement if/when oslo.policy"},{"line_number":161,"context_line":"        # supports lists of target attributes via substitution, allowing us to"},{"line_number":162,"context_line":"        # do something like:"}],"source_content_type":"text/x-python","patch_set":15,"id":"164178e0_39aba510","line":159,"range":{"start_line":159,"start_character":12,"end_line":159,"end_character":24},"updated":"2021-03-01 20:02:22.000000000","message":"You\u0027re doing this with the user\u0027s context. Is there a chance they could have passed the image get on L128, but aren\u0027t able to see the members here and thus get a Forbidden when they would otherwise have been able to do something?","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2c2e19626a2ee1057b69e984e39f0958e6318e50","unresolved":true,"context_lines":[{"line_number":156,"context_line":"        # check strings. Using this approach forces the check_str to be more"},{"line_number":157,"context_line":"        # descriptive."},{"line_number":158,"context_line":"        members \u003d self.image_repo.db_api.image_member_find("},{"line_number":159,"context_line":"            self.context, image_id\u003dimage.image_id)"},{"line_number":160,"context_line":"        # FIXME(lbragstad): Remove this if statement if/when oslo.policy"},{"line_number":161,"context_line":"        # supports lists of target attributes via substitution, allowing us to"},{"line_number":162,"context_line":"        # do something like:"}],"source_content_type":"text/x-python","patch_set":15,"id":"434a9da1_4d0df789","line":159,"range":{"start_line":159,"start_character":12,"end_line":159,"end_character":24},"in_reply_to":"164178e0_39aba510","updated":"2021-03-01 21:13:35.000000000","message":"Good question. I probably should have put some of the detail here into the comment, because this isn\u0027t very easy to understand.\n\nThis calls directly into the database layer for image membership [0]. The only thing the DB API does with the context object is tack on a couple of filters on to the SQL query if the user is *not* an admin [1]. So, the DB API will ask for all image members if the user making the request is an admin. Otherwise, membership will be filtered by the owner of the image and the members of an image, based on the context.\n\nThis is slightly different from other DB API behavior where we do stuff like:\n\n  # somewhere in glance/db/sqlalchemy/*\n  if not _is_image_visible(context, image)\n      raise Forbidden(...)\n\nIf the image membership plumbing used that technique, we\u0027d absolutely need to handle that case here. But as far as I can tell and my understanding of glance-DB-API-context-munging, I don\u0027t think it does.\n\nI could be wrong, though.\n\nI think in the worst case, this just comes back as an empty list.\n\n[0] https://opendev.org/openstack/glance/src/commit/bdbad59dc9605afbb9178d79617ab54e78a9235d/glance/db/sqlalchemy/api.py#L1272-L1289\n[1] https://opendev.org/openstack/glance/src/commit/bdbad59dc9605afbb9178d79617ab54e78a9235d/glance/db/sqlalchemy/api.py#L1298","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"ad94332868fa0cc13e06d828316ee3f01996e0e7","unresolved":true,"context_lines":[{"line_number":156,"context_line":"        # check strings. Using this approach forces the check_str to be more"},{"line_number":157,"context_line":"        # descriptive."},{"line_number":158,"context_line":"        members \u003d self.image_repo.db_api.image_member_find("},{"line_number":159,"context_line":"            self.context, image_id\u003dimage.image_id)"},{"line_number":160,"context_line":"        # FIXME(lbragstad): Remove this if statement if/when oslo.policy"},{"line_number":161,"context_line":"        # supports lists of target attributes via substitution, allowing us to"},{"line_number":162,"context_line":"        # do something like:"}],"source_content_type":"text/x-python","patch_set":15,"id":"90df160b_623d924f","line":159,"range":{"start_line":159,"start_character":12,"end_line":159,"end_character":24},"in_reply_to":"434a9da1_4d0df789","updated":"2021-03-02 14:06:56.000000000","message":"Correct analysis by Lance;\nThis could be come back as empty list but will never raise Forbidden here.","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"c27fb8c4c2e3369cbaf9b62e9804e54a7ac9ccaa","unresolved":false,"context_lines":[{"line_number":156,"context_line":"        # check strings. Using this approach forces the check_str to be more"},{"line_number":157,"context_line":"        # descriptive."},{"line_number":158,"context_line":"        members \u003d self.image_repo.db_api.image_member_find("},{"line_number":159,"context_line":"            self.context, image_id\u003dimage.image_id)"},{"line_number":160,"context_line":"        # FIXME(lbragstad): Remove this if statement if/when oslo.policy"},{"line_number":161,"context_line":"        # supports lists of target attributes via substitution, allowing us to"},{"line_number":162,"context_line":"        # do something like:"}],"source_content_type":"text/x-python","patch_set":15,"id":"21dff9be_84d15e4d","line":159,"range":{"start_line":159,"start_character":12,"end_line":159,"end_character":24},"in_reply_to":"85e596d0_d6af860b","updated":"2021-03-02 15:39:31.000000000","message":"Done","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"de66c9caf4c9e538579db6b3879129f24fe682a6","unresolved":true,"context_lines":[{"line_number":156,"context_line":"        # check strings. Using this approach forces the check_str to be more"},{"line_number":157,"context_line":"        # descriptive."},{"line_number":158,"context_line":"        members \u003d self.image_repo.db_api.image_member_find("},{"line_number":159,"context_line":"            self.context, image_id\u003dimage.image_id)"},{"line_number":160,"context_line":"        # FIXME(lbragstad): Remove this if statement if/when oslo.policy"},{"line_number":161,"context_line":"        # supports lists of target attributes via substitution, allowing us to"},{"line_number":162,"context_line":"        # do something like:"}],"source_content_type":"text/x-python","patch_set":15,"id":"85e596d0_d6af860b","line":159,"range":{"start_line":159,"start_character":12,"end_line":159,"end_character":24},"in_reply_to":"90df160b_623d924f","updated":"2021-03-02 15:16:26.000000000","message":"Okay, sounds like we\u0027re covered, assuming that never changes.","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"fbfcda42222c384a41dd94b5a0d195f583f9a4c9","unresolved":true,"context_lines":[{"line_number":164,"context_line":"        # target[\u0027member_ids\u0027] \u003d set(m[\u0027member\u0027] for m in members)"},{"line_number":165,"context_line":"        for member in members:"},{"line_number":166,"context_line":"            if member[\u0027member\u0027] \u003d\u003d self.context.project_id:"},{"line_number":167,"context_line":"                target[\u0027member_id\u0027] \u003d member[\u0027member\u0027]"},{"line_number":168,"context_line":"        return target"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def list(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":15,"id":"4b8e65ff_eaa871cb","line":167,"updated":"2021-03-01 20:02:22.000000000","message":"Isn\u0027t this going to require the operator to put actual tenant uuids into policy files? Wouldn\u0027t it be better to make this something like:\n\n target[\u0027is_member\u0027] \u003d self.context.project_id in members\n\n? I\u0027m faaar from a policy guru, so maybe that\u0027s dumb, but I\u0027m just thinking about how someone would really go about this. Or is there some other higher-level thing going on with the persona stuff?","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"de66c9caf4c9e538579db6b3879129f24fe682a6","unresolved":true,"context_lines":[{"line_number":164,"context_line":"        # target[\u0027member_ids\u0027] \u003d set(m[\u0027member\u0027] for m in members)"},{"line_number":165,"context_line":"        for member in members:"},{"line_number":166,"context_line":"            if member[\u0027member\u0027] \u003d\u003d self.context.project_id:"},{"line_number":167,"context_line":"                target[\u0027member_id\u0027] \u003d member[\u0027member\u0027]"},{"line_number":168,"context_line":"        return target"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def list(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":15,"id":"68f2b192_eee70e71","line":167,"in_reply_to":"399f92b3_b0e3ad72","updated":"2021-03-02 15:16:26.000000000","message":"Actually I made up is_member just as an example.\n\nBut okay, re-looking at this this morning I think I\u0027m back on the same page. It is confusing to me to have policy rules with so many substitutable things in it, some of which happen in python ahead of time (i.e. f{foo} and %(foo)s) and some of which happen at policy eval time (i.e. an unmarked project_id or similar). So I was worried that if the admin wants to change the default they would need to put uuids in there in places where we do the substs. But, I realize now where I was missing the step.","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2c2e19626a2ee1057b69e984e39f0958e6318e50","unresolved":true,"context_lines":[{"line_number":164,"context_line":"        # target[\u0027member_ids\u0027] \u003d set(m[\u0027member\u0027] for m in members)"},{"line_number":165,"context_line":"        for member in members:"},{"line_number":166,"context_line":"            if member[\u0027member\u0027] \u003d\u003d self.context.project_id:"},{"line_number":167,"context_line":"                target[\u0027member_id\u0027] \u003d member[\u0027member\u0027]"},{"line_number":168,"context_line":"        return target"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def list(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":15,"id":"9e4d94cc_4b41f8cb","line":167,"in_reply_to":"4b8e65ff_eaa871cb","updated":"2021-03-01 21:13:35.000000000","message":"\u003e Isn\u0027t this going to require the operator to put actual tenant uuids into policy files?\n\nThe policy engine will substitute attributes from the target into the check string. We include this here because the new default policy uses:\n\n  (role:reader and project_id:%(member_id)s)\n\nSo, if we were to call this API as project-members of project foo, where foo is the project ID, it would substitute in the following:\n\n  (role:reader and $PROJECT_ID_FROM_CONTEXT:$MEMBER_ID_FROM_TARGET)\n  (role:reader and foo:$MEMBER_ID_FROM_TARGET)\n  (role:reader and foo:foo)\n\nThe second expression will evaluate to True since \u0027foo\u0027 \u003d\u003d \u0027foo\u0027. The roles from the context object get cycled through and matched against \u0027reader\u0027.\n\nSince we\u0027re building a valid target dictionary and the check string uses substitution, we don\u0027t require operators to update policy files, or match project IDs.\n\n\u003e  Wouldn\u0027t it be better to make this something like:\n\u003e \n\u003e  target[\u0027is_member\u0027] \u003d self.context.project_id in members\n\u003e \n\u003e ? I\u0027m faaar from a policy guru, so maybe that\u0027s dumb, but I\u0027m just thinking about how someone would really go about this. Or is there some other higher-level thing going on with the persona stuff?\n\nYeah - that does seem pretty clear when reading the policy.\n\nThen we\u0027d need to update the check string to use [0]:\n\n  (role:reader and True:is_member)\n\nI just might need to dig through the code to figure out what is_member means or is?\n\n[0] https://docs.openstack.org/oslo.policy/latest/reference/api/oslo_policy.html#policy-rule-expressions","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"c27fb8c4c2e3369cbaf9b62e9804e54a7ac9ccaa","unresolved":false,"context_lines":[{"line_number":164,"context_line":"        # target[\u0027member_ids\u0027] \u003d set(m[\u0027member\u0027] for m in members)"},{"line_number":165,"context_line":"        for member in members:"},{"line_number":166,"context_line":"            if member[\u0027member\u0027] \u003d\u003d self.context.project_id:"},{"line_number":167,"context_line":"                target[\u0027member_id\u0027] \u003d member[\u0027member\u0027]"},{"line_number":168,"context_line":"        return target"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def list(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":15,"id":"f4e57e2d_73c87997","line":167,"in_reply_to":"68f2b192_eee70e71","updated":"2021-03-02 15:39:31.000000000","message":"Done","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"ad94332868fa0cc13e06d828316ee3f01996e0e7","unresolved":true,"context_lines":[{"line_number":164,"context_line":"        # target[\u0027member_ids\u0027] \u003d set(m[\u0027member\u0027] for m in members)"},{"line_number":165,"context_line":"        for member in members:"},{"line_number":166,"context_line":"            if member[\u0027member\u0027] \u003d\u003d self.context.project_id:"},{"line_number":167,"context_line":"                target[\u0027member_id\u0027] \u003d member[\u0027member\u0027]"},{"line_number":168,"context_line":"        return target"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def list(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":15,"id":"399f92b3_b0e3ad72","line":167,"in_reply_to":"9e4d94cc_4b41f8cb","updated":"2021-03-02 14:06:56.000000000","message":"AFAIK, is_member is defined in db/simple/api.py which we used to invoke db api\u0027s in unit testing. \n\nhttps://github.com/openstack/glance/blob/master/glance/db/simple/api.py#L272\n\nNOTE: Glance uses sqlite db along with simple_api in unit tests.","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"fbfcda42222c384a41dd94b5a0d195f583f9a4c9","unresolved":true,"context_lines":[{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def list(self, *args, **kwargs):"},{"line_number":171,"context_line":"        target \u003d {\u0027project_id\u0027: self.context.project_id}"},{"line_number":172,"context_line":"        self.policy.enforce(self.context, \u0027get_images\u0027, target)"},{"line_number":173,"context_line":"        return super(ImageRepoProxy, self).list(*args, **kwargs)"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    def save(self, image, from_state\u003dNone):"}],"source_content_type":"text/x-python","patch_set":15,"id":"a38185bd_0175d45b","line":172,"range":{"start_line":172,"start_character":56,"end_line":172,"end_character":62},"updated":"2021-03-01 20:02:22.000000000","message":"Elsewhere the target is an imageish thing. Here, you\u0027re providing a dict (which maybe is supposed to look like a piece of an image?) with a project id always matching ourselves. I\u0027m struggling to think of what policy rules would look like there.","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"c27fb8c4c2e3369cbaf9b62e9804e54a7ac9ccaa","unresolved":true,"context_lines":[{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def list(self, *args, **kwargs):"},{"line_number":171,"context_line":"        target \u003d {\u0027project_id\u0027: self.context.project_id}"},{"line_number":172,"context_line":"        self.policy.enforce(self.context, \u0027get_images\u0027, target)"},{"line_number":173,"context_line":"        return super(ImageRepoProxy, self).list(*args, **kwargs)"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    def save(self, image, from_state\u003dNone):"}],"source_content_type":"text/x-python","patch_set":15,"id":"43dd1224_40352fef","line":172,"range":{"start_line":172,"start_character":56,"end_line":172,"end_character":62},"in_reply_to":"832f6f48_0fdd48a7","updated":"2021-03-02 15:39:31.000000000","message":"++\n\nThe more I think about it, the more I like the idea of implementing policy enforcement and filtering in one fell swoop.\n\nI added a comment to clarify how to do that, but I can take a stab at implementing it if folks are leaning that way.","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2c2e19626a2ee1057b69e984e39f0958e6318e50","unresolved":true,"context_lines":[{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def list(self, *args, **kwargs):"},{"line_number":171,"context_line":"        target \u003d {\u0027project_id\u0027: self.context.project_id}"},{"line_number":172,"context_line":"        self.policy.enforce(self.context, \u0027get_images\u0027, target)"},{"line_number":173,"context_line":"        return super(ImageRepoProxy, self).list(*args, **kwargs)"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    def save(self, image, from_state\u003dNone):"}],"source_content_type":"text/x-python","patch_set":15,"id":"e542f3e1_c0aa0dbc","line":172,"range":{"start_line":172,"start_character":56,"end_line":172,"end_character":62},"in_reply_to":"a38185bd_0175d45b","updated":"2021-03-01 21:13:35.000000000","message":"If I understand your question, the policy rule would be the same as other target cases in this file.\n\nThis is a noop to get the policy to pass because we don\u0027t have a reasonable thing to use for the target when we\u0027re listing all images, short of listing images, calling enforcement on each image, and building the return list based on the enforcement outcome (append if enforcement \u003d\u003d True, otherwise continue).\n\nThis works without implementing filter here because the database builds really specific SQL queries for listing images, based on the context [0][1][2].\n\nWe could implement filtering here, in the policy layer, as a catch all to make sure we don\u0027t leak projects. That might be something we need to do if we ever decide to make the DB API really dumb and not require context objects.\n\n[0] https://opendev.org/openstack/glance/src/branch/master/glance/db/sqlalchemy/api.py#L608\n[1] https://opendev.org/openstack/glance/src/branch/master/glance/db/sqlalchemy/api.py#L654\n[2] https://opendev.org/openstack/glance/src/branch/master/glance/db/sqlalchemy/api.py#L566-L605","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"de66c9caf4c9e538579db6b3879129f24fe682a6","unresolved":true,"context_lines":[{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def list(self, *args, **kwargs):"},{"line_number":171,"context_line":"        target \u003d {\u0027project_id\u0027: self.context.project_id}"},{"line_number":172,"context_line":"        self.policy.enforce(self.context, \u0027get_images\u0027, target)"},{"line_number":173,"context_line":"        return super(ImageRepoProxy, self).list(*args, **kwargs)"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"    def save(self, image, from_state\u003dNone):"}],"source_content_type":"text/x-python","patch_set":15,"id":"832f6f48_0fdd48a7","line":172,"range":{"start_line":172,"start_character":56,"end_line":172,"end_character":62},"in_reply_to":"e542f3e1_c0aa0dbc","updated":"2021-03-02 15:16:26.000000000","message":"\u003e This is a noop to get the policy to pass because we don\u0027t have a reasonable thing to use for the target when we\u0027re listing all images, short of listing images, calling enforcement on each image, and building the return list based on the enforcement outcome (append if enforcement \u003d\u003d True, otherwise continue).\n\nYeah, this was what I was going after: that this is a NOP (i.e. a nice name for a hack).\n\nCould we maybe get a comment here exposing that? If I\u0027m trying to debug why something is or isn\u0027t showing up, or any other sort of seemingly-permission-related thing, I might expect to come here and be confused about what I\u0027m seeing, and/or think it\u0027s broken.","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"8c985c3cec9210a14359e62e62e7f349529fbdf6","unresolved":true,"context_lines":[{"line_number":164,"context_line":"        # target[\u0027member_ids\u0027] \u003d set(m[\u0027member\u0027] for m in members)"},{"line_number":165,"context_line":"        for member in members:"},{"line_number":166,"context_line":"            if member[\u0027member\u0027] \u003d\u003d self.context.project_id:"},{"line_number":167,"context_line":"                target[\u0027member_id\u0027] \u003d member[\u0027member\u0027]"},{"line_number":168,"context_line":"        return target"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def list(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":23,"id":"de02e0a0_d71cefd9","line":167,"updated":"2021-03-03 15:19:02.000000000","message":"We probably should break out of the for loop here as we have found what we were looking for.","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9ebdd108ce3fa61457e594fc44980adba35594c1","unresolved":false,"context_lines":[{"line_number":164,"context_line":"        # target[\u0027member_ids\u0027] \u003d set(m[\u0027member\u0027] for m in members)"},{"line_number":165,"context_line":"        for member in members:"},{"line_number":166,"context_line":"            if member[\u0027member\u0027] \u003d\u003d self.context.project_id:"},{"line_number":167,"context_line":"                target[\u0027member_id\u0027] \u003d member[\u0027member\u0027]"},{"line_number":168,"context_line":"        return target"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"    def list(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":23,"id":"407374a9_00a608a4","line":167,"in_reply_to":"de02e0a0_d71cefd9","updated":"2021-03-04 04:25:17.000000000","message":"Done","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"8c985c3cec9210a14359e62e62e7f349529fbdf6","unresolved":true,"context_lines":[{"line_number":178,"context_line":"        # each image, and then iterate over each image and call policy"},{"line_number":179,"context_line":"        # enforcement. If the user passes policy enforcement, append the image"},{"line_number":180,"context_line":"        # to the list of filtered images. If not, the image should be removed"},{"line_number":181,"context_line":"        # from the list of images returned to the user."},{"line_number":182,"context_line":"        target \u003d {\u0027project_id\u0027: self.context.project_id}"},{"line_number":183,"context_line":"        self.policy.enforce(self.context, \u0027get_images\u0027, target)"},{"line_number":184,"context_line":"        return super(ImageRepoProxy, self).list(*args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":23,"id":"363c7a6d_c456d39f","line":181,"updated":"2021-03-03 15:19:02.000000000","message":"Is this just to accept the list API call and the content of it will not be subject to policies?  This might need some poking and testing from myself to understand correctly.\n\nI\u0027m mainly curious with this dance we need to do, are we letting the user to see detailed image list even if they don\u0027t have rights to actually view the image but to list it?","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"e36af1c5ec56bb7abff58d8c5f95b53f4eedce6b","unresolved":true,"context_lines":[{"line_number":178,"context_line":"        # each image, and then iterate over each image and call policy"},{"line_number":179,"context_line":"        # enforcement. If the user passes policy enforcement, append the image"},{"line_number":180,"context_line":"        # to the list of filtered images. If not, the image should be removed"},{"line_number":181,"context_line":"        # from the list of images returned to the user."},{"line_number":182,"context_line":"        target \u003d {\u0027project_id\u0027: self.context.project_id}"},{"line_number":183,"context_line":"        self.policy.enforce(self.context, \u0027get_images\u0027, target)"},{"line_number":184,"context_line":"        return super(ImageRepoProxy, self).list(*args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":23,"id":"8142e448_df75c9df","line":181,"in_reply_to":"363c7a6d_c456d39f","updated":"2021-03-03 16:01:31.000000000","message":"Correct - the faux target is just to get the policy to pass because we don\u0027t pass in all the images as targets and oslo.policy won\u0027t handle that.\n\nThe point the second part of this comment is a suggestion to how we can use policy enforcement to kill two birds with one stone. The first bird would be even seeing if the user is allowed to list images. The second would be using the policy engine and policy check string to filter images to only what a user can see.\n\nI attempted to poke this behavior in full integration tests.\n\nhttps://review.opendev.org/c/openstack/glance-tempest-plugin/+/773568/27/glance_tempest_plugin/tests/rbac/v2/test_images.py@626","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"8c985c3cec9210a14359e62e62e7f349529fbdf6","unresolved":true,"context_lines":[{"line_number":502,"context_line":"        self.target \u003d target"},{"line_number":503,"context_line":"        self._target_keys \u003d [k for k in dir(ImageProxy)"},{"line_number":504,"context_line":"                             if not k.startswith(\u0027__\u0027)"},{"line_number":505,"context_line":"                             if not k \u003d\u003d \u0027locations\u0027"},{"line_number":506,"context_line":"                             if not callable(getattr(ImageProxy, k))]"},{"line_number":507,"context_line":""},{"line_number":508,"context_line":"    def __getitem__(self, key):"}],"source_content_type":"text/x-python","patch_set":23,"id":"24d20ed5_56faa608","line":505,"updated":"2021-03-03 15:19:02.000000000","message":"Why are we ignoring locations from the check?","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"e36af1c5ec56bb7abff58d8c5f95b53f4eedce6b","unresolved":true,"context_lines":[{"line_number":502,"context_line":"        self.target \u003d target"},{"line_number":503,"context_line":"        self._target_keys \u003d [k for k in dir(ImageProxy)"},{"line_number":504,"context_line":"                             if not k.startswith(\u0027__\u0027)"},{"line_number":505,"context_line":"                             if not k \u003d\u003d \u0027locations\u0027"},{"line_number":506,"context_line":"                             if not callable(getattr(ImageProxy, k))]"},{"line_number":507,"context_line":""},{"line_number":508,"context_line":"    def __getitem__(self, key):"}],"source_content_type":"text/x-python","patch_set":23,"id":"fcd4d420_89a8d898","line":505,"in_reply_to":"24d20ed5_56faa608","updated":"2021-03-03 16:01:31.000000000","message":"These aren\u0027t serialized as anything oslo.policy can use. They\u0027re python objects and oslo.policy doesn\u0027t know what to do with them.\n\nWe could fix this by translating them somehow, but that might only be useful if we expect operators to write custom policy overrides using the location attribute.","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9ebdd108ce3fa61457e594fc44980adba35594c1","unresolved":false,"context_lines":[{"line_number":502,"context_line":"        self.target \u003d target"},{"line_number":503,"context_line":"        self._target_keys \u003d [k for k in dir(ImageProxy)"},{"line_number":504,"context_line":"                             if not k.startswith(\u0027__\u0027)"},{"line_number":505,"context_line":"                             if not k \u003d\u003d \u0027locations\u0027"},{"line_number":506,"context_line":"                             if not callable(getattr(ImageProxy, k))]"},{"line_number":507,"context_line":""},{"line_number":508,"context_line":"    def __getitem__(self, key):"}],"source_content_type":"text/x-python","patch_set":23,"id":"4c35a9be_eae3a34d","line":505,"in_reply_to":"62f71084_c1a14aa2","updated":"2021-03-04 04:25:17.000000000","message":"I added a comment to make this more clear.","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"001cbf932145b7cdffd252b7cae17b1bbb587958","unresolved":true,"context_lines":[{"line_number":502,"context_line":"        self.target \u003d target"},{"line_number":503,"context_line":"        self._target_keys \u003d [k for k in dir(ImageProxy)"},{"line_number":504,"context_line":"                             if not k.startswith(\u0027__\u0027)"},{"line_number":505,"context_line":"                             if not k \u003d\u003d \u0027locations\u0027"},{"line_number":506,"context_line":"                             if not callable(getattr(ImageProxy, k))]"},{"line_number":507,"context_line":""},{"line_number":508,"context_line":"    def __getitem__(self, key):"}],"source_content_type":"text/x-python","patch_set":23,"id":"62f71084_c1a14aa2","line":505,"in_reply_to":"fcd4d420_89a8d898","updated":"2021-03-03 18:47:14.000000000","message":"Interesting, this must get them from some part that is still proxying them as objects. I thought on the level we hit the target the locations would be just clean list. I doubt there is need for location specific overrides so this should be fine then.","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"dbc8e8927c6381eb2e92bc1c9631b40d06620f7d","unresolved":true,"context_lines":[{"line_number":511,"context_line":"                             # before we call enforcement with target"},{"line_number":512,"context_line":"                             # information. Omitting for not until that is a"},{"line_number":513,"context_line":"                             # necessity."},{"line_number":514,"context_line":"                             if not k \u003d\u003d \u0027locations\u0027"},{"line_number":515,"context_line":"                             if not callable(getattr(ImageProxy, k))]"},{"line_number":516,"context_line":""},{"line_number":517,"context_line":"    def __getitem__(self, key):"}],"source_content_type":"text/x-python","patch_set":33,"id":"1c89e84b_5a5d2ef2","line":514,"updated":"2021-03-07 16:24:07.000000000","message":"We may test this implicitly, but I don\u0027t think we\u0027re testing it explicitly. Maybe we can just have a test that passes something simple to ImageTarget and makes sure that locations isn\u0027t in there?","commit_id":"8b07f09af549df39f4a893ab8736aee880660ef0"}],"glance/api/v2/image_data.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"fbfcda42222c384a41dd94b5a0d195f583f9a4c9","unresolved":true,"context_lines":[{"line_number":137,"context_line":"        refresher \u003d None"},{"line_number":138,"context_line":"        cxt \u003d req.context"},{"line_number":139,"context_line":"        try:"},{"line_number":140,"context_line":"            self.policy.enforce(cxt, \u0027upload_image\u0027, {})"},{"line_number":141,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":142,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":143,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":15,"id":"be5dda80_44e2bbbd","side":"PARENT","line":140,"range":{"start_line":140,"start_character":53,"end_line":140,"end_character":55},"updated":"2021-03-01 20:02:22.000000000","message":"So, I\u0027m getting that before this, we allow upload if (a) you have upload permission at all, and (b) if you don\u0027t hit forbidden on L141 below, right? That\u0027s far from what I would have thought :)","commit_id":"09dd275b1a3c27c5143bc3332fcd2dc60d32c968"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"de66c9caf4c9e538579db6b3879129f24fe682a6","unresolved":true,"context_lines":[{"line_number":137,"context_line":"        refresher \u003d None"},{"line_number":138,"context_line":"        cxt \u003d req.context"},{"line_number":139,"context_line":"        try:"},{"line_number":140,"context_line":"            self.policy.enforce(cxt, \u0027upload_image\u0027, {})"},{"line_number":141,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":142,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":143,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":15,"id":"7715055a_16cb396f","side":"PARENT","line":140,"range":{"start_line":140,"start_character":53,"end_line":140,"end_character":55},"in_reply_to":"2efbc65d_067ca1f5","updated":"2021-03-02 15:16:26.000000000","message":"Yeah, this seems a little concerning to me...but that\u0027s not your problem.","commit_id":"09dd275b1a3c27c5143bc3332fcd2dc60d32c968"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2c2e19626a2ee1057b69e984e39f0958e6318e50","unresolved":true,"context_lines":[{"line_number":137,"context_line":"        refresher \u003d None"},{"line_number":138,"context_line":"        cxt \u003d req.context"},{"line_number":139,"context_line":"        try:"},{"line_number":140,"context_line":"            self.policy.enforce(cxt, \u0027upload_image\u0027, {})"},{"line_number":141,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":142,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":143,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":15,"id":"2efbc65d_067ca1f5","side":"PARENT","line":140,"range":{"start_line":140,"start_character":53,"end_line":140,"end_character":55},"in_reply_to":"be5dda80_44e2bbbd","updated":"2021-03-01 21:13:35.000000000","message":"Yeah - both upload_image and get_image default to open[0][1][2]. The majority of the authorization logic is in the database utilities [3], which raise NotFound and Forbidden exceptions.\n\nI think most of that utility logic could be replaced with a check string that checks valid target attributes. For example something like:\n\n  (role:member and (project_id:%(project_id)s or community:%(visibility)s))\n\n[0] https://opendev.org/openstack/glance/src/commit/bdbad59dc9605afbb9178d79617ab54e78a9235d/glance/policies/image.py#L26\n[1] https://opendev.org/openstack/glance/src/commit/bdbad59dc9605afbb9178d79617ab54e78a9235d/glance/policies/base.py#L27\n[2] https://opendev.org/openstack/glance/src/commit/bdbad59dc9605afbb9178d79617ab54e78a9235d/glance/policies/image.py#L19\n[3] https://opendev.org/openstack/glance/src/commit/bdbad59dc9605afbb9178d79617ab54e78a9235d/glance/db/sqlalchemy/api.py#L263-L291","commit_id":"09dd275b1a3c27c5143bc3332fcd2dc60d32c968"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"fbfcda42222c384a41dd94b5a0d195f583f9a4c9","unresolved":true,"context_lines":[{"line_number":139,"context_line":"        try:"},{"line_number":140,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":141,"context_line":"            target \u003d {\u0027project_id\u0027: image.owner}"},{"line_number":142,"context_line":"            self.policy.enforce(cxt, \u0027upload_image\u0027, target)"},{"line_number":143,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":144,"context_line":"            try:"},{"line_number":145,"context_line":"                # create a trust if backend is registry"}],"source_content_type":"text/x-python","patch_set":15,"id":"54d7bb9e_589bca2e","line":142,"updated":"2021-03-01 20:02:22.000000000","message":"So does this create an upgrade problem for operators if they had previously granted upload to users with a rule in some way that it will not longer pass with this target?","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2c2e19626a2ee1057b69e984e39f0958e6318e50","unresolved":true,"context_lines":[{"line_number":139,"context_line":"        try:"},{"line_number":140,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":141,"context_line":"            target \u003d {\u0027project_id\u0027: image.owner}"},{"line_number":142,"context_line":"            self.policy.enforce(cxt, \u0027upload_image\u0027, target)"},{"line_number":143,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":144,"context_line":"            try:"},{"line_number":145,"context_line":"                # create a trust if backend is registry"}],"source_content_type":"text/x-python","patch_set":15,"id":"c531c785_a9c5fccb","line":142,"in_reply_to":"54d7bb9e_589bca2e","updated":"2021-03-01 21:13:35.000000000","message":"I don\u0027t think so? Only because prior to this change the target dictionary was always empty, so any substitution would have failed.\n\nIf I were an operator and I wanted to supply a custom policy for upload_image like:\n\n  \"upload_image\": \"(rule:admin_api) or (role:member and project_id:%(project_id)s)\"\n\nI don\u0027t think that would have ever passed for project-members since the target data was always empty, making project_id substitution impossible to match with a context.project_id.\n\nIs there a different check string case you\u0027re thinking of that I\u0027m glossing over?","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"de66c9caf4c9e538579db6b3879129f24fe682a6","unresolved":true,"context_lines":[{"line_number":139,"context_line":"        try:"},{"line_number":140,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":141,"context_line":"            target \u003d {\u0027project_id\u0027: image.owner}"},{"line_number":142,"context_line":"            self.policy.enforce(cxt, \u0027upload_image\u0027, target)"},{"line_number":143,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":144,"context_line":"            try:"},{"line_number":145,"context_line":"                # create a trust if backend is registry"}],"source_content_type":"text/x-python","patch_set":15,"id":"c9a35819_fe47a2ea","line":142,"in_reply_to":"c531c785_a9c5fccb","updated":"2021-03-02 15:16:26.000000000","message":"\u003e I don\u0027t think so? Only because prior to this change the target dictionary was always empty, so any substitution would have failed.\n\u003e \n\u003e If I were an operator and I wanted to supply a custom policy for upload_image like:\n\u003e \n\u003e   \"upload_image\": \"(rule:admin_api) or (role:member and project_id:%(project_id)s)\"\n\u003e \n\u003e I don\u0027t think that would have ever passed for project-members since the target data was always empty, making project_id substitution impossible to match with a context.project_id.\n\u003e \n\u003e Is there a different check string case you\u0027re thinking of that I\u0027m glossing over?\n\nI guess the policy stuff is so confusing to me that I would not have assumed the above. It\u0027s not clear to me which things in the rule are tokens from the target, properties of the context, or just text that can be ignored. So I would have thought that it was possible to write a rule that did something against context or environment, but not against the target, which we\u0027d be breaking here. If not, I\u0027ll trust your assessment and agree that if _any_ rule here could never work, then we can\u0027t be breaking anyone.","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"99392c16530fc81503c45e057c4777ee1c2a9bc4","unresolved":true,"context_lines":[{"line_number":139,"context_line":"        try:"},{"line_number":140,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":141,"context_line":"            target \u003d {\u0027project_id\u0027: image.owner}"},{"line_number":142,"context_line":"            self.policy.enforce(cxt, \u0027upload_image\u0027, target)"},{"line_number":143,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":144,"context_line":"            try:"},{"line_number":145,"context_line":"                # create a trust if backend is registry"}],"source_content_type":"text/x-python","patch_set":19,"id":"0871ca71_807bfd3a","line":142,"updated":"2021-03-02 15:35:13.000000000","message":"Okay, sorry, I\u0027m still struggling with making sure this is not an upgrade problem.\n\nBefore this, I could upload to any image I can see that is in the right state, correct? So that could be something shared with me via members, or even maybe a public image (really?) that is not yet in the active state.\n\nSince you\u0027re not passing a full target here, we can\u0027t do the members check, so doesn\u0027t this limit me to only upload to images when I am the owner?","commit_id":"db31004f723c601bae58e41fa7bd07bf7492b271"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"cecffe25618cf9a4ea07ff00f6d10f8b2642cfb9","unresolved":true,"context_lines":[{"line_number":139,"context_line":"        try:"},{"line_number":140,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":141,"context_line":"            target \u003d {\u0027project_id\u0027: image.owner}"},{"line_number":142,"context_line":"            self.policy.enforce(cxt, \u0027upload_image\u0027, target)"},{"line_number":143,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":144,"context_line":"            try:"},{"line_number":145,"context_line":"                # create a trust if backend is registry"}],"source_content_type":"text/x-python","patch_set":19,"id":"f7af5ed5_0436cc86","line":142,"in_reply_to":"0871ca71_807bfd3a","updated":"2021-03-02 16:53:51.000000000","message":"\u003e Okay, sorry, I\u0027m still struggling with making sure this is not an upgrade problem.\n\u003e \n\u003e Before this, I could upload to any image I can see that is in the right state, correct? So that could be something shared with me via members, or even maybe a public image (really?) that is not yet in the active state.\n\nActually, I think that would trip over this:\n\nhttps://opendev.org/openstack/glance/src/branch/master/glance/db/sqlalchemy/api.py#L132-L142\n\n\u003e \n\u003e Since you\u0027re not passing a full target here, we can\u0027t do the members check, so doesn\u0027t this limit me to only upload to images when I am the owner?\n\nThis last bit makes sense to me, but I can see how the new policy isn\u0027t really clear for the upload case. I think it works because the database layer still does too much validation on the request (IMHO).\n\nI can update the policy for upload_image to be:\n\n  \"upload_image\": \"role:admin or (role:member and project_id:%(project_id)s)\"\n\nSince right now it\u0027s including attribute checks for visibility, which certainly don\u0027t make sense for upload.","commit_id":"db31004f723c601bae58e41fa7bd07bf7492b271"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"154ca644bbb0e38602e1801f1bf3d6c4307d4915","unresolved":true,"context_lines":[{"line_number":139,"context_line":"        try:"},{"line_number":140,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":141,"context_line":"            target \u003d {\u0027project_id\u0027: image.owner}"},{"line_number":142,"context_line":"            self.policy.enforce(cxt, \u0027upload_image\u0027, target)"},{"line_number":143,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":144,"context_line":"            try:"},{"line_number":145,"context_line":"                # create a trust if backend is registry"}],"source_content_type":"text/x-python","patch_set":19,"id":"909f3dd3_80c366c0","line":142,"in_reply_to":"0871ca71_807bfd3a","updated":"2021-03-02 16:20:40.000000000","message":"I think this is a valid concern for upgrade case.","commit_id":"db31004f723c601bae58e41fa7bd07bf7492b271"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ba595c2eece9c604fb42e9b8d26e7faee50f1ff7","unresolved":true,"context_lines":[{"line_number":139,"context_line":"        try:"},{"line_number":140,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":141,"context_line":"            target \u003d {\u0027project_id\u0027: image.owner}"},{"line_number":142,"context_line":"            self.policy.enforce(cxt, \u0027upload_image\u0027, target)"},{"line_number":143,"context_line":"            image.status \u003d \u0027saving\u0027"},{"line_number":144,"context_line":"            try:"},{"line_number":145,"context_line":"                # create a trust if backend is registry"}],"source_content_type":"text/x-python","patch_set":19,"id":"3135096c_363312e3","line":142,"in_reply_to":"f7af5ed5_0436cc86","updated":"2021-03-02 17:09:00.000000000","message":"\u003e Actually, I think that would trip over this:\n\u003e \n\u003e https://opendev.org/openstack/glance/src/branch/master/glance/db/sqlalchemy/api.py#L132-L142\n\nHmm, okay I guess you\u0027re thinking that the image.save() with the new status\u003d\u0027saving\u0027 would keep us from proceeding?\n\n\u003e This last bit makes sense to me, but I can see how the new policy isn\u0027t really clear for the upload case. I think it works because the database layer still does too much validation on the request (IMHO).\n\nOkay, but I think is_image_mutable() (called from check_mutate_authorization()) actually already prevents the case I was talking about, but obscurely (or at least, not like I was expecting) as above, in that we just can\u0027t change the status of it. So maybe it\u0027s not a problem. It\u0027s just one of those things where you\u0027re changing the ordering of the authorization check, once of which is effectively always a pass, so I just want to make sure we\u0027re not opening up something we shouldn\u0027t, or closing off something that was previously allowed.\n\n\u003e I can update the policy for upload_image to be:\n\u003e \n\u003e   \"upload_image\": \"role:admin or (role:member and project_id:%(project_id)s)\"\n\u003e \n\u003e Since right now it\u0027s including attribute checks for visibility, which certainly don\u0027t make sense for upload.\n\nWhy doesn\u0027t it make sense? I think the image gets a visibility at creation time, such that here during upload we can already consider it.","commit_id":"db31004f723c601bae58e41fa7bd07bf7492b271"}],"glance/policies/base.py":[{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9098f4ff02d3d714cfd81bd48bcd2a4718a5b4cf","unresolved":true,"context_lines":[{"line_number":33,"context_line":"# more than one scope."},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"SYSTEM_ADMIN_OR_PROJECT_MEMBER \u003d \u0027(%s or %s)\u0027 % (SYSTEM_ADMIN, PROJECT_MEMBER)"},{"line_number":36,"context_line":"SYSTEM_OR_PROJECT_READER \u003d \u0027(%s or %s)\u0027 % (SYSTEM_READER, PROJECT_READER)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"rules \u003d ["}],"source_content_type":"text/x-python","patch_set":3,"id":"961764df_60642e81","line":36,"updated":"2021-02-03 17:42:12.000000000","message":"I\u0027m going to pull these into the previous patch so that we have a single place where we\u0027re introducing common personas.","commit_id":"be45a7728307471e2b5f665a3779e30efdc0121e"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"a795e9b1743f140cb6e04e08de4aabed79f16fec","unresolved":false,"context_lines":[{"line_number":33,"context_line":"# more than one scope."},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"SYSTEM_ADMIN_OR_PROJECT_MEMBER \u003d \u0027(%s or %s)\u0027 % (SYSTEM_ADMIN, PROJECT_MEMBER)"},{"line_number":36,"context_line":"SYSTEM_OR_PROJECT_READER \u003d \u0027(%s or %s)\u0027 % (SYSTEM_READER, PROJECT_READER)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"rules \u003d ["}],"source_content_type":"text/x-python","patch_set":3,"id":"aa26c258_b0b0e548","line":36,"in_reply_to":"961764df_60642e81","updated":"2021-02-03 18:07:43.000000000","message":"Done","commit_id":"be45a7728307471e2b5f665a3779e30efdc0121e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"fbfcda42222c384a41dd94b5a0d195f583f9a4c9","unresolved":true,"context_lines":[{"line_number":45,"context_line":"# strings to offer formal support for project membership and a read-only"},{"line_number":46,"context_line":"# variant consistent with other OpenStack services."},{"line_number":47,"context_line":"ADMIN_OR_PROJECT_MEMBER \u003d f\u0027role:admin or ({PROJECT_MEMBER})\u0027"},{"line_number":48,"context_line":"ADMIN_OR_PROJECT_READER \u003d f\u0027role:admin or ({PROJECT_READER})\u0027"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"rules \u003d ["}],"source_content_type":"text/x-python","patch_set":15,"id":"59fcc682_727a9379","line":48,"updated":"2021-03-01 20:02:22.000000000","message":"EW, GROSS.\n\n(Don\u0027t change this, it\u0027s just a knee-jerk reaction. Since this will fail at compile time if the constants above are removed, this may be the one good use of these things).","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2c2e19626a2ee1057b69e984e39f0958e6318e50","unresolved":false,"context_lines":[{"line_number":45,"context_line":"# strings to offer formal support for project membership and a read-only"},{"line_number":46,"context_line":"# variant consistent with other OpenStack services."},{"line_number":47,"context_line":"ADMIN_OR_PROJECT_MEMBER \u003d f\u0027role:admin or ({PROJECT_MEMBER})\u0027"},{"line_number":48,"context_line":"ADMIN_OR_PROJECT_READER \u003d f\u0027role:admin or ({PROJECT_READER})\u0027"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"rules \u003d ["}],"source_content_type":"text/x-python","patch_set":15,"id":"cdcee394_3c3e9b06","line":48,"in_reply_to":"59fcc682_727a9379","updated":"2021-03-01 21:13:35.000000000","message":"Ack","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"8c985c3cec9210a14359e62e62e7f349529fbdf6","unresolved":true,"context_lines":[{"line_number":16,"context_line":"# project, specifically with the member role."},{"line_number":17,"context_line":"PROJECT_MEMBER \u003d ("},{"line_number":18,"context_line":"    \u0027role:member and (project_id:%(project_id)s or project_id:%(member_id)s \u0027"},{"line_number":19,"context_line":"    \u0027or \"community\":%(visibility)s or \"public\":%(visibility)s)\u0027"},{"line_number":20,"context_line":")"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"# Generic check string for checking if a user is authorized on a particular"}],"source_content_type":"text/x-python","patch_set":23,"id":"7c9a6045_e142f837","line":19,"updated":"2021-03-03 15:19:02.000000000","message":"Correct me if I\u0027m wrong but doesn\u0027t this treat user as member of the owning project as soon as the image visibility is \u0027community\u0027 or \u0027public\u0027 and the user is just member of some project?","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"e36af1c5ec56bb7abff58d8c5f95b53f4eedce6b","unresolved":true,"context_lines":[{"line_number":16,"context_line":"# project, specifically with the member role."},{"line_number":17,"context_line":"PROJECT_MEMBER \u003d ("},{"line_number":18,"context_line":"    \u0027role:member and (project_id:%(project_id)s or project_id:%(member_id)s \u0027"},{"line_number":19,"context_line":"    \u0027or \"community\":%(visibility)s or \"public\":%(visibility)s)\u0027"},{"line_number":20,"context_line":")"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"# Generic check string for checking if a user is authorized on a particular"}],"source_content_type":"text/x-python","patch_set":23,"id":"fef6741f_39a1c8e5","line":19,"in_reply_to":"7c9a6045_e142f837","updated":"2021-03-03 16:01:31.000000000","message":"I have a similar comment in the next file.","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"0b7cf52b4a4039dbe36d8ed867d4fab9424bcdad","unresolved":true,"context_lines":[{"line_number":14,"context_line":""},{"line_number":15,"context_line":"# Generic check string for checking if a user is authorized on a particular"},{"line_number":16,"context_line":"# project, specifically with the member role."},{"line_number":17,"context_line":"PROJECT_MEMBER \u003d \u0027role:member and (project_id:%(project_id)s)\u0027"},{"line_number":18,"context_line":"# Generic check string for checking if a user is authorized on a particular"},{"line_number":19,"context_line":"# project but with read-only access. For example, this persona would be able to"},{"line_number":20,"context_line":"# list private images owned by a project but cannot make any writeable changes"}],"source_content_type":"text/x-python","patch_set":35,"id":"76bf6666_f06ea2f0","line":17,"range":{"start_line":17,"start_character":34,"end_line":17,"end_character":35},"updated":"2021-03-08 13:46:37.000000000","message":"Hmm - these slipped in somehow, and they are not necessary.\n\nI\u0027ll propose a follow up to remove them since we do grouping in the composite check strings below.","commit_id":"31414b9f61ac4c923ede30a3c7e32662e9c14e1a"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"1f33a6fbeafe9a7fbf347fc100456901918b727d","unresolved":false,"context_lines":[{"line_number":14,"context_line":""},{"line_number":15,"context_line":"# Generic check string for checking if a user is authorized on a particular"},{"line_number":16,"context_line":"# project, specifically with the member role."},{"line_number":17,"context_line":"PROJECT_MEMBER \u003d \u0027role:member and (project_id:%(project_id)s)\u0027"},{"line_number":18,"context_line":"# Generic check string for checking if a user is authorized on a particular"},{"line_number":19,"context_line":"# project but with read-only access. For example, this persona would be able to"},{"line_number":20,"context_line":"# list private images owned by a project but cannot make any writeable changes"}],"source_content_type":"text/x-python","patch_set":35,"id":"afb8ffc1_4776f2c7","line":17,"range":{"start_line":17,"start_character":34,"end_line":17,"end_character":35},"in_reply_to":"76bf6666_f06ea2f0","updated":"2021-03-09 03:24:30.000000000","message":"Done\n\nhttps://review.opendev.org/c/openstack/glance/+/779266","commit_id":"31414b9f61ac4c923ede30a3c7e32662e9c14e1a"}],"glance/policies/image.py":[{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"efcb266e2e9979bd05d9c80c9c929b87dac1328a","unresolved":true,"context_lines":[{"line_number":17,"context_line":""},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"DEPRECATED_REASON \u003d \"\"\""},{"line_number":20,"context_line":"The metadata API now supports system scope and default roles."},{"line_number":21,"context_line":"\"\"\""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"deprecated_add_image \u003d policy.DeprecatedRule("}],"source_content_type":"text/x-python","patch_set":2,"id":"17dad376_96faf7eb","line":20,"range":{"start_line":20,"start_character":4,"end_line":20,"end_character":12},"updated":"2020-12-02 18:32:43.000000000","message":"These are the image policies, right?","commit_id":"e6c6a2e45021130cc8744cc95710f5b57b7a0a92"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"2987f2b8ea013955db37b19756dbd1e2e0fd00d5","unresolved":true,"context_lines":[{"line_number":17,"context_line":""},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"DEPRECATED_REASON \u003d \"\"\""},{"line_number":20,"context_line":"The metadata API now supports system scope and default roles."},{"line_number":21,"context_line":"\"\"\""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"deprecated_add_image \u003d policy.DeprecatedRule("}],"source_content_type":"text/x-python","patch_set":2,"id":"1e1cc5ca_2754e5ed","line":20,"range":{"start_line":20,"start_character":4,"end_line":20,"end_character":12},"in_reply_to":"17dad376_96faf7eb","updated":"2020-12-02 18:46:07.000000000","message":"Yes","commit_id":"e6c6a2e45021130cc8744cc95710f5b57b7a0a92"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"253218b12acdb35d29261540febf3bb6a8474290","unresolved":true,"context_lines":[{"line_number":17,"context_line":""},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"DEPRECATED_REASON \u003d \"\"\""},{"line_number":20,"context_line":"The metadata API now supports system scope and default roles."},{"line_number":21,"context_line":"\"\"\""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"deprecated_add_image \u003d policy.DeprecatedRule("}],"source_content_type":"text/x-python","patch_set":3,"id":"e354759d_ad522736","line":20,"range":{"start_line":20,"start_character":4,"end_line":20,"end_character":12},"updated":"2021-02-03 18:06:56.000000000","message":"Does this need to be:\n\n  The image API now supports system scope and default roles.\n\n?","commit_id":"be45a7728307471e2b5f665a3779e30efdc0121e"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"6f7c2101891c191a04e6f01b3707bac8fc002087","unresolved":false,"context_lines":[{"line_number":17,"context_line":""},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"DEPRECATED_REASON \u003d \"\"\""},{"line_number":20,"context_line":"The metadata API now supports system scope and default roles."},{"line_number":21,"context_line":"\"\"\""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"deprecated_add_image \u003d policy.DeprecatedRule("}],"source_content_type":"text/x-python","patch_set":3,"id":"ac38cc01_ad41ba75","line":20,"range":{"start_line":20,"start_character":4,"end_line":20,"end_character":12},"in_reply_to":"e354759d_ad522736","updated":"2021-02-03 18:07:54.000000000","message":"Done","commit_id":"be45a7728307471e2b5f665a3779e30efdc0121e"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"531063e48b5b61bbbf85a89e96b09756d78ba96e","unresolved":true,"context_lines":[{"line_number":220,"context_line":"    ),"},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":223,"context_line":"        name\u003d\"download_image\","},{"line_number":224,"context_line":"        check_str\u003dbase.SYSTEM_ADMIN_OR_PROJECT_MEMBER,"},{"line_number":225,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":226,"context_line":"        description\u003d\u0027Downloads given image\u0027,"}],"source_content_type":"text/x-python","patch_set":8,"id":"2b39e63d_b897539e","line":223,"updated":"2021-02-10 22:45:38.000000000","message":"Should this be opened up to both system and project readers?","commit_id":"41c7d2f033c20bef32ed3348411ee1ba275a07a1"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9798691574ba779d53fe4f205f2a08ffb1fee36a","unresolved":true,"context_lines":[{"line_number":220,"context_line":"    ),"},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":223,"context_line":"        name\u003d\"download_image\","},{"line_number":224,"context_line":"        check_str\u003dbase.SYSTEM_ADMIN_OR_PROJECT_MEMBER,"},{"line_number":225,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":226,"context_line":"        description\u003d\u0027Downloads given image\u0027,"}],"source_content_type":"text/x-python","patch_set":8,"id":"e4e24d0f_c9769080","line":223,"in_reply_to":"289064de_f04ac4dc","updated":"2021-02-15 15:38:58.000000000","message":"Technically, it\u0027s a read-only operation. Are you suggesting they shouldn\u0027t be able to download an image because they can\u0027t really do anything with it?","commit_id":"41c7d2f033c20bef32ed3348411ee1ba275a07a1"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"36fde323c948032ac3210e59f2f2e78f554df60f","unresolved":true,"context_lines":[{"line_number":220,"context_line":"    ),"},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":223,"context_line":"        name\u003d\"download_image\","},{"line_number":224,"context_line":"        check_str\u003dbase.SYSTEM_ADMIN_OR_PROJECT_MEMBER,"},{"line_number":225,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":226,"context_line":"        description\u003d\u0027Downloads given image\u0027,"}],"source_content_type":"text/x-python","patch_set":8,"id":"289064de_f04ac4dc","line":223,"in_reply_to":"2b39e63d_b897539e","updated":"2021-02-12 04:59:12.000000000","message":"I am not sure that reader should be able to download the image.","commit_id":"41c7d2f033c20bef32ed3348411ee1ba275a07a1"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"a03eeff6ab92836120fbe2e0ad357e88c9380707","unresolved":false,"context_lines":[{"line_number":220,"context_line":"    ),"},{"line_number":221,"context_line":""},{"line_number":222,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":223,"context_line":"        name\u003d\"download_image\","},{"line_number":224,"context_line":"        check_str\u003dbase.SYSTEM_ADMIN_OR_PROJECT_MEMBER,"},{"line_number":225,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":226,"context_line":"        description\u003d\u0027Downloads given image\u0027,"}],"source_content_type":"text/x-python","patch_set":8,"id":"2da40af6_959bba0a","line":223,"in_reply_to":"e4e24d0f_c9769080","updated":"2021-02-15 19:27:30.000000000","message":"I reverted my changes to expose this to readers.\n\nFor the time being, we can require people to be at least project-members to download images. We can relax it in the future to expose it to project-readers.","commit_id":"41c7d2f033c20bef32ed3348411ee1ba275a07a1"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"c1a1529f00d99b1b276c24ae1f9601685e728048","unresolved":true,"context_lines":[{"line_number":248,"context_line":""},{"line_number":249,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":250,"context_line":"        name\u003d\"delete_image_location\","},{"line_number":251,"context_line":"        check_str\u003dbase.SYSTEM_ADMIN_OR_PROJECT_MEMBER,"},{"line_number":252,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":253,"context_line":"        description\u003d\u0027Deletes the location of given image\u0027,"},{"line_number":254,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":8,"id":"ebeea88c_e902d901","line":251,"range":{"start_line":251,"start_character":23,"end_line":251,"end_character":53},"updated":"2021-02-12 03:02:17.000000000","message":"I don\u0027t think we want regular end users to be able to set this, even for images accessible or within their project, do we?","commit_id":"41c7d2f033c20bef32ed3348411ee1ba275a07a1"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9798691574ba779d53fe4f205f2a08ffb1fee36a","unresolved":true,"context_lines":[{"line_number":248,"context_line":""},{"line_number":249,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":250,"context_line":"        name\u003d\"delete_image_location\","},{"line_number":251,"context_line":"        check_str\u003dbase.SYSTEM_ADMIN_OR_PROJECT_MEMBER,"},{"line_number":252,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":253,"context_line":"        description\u003d\u0027Deletes the location of given image\u0027,"},{"line_number":254,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":8,"id":"b9b57745_bb0999f3","line":251,"range":{"start_line":251,"start_character":23,"end_line":251,"end_character":53},"in_reply_to":"02ddb408_0b5b07c4","updated":"2021-02-15 15:38:58.000000000","message":"I thought image locations were deployment-specific details (e.g., this image is in a file-backed store, this image is in rbd, etc.)\n\nIs there a reason to expose the management of this property to project users? It feels specific to system users.\n\nWill it break a workflow somewhere?","commit_id":"41c7d2f033c20bef32ed3348411ee1ba275a07a1"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"36fde323c948032ac3210e59f2f2e78f554df60f","unresolved":true,"context_lines":[{"line_number":248,"context_line":""},{"line_number":249,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":250,"context_line":"        name\u003d\"delete_image_location\","},{"line_number":251,"context_line":"        check_str\u003dbase.SYSTEM_ADMIN_OR_PROJECT_MEMBER,"},{"line_number":252,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":253,"context_line":"        description\u003d\u0027Deletes the location of given image\u0027,"},{"line_number":254,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":8,"id":"02ddb408_0b5b07c4","line":251,"range":{"start_line":251,"start_character":23,"end_line":251,"end_character":53},"in_reply_to":"ebeea88c_e902d901","updated":"2021-02-12 04:59:12.000000000","message":"I think we don\u0027t but earlier it was rule:default which is equivalent to this persona I guess.","commit_id":"41c7d2f033c20bef32ed3348411ee1ba275a07a1"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"c1a1529f00d99b1b276c24ae1f9601685e728048","unresolved":true,"context_lines":[{"line_number":261,"context_line":"    ),"},{"line_number":262,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":263,"context_line":"        name\u003d\"get_image_location\","},{"line_number":264,"context_line":"        check_str\u003dbase.SYSTEM_OR_PROJECT_READER,"},{"line_number":265,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":266,"context_line":"        description\u003d\u0027Reads the location of the image\u0027,"},{"line_number":267,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":8,"id":"64df3a0f_49467fc0","line":264,"range":{"start_line":264,"start_character":23,"end_line":264,"end_character":47},"updated":"2021-02-12 03:02:17.000000000","message":"Technically, if a user is allowed to get an image, can\u0027t they also view the locations (even if there are multiple)?\n\nThe show_multiple_locations help text eludes to fixing this with policy somehow[0].\n\n[0] https://docs.openstack.org/glance/latest/configuration/glance_api.html#DEFAULT.show_multiple_locations","commit_id":"41c7d2f033c20bef32ed3348411ee1ba275a07a1"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"36fde323c948032ac3210e59f2f2e78f554df60f","unresolved":true,"context_lines":[{"line_number":261,"context_line":"    ),"},{"line_number":262,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":263,"context_line":"        name\u003d\"get_image_location\","},{"line_number":264,"context_line":"        check_str\u003dbase.SYSTEM_OR_PROJECT_READER,"},{"line_number":265,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":266,"context_line":"        description\u003d\u0027Reads the location of the image\u0027,"},{"line_number":267,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":8,"id":"ea3b0a21_e2cfaf5d","line":264,"range":{"start_line":264,"start_character":23,"end_line":264,"end_character":47},"in_reply_to":"64df3a0f_49467fc0","updated":"2021-02-12 04:59:12.000000000","message":"Right, this should be SYSTEM_OR_PROJECT_MEMBER","commit_id":"41c7d2f033c20bef32ed3348411ee1ba275a07a1"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9798691574ba779d53fe4f205f2a08ffb1fee36a","unresolved":true,"context_lines":[{"line_number":261,"context_line":"    ),"},{"line_number":262,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":263,"context_line":"        name\u003d\"get_image_location\","},{"line_number":264,"context_line":"        check_str\u003dbase.SYSTEM_OR_PROJECT_READER,"},{"line_number":265,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":266,"context_line":"        description\u003d\u0027Reads the location of the image\u0027,"},{"line_number":267,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":8,"id":"f44443b5_62e51185","line":264,"range":{"start_line":264,"start_character":23,"end_line":264,"end_character":47},"in_reply_to":"ea3b0a21_e2cfaf5d","updated":"2021-02-15 15:38:58.000000000","message":"The part I\u0027m wondering about is if this should be base.SYSTEM_READER instead of including project personas.\n\nIf we only expose this property to system users, are we going to break common workflows because nova doesn\u0027t know where to find the image?","commit_id":"41c7d2f033c20bef32ed3348411ee1ba275a07a1"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"c1a1529f00d99b1b276c24ae1f9601685e728048","unresolved":true,"context_lines":[{"line_number":274,"context_line":"    ),"},{"line_number":275,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":276,"context_line":"        name\u003d\"set_image_location\","},{"line_number":277,"context_line":"        check_str\u003dbase.SYSTEM_ADMIN_OR_PROJECT_MEMBER,"},{"line_number":278,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":279,"context_line":"        description\u003d\u0027Sets location URI to given image\u0027,"},{"line_number":280,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":8,"id":"3da50ed6_03b8683a","line":277,"range":{"start_line":277,"start_character":23,"end_line":277,"end_character":53},"updated":"2021-02-12 03:02:17.000000000","message":"I don\u0027t think we want regular end users to be able to set this, even for images accessible or within their project, do we?","commit_id":"41c7d2f033c20bef32ed3348411ee1ba275a07a1"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"36fde323c948032ac3210e59f2f2e78f554df60f","unresolved":true,"context_lines":[{"line_number":274,"context_line":"    ),"},{"line_number":275,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":276,"context_line":"        name\u003d\"set_image_location\","},{"line_number":277,"context_line":"        check_str\u003dbase.SYSTEM_ADMIN_OR_PROJECT_MEMBER,"},{"line_number":278,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":279,"context_line":"        description\u003d\u0027Sets location URI to given image\u0027,"},{"line_number":280,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":8,"id":"da09e617_224f28cb","line":277,"range":{"start_line":277,"start_character":23,"end_line":277,"end_character":53},"in_reply_to":"3da50ed6_03b8683a","updated":"2021-02-12 04:59:12.000000000","message":"I think we don\u0027t but earlier it was rule:default which is equivalent to this persona I guess.","commit_id":"41c7d2f033c20bef32ed3348411ee1ba275a07a1"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9798691574ba779d53fe4f205f2a08ffb1fee36a","unresolved":true,"context_lines":[{"line_number":274,"context_line":"    ),"},{"line_number":275,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":276,"context_line":"        name\u003d\"set_image_location\","},{"line_number":277,"context_line":"        check_str\u003dbase.SYSTEM_ADMIN_OR_PROJECT_MEMBER,"},{"line_number":278,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":279,"context_line":"        description\u003d\u0027Sets location URI to given image\u0027,"},{"line_number":280,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":8,"id":"c712c8c4_fd451cf5","line":277,"range":{"start_line":277,"start_character":23,"end_line":277,"end_character":53},"in_reply_to":"da09e617_224f28cb","updated":"2021-02-15 15:38:58.000000000","message":"Similar question as delete_image_location above.","commit_id":"41c7d2f033c20bef32ed3348411ee1ba275a07a1"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"8c985c3cec9210a14359e62e62e7f349529fbdf6","unresolved":true,"context_lines":[{"line_number":224,"context_line":""},{"line_number":225,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":226,"context_line":"        name\u003d\"download_image\","},{"line_number":227,"context_line":"        check_str\u003dbase.SYSTEM_ADMIN_OR_PROJECT_MEMBER,"},{"line_number":228,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":229,"context_line":"        description\u003d\u0027Downloads given image\u0027,"},{"line_number":230,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":10,"id":"e1f1fab3_5928c915","line":227,"updated":"2021-03-03 15:19:02.000000000","message":"I\u0027m wondering if we should have project reader here too. This is probably fine default based on the least permissive approach. Just not exactly sure about the reader scope again.","commit_id":"068bb653fd1dc3a64a1e1a68ad3d14562dfa6009"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"6beab7d438876ddd4be3b2093702169bb3ba9886","unresolved":true,"context_lines":[{"line_number":443,"context_line":""},{"line_number":444,"context_line":""},{"line_number":445,"context_line":"def list_rules():"},{"line_number":446,"context_line":"    if CONF.enforce_new_policies:"},{"line_number":447,"context_line":"        return new_image_policies"},{"line_number":448,"context_line":"    return old_image_policies"}],"source_content_type":"text/x-python","patch_set":10,"id":"3c2e9aa5_e095b43a","line":446,"updated":"2021-02-19 20:09:18.000000000","message":"I\u0027m a little fuzzy on this.\n\nThe oslo.policy library will handle the deprecations depending on how you configure it. And you can opt into only using the new defaults if you don\u0027t want to bother with the legacy RBAC cruft.\n\nIs that the reason for this switch?","commit_id":"068bb653fd1dc3a64a1e1a68ad3d14562dfa6009"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"2b748500b9e0eb01b6f5d31e2f733d0583f8caf5","unresolved":true,"context_lines":[{"line_number":443,"context_line":""},{"line_number":444,"context_line":""},{"line_number":445,"context_line":"def list_rules():"},{"line_number":446,"context_line":"    if CONF.enforce_new_policies:"},{"line_number":447,"context_line":"        return new_image_policies"},{"line_number":448,"context_line":"    return old_image_policies"}],"source_content_type":"text/x-python","patch_set":10,"id":"fd027123_40dff358","line":446,"in_reply_to":"3c2e9aa5_e095b43a","updated":"2021-02-22 06:53:51.000000000","message":"This is kind of double safety. We want to highlight that Secure RBAC is experimental and to enable it deployer/users should explicitly set it on using this switch.","commit_id":"068bb653fd1dc3a64a1e1a68ad3d14562dfa6009"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"fbfcda42222c384a41dd94b5a0d195f583f9a4c9","unresolved":true,"context_lines":[{"line_number":23,"context_line":"image_policies \u003d ["},{"line_number":24,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":25,"context_line":"        name\u003d\"add_image\","},{"line_number":26,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":27,"context_line":"        scope_types\u003d[\u0027project\u0027],"},{"line_number":28,"context_line":"        description\u003d\u0027Create new image\u0027,"},{"line_number":29,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":15,"id":"0acbe800_2dcbc02e","line":26,"updated":"2021-03-01 20:02:22.000000000","message":"I\u0027m not totally sure how this works. When you go to add an image, there is no image to check and see if you\u0027re a member of. Does this actually mean nobody but admins can add images?","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2c2e19626a2ee1057b69e984e39f0958e6318e50","unresolved":true,"context_lines":[{"line_number":23,"context_line":"image_policies \u003d ["},{"line_number":24,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":25,"context_line":"        name\u003d\"add_image\","},{"line_number":26,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":27,"context_line":"        scope_types\u003d[\u0027project\u0027],"},{"line_number":28,"context_line":"        description\u003d\u0027Create new image\u0027,"},{"line_number":29,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":15,"id":"bef31681_09a5cbea","line":26,"in_reply_to":"0acbe800_2dcbc02e","updated":"2021-03-01 21:13:35.000000000","message":"\u003e I\u0027m not totally sure how this works. When you go to add an image, there is no image to check and see if you\u0027re a member of. Does this actually mean nobody but admins can add images?\n\nGood question. We don\u0027t have a reference to pull from the database, but we can use the image reference we\u0027re about to create (assuming policy enforcement passes), which I think we do in the controller [0], then we pass that image reference to the various layers leading down to the DB API, policy being one of those layers [1].\n\nThis is one way we can use policy to ensure users can\u0027t create resource outside their project (or slip a tampered image into someone else\u0027s project).\n\n[0] https://opendev.org/openstack/glance/src/commit/bdbad59dc9605afbb9178d79617ab54e78a9235d/glance/api/v2/images.py#L76-L77\n[1] https://review.opendev.org/c/openstack/glance/+/764754/15/glance/api/policy.py#182","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"de66c9caf4c9e538579db6b3879129f24fe682a6","unresolved":true,"context_lines":[{"line_number":23,"context_line":"image_policies \u003d ["},{"line_number":24,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":25,"context_line":"        name\u003d\"add_image\","},{"line_number":26,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":27,"context_line":"        scope_types\u003d[\u0027project\u0027],"},{"line_number":28,"context_line":"        description\u003d\u0027Create new image\u0027,"},{"line_number":29,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":15,"id":"7651bc4b_d9099276","line":26,"in_reply_to":"bef31681_09a5cbea","updated":"2021-03-02 15:16:26.000000000","message":"\u003e \u003e I\u0027m not totally sure how this works. When you go to add an image, there is no image to check and see if you\u0027re a member of. Does this actually mean nobody but admins can add images?\n\u003e \n\u003e Good question. We don\u0027t have a reference to pull from the database, but we can use the image reference we\u0027re about to create (assuming policy enforcement passes), which I think we do in the controller [0], then we pass that image reference to the various layers leading down to the DB API, policy being one of those layers [1].\n\u003e \n\u003e This is one way we can use policy to ensure users can\u0027t create resource outside their project (or slip a tampered image into someone else\u0027s project).\n\u003e \n\u003e [0] https://opendev.org/openstack/glance/src/commit/bdbad59dc9605afbb9178d79617ab54e78a9235d/glance/api/v2/images.py#L76-L77\n\u003e [1] https://review.opendev.org/c/openstack/glance/+/764754/15/glance/api/policy.py#182\n\nThis makes my head spin a bit, especially the \"use the thing the user is asking us to create in order to authorize the user to create that thing.\" I guess we depend on the lower layers to avoid letting us create an image in some weird security state? I mean, I assume the API schema will prevent someone from trying to create an image with a different owner (or similar) but that seems like a risky thing to rely on for something like this.","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"ad94332868fa0cc13e06d828316ee3f01996e0e7","unresolved":true,"context_lines":[{"line_number":39,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":40,"context_line":"        name\u003d\"delete_image\","},{"line_number":41,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":42,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":43,"context_line":"        description\u003d\u0027Deletes the image\u0027,"},{"line_number":44,"context_line":"        operations\u003d["},{"line_number":45,"context_line":"            {\u0027path\u0027: \u0027/v2/images/{image_id}\u0027,"}],"source_content_type":"text/x-python","patch_set":15,"id":"31b0ad91_225905d6","line":42,"range":{"start_line":42,"start_character":20,"end_line":42,"end_character":41},"updated":"2021-03-02 14:06:56.000000000","message":"Why on project scope for add_image (line #27) and both system and project for other rules?","commit_id":"aec3f4b147c3038f61fc5daf84c3bc135334be54"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"16189a4abe7958c3e847c2c6853484cff89671d7","unresolved":true,"context_lines":[{"line_number":24,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":25,"context_line":"        name\u003d\"add_image\","},{"line_number":26,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":27,"context_line":"        scope_types\u003d[\u0027project\u0027],"},{"line_number":28,"context_line":"        description\u003d\u0027Create new image\u0027,"},{"line_number":29,"context_line":"        operations\u003d["},{"line_number":30,"context_line":"            {\u0027path\u0027: \u0027/v2/images\u0027,"}],"source_content_type":"text/x-python","patch_set":18,"id":"11693ccb_7772b95b","line":27,"range":{"start_line":27,"start_character":20,"end_line":27,"end_character":32},"updated":"2021-03-02 14:12:00.000000000","message":"Any reason why both scopes not defined here and other policy rules have both of them?","commit_id":"47fa359614cab41231e672ff5c3ed46135f6ba78"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"3451d4b58b0c8c7d277f21b3702585ba2fcc9278","unresolved":false,"context_lines":[{"line_number":24,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":25,"context_line":"        name\u003d\"add_image\","},{"line_number":26,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":27,"context_line":"        scope_types\u003d[\u0027project\u0027],"},{"line_number":28,"context_line":"        description\u003d\u0027Create new image\u0027,"},{"line_number":29,"context_line":"        operations\u003d["},{"line_number":30,"context_line":"            {\u0027path\u0027: \u0027/v2/images\u0027,"}],"source_content_type":"text/x-python","patch_set":18,"id":"56ae3aa3_d1577d55","line":27,"range":{"start_line":27,"start_character":20,"end_line":27,"end_character":32},"in_reply_to":"11693ccb_7772b95b","updated":"2021-03-02 15:18:55.000000000","message":"Ah - yeah, this should be fixed.","commit_id":"47fa359614cab41231e672ff5c3ed46135f6ba78"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d02870d1284e336665911468bcd8ad3c29719733","unresolved":true,"context_lines":[{"line_number":326,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":327,"context_line":"        name\u003d\"copy_image\","},{"line_number":328,"context_line":"        check_str\u003d\u0027role:admin\u0027,"},{"line_number":329,"context_line":"        scope_types\u003d[\u0027system\u0027],"},{"line_number":330,"context_line":"        description\u003d\u0027Copy existing image to other stores\u0027,"},{"line_number":331,"context_line":"        operations\u003d["},{"line_number":332,"context_line":"            {\u0027path\u0027: \u0027/v2/images/{image_id}/import\u0027,"}],"source_content_type":"text/x-python","patch_set":21,"id":"8dc25811_0fe2f7dd","line":329,"range":{"start_line":329,"start_character":20,"end_line":329,"end_character":30},"updated":"2021-03-03 00:25:38.000000000","message":"This needs to include \u0027project\u0027 because we need to allow users to be able to be granted this ability for the nova-ceph-multistore interaction. We\u0027d fail that job now if we enabled scope testing.\n\nEven still, I think that allowing project admins to do copy-image by default is fine, and it\u0027s in line with what we have today anyway.","commit_id":"dd60ac6d31ba823c4bf1735cd3f1423a74741204"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"ed9ade29da2bd568b41845d6301f2b9f1aa0bbe5","unresolved":false,"context_lines":[{"line_number":326,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":327,"context_line":"        name\u003d\"copy_image\","},{"line_number":328,"context_line":"        check_str\u003d\u0027role:admin\u0027,"},{"line_number":329,"context_line":"        scope_types\u003d[\u0027system\u0027],"},{"line_number":330,"context_line":"        description\u003d\u0027Copy existing image to other stores\u0027,"},{"line_number":331,"context_line":"        operations\u003d["},{"line_number":332,"context_line":"            {\u0027path\u0027: \u0027/v2/images/{image_id}/import\u0027,"}],"source_content_type":"text/x-python","patch_set":21,"id":"5a36fe6a_7fe2d99e","line":329,"range":{"start_line":329,"start_character":20,"end_line":329,"end_character":30},"in_reply_to":"8dc25811_0fe2f7dd","updated":"2021-03-03 00:30:11.000000000","message":"Done","commit_id":"dd60ac6d31ba823c4bf1735cd3f1423a74741204"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"00ca0f84e95a37a3b7f9523da86a2915bc981aa9","unresolved":true,"context_lines":[{"line_number":190,"context_line":"    ),"},{"line_number":191,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":192,"context_line":"        name\u003d\"set_image_location\","},{"line_number":193,"context_line":"        check_str\u003d\u0027role:admin\u0027,"},{"line_number":194,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":195,"context_line":"        description\u003d\u0027Sets location URI to given image\u0027,"},{"line_number":196,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":22,"id":"c91c17da_71ce9e6c","line":193,"updated":"2021-03-03 00:40:01.000000000","message":"I think this may break nova\u0027s ability to do direct rbd snapshots, but I\u0027m not positive. Note that it was rule:default before.\n\nTomorrow I will dig into that stuff and refresh my memory on how that works.","commit_id":"4f0f579ecf34ffe61e402cf1382bc90fa0668298"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"4c46bc46f963c5359abc05b2dcdb9c065b61a380","unresolved":true,"context_lines":[{"line_number":190,"context_line":"    ),"},{"line_number":191,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":192,"context_line":"        name\u003d\"set_image_location\","},{"line_number":193,"context_line":"        check_str\u003d\u0027role:admin\u0027,"},{"line_number":194,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":195,"context_line":"        description\u003d\u0027Sets location URI to given image\u0027,"},{"line_number":196,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":22,"id":"d2f192b9_218d7074","line":193,"in_reply_to":"3f17de16_4e55c4d5","updated":"2021-03-03 14:51:01.000000000","message":"If the nova case is using a project-admin or system-admin persona to execute this policy then yes the new default will continue to work.\n\nOtherwise - we might need to massage this to push it down to end users assuming nova executes this with a user\u0027s token and not a nova service token (e.g., ADMIN_OR_PROJECT_MEMBER).","commit_id":"4f0f579ecf34ffe61e402cf1382bc90fa0668298"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2cc702f3a0c222b19f5472429584bca1faad70f5","unresolved":true,"context_lines":[{"line_number":190,"context_line":"    ),"},{"line_number":191,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":192,"context_line":"        name\u003d\"set_image_location\","},{"line_number":193,"context_line":"        check_str\u003d\u0027role:admin\u0027,"},{"line_number":194,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":195,"context_line":"        description\u003d\u0027Sets location URI to given image\u0027,"},{"line_number":196,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":22,"id":"e4a297c2_87afcfc4","line":193,"in_reply_to":"88701c8a_0ed1766a","updated":"2021-03-03 15:16:09.000000000","message":"Yeah - that\u0027s a good idea. Just to clairfy, the job isn\u0027t going to break (because of this) as soon as this merges. Only when enforce_scope\u003dTrue by default, and we don\u0027t know when that will happen, yet.","commit_id":"4f0f579ecf34ffe61e402cf1382bc90fa0668298"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"81f2f8d17d31e2b5b888f1a64eddcf6fcaecf623","unresolved":true,"context_lines":[{"line_number":190,"context_line":"    ),"},{"line_number":191,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":192,"context_line":"        name\u003d\"set_image_location\","},{"line_number":193,"context_line":"        check_str\u003d\u0027role:admin\u0027,"},{"line_number":194,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":195,"context_line":"        description\u003d\u0027Sets location URI to given image\u0027,"},{"line_number":196,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":22,"id":"e4b84d41_e81797bc","line":193,"in_reply_to":"c91c17da_71ce9e6c","updated":"2021-03-03 03:23:18.000000000","message":"Right - this would break if that specific job sets:\n\n  glance-api.conf\n    [oslo_policy]\n    enforce_new_defaults\u003dTrue\n\nIf not, then oslo.policy will handle this by logically OR\u0027ing the old check string with the new check string.","commit_id":"4f0f579ecf34ffe61e402cf1382bc90fa0668298"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a6188e99278d525f257a3c03b684c140a2474229","unresolved":true,"context_lines":[{"line_number":190,"context_line":"    ),"},{"line_number":191,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":192,"context_line":"        name\u003d\"set_image_location\","},{"line_number":193,"context_line":"        check_str\u003d\u0027role:admin\u0027,"},{"line_number":194,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":195,"context_line":"        description\u003d\u0027Sets location URI to given image\u0027,"},{"line_number":196,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":22,"id":"88701c8a_0ed1766a","line":193,"in_reply_to":"d2f192b9_218d7074","updated":"2021-03-03 15:07:57.000000000","message":"AFAIK, it uses the user\u0027s credentials all the same.\n\nPerhaps we need to spark up a DNM patch that will run a nova/ceph job against this with enforce_new_defaults\u003dTrue to check?","commit_id":"4f0f579ecf34ffe61e402cf1382bc90fa0668298"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"08d4be0cb4c0f7bec03d3fcd5f4d6a700223801a","unresolved":true,"context_lines":[{"line_number":190,"context_line":"    ),"},{"line_number":191,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":192,"context_line":"        name\u003d\"set_image_location\","},{"line_number":193,"context_line":"        check_str\u003d\u0027role:admin\u0027,"},{"line_number":194,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":195,"context_line":"        description\u003d\u0027Sets location URI to given image\u0027,"},{"line_number":196,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":22,"id":"ad80ac6a_650d8cc8","line":193,"in_reply_to":"e4a297c2_87afcfc4","updated":"2021-03-03 15:46:59.000000000","message":"Is it enforce_scope or enforce_new_defaults? The scope shouldn\u0027t be the problem, it\u0027s the fact that you\u0027re making this admin-only by default I think.","commit_id":"4f0f579ecf34ffe61e402cf1382bc90fa0668298"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"faba9d508123bb1e6cd1414019d4f05a623e6461","unresolved":true,"context_lines":[{"line_number":190,"context_line":"    ),"},{"line_number":191,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":192,"context_line":"        name\u003d\"set_image_location\","},{"line_number":193,"context_line":"        check_str\u003d\u0027role:admin\u0027,"},{"line_number":194,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":195,"context_line":"        description\u003d\u0027Sets location URI to given image\u0027,"},{"line_number":196,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":22,"id":"3f17de16_4e55c4d5","line":193,"in_reply_to":"e4b84d41_e81797bc","updated":"2021-03-03 14:28:39.000000000","message":"Okay, but my point is - the new default should also work for the default (rbd) case in nova right?","commit_id":"4f0f579ecf34ffe61e402cf1382bc90fa0668298"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"00ca0f84e95a37a3b7f9523da86a2915bc981aa9","unresolved":true,"context_lines":[{"line_number":329,"context_line":"        # Eventually, we need to make sure we update the check string here to"},{"line_number":330,"context_line":"        # be scope-aware, but for now this is restricted to system-admins and"},{"line_number":331,"context_line":"        # project-admins. That might change in the future if we decide to push"},{"line_number":332,"context_line":"        # this functionality down to project-members."},{"line_number":333,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":334,"context_line":"        description\u003d\u0027Copy existing image to other stores\u0027,"},{"line_number":335,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":22,"id":"cbcca260_ede63cc3","line":332,"updated":"2021-03-03 00:40:01.000000000","message":"\"decide to push this down to members\" meaning by default, right? Presumably I can still override my check_str in my own policy file to allow normal users to do this if I want, right?","commit_id":"4f0f579ecf34ffe61e402cf1382bc90fa0668298"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"4c46bc46f963c5359abc05b2dcdb9c065b61a380","unresolved":false,"context_lines":[{"line_number":329,"context_line":"        # Eventually, we need to make sure we update the check string here to"},{"line_number":330,"context_line":"        # be scope-aware, but for now this is restricted to system-admins and"},{"line_number":331,"context_line":"        # project-admins. That might change in the future if we decide to push"},{"line_number":332,"context_line":"        # this functionality down to project-members."},{"line_number":333,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":334,"context_line":"        description\u003d\u0027Copy existing image to other stores\u0027,"},{"line_number":335,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":22,"id":"429a6689_9c8a5709","line":332,"in_reply_to":"2fb365d5_25313fb4","updated":"2021-03-03 14:51:01.000000000","message":"Ack","commit_id":"4f0f579ecf34ffe61e402cf1382bc90fa0668298"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"81f2f8d17d31e2b5b888f1a64eddcf6fcaecf623","unresolved":true,"context_lines":[{"line_number":329,"context_line":"        # Eventually, we need to make sure we update the check string here to"},{"line_number":330,"context_line":"        # be scope-aware, but for now this is restricted to system-admins and"},{"line_number":331,"context_line":"        # project-admins. That might change in the future if we decide to push"},{"line_number":332,"context_line":"        # this functionality down to project-members."},{"line_number":333,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":334,"context_line":"        description\u003d\u0027Copy existing image to other stores\u0027,"},{"line_number":335,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":22,"id":"e178d3c8_59a2526c","line":332,"in_reply_to":"cbcca260_ede63cc3","updated":"2021-03-03 03:23:18.000000000","message":"Yes - you can still override this within your policy file.\n\nI meant if glance decided to adjust the default to expose this to more end users directly, assuming the API doesn\u0027t leak tenancy information that could be used by end users to compromise other tenants (e.g., I have no idea if it does or not).","commit_id":"4f0f579ecf34ffe61e402cf1382bc90fa0668298"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"faba9d508123bb1e6cd1414019d4f05a623e6461","unresolved":true,"context_lines":[{"line_number":329,"context_line":"        # Eventually, we need to make sure we update the check string here to"},{"line_number":330,"context_line":"        # be scope-aware, but for now this is restricted to system-admins and"},{"line_number":331,"context_line":"        # project-admins. That might change in the future if we decide to push"},{"line_number":332,"context_line":"        # this functionality down to project-members."},{"line_number":333,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":334,"context_line":"        description\u003d\u0027Copy existing image to other stores\u0027,"},{"line_number":335,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":22,"id":"2fb365d5_25313fb4","line":332,"in_reply_to":"e178d3c8_59a2526c","updated":"2021-03-03 14:28:39.000000000","message":"Ack, I think the default never really needs to include regular members, so I would nuke this comment in the next round, just to avoid confusion. If anything, you could explain that it includes \"project\" scope so that it _can_ be granted to users, just so nobody comes along and \"fixes\" it later :)","commit_id":"4f0f579ecf34ffe61e402cf1382bc90fa0668298"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"1600e7bfa0f21180738efef88936f0bc6ba9096e","unresolved":true,"context_lines":[{"line_number":23,"context_line":"image_policies \u003d ["},{"line_number":24,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":25,"context_line":"        name\u003d\"add_image\","},{"line_number":26,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":27,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":28,"context_line":"        description\u003d\u0027Create new image\u0027,"},{"line_number":29,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"c17e13a4_bcd5847a","line":26,"range":{"start_line":26,"start_character":23,"end_line":26,"end_character":46},"updated":"2021-03-03 03:32:16.000000000","message":"I\u0027m not sure how I feel about re-using this check string here. ADMIN_OR_PROJECT_MEMBER translates to:\n\n  \"add_image\": \"role:admin or (role:member and (project_id:%(project_id)s or project_id:%(member_id)s or \"community\":%(visibility)s or \"public\":%(visibility)s)\n\nTechnically, project-members shouldn\u0027t be allowed to create public images, which is a behavior that existed prior to any of this work. The policy here can be misleading to operators if they read it and think \"oh, project-members can create public images.\"\n\nThis doesn\u0027t actually have any impact on our tests in glance-tempest-plugin, which ensures project-members *can\u0027t* do this, because the publicize_image policy is called when the image factory executes and before add_image is invoked.\n\nSo, execution when a project-member attempts to create a public image would never reach this check, but it can be misleading for readers.\n\nFull disclosure, I wrote this check this way to get it to work for get_image, in which case exposing public and community images to project-members is completely valid.","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9ebdd108ce3fa61457e594fc44980adba35594c1","unresolved":false,"context_lines":[{"line_number":23,"context_line":"image_policies \u003d ["},{"line_number":24,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":25,"context_line":"        name\u003d\"add_image\","},{"line_number":26,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":27,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":28,"context_line":"        description\u003d\u0027Create new image\u0027,"},{"line_number":29,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"23d97e7f_094bd993","line":26,"range":{"start_line":26,"start_character":23,"end_line":26,"end_character":46},"in_reply_to":"c17e13a4_bcd5847a","updated":"2021-03-04 04:25:17.000000000","message":"Done","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"8c985c3cec9210a14359e62e62e7f349529fbdf6","unresolved":true,"context_lines":[{"line_number":38,"context_line":"    ),"},{"line_number":39,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":40,"context_line":"        name\u003d\"delete_image\","},{"line_number":41,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":42,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":43,"context_line":"        description\u003d\u0027Deletes the image\u0027,"},{"line_number":44,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"05e11557_6d5a8aaa","line":41,"range":{"start_line":41,"start_character":23,"end_line":41,"end_character":46},"updated":"2021-03-03 15:19:02.000000000","message":"So how does that same checkstring affect us here?","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"e36af1c5ec56bb7abff58d8c5f95b53f4eedce6b","unresolved":true,"context_lines":[{"line_number":38,"context_line":"    ),"},{"line_number":39,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":40,"context_line":"        name\u003d\"delete_image\","},{"line_number":41,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":42,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":43,"context_line":"        description\u003d\u0027Deletes the image\u0027,"},{"line_number":44,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"26d3d3ac_7cb4977a","line":41,"range":{"start_line":41,"start_character":23,"end_line":41,"end_character":46},"in_reply_to":"05e11557_6d5a8aaa","updated":"2021-03-03 16:01:31.000000000","message":"I essentially had the same comment above.","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9ebdd108ce3fa61457e594fc44980adba35594c1","unresolved":false,"context_lines":[{"line_number":38,"context_line":"    ),"},{"line_number":39,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":40,"context_line":"        name\u003d\"delete_image\","},{"line_number":41,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":42,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":43,"context_line":"        description\u003d\u0027Deletes the image\u0027,"},{"line_number":44,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"38efd76e_19ed2d33","line":41,"range":{"start_line":41,"start_character":23,"end_line":41,"end_character":46},"in_reply_to":"26d3d3ac_7cb4977a","updated":"2021-03-04 04:25:17.000000000","message":"Done, I updated the policies to include checks that are specific to their use-case.\n\nSo, the only two that are really complicated are get_image and download_image. The rest are pretty straight forward.","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"8c985c3cec9210a14359e62e62e7f349529fbdf6","unresolved":true,"context_lines":[{"line_number":83,"context_line":"    ),"},{"line_number":84,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":85,"context_line":"        name\u003d\"modify_image\","},{"line_number":86,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":87,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":88,"context_line":"        description\u003d\u0027Updates given image\u0027,"},{"line_number":89,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"1341d54a_4f90c88d","line":86,"range":{"start_line":86,"start_character":23,"end_line":86,"end_character":46},"updated":"2021-03-03 15:19:02.000000000","message":"ditto","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9ebdd108ce3fa61457e594fc44980adba35594c1","unresolved":false,"context_lines":[{"line_number":83,"context_line":"    ),"},{"line_number":84,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":85,"context_line":"        name\u003d\"modify_image\","},{"line_number":86,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":87,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":88,"context_line":"        description\u003d\u0027Updates given image\u0027,"},{"line_number":89,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"28dad1f8_c1bd4c72","line":86,"range":{"start_line":86,"start_character":23,"end_line":86,"end_character":46},"in_reply_to":"1341d54a_4f90c88d","updated":"2021-03-04 04:25:17.000000000","message":"Done","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"8c985c3cec9210a14359e62e62e7f349529fbdf6","unresolved":true,"context_lines":[{"line_number":170,"context_line":"    ),"},{"line_number":171,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":172,"context_line":"        name\u003d\"get_image_location\","},{"line_number":173,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_READER,"},{"line_number":174,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":175,"context_line":"        description\u003d\u0027Reads the location of the image\u0027,"},{"line_number":176,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"2af209c1_1578e321","line":173,"updated":"2021-03-03 15:19:02.000000000","message":"I don\u0027t think we should extend this by default to everyone.","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"08d4be0cb4c0f7bec03d3fcd5f4d6a700223801a","unresolved":true,"context_lines":[{"line_number":170,"context_line":"    ),"},{"line_number":171,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":172,"context_line":"        name\u003d\"get_image_location\","},{"line_number":173,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_READER,"},{"line_number":174,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":175,"context_line":"        description\u003d\u0027Reads the location of the image\u0027,"},{"line_number":176,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"ee843725_2d4418cf","line":173,"in_reply_to":"2af209c1_1578e321","updated":"2021-03-03 15:46:59.000000000","message":"We currently do, AFAICT, and a nova-glance-rbd configuration requires it in order to work. If we\u0027re going to change the default, I think we need to signal it well ahead of time.","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9ebdd108ce3fa61457e594fc44980adba35594c1","unresolved":false,"context_lines":[{"line_number":170,"context_line":"    ),"},{"line_number":171,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":172,"context_line":"        name\u003d\"get_image_location\","},{"line_number":173,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_READER,"},{"line_number":174,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":175,"context_line":"        description\u003d\u0027Reads the location of the image\u0027,"},{"line_number":176,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"61b8e6ac_b8dfe0f6","line":173,"in_reply_to":"bb8909fc_060fb7fd","updated":"2021-03-04 04:25:17.000000000","message":"Done","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"e36af1c5ec56bb7abff58d8c5f95b53f4eedce6b","unresolved":true,"context_lines":[{"line_number":170,"context_line":"    ),"},{"line_number":171,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":172,"context_line":"        name\u003d\"get_image_location\","},{"line_number":173,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_READER,"},{"line_number":174,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":175,"context_line":"        description\u003d\u0027Reads the location of the image\u0027,"},{"line_number":176,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"f5c5e8a1_60901d43","line":173,"in_reply_to":"ee843725_2d4418cf","updated":"2021-03-03 16:01:31.000000000","message":"Right - the default (rule:default) is technically an open check (e.g., \"\"), which means you can pass this policy with simply a valid token (nevermind the project that token is scoped to).","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"001cbf932145b7cdffd252b7cae17b1bbb587958","unresolved":true,"context_lines":[{"line_number":170,"context_line":"    ),"},{"line_number":171,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":172,"context_line":"        name\u003d\"get_image_location\","},{"line_number":173,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_READER,"},{"line_number":174,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":175,"context_line":"        description\u003d\u0027Reads the location of the image\u0027,"},{"line_number":176,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"bb8909fc_060fb7fd","line":173,"in_reply_to":"f5c5e8a1_60901d43","updated":"2021-03-03 18:47:14.000000000","message":"Oh, it was that way around. I thought we swapped the default on this to favour the policy rather than the config option for blocking the locations usage. (We definitely did discuss that) Might have been the case that we just discussed and never actually got it done or it tripped yet again on tempest.","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"14feb305536e594c76631e1fce54e48b214120f8","unresolved":true,"context_lines":[{"line_number":284,"context_line":"            name\u003d\"manage_image_cache\", check_str\u003d\"rule:default\""},{"line_number":285,"context_line":"        ),"},{"line_number":286,"context_line":"        deprecated_reason\u003dDEPRECATED_REASON,"},{"line_number":287,"context_line":"        deprecated_since\u003dversionutils.deprecated.WALLABY"},{"line_number":288,"context_line":"    ),"},{"line_number":289,"context_line":""},{"line_number":290,"context_line":"    policy.DocumentedRuleDefault("}],"source_content_type":"text/x-python","patch_set":23,"id":"b7491f9f_6d157c32","line":287,"updated":"2021-03-03 19:48:19.000000000","message":"Remove this- it\u0027s bad copy/pasta","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9ebdd108ce3fa61457e594fc44980adba35594c1","unresolved":false,"context_lines":[{"line_number":284,"context_line":"            name\u003d\"manage_image_cache\", check_str\u003d\"rule:default\""},{"line_number":285,"context_line":"        ),"},{"line_number":286,"context_line":"        deprecated_reason\u003dDEPRECATED_REASON,"},{"line_number":287,"context_line":"        deprecated_since\u003dversionutils.deprecated.WALLABY"},{"line_number":288,"context_line":"    ),"},{"line_number":289,"context_line":""},{"line_number":290,"context_line":"    policy.DocumentedRuleDefault("}],"source_content_type":"text/x-python","patch_set":23,"id":"08588d4a_83102a31","line":287,"in_reply_to":"b7491f9f_6d157c32","updated":"2021-03-04 04:25:17.000000000","message":"Done","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"8c985c3cec9210a14359e62e62e7f349529fbdf6","unresolved":true,"context_lines":[{"line_number":304,"context_line":"    ),"},{"line_number":305,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":306,"context_line":"        name\u003d\"reactivate\","},{"line_number":307,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":308,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":309,"context_line":"        description\u003d\u0027Reactivate image\u0027,"},{"line_number":310,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"3d468f73_12a9220c","line":307,"updated":"2021-03-03 15:19:02.000000000","message":"This should be admin only, not project memeber.","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"e36af1c5ec56bb7abff58d8c5f95b53f4eedce6b","unresolved":true,"context_lines":[{"line_number":304,"context_line":"    ),"},{"line_number":305,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":306,"context_line":"        name\u003d\"reactivate\","},{"line_number":307,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":308,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":309,"context_line":"        description\u003d\u0027Reactivate image\u0027,"},{"line_number":310,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"45e2b159_55f7b61f","line":307,"in_reply_to":"3d468f73_12a9220c","updated":"2021-03-03 16:01:31.000000000","message":"What about deactivate? Should that be admin-only, too?","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"001cbf932145b7cdffd252b7cae17b1bbb587958","unresolved":true,"context_lines":[{"line_number":304,"context_line":"    ),"},{"line_number":305,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":306,"context_line":"        name\u003d\"reactivate\","},{"line_number":307,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":308,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":309,"context_line":"        description\u003d\u0027Reactivate image\u0027,"},{"line_number":310,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"b506f773_e7db4edd","line":307,"in_reply_to":"45e2b159_55f7b61f","updated":"2021-03-03 18:47:14.000000000","message":"Nope, so the idea with deactivate/reactivate is that user can deactivate an image for inspection. It will block all non-admin users to access the image data operations (including delete). This is for example in a case they suspect the image containing backdoor, exploit, something harmful that is worthy of checking.\n\nIt will then require admin action to either release it back into use or delete it.","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9ebdd108ce3fa61457e594fc44980adba35594c1","unresolved":true,"context_lines":[{"line_number":304,"context_line":"    ),"},{"line_number":305,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":306,"context_line":"        name\u003d\"reactivate\","},{"line_number":307,"context_line":"        check_str\u003dbase.ADMIN_OR_PROJECT_MEMBER,"},{"line_number":308,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":309,"context_line":"        description\u003d\u0027Reactivate image\u0027,"},{"line_number":310,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":23,"id":"6153f709_4a078b87","line":307,"in_reply_to":"b506f773_e7db4edd","updated":"2021-03-04 04:25:17.000000000","message":"I think we worked through this in IRC and we decided to keep situation #3, where users and deactivate and reactivate images?","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"14feb305536e594c76631e1fce54e48b214120f8","unresolved":true,"context_lines":[{"line_number":334,"context_line":"        deprecated_rule\u003dpolicy.DeprecatedRule("},{"line_number":335,"context_line":"            name\u003d\"copy_image\", check_str\u003d\"rule:default\""},{"line_number":336,"context_line":"        ),"},{"line_number":337,"context_line":"        deprecated_reason\u003dDEPRECATED_REASON,"},{"line_number":338,"context_line":"        deprecated_since\u003dversionutils.deprecated.WALLABY"},{"line_number":339,"context_line":"    ),"},{"line_number":340,"context_line":"]"}],"source_content_type":"text/x-python","patch_set":23,"id":"57b70006_bacbac58","line":337,"updated":"2021-03-03 19:48:19.000000000","message":"Remove this - it\u0027s bad copy/pasta","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"9ebdd108ce3fa61457e594fc44980adba35594c1","unresolved":false,"context_lines":[{"line_number":334,"context_line":"        deprecated_rule\u003dpolicy.DeprecatedRule("},{"line_number":335,"context_line":"            name\u003d\"copy_image\", check_str\u003d\"rule:default\""},{"line_number":336,"context_line":"        ),"},{"line_number":337,"context_line":"        deprecated_reason\u003dDEPRECATED_REASON,"},{"line_number":338,"context_line":"        deprecated_since\u003dversionutils.deprecated.WALLABY"},{"line_number":339,"context_line":"    ),"},{"line_number":340,"context_line":"]"}],"source_content_type":"text/x-python","patch_set":23,"id":"0f6d63e8_b4f27d41","line":337,"in_reply_to":"57b70006_bacbac58","updated":"2021-03-04 04:25:17.000000000","message":"Done","commit_id":"607623910a3a70a5af7ee3072bac98d04a021b6c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"01bfaeaf11d0610135caf3a75fdcc72f99f9c765","unresolved":true,"context_lines":[{"line_number":185,"context_line":"    ),"},{"line_number":186,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":187,"context_line":"        name\u003d\"set_image_location\","},{"line_number":188,"context_line":"        check_str\u003d\u0027role:admin\u0027,"},{"line_number":189,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":190,"context_line":"        description\u003d\u0027Sets location URI to given image\u0027,"},{"line_number":191,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":25,"id":"dd9fce80_bf80bdd8","line":188,"updated":"2021-03-04 19:51:03.000000000","message":"Locally confirmed the nova-snapshot case with RBD. Specifically that right now, nova-glance-ceph can snapshot images out of the box with default configs. If we change this to role:admin, we\u0027ll break that default (and uuber common) workflow with enforce_new_defaults\u003dTrue.\n\nSo, I think we need to turn this back to project_member to match what we have today. We can have the \"should this be tighter?\" conversation and plan appropriate signaling and coordination if we need to, but I don\u0027t think it should be intermingled with this patch.","commit_id":"0cbff29da4741610824fffb8cbf90fecabbf0ff0"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"6584010b35bf8d68e93b148544cb19998086bbd9","unresolved":false,"context_lines":[{"line_number":185,"context_line":"    ),"},{"line_number":186,"context_line":"    policy.DocumentedRuleDefault("},{"line_number":187,"context_line":"        name\u003d\"set_image_location\","},{"line_number":188,"context_line":"        check_str\u003d\u0027role:admin\u0027,"},{"line_number":189,"context_line":"        scope_types\u003d[\u0027system\u0027, \u0027project\u0027],"},{"line_number":190,"context_line":"        description\u003d\u0027Sets location URI to given image\u0027,"},{"line_number":191,"context_line":"        operations\u003d["}],"source_content_type":"text/x-python","patch_set":25,"id":"480fd279_587f3c69","line":188,"in_reply_to":"dd9fce80_bf80bdd8","updated":"2021-03-04 19:53:21.000000000","message":"Done","commit_id":"0cbff29da4741610824fffb8cbf90fecabbf0ff0"}],"glance/tests/unit/test_cache_middleware.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"89abbd1c331afc4e4cff50b1ee434759f9bc7929","unresolved":true,"context_lines":[{"line_number":55,"context_line":"        self.os_hash_value \u003d None"},{"line_number":56,"context_line":"        self.owner \u003d owner"},{"line_number":57,"context_line":"        self.virtual_size \u003d 0"},{"line_number":58,"context_line":"        self.tags \u003d []"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"class TestCacheMiddlewareURLMatching(testtools.TestCase):"}],"source_content_type":"text/x-python","patch_set":32,"id":"b31548a0_389b64d4","line":58,"updated":"2021-03-06 04:48:31.000000000","message":"We could pull this test refactor out to reduce the size of this a bit. This patch is starting to grow, with all the things it\u0027s collecting. Not a huge deal, and I can maybe take a look at doing that later if there\u0027s a convenient time.","commit_id":"f01563ada091c9ca16e7dc79ceeed92ea02a1ff0"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2451fd4f1d9513c8b57f509a0a0b3dbcee2fd368","unresolved":true,"context_lines":[{"line_number":55,"context_line":"        self.os_hash_value \u003d None"},{"line_number":56,"context_line":"        self.owner \u003d owner"},{"line_number":57,"context_line":"        self.virtual_size \u003d 0"},{"line_number":58,"context_line":"        self.tags \u003d []"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"class TestCacheMiddlewareURLMatching(testtools.TestCase):"}],"source_content_type":"text/x-python","patch_set":32,"id":"084cf82d_e65bf50d","line":58,"in_reply_to":"b31548a0_389b64d4","updated":"2021-03-06 15:23:30.000000000","message":"Agreed.","commit_id":"f01563ada091c9ca16e7dc79ceeed92ea02a1ff0"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"ecaad8e3d84d7046021d91974eea016316e219b0","unresolved":true,"context_lines":[{"line_number":390,"context_line":"            return (\u0027test1\u0027, \u0027GET\u0027, \u0027v2\u0027)"},{"line_number":391,"context_line":""},{"line_number":392,"context_line":"        def fake_get_v2_image_metadata(*args, **kwargs):"},{"line_number":393,"context_line":"            image \u003d test_policy.ImageStub("},{"line_number":394,"context_line":"                image_id, extra_properties\u003dextra_properties)"},{"line_number":395,"context_line":"            request.environ[\u0027api.cache.image\u0027] \u003d image"},{"line_number":396,"context_line":"            return glance.api.policy.ImageTarget(image)"}],"source_content_type":"text/x-python","patch_set":32,"id":"42ff7a26_31d92ed3","line":393,"range":{"start_line":393,"start_character":20,"end_line":393,"end_character":31},"updated":"2021-03-05 21:02:38.000000000","message":"Maybe we don\u0027t need to keep this anymore now that the local copy is working?","commit_id":"f01563ada091c9ca16e7dc79ceeed92ea02a1ff0"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"23b685488c36454f20915c4d98596847b79b87e6","unresolved":true,"context_lines":[{"line_number":390,"context_line":"            return (\u0027test1\u0027, \u0027GET\u0027, \u0027v2\u0027)"},{"line_number":391,"context_line":""},{"line_number":392,"context_line":"        def fake_get_v2_image_metadata(*args, **kwargs):"},{"line_number":393,"context_line":"            image \u003d test_policy.ImageStub("},{"line_number":394,"context_line":"                image_id, extra_properties\u003dextra_properties)"},{"line_number":395,"context_line":"            request.environ[\u0027api.cache.image\u0027] \u003d image"},{"line_number":396,"context_line":"            return glance.api.policy.ImageTarget(image)"}],"source_content_type":"text/x-python","patch_set":32,"id":"0d05ca18_8736431b","line":393,"range":{"start_line":393,"start_character":20,"end_line":393,"end_character":31},"in_reply_to":"42ff7a26_31d92ed3","updated":"2021-03-05 21:09:33.000000000","message":"Ah, yeah I didn\u0027t even notice, but yep.","commit_id":"f01563ada091c9ca16e7dc79ceeed92ea02a1ff0"}],"glance/tests/unit/test_policy.py":[{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"eb1621b92e0cd156b421debefe36c37874350b0d","unresolved":false,"context_lines":[{"line_number":358,"context_line":""},{"line_number":359,"context_line":"        enforcer \u003d glance.api.policy.Enforcer()"},{"line_number":360,"context_line":""},{"line_number":361,"context_line":"        unauthorized_context\u003d glance.context.RequestContext(roles\u003d[])"},{"line_number":362,"context_line":"        self.assertRaises(exception.Forbidden,"},{"line_number":363,"context_line":"                          enforcer.enforce, unauthorized_context,"},{"line_number":364,"context_line":"                          \u0027manage_image_cache\u0027, {})"}],"source_content_type":"text/x-python","patch_set":5,"id":"928f5219_cbb815f8","line":361,"in_reply_to":"1bc5a063_ba9074b0","updated":"2021-02-03 22:32:31.000000000","message":"\u003e pep8: E225 missing whitespace around operator\n\nPlease fix.","commit_id":"83c16abccb62070f14c89df752e2b368c672dc90"},{"author":{"_account_id":8122,"name":"Cyril Roelandt","email":"cyril@redhat.com","username":"cyril.roelandt.enovance"},"change_message_id":"df4cc852e63e6971c3c1a06975535c9525d8c678","unresolved":true,"context_lines":[{"line_number":429,"context_line":"        system_admin_context \u003d glance.context.RequestContext("},{"line_number":430,"context_line":"            roles\u003d[\u0027admin\u0027, \u0027member\u0027, \u0027reader\u0027],"},{"line_number":431,"context_line":"            system_scope\u003d\u0027all\u0027"},{"line_number":432,"context_line":"        )"},{"line_number":433,"context_line":"        enforcer.enforce(system_admin_context, \u0027manage_image_cache\u0027, {})"},{"line_number":434,"context_line":""},{"line_number":435,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"d76f9e61_d876ea00","line":432,"range":{"start_line":432,"start_character":0,"end_line":432,"end_character":9},"updated":"2021-02-17 19:44:02.000000000","message":"This block of code+comments appears twice in this file. Are we likely to keep using it in the future? Would it make sense to have a get_system_admin_context() method instead, maybe in glance/tests/utils.py?","commit_id":"b5f4209f1c7484da38ab492320edef8724113c46"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"489c8a8d36a64deeed49ba3daad6b1a84b478c3a","unresolved":true,"context_lines":[{"line_number":560,"context_line":"                self.context, \"get_images\", expected_target)"},{"line_number":561,"context_line":""},{"line_number":562,"context_line":"    def test_modify_image_not_allowed(self):"},{"line_number":563,"context_line":"        self.policy.enforce.side_effect \u003d exception.Forbidden"},{"line_number":564,"context_line":"        image_repo \u003d glance.api.policy.ImageRepoProxy("},{"line_number":565,"context_line":"            self.image_repo_stub, self.context, self.policy)"},{"line_number":566,"context_line":"        image \u003d glance.api.policy.ImageProxy("}],"source_content_type":"text/x-python","patch_set":19,"id":"e263d469_fa61ba05","line":563,"range":{"start_line":563,"start_character":8,"end_line":563,"end_character":61},"updated":"2021-03-02 15:56:39.000000000","message":"It seems like we\u0027re short-circuiting all of the important policy checks with a mock here, instead of actually relying on the policy stuff to reject. I know that was already here in these tests, but is there any reason we should not go ahead and modify these to actually fail or pass based on the policy check? Or at least add some more tests that will do that. Honestly, maybe functional tests would be the better option, but it feels to me like we\u0027re testing the edges of this (that enforce is called as we expect) but not really validating that we\u0027re able to enforce the important things (i.e. the rejections).","commit_id":"db31004f723c601bae58e41fa7bd07bf7492b271"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ba595c2eece9c604fb42e9b8d26e7faee50f1ff7","unresolved":true,"context_lines":[{"line_number":560,"context_line":"                self.context, \"get_images\", expected_target)"},{"line_number":561,"context_line":""},{"line_number":562,"context_line":"    def test_modify_image_not_allowed(self):"},{"line_number":563,"context_line":"        self.policy.enforce.side_effect \u003d exception.Forbidden"},{"line_number":564,"context_line":"        image_repo \u003d glance.api.policy.ImageRepoProxy("},{"line_number":565,"context_line":"            self.image_repo_stub, self.context, self.policy)"},{"line_number":566,"context_line":"        image \u003d glance.api.policy.ImageProxy("}],"source_content_type":"text/x-python","patch_set":19,"id":"a1938104_9c45e73b","line":563,"range":{"start_line":563,"start_character":8,"end_line":563,"end_character":61},"in_reply_to":"e1314b3a_ed218799","updated":"2021-03-02 17:09:00.000000000","message":"Yeah, I\u0027ve skimmed those tests and think that my concerns are probably covered (or cover-able) there but need to dig in a little deeper.","commit_id":"db31004f723c601bae58e41fa7bd07bf7492b271"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"e001ffde25d27a6b433b927e70b0f0e90bf8c0c0","unresolved":true,"context_lines":[{"line_number":560,"context_line":"                self.context, \"get_images\", expected_target)"},{"line_number":561,"context_line":""},{"line_number":562,"context_line":"    def test_modify_image_not_allowed(self):"},{"line_number":563,"context_line":"        self.policy.enforce.side_effect \u003d exception.Forbidden"},{"line_number":564,"context_line":"        image_repo \u003d glance.api.policy.ImageRepoProxy("},{"line_number":565,"context_line":"            self.image_repo_stub, self.context, self.policy)"},{"line_number":566,"context_line":"        image \u003d glance.api.policy.ImageProxy("}],"source_content_type":"text/x-python","patch_set":19,"id":"e1314b3a_ed218799","line":563,"range":{"start_line":563,"start_character":8,"end_line":563,"end_character":61},"in_reply_to":"e263d469_fa61ba05","updated":"2021-03-02 16:57:18.000000000","message":"Correct - the only thing this is really testing is the order of the arguments and the fact that mock is doing the right thing \u003d/\n\nI left this the way it is since it existed prior to these changes and I put the protection testing in a full integration test with glance-tempest-plugin.","commit_id":"db31004f723c601bae58e41fa7bd07bf7492b271"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"dbc8e8927c6381eb2e92bc1c9631b40d06620f7d","unresolved":true,"context_lines":[{"line_number":524,"context_line":"            target.return_value \u003d image_target"},{"line_number":525,"context_line":"            image_repo \u003d glance.api.policy.ImageRepoProxy("},{"line_number":526,"context_line":"                self.image_repo_stub, self.context, self.policy)"},{"line_number":527,"context_line":"            self.assertRaises(exception.NotFound, image_repo.get, UUID1)"},{"line_number":528,"context_line":"        expected_target \u003d {\u0027project_id\u0027: \u0027tenant1\u0027}"},{"line_number":529,"context_line":"        self.policy.enforce.assert_called_once_with("},{"line_number":530,"context_line":"            self.context, \"get_image\", expected_target)"}],"source_content_type":"text/x-python","patch_set":33,"id":"f9a0d61b_0c2495dd","line":527,"updated":"2021-03-07 16:24:07.000000000","message":"Okay, note to self, this is the validation of this behavioral change:\n\nhttps://review.opendev.org/c/openstack/glance/+/764754/33/glance/api/policy.py#130","commit_id":"8b07f09af549df39f4a893ab8736aee880660ef0"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bd0215aa441622243cbc3004874d4be13a9ba5cc","unresolved":true,"context_lines":[{"line_number":1096,"context_line":"    def test_image_target_ignores_locations(self):"},{"line_number":1097,"context_line":"        image \u003d ImageStub()"},{"line_number":1098,"context_line":"        target \u003d glance.api.policy.ImageTarget(image)"},{"line_number":1099,"context_line":"        self.assertNotIn(\u0027locations\u0027, list(target))"}],"source_content_type":"text/x-python","patch_set":34,"id":"040ee8e7_25eb9afa","line":1099,"updated":"2021-03-07 16:35:36.000000000","message":"Added this to address my comment.","commit_id":"fc3484bc7696492bbc0d3863a407f0b618341965"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2753eb93a3236216d7264896d17818ddbd7de1ca","unresolved":true,"context_lines":[{"line_number":1096,"context_line":"    def test_image_target_ignores_locations(self):"},{"line_number":1097,"context_line":"        image \u003d ImageStub()"},{"line_number":1098,"context_line":"        target \u003d glance.api.policy.ImageTarget(image)"},{"line_number":1099,"context_line":"        self.assertNotIn(\u0027locations\u0027, list(target))"}],"source_content_type":"text/x-python","patch_set":34,"id":"b0ad2b66_cd29d152","line":1099,"in_reply_to":"040ee8e7_25eb9afa","updated":"2021-03-08 13:40:33.000000000","message":"Nice - thanks for adding this.","commit_id":"fc3484bc7696492bbc0d3863a407f0b618341965"}]}