)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1916,"name":"Guang Yee","email":"gyee@suse.com","username":"guang-yee"},"change_message_id":"5a5564b4488854ec48f4a9648a96d24f94bc1349","unresolved":false,"context_lines":[{"line_number":5,"context_line":"CommitDate: 2019-04-09 23:13:53 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Fixing dn_to_id function for cases where id is not in the DN"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The more common scenario to return the uid as part of the RDN in a DN,"},{"line_number":10,"context_line":"However, it\u0027s a valid case to not have the uid in the RDN, so we need to"},{"line_number":11,"context_line":"search in the LDAP based on the DN and return the uid in the entire object."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"3fce034c_206c5207","line":8,"updated":"2019-04-10 23:23:54.000000000","message":"is there a bug for this?","commit_id":"10654b3556bee22a7f4c63619162771e1f082fb5"}],"keystone/identity/backends/ldap/common.py":[{"author":{"_account_id":9098,"name":"Nathan Kinder","email":"nkinder@redhat.com","username":"nkinder"},"change_message_id":"db93b629002cf90b7cda461e9b7734ec4d1ef100","unresolved":false,"context_lines":[{"line_number":1295,"context_line":""},{"line_number":1296,"context_line":"    def _dn_to_id(self, dn):"},{"line_number":1297,"context_line":"        # Check if the naming attribute in the DN is the same as keystone\u0027s"},{"line_number":1298,"context_line":"        # configured \u0027id\u0027 attribute\u0027.  If so, extract the ID value from the"},{"line_number":1299,"context_line":"        if self.id_attr \u003d\u003d utf8_decode("},{"line_number":1300,"context_line":"                ldap.dn.str2dn(utf8_encode(dn))[0][0][0].lower()):"},{"line_number":1301,"context_line":"            return utf8_decode(ldap.dn.str2dn(utf8_encode(dn))[0][0][1])"}],"source_content_type":"text/x-python","patch_set":5,"id":"5fc1f717_3c6a61b2","line":1298,"updated":"2019-04-10 18:47:54.000000000","message":"(nit) Your comment is truncated.  I assume you meant \"extract the ID value from the DN.\".","commit_id":"9ab921f4ce971895d22e19482f8ad23c1be96729"},{"author":{"_account_id":9098,"name":"Nathan Kinder","email":"nkinder@redhat.com","username":"nkinder"},"change_message_id":"db93b629002cf90b7cda461e9b7734ec4d1ef100","unresolved":false,"context_lines":[{"line_number":1308,"context_line":"            if search_result:"},{"line_number":1309,"context_line":"                id_attr \u003d search_result[0][1][self.id_attr][0]"},{"line_number":1310,"context_line":"                if len(id_attr) \u003e 1:"},{"line_number":1311,"context_line":"                    raise exception.Conflict(type\u003did_attr,"},{"line_number":1312,"context_line":"                                             details\u003d_(\u0027Multiple id, %s.\u0027) %"},{"line_number":1313,"context_line":"                                             id_attr)"},{"line_number":1314,"context_line":"                else:"}],"source_content_type":"text/x-python","patch_set":5,"id":"5fc1f717_fc8db97b","line":1311,"updated":"2019-04-10 18:47:54.000000000","message":"Is it possible to add a test that triggers this exception?","commit_id":"9ab921f4ce971895d22e19482f8ad23c1be96729"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"720552d9e60cb6eb6594291a71b9f610f238bb14","unresolved":false,"context_lines":[{"line_number":1295,"context_line":""},{"line_number":1296,"context_line":"    def _dn_to_id(self, dn):"},{"line_number":1297,"context_line":"        # Check if the naming attribute in the DN is the same as keystone\u0027s"},{"line_number":1298,"context_line":"        # configured \u0027id\u0027 attribute\u0027.  If so, extract the ID value from the"},{"line_number":1299,"context_line":"        if self.id_attr \u003d\u003d utf8_decode("},{"line_number":1300,"context_line":"                ldap.dn.str2dn(utf8_encode(dn))[0][0][0].lower()):"},{"line_number":1301,"context_line":"            return utf8_decode(ldap.dn.str2dn(utf8_encode(dn))[0][0][1])"}],"source_content_type":"text/x-python","patch_set":6,"id":"3fce034c_4c051f7a","line":1298,"range":{"start_line":1298,"start_character":67,"end_line":1298,"end_character":75},"updated":"2019-04-10 19:43:02.000000000","message":"from the ?","commit_id":"10654b3556bee22a7f4c63619162771e1f082fb5"},{"author":{"_account_id":8866,"name":"Raildo Mascena de Sousa Filho","email":"rmascena@redhat.com","username":"raildo"},"change_message_id":"6727b21b7bb18adffa9151a391f176dbb5fc9065","unresolved":false,"context_lines":[{"line_number":1295,"context_line":""},{"line_number":1296,"context_line":"    def _dn_to_id(self, dn):"},{"line_number":1297,"context_line":"        # Check if the naming attribute in the DN is the same as keystone\u0027s"},{"line_number":1298,"context_line":"        # configured \u0027id\u0027 attribute\u0027.  If so, extract the ID value from the"},{"line_number":1299,"context_line":"        if self.id_attr \u003d\u003d utf8_decode("},{"line_number":1300,"context_line":"                ldap.dn.str2dn(utf8_encode(dn))[0][0][0].lower()):"},{"line_number":1301,"context_line":"            return utf8_decode(ldap.dn.str2dn(utf8_encode(dn))[0][0][1])"}],"source_content_type":"text/x-python","patch_set":6,"id":"3fce034c_6c501b34","line":1298,"range":{"start_line":1298,"start_character":67,"end_line":1298,"end_character":75},"in_reply_to":"3fce034c_4c051f7a","updated":"2019-04-10 19:46:13.000000000","message":"damn, I deleted the last comment line, I\u0027ll update it properly.","commit_id":"10654b3556bee22a7f4c63619162771e1f082fb5"},{"author":{"_account_id":9098,"name":"Nathan Kinder","email":"nkinder@redhat.com","username":"nkinder"},"change_message_id":"02f300b58d5672d493e1c827682c4bec32df74b4","unresolved":false,"context_lines":[{"line_number":1307,"context_line":""},{"line_number":1308,"context_line":"            if search_result:"},{"line_number":1309,"context_line":"                id_attr \u003d search_result[0][1][self.id_attr][0]"},{"line_number":1310,"context_line":"                if len(id_attr) \u003e 1:"},{"line_number":1311,"context_line":"                    raise exception.Conflict(type\u003did_attr,"},{"line_number":1312,"context_line":"                                             details\u003d_(\u0027Multiple id, %s.\u0027) %"},{"line_number":1313,"context_line":"                                             id_attr)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5fc1f717_3c226153","line":1310,"updated":"2019-04-10 18:57:00.000000000","message":"This looks like it might be incorrect.  We should have a string with the first ID value in \u0027id_attr\u0027.  You are trying to check for a multi-valued LDAP attribute, so I think you would want to check how many items are in here instead:\n\n  search_result[0][1][self.id_attr]","commit_id":"10654b3556bee22a7f4c63619162771e1f082fb5"},{"author":{"_account_id":2218,"name":"Adam Young","email":"adam@younglogic.com","username":"ayoung"},"change_message_id":"503705ab2d3abaaede2c626e762d1cb8baa548d8","unresolved":false,"context_lines":[{"line_number":1307,"context_line":""},{"line_number":1308,"context_line":"            if search_result:"},{"line_number":1309,"context_line":"                id_attr \u003d search_result[0][1][self.id_attr][0]"},{"line_number":1310,"context_line":"                if len(id_attr) \u003e 1:"},{"line_number":1311,"context_line":"                    raise exception.Conflict(type\u003did_attr,"},{"line_number":1312,"context_line":"                                             details\u003d_(\u0027Multiple id, %s.\u0027) %"},{"line_number":1313,"context_line":"                                             id_attr)"}],"source_content_type":"text/x-python","patch_set":6,"id":"dfbec78f_ac641423","line":1310,"in_reply_to":"3fce034c_fb37dbe2","updated":"2019-05-14 17:03:11.000000000","message":"I wrote it, long before any of you were on the project.  Apologies.\nIs it safe to say that there is ALWAYS going to be an attribute in the user object that will contain the ID?  THe assumption when I wrote it was the ID was part of the DN, but that was long since supplanted by other mechansims.  I thought that, if you specified an ID attribute, this code was skipped.","commit_id":"10654b3556bee22a7f4c63619162771e1f082fb5"},{"author":{"_account_id":9098,"name":"Nathan Kinder","email":"nkinder@redhat.com","username":"nkinder"},"change_message_id":"61e2c35a9ebe1244521fbd5e2ef9d9ecc72b2c01","unresolved":false,"context_lines":[{"line_number":1307,"context_line":""},{"line_number":1308,"context_line":"            if search_result:"},{"line_number":1309,"context_line":"                id_attr \u003d search_result[0][1][self.id_attr][0]"},{"line_number":1310,"context_line":"                if len(id_attr) \u003e 1:"},{"line_number":1311,"context_line":"                    raise exception.Conflict(type\u003did_attr,"},{"line_number":1312,"context_line":"                                             details\u003d_(\u0027Multiple id, %s.\u0027) %"},{"line_number":1313,"context_line":"                                             id_attr)"}],"source_content_type":"text/x-python","patch_set":6,"id":"3fce034c_4c367fb7","line":1310,"in_reply_to":"5fc1f717_3c226153","updated":"2019-04-10 19:20:23.000000000","message":"My comment might be a bit unclear.  You are successfully extracting the first value of the ID attribute from the LDAP entry here at line 1309.  The check at line 1310 is supposed to check if you have multiple values in LDAP for the ID attribute, but you are checking the length of the first ID value.  I propose you do something like this:\n\n  if search_result:\n      if len(search_result[0][1][self.id_attr]) \u003e 1:\n          raise exception....\n      else:\n          return search_result[0][1][self.id_attr][0]\n\nWe really have 4 tests that should exercise this code:\n\n1 - The successful case where the ID attribute is single-valued and exists in the DN.  This is very common, and should already be covered by some of the basic LDAP tests we have.\n\n2 - The successful case where the ID attribute is single-valued and DOES NOT exist in the DN.  This is covered by your new \u0027test_get_id_from_dn\u0027 test in patch set 6.\n\n3 - The successful case where the ID attribute is multi-valued, but one of those values exists in the DN.  While we can\u0027t reliably support multi-valued ID attributes, we already used a heuristic to check if the ID attribute is in the DN and to just use that value even if more values exist in the entry.  Your new code still handles this case, but we do not have test coverage for it.  Please add a test.\n\n4 - The failure case where the ID attribute is multi-valued and DOES NOT exist in the DN.  In this case, an exception should be raised since we can\u0027t reliably determine the correct ID value to use.  You should cover this in your new \u0027test_raise_not_found_dn_for_multivalued_attribute_id\u0027 test in patch set 6, but the exception in that test seems wrong.  It currently looks for exception.NotFound, which means the LDAP entry wasn\u0027t even found.  I would expect us to hit exception.Conflict instead, thrown from line 1311 here in this file.","commit_id":"10654b3556bee22a7f4c63619162771e1f082fb5"},{"author":{"_account_id":8866,"name":"Raildo Mascena de Sousa Filho","email":"rmascena@redhat.com","username":"raildo"},"change_message_id":"9bf83147e8118dcad27b3cdc7af13ecfc897b42d","unresolved":false,"context_lines":[{"line_number":1307,"context_line":""},{"line_number":1308,"context_line":"            if search_result:"},{"line_number":1309,"context_line":"                id_attr \u003d search_result[0][1][self.id_attr][0]"},{"line_number":1310,"context_line":"                if len(id_attr) \u003e 1:"},{"line_number":1311,"context_line":"                    raise exception.Conflict(type\u003did_attr,"},{"line_number":1312,"context_line":"                                             details\u003d_(\u0027Multiple id, %s.\u0027) %"},{"line_number":1313,"context_line":"                                             id_attr)"}],"source_content_type":"text/x-python","patch_set":6,"id":"5fc1f717_dcbf9dcd","line":1310,"in_reply_to":"5fc1f717_3c226153","updated":"2019-04-10 19:21:02.000000000","message":"good catch, I\u0027m going to update this in a follow-up patch.","commit_id":"10654b3556bee22a7f4c63619162771e1f082fb5"},{"author":{"_account_id":1916,"name":"Guang Yee","email":"gyee@suse.com","username":"guang-yee"},"change_message_id":"5a5564b4488854ec48f4a9648a96d24f94bc1349","unresolved":false,"context_lines":[{"line_number":1307,"context_line":""},{"line_number":1308,"context_line":"            if search_result:"},{"line_number":1309,"context_line":"                id_attr \u003d search_result[0][1][self.id_attr][0]"},{"line_number":1310,"context_line":"                if len(id_attr) \u003e 1:"},{"line_number":1311,"context_line":"                    raise exception.Conflict(type\u003did_attr,"},{"line_number":1312,"context_line":"                                             details\u003d_(\u0027Multiple id, %s.\u0027) %"},{"line_number":1313,"context_line":"                                             id_attr)"}],"source_content_type":"text/x-python","patch_set":6,"id":"3fce034c_fb37dbe2","line":1310,"in_reply_to":"5fc1f717_dcbf9dcd","updated":"2019-04-10 23:23:54.000000000","message":"I don\u0027t think we should use this logic to determine the ID at all. It\u0027s rather inconsistent and unreliable as Nathan had indicated earlier. We should\u0027ve always use search to determine the ID.\n\nIf memory serves, I didn\u0027t introduce this code btw. :-) I couldn\u0027t get rid of it fast enough because we were still supporting read/write LDAP at the time I believe. Maybe now is the *right time* to get rid of it.","commit_id":"10654b3556bee22a7f4c63619162771e1f082fb5"},{"author":{"_account_id":8116,"name":"John Dennis","email":"jdennis@redhat.com","username":"jdennis"},"change_message_id":"4eebc8106cadc826fea1077254ec74cd6f07a9d3","unresolved":false,"context_lines":[{"line_number":1306,"context_line":"                search_result \u003d conn.search_s(dn, self.LDAP_SCOPE)"},{"line_number":1307,"context_line":""},{"line_number":1308,"context_line":"            if search_result:"},{"line_number":1309,"context_line":"                id_list \u003d search_result[0][1][self.id_attr]"},{"line_number":1310,"context_line":"                if len(id_list) \u003e 1:"},{"line_number":1311,"context_line":"                    raise exception.Conflict(type\u003dself.id_attr,"},{"line_number":1312,"context_line":"                                             details\u003d_(\u0027Multiple id, %s.\u0027) %"}],"source_content_type":"text/x-python","patch_set":9,"id":"9fb8cfa7_a0e8277c","line":1309,"updated":"2019-06-18 23:53:21.000000000","message":"Shouldn\u0027t the indexing here be protected with a try block that catches IndexError and KeyError? The IndexError is unlikely because python-ldap has returned a list with at least 1 2-tuple (because search result is non-empty). But it\u0027s entirely plausible you might get a KeyError on the id_attr.","commit_id":"103b892ee61333b9fc929ffafd43ad8718f3c75e"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"404aebd2864cbcfc8f9fb2bed061db72169b71ca","unresolved":false,"context_lines":[{"line_number":1309,"context_line":"                try:"},{"line_number":1310,"context_line":"                    id_list \u003d search_result[0][1][self.id_attr]"},{"line_number":1311,"context_line":"                except KeyError:"},{"line_number":1312,"context_line":"                    raise ValueError("},{"line_number":1313,"context_line":"                        _(\u0027Invalid LDAP search: %(search)\u0027) % {"},{"line_number":1314,"context_line":"                            \u0027search\u0027: search_result})"},{"line_number":1315,"context_line":"                if len(id_list) \u003e 1:"}],"source_content_type":"text/x-python","patch_set":10,"id":"9fb8cfa7_d56b721c","line":1312,"range":{"start_line":1312,"start_character":26,"end_line":1312,"end_character":36},"updated":"2019-06-20 18:12:48.000000000","message":"I looked at the immediate callers and they don\u0027t appear to handle this exception directly. Is this handled somewhere else up the stack?\n\nAlso, we are going to be returning the search to the end user? If so, should we be doing that or should we just log it and return a less sensitive message?","commit_id":"525fac0bc4199d977b59b5ae4057a0bb515ee7ab"},{"author":{"_account_id":8866,"name":"Raildo Mascena de Sousa Filho","email":"rmascena@redhat.com","username":"raildo"},"change_message_id":"36bdee46849d0500493649a8075a66335c27f437","unresolved":false,"context_lines":[{"line_number":1309,"context_line":"                try:"},{"line_number":1310,"context_line":"                    id_list \u003d search_result[0][1][self.id_attr]"},{"line_number":1311,"context_line":"                except KeyError:"},{"line_number":1312,"context_line":"                    raise ValueError("},{"line_number":1313,"context_line":"                        _(\u0027Invalid LDAP search: %(search)\u0027) % {"},{"line_number":1314,"context_line":"                            \u0027search\u0027: search_result})"},{"line_number":1315,"context_line":"                if len(id_list) \u003e 1:"}],"source_content_type":"text/x-python","patch_set":10,"id":"9fb8cfa7_5089d023","line":1312,"range":{"start_line":1312,"start_character":26,"end_line":1312,"end_character":36},"in_reply_to":"9fb8cfa7_d56b721c","updated":"2019-06-20 19:07:56.000000000","message":"Good catch, maybe ValueError should not be the right exception here, also I\u0027m ok on just logging this as an invalid search query","commit_id":"525fac0bc4199d977b59b5ae4057a0bb515ee7ab"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"c963030c113069db4e4d91645fb3eb2b51d7f327","unresolved":false,"context_lines":[{"line_number":1313,"context_line":"                               \u0027object %(dn)s\u0027) % ({\u0027id_attr\u0027: self.id_attr,"},{"line_number":1314,"context_line":"                                                    \u0027dn\u0027: search_result})"},{"line_number":1315,"context_line":"                    LOG.warning(message)"},{"line_number":1316,"context_line":"                    raise exception.NotFound(message\u003dmessage)"},{"line_number":1317,"context_line":"                if len(id_list) \u003e 1:"},{"line_number":1318,"context_line":"                    raise exception.Conflict(type\u003dself.id_attr,"},{"line_number":1319,"context_line":"                                             details\u003d_(\u0027Multiple id, %s.\u0027) %"}],"source_content_type":"text/x-python","patch_set":12,"id":"9fb8cfa7_e6dc7775","line":1316,"range":{"start_line":1316,"start_character":20,"end_line":1316,"end_character":61},"updated":"2019-06-26 19:13:02.000000000","message":"What would this have done before if there were multiple values?","commit_id":"135435ec87a8e6ae62010a0f24585ce9bdaf99e5"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"c36dfeda99559f879b86c74f01640e032b7e67b9","unresolved":false,"context_lines":[{"line_number":1313,"context_line":"                               \u0027object %(dn)s\u0027) % ({\u0027id_attr\u0027: self.id_attr,"},{"line_number":1314,"context_line":"                                                    \u0027dn\u0027: search_result})"},{"line_number":1315,"context_line":"                    LOG.warning(message)"},{"line_number":1316,"context_line":"                    raise exception.NotFound(message\u003dmessage)"},{"line_number":1317,"context_line":"                if len(id_list) \u003e 1:"},{"line_number":1318,"context_line":"                    raise exception.Conflict(type\u003dself.id_attr,"},{"line_number":1319,"context_line":"                                             details\u003d_(\u0027Multiple id, %s.\u0027) %"}],"source_content_type":"text/x-python","patch_set":12,"id":"9fb8cfa7_48dd4d6e","line":1316,"range":{"start_line":1316,"start_character":20,"end_line":1316,"end_character":61},"in_reply_to":"9fb8cfa7_05ff40ab","updated":"2019-06-26 22:29:01.000000000","message":"Yeah - that would be reasonable. A detailed log message would be great for operators to diagnose problems in the event someone opens a ticket for getting the wrong value (assuming they know they have multiple values in LDAP - but maybe that\u0027s a bold assumption).","commit_id":"135435ec87a8e6ae62010a0f24585ce9bdaf99e5"},{"author":{"_account_id":8866,"name":"Raildo Mascena de Sousa Filho","email":"rmascena@redhat.com","username":"raildo"},"change_message_id":"53acd8710c599409d8cecef2d68557e3091e6664","unresolved":false,"context_lines":[{"line_number":1313,"context_line":"                               \u0027object %(dn)s\u0027) % ({\u0027id_attr\u0027: self.id_attr,"},{"line_number":1314,"context_line":"                                                    \u0027dn\u0027: search_result})"},{"line_number":1315,"context_line":"                    LOG.warning(message)"},{"line_number":1316,"context_line":"                    raise exception.NotFound(message\u003dmessage)"},{"line_number":1317,"context_line":"                if len(id_list) \u003e 1:"},{"line_number":1318,"context_line":"                    raise exception.Conflict(type\u003dself.id_attr,"},{"line_number":1319,"context_line":"                                             details\u003d_(\u0027Multiple id, %s.\u0027) %"}],"source_content_type":"text/x-python","patch_set":12,"id":"9fb8cfa7_ba6aebe9","line":1316,"range":{"start_line":1316,"start_character":20,"end_line":1316,"end_character":61},"in_reply_to":"9fb8cfa7_48dd4d6e","updated":"2019-06-28 14:14:36.000000000","message":"Great, I\u0027ll more details explaining that we\u0027re returning the first id value in the case of multivalue id in the dn.","commit_id":"135435ec87a8e6ae62010a0f24585ce9bdaf99e5"},{"author":{"_account_id":8116,"name":"John Dennis","email":"jdennis@redhat.com","username":"jdennis"},"change_message_id":"c3b7d98559e31f2490393466ff16c8e1dfee07b6","unresolved":false,"context_lines":[{"line_number":1313,"context_line":"                               \u0027object %(dn)s\u0027) % ({\u0027id_attr\u0027: self.id_attr,"},{"line_number":1314,"context_line":"                                                    \u0027dn\u0027: search_result})"},{"line_number":1315,"context_line":"                    LOG.warning(message)"},{"line_number":1316,"context_line":"                    raise exception.NotFound(message\u003dmessage)"},{"line_number":1317,"context_line":"                if len(id_list) \u003e 1:"},{"line_number":1318,"context_line":"                    raise exception.Conflict(type\u003dself.id_attr,"},{"line_number":1319,"context_line":"                                             details\u003d_(\u0027Multiple id, %s.\u0027) %"}],"source_content_type":"text/x-python","patch_set":12,"id":"9fb8cfa7_05ff40ab","line":1316,"range":{"start_line":1316,"start_character":20,"end_line":1316,"end_character":61},"in_reply_to":"9fb8cfa7_e6dc7775","updated":"2019-06-26 21:25:31.000000000","message":"It would have returned the first value. Multiple values are not ordered so it would have been arbitrary which value was first. I see where you are going with this, it\u0027s a change in behavior, perhaps it\u0027s better to log a warning and continue to return the first value.","commit_id":"135435ec87a8e6ae62010a0f24585ce9bdaf99e5"},{"author":{"_account_id":8116,"name":"John Dennis","email":"jdennis@redhat.com","username":"jdennis"},"change_message_id":"abdfc6cb15aed803fa28ae1c9f45f205fabe52e5","unresolved":false,"context_lines":[{"line_number":1310,"context_line":"                    id_list \u003d search_result[0][1][self.id_attr]"},{"line_number":1311,"context_line":"                except KeyError:"},{"line_number":1312,"context_line":"                    message \u003d (\u0027ID attribute %(id_attr)s not found in LDAP \u0027"},{"line_number":1313,"context_line":"                               \u0027object %(dn)s. Note that, in order to keep \u0027"},{"line_number":1314,"context_line":"                               \u0027backward compatibility, in the case of \u0027"},{"line_number":1315,"context_line":"                               \u0027multivalued ids, we are returning \u0027"},{"line_number":1316,"context_line":"                               \u0027the first id in the DN.\u0027) % ({"}],"source_content_type":"text/x-python","patch_set":13,"id":"9fb8cfa7_607fe614","line":1313,"updated":"2019-06-28 15:30:19.000000000","message":"I think you\u0027ve added the message on the wrong error case, you\u0027ve added it to the case where the attribute is not found. The case for when the attribute is multi-valued is below.","commit_id":"d1ac2c7093de299ebb5fda1f5fa3e3ae473c0caf"},{"author":{"_account_id":8866,"name":"Raildo Mascena de Sousa Filho","email":"rmascena@redhat.com","username":"raildo"},"change_message_id":"84731105f6ddbec16cf156562630df52b3c2ff42","unresolved":false,"context_lines":[{"line_number":1310,"context_line":"                    id_list \u003d search_result[0][1][self.id_attr]"},{"line_number":1311,"context_line":"                except KeyError:"},{"line_number":1312,"context_line":"                    message \u003d (\u0027ID attribute %(id_attr)s not found in LDAP \u0027"},{"line_number":1313,"context_line":"                               \u0027object %(dn)s. Note that, in order to keep \u0027"},{"line_number":1314,"context_line":"                               \u0027backward compatibility, in the case of \u0027"},{"line_number":1315,"context_line":"                               \u0027multivalued ids, we are returning \u0027"},{"line_number":1316,"context_line":"                               \u0027the first id in the DN.\u0027) % ({"}],"source_content_type":"text/x-python","patch_set":13,"id":"9fb8cfa7_6b6ee72f","line":1313,"in_reply_to":"9fb8cfa7_607fe614","updated":"2019-06-28 16:29:34.000000000","message":"Done","commit_id":"d1ac2c7093de299ebb5fda1f5fa3e3ae473c0caf"},{"author":{"_account_id":1916,"name":"Guang Yee","email":"gyee@suse.com","username":"guang-yee"},"change_message_id":"c8de637b18a9abea71df2b122b52b0b059ec8d05","unresolved":false,"context_lines":[{"line_number":1303,"context_line":"            # The \u0027ID\u0027 attribute is NOT in the DN, so we need to perform an"},{"line_number":1304,"context_line":"            # LDAP search to look it up from the user entry itself."},{"line_number":1305,"context_line":"            with self.get_connection() as conn:"},{"line_number":1306,"context_line":"                search_result \u003d conn.search_s(dn, self.LDAP_SCOPE)"},{"line_number":1307,"context_line":""},{"line_number":1308,"context_line":"            if search_result:"},{"line_number":1309,"context_line":"                try:"}],"source_content_type":"text/x-python","patch_set":14,"id":"7faddb67_0cd8ad78","line":1306,"range":{"start_line":1306,"start_character":50,"end_line":1306,"end_character":65},"updated":"2019-07-17 18:02:20.000000000","message":"Since we are really doing lookup by DN here, we should be able to optimize it by using ldap.SCOPE_BASE instead of self.LDAP_SCOPE. If self.LDAP_SCOPE is set to ldap.SCOPE_SUBTREE, this could take awhile if we have a large subtree, which is uncommon in a production environment.","commit_id":"2be93fde2d2157d576252b8ba56a17916e11f130"},{"author":{"_account_id":8866,"name":"Raildo Mascena de Sousa Filho","email":"rmascena@redhat.com","username":"raildo"},"change_message_id":"8036fdbefa7c699f56e65dea03015704e8418745","unresolved":false,"context_lines":[{"line_number":1303,"context_line":"            # The \u0027ID\u0027 attribute is NOT in the DN, so we need to perform an"},{"line_number":1304,"context_line":"            # LDAP search to look it up from the user entry itself."},{"line_number":1305,"context_line":"            with self.get_connection() as conn:"},{"line_number":1306,"context_line":"                search_result \u003d conn.search_s(dn, self.LDAP_SCOPE)"},{"line_number":1307,"context_line":""},{"line_number":1308,"context_line":"            if search_result:"},{"line_number":1309,"context_line":"                try:"}],"source_content_type":"text/x-python","patch_set":14,"id":"7faddb67_bcb3b5e8","line":1306,"range":{"start_line":1306,"start_character":50,"end_line":1306,"end_character":65},"in_reply_to":"7faddb67_0cd8ad78","updated":"2019-07-18 19:00:57.000000000","message":"Done","commit_id":"2be93fde2d2157d576252b8ba56a17916e11f130"},{"author":{"_account_id":11805,"name":"Corey Bryant","email":"corey.bryant@canonical.com","username":"coreycb"},"change_message_id":"edbae0c112322ea27a4278142767bd621d69910f","unresolved":false,"context_lines":[{"line_number":1297,"context_line":"        # Check if the naming attribute in the DN is the same as keystone\u0027s"},{"line_number":1298,"context_line":"        # configured \u0027id\u0027 attribute\u0027.  If so, extract the ID value from the DN"},{"line_number":1299,"context_line":"        if self.id_attr \u003d\u003d utf8_decode("},{"line_number":1300,"context_line":"                ldap.dn.str2dn(utf8_encode(dn))[0][0][0].lower()):"},{"line_number":1301,"context_line":"            return utf8_decode(ldap.dn.str2dn(utf8_encode(dn))[0][0][1])"},{"line_number":1302,"context_line":"        else:"},{"line_number":1303,"context_line":"            # The \u0027ID\u0027 attribute is NOT in the DN, so we need to perform an"}],"source_content_type":"text/x-python","patch_set":17,"id":"7faddb67_8493be5c","line":1300,"updated":"2019-07-23 20:36:23.000000000","message":"Sorry I\u0027m late to this but isn\u0027t this incorrect for python3-ldap and bytes_mode\u003dFalse? For more context: https://opendev.org/openstack/keystone/commit/a2e7ccb4b32140f122c0beee0f3fcc1109db36bf","commit_id":"a1dc21f3d34ae34bc6a5c9acebc0eb752495ae7a"},{"author":{"_account_id":8116,"name":"John Dennis","email":"jdennis@redhat.com","username":"jdennis"},"change_message_id":"6de59da429cd9eaf79efbca6c539469a36860369","unresolved":false,"context_lines":[{"line_number":1297,"context_line":"        # Check if the naming attribute in the DN is the same as keystone\u0027s"},{"line_number":1298,"context_line":"        # configured \u0027id\u0027 attribute\u0027.  If so, extract the ID value from the DN"},{"line_number":1299,"context_line":"        if self.id_attr \u003d\u003d utf8_decode("},{"line_number":1300,"context_line":"                ldap.dn.str2dn(utf8_encode(dn))[0][0][0].lower()):"},{"line_number":1301,"context_line":"            return utf8_decode(ldap.dn.str2dn(utf8_encode(dn))[0][0][1])"},{"line_number":1302,"context_line":"        else:"},{"line_number":1303,"context_line":"            # The \u0027ID\u0027 attribute is NOT in the DN, so we need to perform an"}],"source_content_type":"text/x-python","patch_set":17,"id":"7faddb67_3c642221","line":1300,"in_reply_to":"7faddb67_8493be5c","updated":"2019-08-28 18:37:53.000000000","message":"Good catch Corey! I believe you\u0027re right and this probably copied before your patch landed. I should have picked up on this myself but missed it.","commit_id":"a1dc21f3d34ae34bc6a5c9acebc0eb752495ae7a"}],"keystone/tests/unit/test_backend_ldap.py":[{"author":{"_account_id":9098,"name":"Nathan Kinder","email":"nkinder@redhat.com","username":"nkinder"},"change_message_id":"db93b629002cf90b7cda461e9b7734ec4d1ef100","unresolved":false,"context_lines":[{"line_number":1837,"context_line":"            }"},{"line_number":1838,"context_line":"        )"},{"line_number":1839,"context_line":""},{"line_number":1840,"context_line":"        # This is not a valid scenario, since we do not support multiple value"},{"line_number":1841,"context_line":"        # attribute id on DN."},{"line_number":1842,"context_line":"        self.assertRaises(exception.NotFound,"},{"line_number":1843,"context_line":"                          PROVIDERS.identity_api.get_user, email1)"}],"source_content_type":"text/x-python","patch_set":5,"id":"5fc1f717_79423b6a","line":1840,"updated":"2019-04-10 18:47:54.000000000","message":"Good catch on a broken test.  The old test returned a value that wasn\u0027t even from the configured \u0027id\u0027 attribute!  Throwing an exception seems appropriate for this case.\n\nI think you should add another variation on this test though.  We should test the case where a multi-valued \u0027id\u0027 attribute is present in the entry, but the DN of the entry has one of those \u0027id\u0027 values as the left-most RDN.  The code has logic to handle this case, as it will find that it has a multi-valued attribute and will then fall-back to checking for the \u0027id\u0027 in the DN.  You can just duplicate this test (renaming it of course), but modify the DN to be \u0027mail\u003d\u003cemail1 UUID value\u003e\u0027.  The test can then check for these results:\n\n        user_ref \u003d PROVIDERS.identity_api.get_user(email1)\n        self.assertEqual(email1, user_ref[\u0027id\u0027])","commit_id":"9ab921f4ce971895d22e19482f8ad23c1be96729"},{"author":{"_account_id":8866,"name":"Raildo Mascena de Sousa Filho","email":"rmascena@redhat.com","username":"raildo"},"change_message_id":"9bf83147e8118dcad27b3cdc7af13ecfc897b42d","unresolved":false,"context_lines":[{"line_number":1837,"context_line":"            }"},{"line_number":1838,"context_line":"        )"},{"line_number":1839,"context_line":""},{"line_number":1840,"context_line":"        # This is not a valid scenario, since we do not support multiple value"},{"line_number":1841,"context_line":"        # attribute id on DN."},{"line_number":1842,"context_line":"        self.assertRaises(exception.NotFound,"},{"line_number":1843,"context_line":"                          PROVIDERS.identity_api.get_user, email1)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fce034c_ac17e3ed","line":1840,"in_reply_to":"5fc1f717_79423b6a","updated":"2019-04-10 19:21:02.000000000","message":"It totally makes sense add another test scenario for it, I\u0027ll create this new test scenarios soon","commit_id":"9ab921f4ce971895d22e19482f8ad23c1be96729"},{"author":{"_account_id":9098,"name":"Nathan Kinder","email":"nkinder@redhat.com","username":"nkinder"},"change_message_id":"db93b629002cf90b7cda461e9b7734ec4d1ef100","unresolved":false,"context_lines":[{"line_number":1843,"context_line":"                          PROVIDERS.identity_api.get_user, email1)"},{"line_number":1844,"context_line":""},{"line_number":1845,"context_line":"    @mock.patch.object(common_ldap.BaseLdap, \u0027_ldap_get\u0027)"},{"line_number":1846,"context_line":"    def test_get_id_from_dn(self, mock_ldap_get):"},{"line_number":1847,"context_line":"        driver \u003d PROVIDERS.identity_api._select_identity_driver("},{"line_number":1848,"context_line":"            CONF.identity.default_domain_id)"},{"line_number":1849,"context_line":"        driver.user.id_attr \u003d \u0027mail\u0027"}],"source_content_type":"text/x-python","patch_set":5,"id":"5fc1f717_fc185976","line":1846,"updated":"2019-04-10 18:47:54.000000000","message":"It might be more appropriate to name this something like \u0027test_get_id_not_in_dn\u0027, as that is really what the test is doing.  In this case, the \u0027id\u0027 will be looked up from an attribute that is not in the DN.","commit_id":"9ab921f4ce971895d22e19482f8ad23c1be96729"},{"author":{"_account_id":8866,"name":"Raildo Mascena de Sousa Filho","email":"rmascena@redhat.com","username":"raildo"},"change_message_id":"9bf83147e8118dcad27b3cdc7af13ecfc897b42d","unresolved":false,"context_lines":[{"line_number":1843,"context_line":"                          PROVIDERS.identity_api.get_user, email1)"},{"line_number":1844,"context_line":""},{"line_number":1845,"context_line":"    @mock.patch.object(common_ldap.BaseLdap, \u0027_ldap_get\u0027)"},{"line_number":1846,"context_line":"    def test_get_id_from_dn(self, mock_ldap_get):"},{"line_number":1847,"context_line":"        driver \u003d PROVIDERS.identity_api._select_identity_driver("},{"line_number":1848,"context_line":"            CONF.identity.default_domain_id)"},{"line_number":1849,"context_line":"        driver.user.id_attr \u003d \u0027mail\u0027"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fce034c_0c271767","line":1846,"in_reply_to":"5fc1f717_fc185976","updated":"2019-04-10 19:21:02.000000000","message":"Done","commit_id":"9ab921f4ce971895d22e19482f8ad23c1be96729"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"720552d9e60cb6eb6594291a71b9f610f238bb14","unresolved":false,"context_lines":[{"line_number":1838,"context_line":"        )"},{"line_number":1839,"context_line":""},{"line_number":1840,"context_line":"        # This is not a valid scenario, since we do not support multiple value"},{"line_number":1841,"context_line":"        # attribute id on DN."},{"line_number":1842,"context_line":"        self.assertRaises(exception.NotFound,"},{"line_number":1843,"context_line":"                          PROVIDERS.identity_api.get_user, email1)"},{"line_number":1844,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"3fce034c_ecadeb31","line":1841,"updated":"2019-04-10 19:43:02.000000000","message":"Isn\u0027t this a regression?","commit_id":"10654b3556bee22a7f4c63619162771e1f082fb5"},{"author":{"_account_id":9098,"name":"Nathan Kinder","email":"nkinder@redhat.com","username":"nkinder"},"change_message_id":"f9b0dafa64405ed9b95d99feea7a0f8ccd6b1737","unresolved":false,"context_lines":[{"line_number":1838,"context_line":"        )"},{"line_number":1839,"context_line":""},{"line_number":1840,"context_line":"        # This is not a valid scenario, since we do not support multiple value"},{"line_number":1841,"context_line":"        # attribute id on DN."},{"line_number":1842,"context_line":"        self.assertRaises(exception.NotFound,"},{"line_number":1843,"context_line":"                          PROVIDERS.identity_api.get_user, email1)"},{"line_number":1844,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"3fce034c_4f751198","line":1841,"in_reply_to":"3fce034c_cc758fce","updated":"2019-04-10 19:57:13.000000000","message":"I don\u0027t think it is a regression.  Basically, we had a workaround in the code that Guang added quite some time back where we just grab the leftmost attribute value from the DN and return it as the ID when we encounter a multi-valued ID attribute.  This is not great since it is not guaranteed to be the right ID, but it should be consistent and works around a corner case.  I think we should leave this functionality for backwards compatibility in case someone is depending on it.  We want to try to work when we encounter a multi-valued attribute, and the DN check logic allows that in limited cases.\n\nThe new exception comes in where the above heuristic was not working properly.  We were never checking that the value we pull out of the DN and return is actually the ID attribute!  If you look at the old version of this test, we use \u0027mail\u0027 as the ID attribute in LDAP.  The test was ensuring that the returned value is \u0027nobodycares\u0027, which is the value of the \u0027cn\u0027 attribute.  This is never correct, as it is not the ID attribute.  In fact, this behavior is the bug that Raildo is fixing here, as the incorrect ID will lead to things like incorrect group membership evaluation in Keystone.","commit_id":"10654b3556bee22a7f4c63619162771e1f082fb5"},{"author":{"_account_id":8482,"name":"Colleen Murphy","email":"colleen@gazlene.net","username":"krinkle"},"change_message_id":"495ca9bc24a197de69e55f1d9183dbace72293a0","unresolved":false,"context_lines":[{"line_number":1838,"context_line":"        )"},{"line_number":1839,"context_line":""},{"line_number":1840,"context_line":"        # This is not a valid scenario, since we do not support multiple value"},{"line_number":1841,"context_line":"        # attribute id on DN."},{"line_number":1842,"context_line":"        self.assertRaises(exception.NotFound,"},{"line_number":1843,"context_line":"                          PROVIDERS.identity_api.get_user, email1)"},{"line_number":1844,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"3fce034c_4f2751b8","line":1841,"in_reply_to":"3fce034c_cc758fce","updated":"2019-04-10 19:53:10.000000000","message":"No, I mean currently we support multiple value attributes and this change breaks that, this change is introducing a regression.","commit_id":"10654b3556bee22a7f4c63619162771e1f082fb5"},{"author":{"_account_id":8866,"name":"Raildo Mascena de Sousa Filho","email":"rmascena@redhat.com","username":"raildo"},"change_message_id":"6727b21b7bb18adffa9151a391f176dbb5fc9065","unresolved":false,"context_lines":[{"line_number":1838,"context_line":"        )"},{"line_number":1839,"context_line":""},{"line_number":1840,"context_line":"        # This is not a valid scenario, since we do not support multiple value"},{"line_number":1841,"context_line":"        # attribute id on DN."},{"line_number":1842,"context_line":"        self.assertRaises(exception.NotFound,"},{"line_number":1843,"context_line":"                          PROVIDERS.identity_api.get_user, email1)"},{"line_number":1844,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"3fce034c_cc758fce","line":1841,"in_reply_to":"3fce034c_ecadeb31","updated":"2019-04-10 19:46:13.000000000","message":"it\u0027s a regression, I need to backport it later for all supported releases.","commit_id":"10654b3556bee22a7f4c63619162771e1f082fb5"}]}