)]}'
{"keystone/federation/controllers.py":[{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"3d638ca487e2b70622aeab96d40686eb0d542401","unresolved":false,"context_lines":[{"line_number":125,"context_line":"        # then we need to invalidate the token cache. Otherwise it will be"},{"line_number":126,"context_line":"        # possible for federated tokens to be considered valid after a service"},{"line_number":127,"context_line":"        # provider removes a federated identity provider resource."},{"line_number":128,"context_line":"        token_provider.TOKENS_REGION.invalidate()"},{"line_number":129,"context_line":""},{"line_number":130,"context_line":"    @controller.protected()"},{"line_number":131,"context_line":"    def update_identity_provider(self, request, idp_id, identity_provider):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9f91af0f_5ff507ec","line":128,"updated":"2018-01-08 22:20:15.000000000","message":"We take a similar approach in the resource manager, but doing this in the federation manager creates a circular dependency [0]. To prove that this worked I pulled this up the controller layer (which might not be the right place for this). \n\nOne alternative we can do is to create a called back to revoke the token cache instead.\n\n[0] https://github.com/openstack/keystone/blob/2c2cc6ce2e12fc4758cf5e45cb20269a15fcb834/keystone/resource/core.py#L382-L392","commit_id":"1316f7b0dfabc9d20cdfca74786809bd1d209189"}],"keystone/tests/unit/test_v3_federation.py":[{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"3d638ca487e2b70622aeab96d40686eb0d542401","unresolved":false,"context_lines":[{"line_number":1822,"context_line":"    def auth_plugin_config_override(self):"},{"line_number":1823,"context_line":"        # NOTE(lbragstad): Even though these are federation tests, we pass"},{"line_number":1824,"context_line":"        # \u0027password\u0027 in as an auth plugin so that we can use the self.\u003cmethod\u003e"},{"line_number":1825,"context_line":"        # testing infrastructure with an administrator account."},{"line_number":1826,"context_line":"        methods \u003d [\u0027saml2\u0027, \u0027token\u0027, \u0027password\u0027]"},{"line_number":1827,"context_line":"        super(FederatedTokenTests, self).auth_plugin_config_override(methods)"},{"line_number":1828,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"9f91af0f_df2fd785","line":1825,"updated":"2018-01-08 22:20:15.000000000","message":"Not sure if this is the best way to do this... or if this should just be pulled into the test itself.","commit_id":"b386ba9b1344118ca1b4b122eaab60c0d57531ca"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"f23692ba23fd9f57769885725f112ead37fab305","unresolved":false,"context_lines":[{"line_number":2073,"context_line":"            \u0027X-Subject-Token\u0027: token_id"},{"line_number":2074,"context_line":"        }"},{"line_number":2075,"context_line":"        # NOTE(lbragstad): This raises a 404 NOT FOUND because the identity"},{"line_number":2076,"context_line":"        # provider is no longer present. We raise 404 NOT FOUND when we"},{"line_number":2077,"context_line":"        # validate a token and a project or domain no longer exists."},{"line_number":2078,"context_line":"        self.get("},{"line_number":2079,"context_line":"            \u0027/auth/tokens/\u0027,"},{"line_number":2080,"context_line":"            token\u003dtoken_id,"}],"source_content_type":"text/x-python","patch_set":5,"id":"5f93b717_1fbe70b3","line":2077,"range":{"start_line":2076,"start_character":41,"end_line":2077,"end_character":67},"updated":"2018-02-01 17:34:36.000000000","message":"Huh, is that correct though?","commit_id":"025d691a97b15727ec393bfb9a950a346dfac077"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"0b6780dd87ed799df5ee430acf7c858147e01c56","unresolved":false,"context_lines":[{"line_number":2073,"context_line":"            \u0027X-Subject-Token\u0027: token_id"},{"line_number":2074,"context_line":"        }"},{"line_number":2075,"context_line":"        # NOTE(lbragstad): This raises a 404 NOT FOUND because the identity"},{"line_number":2076,"context_line":"        # provider is no longer present. We raise 404 NOT FOUND when we"},{"line_number":2077,"context_line":"        # validate a token and a project or domain no longer exists."},{"line_number":2078,"context_line":"        self.get("},{"line_number":2079,"context_line":"            \u0027/auth/tokens/\u0027,"},{"line_number":2080,"context_line":"            token\u003dtoken_id,"}],"source_content_type":"text/x-python","patch_set":5,"id":"5f93b717_304efbc0","line":2077,"range":{"start_line":2076,"start_character":41,"end_line":2077,"end_character":67},"in_reply_to":"5f93b717_10d89f78","updated":"2018-02-01 19:45:27.000000000","message":"I think where we\u0027re already raising 404s we can\u0027t trade it for a 401. But I think we could trade the 404 Project Not Found for a 404 Token Not Found.\n\nFor the case of deleting an IdP I\u0027m not sure what we should do. I think a 404 is not correct and also a bit too much information from a security point of view. But keeping things consistent is valuable too. ¯\\_(ツ)_/¯","commit_id":"025d691a97b15727ec393bfb9a950a346dfac077"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"692967373dbe7fcada8ced3fad5664718821425b","unresolved":false,"context_lines":[{"line_number":2073,"context_line":"            \u0027X-Subject-Token\u0027: token_id"},{"line_number":2074,"context_line":"        }"},{"line_number":2075,"context_line":"        # NOTE(lbragstad): This raises a 404 NOT FOUND because the identity"},{"line_number":2076,"context_line":"        # provider is no longer present. We raise 404 NOT FOUND when we"},{"line_number":2077,"context_line":"        # validate a token and a project or domain no longer exists."},{"line_number":2078,"context_line":"        self.get("},{"line_number":2079,"context_line":"            \u0027/auth/tokens/\u0027,"},{"line_number":2080,"context_line":"            token\u003dtoken_id,"}],"source_content_type":"text/x-python","patch_set":5,"id":"5f93b717_7f41b4a8","line":2077,"range":{"start_line":2076,"start_character":41,"end_line":2077,"end_character":67},"in_reply_to":"5f93b717_1fbe70b3","updated":"2018-02-01 17:40:11.000000000","message":"https://specs.openstack.org/openstack/api-wg/guidelines/http.html#failure-code-clarifications\n\n\"RFC 7231 (section 6.5.4) mentions the origin server did not find a current representation for the target resource for 404 and representation for the target resource means a URI.\"\n\nIf we\u0027ve been using resource_api.get_project() and just raising the 404 from that I think that\u0027s from laziness not correctness.","commit_id":"025d691a97b15727ec393bfb9a950a346dfac077"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"fe2bca5ed18496742c75c80ec67d16c848899b14","unresolved":false,"context_lines":[{"line_number":2073,"context_line":"            \u0027X-Subject-Token\u0027: token_id"},{"line_number":2074,"context_line":"        }"},{"line_number":2075,"context_line":"        # NOTE(lbragstad): This raises a 404 NOT FOUND because the identity"},{"line_number":2076,"context_line":"        # provider is no longer present. We raise 404 NOT FOUND when we"},{"line_number":2077,"context_line":"        # validate a token and a project or domain no longer exists."},{"line_number":2078,"context_line":"        self.get("},{"line_number":2079,"context_line":"            \u0027/auth/tokens/\u0027,"},{"line_number":2080,"context_line":"            token\u003dtoken_id,"}],"source_content_type":"text/x-python","patch_set":5,"id":"5f93b717_e6be99d3","line":2077,"range":{"start_line":2076,"start_character":41,"end_line":2077,"end_character":67},"in_reply_to":"5f93b717_304efbc0","updated":"2018-02-01 21:14:29.000000000","message":"Right, I agree. I meant we should trade raising a 404 Project Not Found from the resource API to raising a 401 Unauthorized, which gets excepted later and traded for a 404 Token Not Found. We\u0027d be invoking a 404 by way of raising a 401, which does seem kind of funny but apparently that seems like the intended way to do things based on how the business logic of the token provider API is setup - shrug... idk, i just work here :) \n\nI can pull this down and work on another revision to see what that would look like if that helps.","commit_id":"025d691a97b15727ec393bfb9a950a346dfac077"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"57a0067778505ca5f1a0aeca0276881e05b39fd7","unresolved":false,"context_lines":[{"line_number":2073,"context_line":"            \u0027X-Subject-Token\u0027: token_id"},{"line_number":2074,"context_line":"        }"},{"line_number":2075,"context_line":"        # NOTE(lbragstad): This raises a 404 NOT FOUND because the identity"},{"line_number":2076,"context_line":"        # provider is no longer present. We raise 404 NOT FOUND when we"},{"line_number":2077,"context_line":"        # validate a token and a project or domain no longer exists."},{"line_number":2078,"context_line":"        self.get("},{"line_number":2079,"context_line":"            \u0027/auth/tokens/\u0027,"},{"line_number":2080,"context_line":"            token\u003dtoken_id,"}],"source_content_type":"text/x-python","patch_set":5,"id":"5f93b717_004116a9","line":2077,"range":{"start_line":2076,"start_character":41,"end_line":2077,"end_character":67},"in_reply_to":"5f93b717_467aedb9","updated":"2018-02-02 16:23:04.000000000","message":"Ok - I did some more digging here because I wanted to try and prove that catching an IdentityProviderNotFound and raising an Unauthorized would result in a 404 TokenNotFound... but that actually doesn\u0027t happen because of *another* try/except in keystonemiddleware.\n\nHere\u0027s the diff I applied locally to see what was getting raise and where [0]. Here is the output from the test [1].\n\nSo even given the way things operate in keystone, we\u0027d have to patch middleware to get the behavior we expect (which would be raise 404, except it, reraise 401, except it, raise a 404 TokenNotFound).\n\n\n[0] http://paste.openstack.org/show/658836/\n[1] http://paste.openstack.org/show/658839/","commit_id":"025d691a97b15727ec393bfb9a950a346dfac077"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"d0575d7285f1e278c076975ccae04d74a1d4e072","unresolved":false,"context_lines":[{"line_number":2073,"context_line":"            \u0027X-Subject-Token\u0027: token_id"},{"line_number":2074,"context_line":"        }"},{"line_number":2075,"context_line":"        # NOTE(lbragstad): This raises a 404 NOT FOUND because the identity"},{"line_number":2076,"context_line":"        # provider is no longer present. We raise 404 NOT FOUND when we"},{"line_number":2077,"context_line":"        # validate a token and a project or domain no longer exists."},{"line_number":2078,"context_line":"        self.get("},{"line_number":2079,"context_line":"            \u0027/auth/tokens/\u0027,"},{"line_number":2080,"context_line":"            token\u003dtoken_id,"}],"source_content_type":"text/x-python","patch_set":5,"id":"5f93b717_10d89f78","line":2077,"range":{"start_line":2076,"start_character":41,"end_line":2077,"end_character":67},"in_reply_to":"5f93b717_7f41b4a8","updated":"2018-02-01 19:30:08.000000000","message":"You\u0027re probably right, but I think that\u0027s how the API has always behaved. Digging into the code we do something that makes sense in the validate_token method, which is wrap the call to validate the token with a try/except. If we catch an Unauthorized, we raise a Not Found [0]. But in other parts of token validation, when we go to validate the project information, we raise a 404 Project Not Found if the project doesn\u0027t exist [1]. Since they are both 404s, I wonder if we can raise a 401 in the cases where the project or identity provider doesn\u0027t exist and trade that for a 404 Token Not Found instead.\n\nThought?\n\n[0] https://github.com/openstack/keystone/blob/d3818f05e31729ac86415d6f114f658d98a704ab/keystone/token/provider.py#L146-L161\n[1] https://github.com/openstack/keystone/blob/d3818f05e31729ac86415d6f114f658d98a704ab/keystone/token/providers/common.py#L138-L143","commit_id":"025d691a97b15727ec393bfb9a950a346dfac077"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"76883ef70b3cb028f1131e0cfc5a0e32e86ea9f1","unresolved":false,"context_lines":[{"line_number":2073,"context_line":"            \u0027X-Subject-Token\u0027: token_id"},{"line_number":2074,"context_line":"        }"},{"line_number":2075,"context_line":"        # NOTE(lbragstad): This raises a 404 NOT FOUND because the identity"},{"line_number":2076,"context_line":"        # provider is no longer present. We raise 404 NOT FOUND when we"},{"line_number":2077,"context_line":"        # validate a token and a project or domain no longer exists."},{"line_number":2078,"context_line":"        self.get("},{"line_number":2079,"context_line":"            \u0027/auth/tokens/\u0027,"},{"line_number":2080,"context_line":"            token\u003dtoken_id,"}],"source_content_type":"text/x-python","patch_set":5,"id":"5f93b717_467aedb9","line":2077,"range":{"start_line":2076,"start_character":41,"end_line":2077,"end_character":67},"in_reply_to":"5f93b717_e6be99d3","updated":"2018-02-01 21:33:01.000000000","message":"Ugh yeah I don\u0027t know. I\u0027m fine with keeping it consistent and having it raise a 404.\n\nIt does seem to need a pep8 fix though.","commit_id":"025d691a97b15727ec393bfb9a950a346dfac077"}],"keystone/token/provider.py":[{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"bb0e882e8cc0356781daa7dd808e6e67a608c922","unresolved":false,"context_lines":[{"line_number":346,"context_line":"          )"},{"line_number":347,"context_line":""},{"line_number":348,"context_line":"        \"\"\""},{"line_number":349,"context_line":"        TOKENS_REGION.invalidate()"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f91af0f_10f3d275","line":349,"updated":"2018-01-09 22:02:09.000000000","message":"All the other callbacks here also invalidate the token cache, but they\u0027re named descriptively. The event shows up in the logs (and I guess in whatever CADF listener someone has set up?), so it\u0027s nice if it\u0027s descriptive. For example when a role assignment is removed it looks like this:\n\n Invoking callback _delete_user_project_tokens_callback for event identity invalidate_user_project_tokens internal for {\u0027resource_info\u0027: {\u0027project_id\u0027: u\u00278fce84e45fe8484f9bac5c6b74260e9b\u0027, \u0027user_id\u0027: u\u0027a6dce24c34914450b092dc08a6f2e23b\u0027}}\n\nWith your change, when an identity provider is removed, the log looks like this:\n\n Invoking callback _invalidate_token_cache_callback for event identity invalidate_token_cache internal for {\u0027resource_info\u0027: u\u0027myidp\u0027}\n\nNo way to know that was related to the removal of a federated identity provider.\n\nDo you think that information is important?","commit_id":"4615b20ab7306e7bd163ee796d8b196be7d2b776"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"44702ed3d0592c32e7a8a4cabf767a24ecb6c55a","unresolved":false,"context_lines":[{"line_number":346,"context_line":"          )"},{"line_number":347,"context_line":""},{"line_number":348,"context_line":"        \"\"\""},{"line_number":349,"context_line":"        TOKENS_REGION.invalidate()"}],"source_content_type":"text/x-python","patch_set":3,"id":"9f91af0f_f0b98e78","line":349,"in_reply_to":"9f91af0f_10f3d275","updated":"2018-01-09 22:06:03.000000000","message":"I would consider that to be important information. Maybe we need to elaborate on the notification implementation. It\u0027d be nice to not have to pass information that is not relevant but get more descriptive logging, I agree.\n\nI\u0027ll keep tinkering with this and see what I can come up with.","commit_id":"4615b20ab7306e7bd163ee796d8b196be7d2b776"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"74a255f8417e76614cc9d2f80ad58fee674a6856","unresolved":false,"context_lines":[{"line_number":346,"context_line":"          )"},{"line_number":347,"context_line":""},{"line_number":348,"context_line":"        \"\"\""},{"line_number":349,"context_line":"        TOKENS_REGION.invalidate()"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f93b717_61572028","line":349,"in_reply_to":"9f91af0f_f0b98e78","updated":"2018-01-30 15:39:05.000000000","message":"The only change I made was to make the method name more specific:\n\n  Invoking callback _invalidate_token_cache_from_deleted_idp_callback for event identity invalidate_token_cache internal for {\u0027resource_info\u0027: u\u0027my_idp\u0027}\n\nWhich makes things a bit easier to put together when you read the message. I suppose this won\u0027t make the invalidate token cache callback as reusable as I was hoping, but maybe that\u0027s OK since the method name is used in debug logging with data in the payload that can be different.","commit_id":"4615b20ab7306e7bd163ee796d8b196be7d2b776"},{"author":{"_account_id":16465,"name":"Kristi Nikolla","email":"knikolla@bu.edu","username":"knikolla"},"change_message_id":"483bab818c3cefd35a73b14d92f8939c38235d2b","unresolved":false,"context_lines":[{"line_number":91,"context_line":"                    self._delete_user_project_tokens_callback],"},{"line_number":92,"context_line":"                [notifications.INVALIDATE_USER_OAUTH_CONSUMER_TOKENS,"},{"line_number":93,"context_line":"                    self._delete_user_oauth_consumer_tokens_callback],"},{"line_number":94,"context_line":"                [notifications.INVALIDATE_TOKEN_CACHE,"},{"line_number":95,"context_line":"                    self._invalidate_token_cache_from_deleted_idp_callback],"},{"line_number":96,"context_line":"            ]"},{"line_number":97,"context_line":"        }"},{"line_number":98,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"5f93b717_fc015206","line":95,"range":{"start_line":94,"start_character":17,"end_line":95,"end_character":74},"updated":"2018-02-01 16:50:57.000000000","message":"You\u0027re mapping from a generic `invalidate token cache` notification, to a more specific one about `deleted idp`. This doesn\u0027t seem right. One thought, could you use one of the `ACTIONS.deleted` notifications like in a few lines above instead of creating a new one?","commit_id":"fa6052b530b7d182a3e52a116bb7178fb055b45d"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"8426211a72f74995b5598fcc5860aaecc806cee9","unresolved":false,"context_lines":[{"line_number":91,"context_line":"                    self._delete_user_project_tokens_callback],"},{"line_number":92,"context_line":"                [notifications.INVALIDATE_USER_OAUTH_CONSUMER_TOKENS,"},{"line_number":93,"context_line":"                    self._delete_user_oauth_consumer_tokens_callback],"},{"line_number":94,"context_line":"                [notifications.INVALIDATE_TOKEN_CACHE,"},{"line_number":95,"context_line":"                    self._invalidate_token_cache_from_deleted_idp_callback],"},{"line_number":96,"context_line":"            ]"},{"line_number":97,"context_line":"        }"},{"line_number":98,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"5f93b717_3c625ad9","line":95,"range":{"start_line":94,"start_character":17,"end_line":95,"end_character":74},"in_reply_to":"5f93b717_fc015206","updated":"2018-02-01 16:53:29.000000000","message":"Colleen had a comment about this in a previous patch set, which is why i decided to make it specific to the deletion of the idp.","commit_id":"fa6052b530b7d182a3e52a116bb7178fb055b45d"}]}