Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument
On 21.11.2016 17:08, Standa Laznicka wrote: On 10/10/2016 08:47 AM, Standa Laznicka wrote: On 10/10/2016 07:53 AM, Jan Cholasta wrote: On 7.10.2016 12:23, Standa Laznicka wrote: On 10/07/2016 08:31 AM, Jan Cholasta wrote: On 17.8.2016 13:47, Stanislav Laznicka wrote: On 08/11/2016 02:59 PM, Stanislav Laznicka wrote: On 08/11/2016 07:49 AM, Jan Cholasta wrote: On 2.8.2016 13:47, Stanislav Laznicka wrote: On 07/19/2016 09:20 AM, Jan Cholasta wrote: Hi, On 14.7.2016 14:36, Stanislav Laznicka wrote: Hello, This patch fixes https://fedorahosted.org/freeipa/ticket/5640. With not so much experience with the framework, it raises question in my head whether ipaldap.get_entries is used properly throughout the system - does it always assume that it gets ALL the requested entries or just a few of those as configured by the 'ipaSearchRecordsLimit' attribute of ipaConfig.etc which it actually gets? That depends. If you call get_entries() on the ldap2 plugin (which is usually the case in the framework), then ipaSearchRecordsLimit is used. If you call it on some arbitrary LDAPClient instance, the hardcoded default (= unlimited) is used. One spot that I know the get_entries method was definitely not used properly before this patch is in the baseldap.LDAPObject.get_memberindirect() method: 692 result = self.backend.get_entries( 693 self.api.env.basedn, 694 filter=filter, 695 attrs_list=['member'], 696 size_limit=-1, # paged search will get everything anyway 697 paged_search=True) which to me seems kind of important if the environment size_limit is not set properly :) The patch does not fix the non-propagation of the paged_search, though. Why do you think size_limit is not used properly here? AFAIU it is desired that the search is unlimited. However, due to the fact that neither size_limit nor paged_search are passed from ldap2.get_entries() to ldap2.find_entries() (methods inherited from LDAPClient), only the number of records specified by ipaSearchRecordsLimit is returned. That could eventually cause problems should ipaSearchRecordsLimit be set to a low value as in the ticket. I see. This is *not* intentional, the **kwargs of get_entries() should be passed to find_entries(). This definitely needs to be fixed. Anyway, this ticket is not really easily fixable without more profound changes. Often, multiple LDAP searches are done during command execution. What do you do with the size limit then? Do you pass the same size limit to all the searches? Do you subtract the result size from the size limit after each search? Do you do something else with it? ... The answer is that it depends on the purpose of each individual LDAP search (like in get_memberindirect() above, we have to do unlimited search, otherwise the resulting entry would be incomplete), and fixing this accross the whole framework is a non-trivial task. I do realize that the proposed fix for the permission plugin is not perfect, it would probably be better to subtract the number of currently loaded records from the sizelimit, although in the end the number of returned values will not be higher than the given size_limit. However, it seems reasonable that if get_entries is passed a size limit, it should apply it over current ipaSearchRecordsLimit rather than ignoring it. Then, any use of get_entries could be fixed accordingly if someone sees fit. Right. Anyway, this is a different issue than above, so please put this into a separate commit. Please see the attached patches, then. Self-NACK, with Honza's help I found there was a mistake in the code. I also found an off-by-one bug which I hope I could stick to the other two patches (attaching only the modified and new patches). Works for me, but this bit in patch 0064 looks suspicious to me: +if max_entries > 0 and len(entries) == max_entries: Shouldn't it rather be: +if max_entries > 0 and len(entries) >= max_entries: ? The length of entries list should not exceed max_entries if size_limit is properly implemented. Therefore the list you get from execute() should not have more then max_entries entries. You shouldn't also be able to append a legacy entry to entries list if this check is the first thing you do. That's a lot of shoulds :-) I would expect at least an assert statement to make sure everything is right. That being said, >= could be used but then some popping of entries from entries list would be necessary. But it's perhaps safer to do, although if there's a bug, it won't be that obvious :) OK, nevermind then, just add the assert please, to make bugs *very* obvious. An assert seems like a very good idea, attached is an asserting patch. Attached is the patch rebased on the current master. Renumbered it a bit as previous numbers could've been confusing so I also omitted the revision number. ACK. Removed a stray
Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument
On 10/10/2016 08:47 AM, Standa Laznicka wrote: On 10/10/2016 07:53 AM, Jan Cholasta wrote: On 7.10.2016 12:23, Standa Laznicka wrote: On 10/07/2016 08:31 AM, Jan Cholasta wrote: On 17.8.2016 13:47, Stanislav Laznicka wrote: On 08/11/2016 02:59 PM, Stanislav Laznicka wrote: On 08/11/2016 07:49 AM, Jan Cholasta wrote: On 2.8.2016 13:47, Stanislav Laznicka wrote: On 07/19/2016 09:20 AM, Jan Cholasta wrote: Hi, On 14.7.2016 14:36, Stanislav Laznicka wrote: Hello, This patch fixes https://fedorahosted.org/freeipa/ticket/5640. With not so much experience with the framework, it raises question in my head whether ipaldap.get_entries is used properly throughout the system - does it always assume that it gets ALL the requested entries or just a few of those as configured by the 'ipaSearchRecordsLimit' attribute of ipaConfig.etc which it actually gets? That depends. If you call get_entries() on the ldap2 plugin (which is usually the case in the framework), then ipaSearchRecordsLimit is used. If you call it on some arbitrary LDAPClient instance, the hardcoded default (= unlimited) is used. One spot that I know the get_entries method was definitely not used properly before this patch is in the baseldap.LDAPObject.get_memberindirect() method: 692 result = self.backend.get_entries( 693 self.api.env.basedn, 694 filter=filter, 695 attrs_list=['member'], 696 size_limit=-1, # paged search will get everything anyway 697 paged_search=True) which to me seems kind of important if the environment size_limit is not set properly :) The patch does not fix the non-propagation of the paged_search, though. Why do you think size_limit is not used properly here? AFAIU it is desired that the search is unlimited. However, due to the fact that neither size_limit nor paged_search are passed from ldap2.get_entries() to ldap2.find_entries() (methods inherited from LDAPClient), only the number of records specified by ipaSearchRecordsLimit is returned. That could eventually cause problems should ipaSearchRecordsLimit be set to a low value as in the ticket. I see. This is *not* intentional, the **kwargs of get_entries() should be passed to find_entries(). This definitely needs to be fixed. Anyway, this ticket is not really easily fixable without more profound changes. Often, multiple LDAP searches are done during command execution. What do you do with the size limit then? Do you pass the same size limit to all the searches? Do you subtract the result size from the size limit after each search? Do you do something else with it? ... The answer is that it depends on the purpose of each individual LDAP search (like in get_memberindirect() above, we have to do unlimited search, otherwise the resulting entry would be incomplete), and fixing this accross the whole framework is a non-trivial task. I do realize that the proposed fix for the permission plugin is not perfect, it would probably be better to subtract the number of currently loaded records from the sizelimit, although in the end the number of returned values will not be higher than the given size_limit. However, it seems reasonable that if get_entries is passed a size limit, it should apply it over current ipaSearchRecordsLimit rather than ignoring it. Then, any use of get_entries could be fixed accordingly if someone sees fit. Right. Anyway, this is a different issue than above, so please put this into a separate commit. Please see the attached patches, then. Self-NACK, with Honza's help I found there was a mistake in the code. I also found an off-by-one bug which I hope I could stick to the other two patches (attaching only the modified and new patches). Works for me, but this bit in patch 0064 looks suspicious to me: +if max_entries > 0 and len(entries) == max_entries: Shouldn't it rather be: +if max_entries > 0 and len(entries) >= max_entries: ? The length of entries list should not exceed max_entries if size_limit is properly implemented. Therefore the list you get from execute() should not have more then max_entries entries. You shouldn't also be able to append a legacy entry to entries list if this check is the first thing you do. That's a lot of shoulds :-) I would expect at least an assert statement to make sure everything is right. That being said, >= could be used but then some popping of entries from entries list would be necessary. But it's perhaps safer to do, although if there's a bug, it won't be that obvious :) OK, nevermind then, just add the assert please, to make bugs *very* obvious. An assert seems like a very good idea, attached is an asserting patch. Attached is the patch rebased on the current master. Renumbered it a bit as previous numbers could've been confusing so I also omitted the revision number. From 5e54c0d49ab680b75af872031d9c
Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument
On 10/10/2016 07:53 AM, Jan Cholasta wrote: On 7.10.2016 12:23, Standa Laznicka wrote: On 10/07/2016 08:31 AM, Jan Cholasta wrote: On 17.8.2016 13:47, Stanislav Laznicka wrote: On 08/11/2016 02:59 PM, Stanislav Laznicka wrote: On 08/11/2016 07:49 AM, Jan Cholasta wrote: On 2.8.2016 13:47, Stanislav Laznicka wrote: On 07/19/2016 09:20 AM, Jan Cholasta wrote: Hi, On 14.7.2016 14:36, Stanislav Laznicka wrote: Hello, This patch fixes https://fedorahosted.org/freeipa/ticket/5640. With not so much experience with the framework, it raises question in my head whether ipaldap.get_entries is used properly throughout the system - does it always assume that it gets ALL the requested entries or just a few of those as configured by the 'ipaSearchRecordsLimit' attribute of ipaConfig.etc which it actually gets? That depends. If you call get_entries() on the ldap2 plugin (which is usually the case in the framework), then ipaSearchRecordsLimit is used. If you call it on some arbitrary LDAPClient instance, the hardcoded default (= unlimited) is used. One spot that I know the get_entries method was definitely not used properly before this patch is in the baseldap.LDAPObject.get_memberindirect() method: 692 result = self.backend.get_entries( 693 self.api.env.basedn, 694 filter=filter, 695 attrs_list=['member'], 696 size_limit=-1, # paged search will get everything anyway 697 paged_search=True) which to me seems kind of important if the environment size_limit is not set properly :) The patch does not fix the non-propagation of the paged_search, though. Why do you think size_limit is not used properly here? AFAIU it is desired that the search is unlimited. However, due to the fact that neither size_limit nor paged_search are passed from ldap2.get_entries() to ldap2.find_entries() (methods inherited from LDAPClient), only the number of records specified by ipaSearchRecordsLimit is returned. That could eventually cause problems should ipaSearchRecordsLimit be set to a low value as in the ticket. I see. This is *not* intentional, the **kwargs of get_entries() should be passed to find_entries(). This definitely needs to be fixed. Anyway, this ticket is not really easily fixable without more profound changes. Often, multiple LDAP searches are done during command execution. What do you do with the size limit then? Do you pass the same size limit to all the searches? Do you subtract the result size from the size limit after each search? Do you do something else with it? ... The answer is that it depends on the purpose of each individual LDAP search (like in get_memberindirect() above, we have to do unlimited search, otherwise the resulting entry would be incomplete), and fixing this accross the whole framework is a non-trivial task. I do realize that the proposed fix for the permission plugin is not perfect, it would probably be better to subtract the number of currently loaded records from the sizelimit, although in the end the number of returned values will not be higher than the given size_limit. However, it seems reasonable that if get_entries is passed a size limit, it should apply it over current ipaSearchRecordsLimit rather than ignoring it. Then, any use of get_entries could be fixed accordingly if someone sees fit. Right. Anyway, this is a different issue than above, so please put this into a separate commit. Please see the attached patches, then. Self-NACK, with Honza's help I found there was a mistake in the code. I also found an off-by-one bug which I hope I could stick to the other two patches (attaching only the modified and new patches). Works for me, but this bit in patch 0064 looks suspicious to me: +if max_entries > 0 and len(entries) == max_entries: Shouldn't it rather be: +if max_entries > 0 and len(entries) >= max_entries: ? The length of entries list should not exceed max_entries if size_limit is properly implemented. Therefore the list you get from execute() should not have more then max_entries entries. You shouldn't also be able to append a legacy entry to entries list if this check is the first thing you do. That's a lot of shoulds :-) I would expect at least an assert statement to make sure everything is right. That being said, >= could be used but then some popping of entries from entries list would be necessary. But it's perhaps safer to do, although if there's a bug, it won't be that obvious :) OK, nevermind then, just add the assert please, to make bugs *very* obvious. An assert seems like a very good idea, attached is an asserting patch. From 8af317a2fb8d3d3976c69620baf70171653cb925 Mon Sep 17 00:00:00 2001 From: Stanislav Laznicka Date: Wed, 17 Aug 2016 13:35:04 +0200 Subject: [PATCH] permission-find: fix a sizelimit off-by-one bug permission-find: sizelimit option set to number of pe
Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument
On 7.10.2016 12:23, Standa Laznicka wrote: On 10/07/2016 08:31 AM, Jan Cholasta wrote: On 17.8.2016 13:47, Stanislav Laznicka wrote: On 08/11/2016 02:59 PM, Stanislav Laznicka wrote: On 08/11/2016 07:49 AM, Jan Cholasta wrote: On 2.8.2016 13:47, Stanislav Laznicka wrote: On 07/19/2016 09:20 AM, Jan Cholasta wrote: Hi, On 14.7.2016 14:36, Stanislav Laznicka wrote: Hello, This patch fixes https://fedorahosted.org/freeipa/ticket/5640. With not so much experience with the framework, it raises question in my head whether ipaldap.get_entries is used properly throughout the system - does it always assume that it gets ALL the requested entries or just a few of those as configured by the 'ipaSearchRecordsLimit' attribute of ipaConfig.etc which it actually gets? That depends. If you call get_entries() on the ldap2 plugin (which is usually the case in the framework), then ipaSearchRecordsLimit is used. If you call it on some arbitrary LDAPClient instance, the hardcoded default (= unlimited) is used. One spot that I know the get_entries method was definitely not used properly before this patch is in the baseldap.LDAPObject.get_memberindirect() method: 692 result = self.backend.get_entries( 693 self.api.env.basedn, 694 filter=filter, 695 attrs_list=['member'], 696 size_limit=-1, # paged search will get everything anyway 697 paged_search=True) which to me seems kind of important if the environment size_limit is not set properly :) The patch does not fix the non-propagation of the paged_search, though. Why do you think size_limit is not used properly here? AFAIU it is desired that the search is unlimited. However, due to the fact that neither size_limit nor paged_search are passed from ldap2.get_entries() to ldap2.find_entries() (methods inherited from LDAPClient), only the number of records specified by ipaSearchRecordsLimit is returned. That could eventually cause problems should ipaSearchRecordsLimit be set to a low value as in the ticket. I see. This is *not* intentional, the **kwargs of get_entries() should be passed to find_entries(). This definitely needs to be fixed. Anyway, this ticket is not really easily fixable without more profound changes. Often, multiple LDAP searches are done during command execution. What do you do with the size limit then? Do you pass the same size limit to all the searches? Do you subtract the result size from the size limit after each search? Do you do something else with it? ... The answer is that it depends on the purpose of each individual LDAP search (like in get_memberindirect() above, we have to do unlimited search, otherwise the resulting entry would be incomplete), and fixing this accross the whole framework is a non-trivial task. I do realize that the proposed fix for the permission plugin is not perfect, it would probably be better to subtract the number of currently loaded records from the sizelimit, although in the end the number of returned values will not be higher than the given size_limit. However, it seems reasonable that if get_entries is passed a size limit, it should apply it over current ipaSearchRecordsLimit rather than ignoring it. Then, any use of get_entries could be fixed accordingly if someone sees fit. Right. Anyway, this is a different issue than above, so please put this into a separate commit. Please see the attached patches, then. Self-NACK, with Honza's help I found there was a mistake in the code. I also found an off-by-one bug which I hope I could stick to the other two patches (attaching only the modified and new patches). Works for me, but this bit in patch 0064 looks suspicious to me: +if max_entries > 0 and len(entries) == max_entries: Shouldn't it rather be: +if max_entries > 0 and len(entries) >= max_entries: ? The length of entries list should not exceed max_entries if size_limit is properly implemented. Therefore the list you get from execute() should not have more then max_entries entries. You shouldn't also be able to append a legacy entry to entries list if this check is the first thing you do. That's a lot of shoulds :-) I would expect at least an assert statement to make sure everything is right. That being said, >= could be used but then some popping of entries from entries list would be necessary. But it's perhaps safer to do, although if there's a bug, it won't be that obvious :) OK, nevermind then, just add the assert please, to make bugs *very* obvious. -- Jan Cholasta -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument
On 10/07/2016 08:31 AM, Jan Cholasta wrote: On 17.8.2016 13:47, Stanislav Laznicka wrote: On 08/11/2016 02:59 PM, Stanislav Laznicka wrote: On 08/11/2016 07:49 AM, Jan Cholasta wrote: On 2.8.2016 13:47, Stanislav Laznicka wrote: On 07/19/2016 09:20 AM, Jan Cholasta wrote: Hi, On 14.7.2016 14:36, Stanislav Laznicka wrote: Hello, This patch fixes https://fedorahosted.org/freeipa/ticket/5640. With not so much experience with the framework, it raises question in my head whether ipaldap.get_entries is used properly throughout the system - does it always assume that it gets ALL the requested entries or just a few of those as configured by the 'ipaSearchRecordsLimit' attribute of ipaConfig.etc which it actually gets? That depends. If you call get_entries() on the ldap2 plugin (which is usually the case in the framework), then ipaSearchRecordsLimit is used. If you call it on some arbitrary LDAPClient instance, the hardcoded default (= unlimited) is used. One spot that I know the get_entries method was definitely not used properly before this patch is in the baseldap.LDAPObject.get_memberindirect() method: 692 result = self.backend.get_entries( 693 self.api.env.basedn, 694 filter=filter, 695 attrs_list=['member'], 696 size_limit=-1, # paged search will get everything anyway 697 paged_search=True) which to me seems kind of important if the environment size_limit is not set properly :) The patch does not fix the non-propagation of the paged_search, though. Why do you think size_limit is not used properly here? AFAIU it is desired that the search is unlimited. However, due to the fact that neither size_limit nor paged_search are passed from ldap2.get_entries() to ldap2.find_entries() (methods inherited from LDAPClient), only the number of records specified by ipaSearchRecordsLimit is returned. That could eventually cause problems should ipaSearchRecordsLimit be set to a low value as in the ticket. I see. This is *not* intentional, the **kwargs of get_entries() should be passed to find_entries(). This definitely needs to be fixed. Anyway, this ticket is not really easily fixable without more profound changes. Often, multiple LDAP searches are done during command execution. What do you do with the size limit then? Do you pass the same size limit to all the searches? Do you subtract the result size from the size limit after each search? Do you do something else with it? ... The answer is that it depends on the purpose of each individual LDAP search (like in get_memberindirect() above, we have to do unlimited search, otherwise the resulting entry would be incomplete), and fixing this accross the whole framework is a non-trivial task. I do realize that the proposed fix for the permission plugin is not perfect, it would probably be better to subtract the number of currently loaded records from the sizelimit, although in the end the number of returned values will not be higher than the given size_limit. However, it seems reasonable that if get_entries is passed a size limit, it should apply it over current ipaSearchRecordsLimit rather than ignoring it. Then, any use of get_entries could be fixed accordingly if someone sees fit. Right. Anyway, this is a different issue than above, so please put this into a separate commit. Please see the attached patches, then. Self-NACK, with Honza's help I found there was a mistake in the code. I also found an off-by-one bug which I hope I could stick to the other two patches (attaching only the modified and new patches). Works for me, but this bit in patch 0064 looks suspicious to me: +if max_entries > 0 and len(entries) == max_entries: Shouldn't it rather be: +if max_entries > 0 and len(entries) >= max_entries: ? The length of entries list should not exceed max_entries if size_limit is properly implemented. Therefore the list you get from execute() should not have more then max_entries entries. You shouldn't also be able to append a legacy entry to entries list if this check is the first thing you do. That being said, >= could be used but then some popping of entries from entries list would be necessary. But it's perhaps safer to do, although if there's a bug, it won't be that obvious :) -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument
On 17.8.2016 13:47, Stanislav Laznicka wrote: On 08/11/2016 02:59 PM, Stanislav Laznicka wrote: On 08/11/2016 07:49 AM, Jan Cholasta wrote: On 2.8.2016 13:47, Stanislav Laznicka wrote: On 07/19/2016 09:20 AM, Jan Cholasta wrote: Hi, On 14.7.2016 14:36, Stanislav Laznicka wrote: Hello, This patch fixes https://fedorahosted.org/freeipa/ticket/5640. With not so much experience with the framework, it raises question in my head whether ipaldap.get_entries is used properly throughout the system - does it always assume that it gets ALL the requested entries or just a few of those as configured by the 'ipaSearchRecordsLimit' attribute of ipaConfig.etc which it actually gets? That depends. If you call get_entries() on the ldap2 plugin (which is usually the case in the framework), then ipaSearchRecordsLimit is used. If you call it on some arbitrary LDAPClient instance, the hardcoded default (= unlimited) is used. One spot that I know the get_entries method was definitely not used properly before this patch is in the baseldap.LDAPObject.get_memberindirect() method: 692 result = self.backend.get_entries( 693 self.api.env.basedn, 694 filter=filter, 695 attrs_list=['member'], 696 size_limit=-1, # paged search will get everything anyway 697 paged_search=True) which to me seems kind of important if the environment size_limit is not set properly :) The patch does not fix the non-propagation of the paged_search, though. Why do you think size_limit is not used properly here? AFAIU it is desired that the search is unlimited. However, due to the fact that neither size_limit nor paged_search are passed from ldap2.get_entries() to ldap2.find_entries() (methods inherited from LDAPClient), only the number of records specified by ipaSearchRecordsLimit is returned. That could eventually cause problems should ipaSearchRecordsLimit be set to a low value as in the ticket. I see. This is *not* intentional, the **kwargs of get_entries() should be passed to find_entries(). This definitely needs to be fixed. Anyway, this ticket is not really easily fixable without more profound changes. Often, multiple LDAP searches are done during command execution. What do you do with the size limit then? Do you pass the same size limit to all the searches? Do you subtract the result size from the size limit after each search? Do you do something else with it? ... The answer is that it depends on the purpose of each individual LDAP search (like in get_memberindirect() above, we have to do unlimited search, otherwise the resulting entry would be incomplete), and fixing this accross the whole framework is a non-trivial task. I do realize that the proposed fix for the permission plugin is not perfect, it would probably be better to subtract the number of currently loaded records from the sizelimit, although in the end the number of returned values will not be higher than the given size_limit. However, it seems reasonable that if get_entries is passed a size limit, it should apply it over current ipaSearchRecordsLimit rather than ignoring it. Then, any use of get_entries could be fixed accordingly if someone sees fit. Right. Anyway, this is a different issue than above, so please put this into a separate commit. Please see the attached patches, then. Self-NACK, with Honza's help I found there was a mistake in the code. I also found an off-by-one bug which I hope I could stick to the other two patches (attaching only the modified and new patches). Works for me, but this bit in patch 0064 looks suspicious to me: +if max_entries > 0 and len(entries) == max_entries: Shouldn't it rather be: +if max_entries > 0 and len(entries) >= max_entries: ? -- Jan Cholasta -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument
On 08/11/2016 02:59 PM, Stanislav Laznicka wrote: On 08/11/2016 07:49 AM, Jan Cholasta wrote: On 2.8.2016 13:47, Stanislav Laznicka wrote: On 07/19/2016 09:20 AM, Jan Cholasta wrote: Hi, On 14.7.2016 14:36, Stanislav Laznicka wrote: Hello, This patch fixes https://fedorahosted.org/freeipa/ticket/5640. With not so much experience with the framework, it raises question in my head whether ipaldap.get_entries is used properly throughout the system - does it always assume that it gets ALL the requested entries or just a few of those as configured by the 'ipaSearchRecordsLimit' attribute of ipaConfig.etc which it actually gets? That depends. If you call get_entries() on the ldap2 plugin (which is usually the case in the framework), then ipaSearchRecordsLimit is used. If you call it on some arbitrary LDAPClient instance, the hardcoded default (= unlimited) is used. One spot that I know the get_entries method was definitely not used properly before this patch is in the baseldap.LDAPObject.get_memberindirect() method: 692 result = self.backend.get_entries( 693 self.api.env.basedn, 694 filter=filter, 695 attrs_list=['member'], 696 size_limit=-1, # paged search will get everything anyway 697 paged_search=True) which to me seems kind of important if the environment size_limit is not set properly :) The patch does not fix the non-propagation of the paged_search, though. Why do you think size_limit is not used properly here? AFAIU it is desired that the search is unlimited. However, due to the fact that neither size_limit nor paged_search are passed from ldap2.get_entries() to ldap2.find_entries() (methods inherited from LDAPClient), only the number of records specified by ipaSearchRecordsLimit is returned. That could eventually cause problems should ipaSearchRecordsLimit be set to a low value as in the ticket. I see. This is *not* intentional, the **kwargs of get_entries() should be passed to find_entries(). This definitely needs to be fixed. Anyway, this ticket is not really easily fixable without more profound changes. Often, multiple LDAP searches are done during command execution. What do you do with the size limit then? Do you pass the same size limit to all the searches? Do you subtract the result size from the size limit after each search? Do you do something else with it? ... The answer is that it depends on the purpose of each individual LDAP search (like in get_memberindirect() above, we have to do unlimited search, otherwise the resulting entry would be incomplete), and fixing this accross the whole framework is a non-trivial task. I do realize that the proposed fix for the permission plugin is not perfect, it would probably be better to subtract the number of currently loaded records from the sizelimit, although in the end the number of returned values will not be higher than the given size_limit. However, it seems reasonable that if get_entries is passed a size limit, it should apply it over current ipaSearchRecordsLimit rather than ignoring it. Then, any use of get_entries could be fixed accordingly if someone sees fit. Right. Anyway, this is a different issue than above, so please put this into a separate commit. Please see the attached patches, then. Self-NACK, with Honza's help I found there was a mistake in the code. I also found an off-by-one bug which I hope I could stick to the other two patches (attaching only the modified and new patches). From a0b900321fd0bcb9d93f9e558f48af6854e853df Mon Sep 17 00:00:00 2001 From: Stanislav Laznicka Date: Thu, 11 Aug 2016 14:09:22 +0200 Subject: [PATCH 1/2] fix permission_find fail on low search size limit permission_find() method would have failed if size_limit in config is too small caused by a search in post_callback. This search should also respect the passed sizelimit or the sizelimit from ipa config if no sizelimit is passed. https://fedorahosted.org/freeipa/ticket/5640 --- ipaserver/plugins/permission.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ipaserver/plugins/permission.py b/ipaserver/plugins/permission.py index 830773ae7a09f0197da702e4ec31b0b58f1214dd..f11f32fe75abe79afd975f20a17d7074d7fdaf76 100644 --- a/ipaserver/plugins/permission.py +++ b/ipaserver/plugins/permission.py @@ -1305,10 +1305,10 @@ class permission_find(baseldap.LDAPSearch): if options.get('all'): attrs_list.append('*') try: -legacy_entries = ldap.get_entries( +(legacy_entries, truncated) = ldap.find_entries( base_dn=DN(self.obj.container_dn, self.api.env.basedn), filter=ldap.combine_filters(filters, rules=ldap.MATCH_ALL), -attrs_list=attrs_list) +attrs_list=attrs_list, size_limit=max_entries) # Retrieve the root entry (with all legacy
Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument
On 08/11/2016 07:49 AM, Jan Cholasta wrote: On 2.8.2016 13:47, Stanislav Laznicka wrote: On 07/19/2016 09:20 AM, Jan Cholasta wrote: Hi, On 14.7.2016 14:36, Stanislav Laznicka wrote: Hello, This patch fixes https://fedorahosted.org/freeipa/ticket/5640. With not so much experience with the framework, it raises question in my head whether ipaldap.get_entries is used properly throughout the system - does it always assume that it gets ALL the requested entries or just a few of those as configured by the 'ipaSearchRecordsLimit' attribute of ipaConfig.etc which it actually gets? That depends. If you call get_entries() on the ldap2 plugin (which is usually the case in the framework), then ipaSearchRecordsLimit is used. If you call it on some arbitrary LDAPClient instance, the hardcoded default (= unlimited) is used. One spot that I know the get_entries method was definitely not used properly before this patch is in the baseldap.LDAPObject.get_memberindirect() method: 692 result = self.backend.get_entries( 693 self.api.env.basedn, 694 filter=filter, 695 attrs_list=['member'], 696 size_limit=-1, # paged search will get everything anyway 697 paged_search=True) which to me seems kind of important if the environment size_limit is not set properly :) The patch does not fix the non-propagation of the paged_search, though. Why do you think size_limit is not used properly here? AFAIU it is desired that the search is unlimited. However, due to the fact that neither size_limit nor paged_search are passed from ldap2.get_entries() to ldap2.find_entries() (methods inherited from LDAPClient), only the number of records specified by ipaSearchRecordsLimit is returned. That could eventually cause problems should ipaSearchRecordsLimit be set to a low value as in the ticket. I see. This is *not* intentional, the **kwargs of get_entries() should be passed to find_entries(). This definitely needs to be fixed. Anyway, this ticket is not really easily fixable without more profound changes. Often, multiple LDAP searches are done during command execution. What do you do with the size limit then? Do you pass the same size limit to all the searches? Do you subtract the result size from the size limit after each search? Do you do something else with it? ... The answer is that it depends on the purpose of each individual LDAP search (like in get_memberindirect() above, we have to do unlimited search, otherwise the resulting entry would be incomplete), and fixing this accross the whole framework is a non-trivial task. I do realize that the proposed fix for the permission plugin is not perfect, it would probably be better to subtract the number of currently loaded records from the sizelimit, although in the end the number of returned values will not be higher than the given size_limit. However, it seems reasonable that if get_entries is passed a size limit, it should apply it over current ipaSearchRecordsLimit rather than ignoring it. Then, any use of get_entries could be fixed accordingly if someone sees fit. Right. Anyway, this is a different issue than above, so please put this into a separate commit. Please see the attached patches, then. From 75d8cf9c3708b68c9b3a9ba999b3d034b1ddc33a Mon Sep 17 00:00:00 2001 From: Stanislav Laznicka Date: Thu, 11 Aug 2016 14:08:33 +0200 Subject: [PATCH 1/2] Make get_entries() not ignore its limit arguments get_entries() wouldn't pass some arguments deeper to find_entries() function it wraps. This would cause unexpected behavior in some cases throughout the framework where specific (non-)limitations are expected. https://fedorahosted.org/freeipa/ticket/5640 --- ipapython/ipaldap.py | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py index 704e71a9471c27430328a8c7c6a319aa72a9d482..a3f0a5668616f66ba744c336832064269882eb8b 100644 --- a/ipapython/ipaldap.py +++ b/ipapython/ipaldap.py @@ -1298,7 +1298,8 @@ class LDAPClient(object): for their description. """ entries, truncated = self.find_entries( -base_dn=base_dn, scope=scope, filter=filter, attrs_list=attrs_list) +base_dn=base_dn, scope=scope, filter=filter, attrs_list=attrs_list, +**kwargs) try: self.handle_truncated_result(truncated) except errors.LimitsExceeded as e: @@ -1313,7 +1314,8 @@ class LDAPClient(object): def find_entries(self, filter=None, attrs_list=None, base_dn=None, scope=ldap.SCOPE_SUBTREE, time_limit=None, - size_limit=None, search_refs=False, paged_search=False): + size_limit=None, search_refs=False, paged_search=False, + **kwargs): """ Return a list of entries and indication of whether the results were truncated ([(dn, entry_attr
Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument
On 2.8.2016 13:47, Stanislav Laznicka wrote: On 07/19/2016 09:20 AM, Jan Cholasta wrote: Hi, On 14.7.2016 14:36, Stanislav Laznicka wrote: Hello, This patch fixes https://fedorahosted.org/freeipa/ticket/5640. With not so much experience with the framework, it raises question in my head whether ipaldap.get_entries is used properly throughout the system - does it always assume that it gets ALL the requested entries or just a few of those as configured by the 'ipaSearchRecordsLimit' attribute of ipaConfig.etc which it actually gets? That depends. If you call get_entries() on the ldap2 plugin (which is usually the case in the framework), then ipaSearchRecordsLimit is used. If you call it on some arbitrary LDAPClient instance, the hardcoded default (= unlimited) is used. One spot that I know the get_entries method was definitely not used properly before this patch is in the baseldap.LDAPObject.get_memberindirect() method: 692 result = self.backend.get_entries( 693 self.api.env.basedn, 694 filter=filter, 695 attrs_list=['member'], 696 size_limit=-1, # paged search will get everything anyway 697 paged_search=True) which to me seems kind of important if the environment size_limit is not set properly :) The patch does not fix the non-propagation of the paged_search, though. Why do you think size_limit is not used properly here? AFAIU it is desired that the search is unlimited. However, due to the fact that neither size_limit nor paged_search are passed from ldap2.get_entries() to ldap2.find_entries() (methods inherited from LDAPClient), only the number of records specified by ipaSearchRecordsLimit is returned. That could eventually cause problems should ipaSearchRecordsLimit be set to a low value as in the ticket. I see. This is *not* intentional, the **kwargs of get_entries() should be passed to find_entries(). This definitely needs to be fixed. Anyway, this ticket is not really easily fixable without more profound changes. Often, multiple LDAP searches are done during command execution. What do you do with the size limit then? Do you pass the same size limit to all the searches? Do you subtract the result size from the size limit after each search? Do you do something else with it? ... The answer is that it depends on the purpose of each individual LDAP search (like in get_memberindirect() above, we have to do unlimited search, otherwise the resulting entry would be incomplete), and fixing this accross the whole framework is a non-trivial task. I do realize that the proposed fix for the permission plugin is not perfect, it would probably be better to subtract the number of currently loaded records from the sizelimit, although in the end the number of returned values will not be higher than the given size_limit. However, it seems reasonable that if get_entries is passed a size limit, it should apply it over current ipaSearchRecordsLimit rather than ignoring it. Then, any use of get_entries could be fixed accordingly if someone sees fit. Right. Anyway, this is a different issue than above, so please put this into a separate commit. -- Jan Cholasta -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument
On 07/19/2016 09:20 AM, Jan Cholasta wrote: Hi, On 14.7.2016 14:36, Stanislav Laznicka wrote: Hello, This patch fixes https://fedorahosted.org/freeipa/ticket/5640. With not so much experience with the framework, it raises question in my head whether ipaldap.get_entries is used properly throughout the system - does it always assume that it gets ALL the requested entries or just a few of those as configured by the 'ipaSearchRecordsLimit' attribute of ipaConfig.etc which it actually gets? That depends. If you call get_entries() on the ldap2 plugin (which is usually the case in the framework), then ipaSearchRecordsLimit is used. If you call it on some arbitrary LDAPClient instance, the hardcoded default (= unlimited) is used. One spot that I know the get_entries method was definitely not used properly before this patch is in the baseldap.LDAPObject.get_memberindirect() method: 692 result = self.backend.get_entries( 693 self.api.env.basedn, 694 filter=filter, 695 attrs_list=['member'], 696 size_limit=-1, # paged search will get everything anyway 697 paged_search=True) which to me seems kind of important if the environment size_limit is not set properly :) The patch does not fix the non-propagation of the paged_search, though. Why do you think size_limit is not used properly here? AFAIU it is desired that the search is unlimited. However, due to the fact that neither size_limit nor paged_search are passed from ldap2.get_entries() to ldap2.find_entries() (methods inherited from LDAPClient), only the number of records specified by ipaSearchRecordsLimit is returned. That could eventually cause problems should ipaSearchRecordsLimit be set to a low value as in the ticket. Anyway, this ticket is not really easily fixable without more profound changes. Often, multiple LDAP searches are done during command execution. What do you do with the size limit then? Do you pass the same size limit to all the searches? Do you subtract the result size from the size limit after each search? Do you do something else with it? ... The answer is that it depends on the purpose of each individual LDAP search (like in get_memberindirect() above, we have to do unlimited search, otherwise the resulting entry would be incomplete), and fixing this accross the whole framework is a non-trivial task. I do realize that the proposed fix for the permission plugin is not perfect, it would probably be better to subtract the number of currently loaded records from the sizelimit, although in the end the number of returned values will not be higher than the given size_limit. However, it seems reasonable that if get_entries is passed a size limit, it should apply it over current ipaSearchRecordsLimit rather than ignoring it. Then, any use of get_entries could be fixed accordingly if someone sees fit. -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument
Hi, On 14.7.2016 14:36, Stanislav Laznicka wrote: Hello, This patch fixes https://fedorahosted.org/freeipa/ticket/5640. With not so much experience with the framework, it raises question in my head whether ipaldap.get_entries is used properly throughout the system - does it always assume that it gets ALL the requested entries or just a few of those as configured by the 'ipaSearchRecordsLimit' attribute of ipaConfig.etc which it actually gets? That depends. If you call get_entries() on the ldap2 plugin (which is usually the case in the framework), then ipaSearchRecordsLimit is used. If you call it on some arbitrary LDAPClient instance, the hardcoded default (= unlimited) is used. One spot that I know the get_entries method was definitely not used properly before this patch is in the baseldap.LDAPObject.get_memberindirect() method: 692 result = self.backend.get_entries( 693 self.api.env.basedn, 694 filter=filter, 695 attrs_list=['member'], 696 size_limit=-1, # paged search will get everything anyway 697 paged_search=True) which to me seems kind of important if the environment size_limit is not set properly :) The patch does not fix the non-propagation of the paged_search, though. Why do you think size_limit is not used properly here? Anyway, this ticket is not really easily fixable without more profound changes. Often, multiple LDAP searches are done during command execution. What do you do with the size limit then? Do you pass the same size limit to all the searches? Do you subtract the result size from the size limit after each search? Do you do something else with it? ... The answer is that it depends on the purpose of each individual LDAP search (like in get_memberindirect() above, we have to do unlimited search, otherwise the resulting entry would be incomplete), and fixing this accross the whole framework is a non-trivial task. Honza -- Jan Cholasta -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
[Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument
Hello, This patch fixes https://fedorahosted.org/freeipa/ticket/5640. With not so much experience with the framework, it raises question in my head whether ipaldap.get_entries is used properly throughout the system - does it always assume that it gets ALL the requested entries or just a few of those as configured by the 'ipaSearchRecordsLimit' attribute of ipaConfig.etc which it actually gets? One spot that I know the get_entries method was definitely not used properly before this patch is in the baseldap.LDAPObject.get_memberindirect() method: 692 result = self.backend.get_entries( 693 self.api.env.basedn, 694 filter=filter, 695 attrs_list=['member'], 696 size_limit=-1, # paged search will get everything anyway 697 paged_search=True) which to me seems kind of important if the environment size_limit is not set properly :) The patch does not fix the non-propagation of the paged_search, though. Cheers, Standa From f76d301b418219b61a571e12ad7404eaf91a5046 Mon Sep 17 00:00:00 2001 From: Stanislav Laznicka Date: Thu, 14 Jul 2016 13:53:56 +0200 Subject: [PATCH] Make get_entries() not ignore size_limit argument The permission_find command would in some cases ignore the sizelimit parameter passed to it. This was caused by the ipaldap.get_entries() method not passing one of its parameters further down. https://fedorahosted.org/freeipa/ticket/5640 --- ipapython/ipaldap.py| 5 +++-- ipaserver/plugins/permission.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py index 704e71a9471c27430328a8c7c6a319aa72a9d482..74d985e8546ad2553bdac5a61da7df8acb6a0923 100644 --- a/ipapython/ipaldap.py +++ b/ipapython/ipaldap.py @@ -1283,7 +1283,7 @@ class LDAPClient(object): return cls.combine_filters(flts, rules) def get_entries(self, base_dn, scope=ldap.SCOPE_SUBTREE, filter=None, -attrs_list=None, **kwargs): +attrs_list=None, size_limit=None, **kwargs): """Return a list of matching entries. :raises: errors.LimitsExceeded if the list is truncated by the server @@ -1298,7 +1298,8 @@ class LDAPClient(object): for their description. """ entries, truncated = self.find_entries( -base_dn=base_dn, scope=scope, filter=filter, attrs_list=attrs_list) +base_dn=base_dn, scope=scope, filter=filter, attrs_list=attrs_list, +size_limit=size_limit) try: self.handle_truncated_result(truncated) except errors.LimitsExceeded as e: diff --git a/ipaserver/plugins/permission.py b/ipaserver/plugins/permission.py index 830773ae7a09f0197da702e4ec31b0b58f1214dd..e05fc0030c62583e0bcd495c22045274cae14660 100644 --- a/ipaserver/plugins/permission.py +++ b/ipaserver/plugins/permission.py @@ -1308,7 +1308,7 @@ class permission_find(baseldap.LDAPSearch): legacy_entries = ldap.get_entries( base_dn=DN(self.obj.container_dn, self.api.env.basedn), filter=ldap.combine_filters(filters, rules=ldap.MATCH_ALL), -attrs_list=attrs_list) +attrs_list=attrs_list, size_limit=max_entries) # Retrieve the root entry (with all legacy ACIs) at once root_entry = ldap.get_entry(DN(api.env.basedn), ['aci']) except errors.NotFound: -- 2.7.4 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code