$rbac->EntityType->PathID(): Tests Fail On Linux Server about rbac

[jburns131]

This doc might be pertinent: http://dev.mysql.com/doc/refman/5.1/en/group-by-extensions.html

[abiusx]

hmm i have no idea, yet.
check mysql versions on both machines and report

---

Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Aug 25, 2013, at 6:20 PM, Jesse Burns [email protected] wrote:

This doc might be pertinent: http://dev.mysql.com/doc/refman/5.1/en/group-by-extensions.html

—
Reply to this email directly or view it on GitHub.

[jburns131]

Local Windows Box:

• Server: 127.0.0.1 via TCP/IP
• Software: MySQL
• Software version: 5.5.27 - MySQL Community Server (GPL)
• Protocol version: 10
• Server charset: UTF-8 Unicode (utf8)

Remote Linux Box:

• Server: Localhost via UNIX socket
• Server type: MySQL
• Server version: 5.1.70-cll - MySQL Community Server (GPL)
• Protocol version: 10
• Server charset: UTF-8 Unicode (utf8)

[abiusx]

These are both Windows boxes?!:D
where's the linux one?

---

Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Aug 25, 2013, at 7:09 PM, Jesse Burns [email protected] wrote:

Local Windows Box:

Server: 127.0.0.1 via TCP/IP
Software: MySQL
Software version: 5.5.27 - MySQL Community Server (GPL)
Protocol version: 10
Server charset: UTF-8 Unicode (utf8)
Remote Windows Box:

Server: Localhost via UNIX socket
Server type: MySQL
Server version: 5.1.70-cll - MySQL Community Server (GPL)
Protocol version: 10
Server charset: UTF-8 Unicode (utf8)
—
Reply to this email directly or view it on GitHub.

[jburns131]

Whoops, updated that info lol

[jburns131]

I've installed a local VMWare machine installing a recent linux distro with the most current lamp stack (mysql 5.5), and the tests are still failing at exactly the same spots. I've removed the 'ORDER BY parent.Lft' as stated above and tests pass on all platforms.

I'm in the process of debugging the MySQL query in $rbac->Check(), which seems to be the last method to be failing on a linux platform.

I'll get back to you when I either have a resolution or I get stuck.

[jburns131]

Well, the issue with $rbac->Check() was a case issue (capitol D should be lowercase) :-p

That's it. All tests are green on both platforms.

Once you get a chance to evaluate PathID and come to a conclusion about removing the 'ORDER BY parent.Lft', we can apply the needed fix and then we can start packaging this puppy up.

I'd like to add some general docs, an INSTALL.md, something explaining setting up and running the Unit Tests.

But lets figure out PathID first, then I'll open up an issue to discuss packaging and what not.

Oh yah!

[abiusx]

I already knew it was a case issue, but you told in ur first message that you checked for case issues! So I thought you had that one fixed.

Please add a throwing exception in PathID so that if the length is above 1024 characters (or whatever it can take), it throws an error. Otherwise it would silently fail, which is very bad.

---

Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Aug 26, 2013, at 8:19 AM, Jesse Burns [email protected] wrote:

Well, the issue with $rbac->Check() was a case issue (capitol D should be lowercase) :-p

That's it. All tests are green on both platforms.

Once you get a chance to evaluate PathID and come to a conclusion about removing the 'ORDER BY parent.Lft', we can apply the needed fix and then we can start packaging this puppy up.

I'd like to add some general docs, an INSTALL.md, something explaining setting up and running the Unit Tests.

But lets figure out PathID first, then I'll open up an issue to discuss packaging and what not.

Oh yah!

—
Reply to this email directly or view it on GitHub.

[jburns131]

Yeah, the PathID failed tests weren't related to case, and I did check the php code around the sql query in the Check method, but when it was the query failing I had thought it was another linux/windows query issue.

I always find the biggest pain in the butt about errors and debugging are those darn missing/incorrect symbols (missing comma or == vs =), and case issues. I'm usually looking for big problems in the logic and tend to overlook the (not so) little stuff lol.

I'll look into the PathID exception.

[jburns131]

I have just pushed the changes regarding the PathID 'GROUP_CONCAT' character limit.

See commit diff for changes.

Explanation for code changes...

There are two issues you can run into with PathID and long paths:

Minor Issue

Long paths can cause PHP to exceed the execution time limit. This mainly happens on slower machines, and during sqlite tests. This shouldn't be much of a problem on production servers.

Major Issue

The only limit of the query in question is that the result of 'GROUP_CONCAT' cannot exceed a character limit of 1024 characters. The only accurate/reliable solution is:

• First we count the amount of rows being returned so we can use that result, minus 1, to account for the / (slashes) that 'GROUP_CONCAT' adds to concatenate row Titles.
• Then we need to count the character length of all returned rows and add that to the number of / (slashes) that 'GROUP_CONCAT' will add. That will give us the total character count of the 'GROUP_CONCAT' result set.
• If the total character count exceeds the 'GROUP_CONCAT' character limit of 1024 then we throw an exception.

I've created a Unit Tests for this exception and the tests pass for both MySQL and Sqlite.

I do understand that the added queries do add a little more overhead, but it was negligible on my production server, and it's also the only way we can accurately define that there is an error and properly throw an exception if needed.

If this solution is satisfactory then we can close this Issue.

[abiusx]

if the path is directly passed to the string,
why not just check its length?

---

Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Aug 27, 2013, at 11:31 AM, Jesse Burns [email protected] wrote:

I have just pushed the changes regarding the PathID 'GROUP_CONCAT' character limit.

See commit diff for changes.

Explanation for code changes...

There are two issues you can run into with PathID and long paths:

Minor Issue

Long paths can cause PHP to exceed the execution time limit. This mainly happens on slower machines, and during sqlite tests. This shouldn't be much of a problem on production servers.

Major Issue

The only limit of the query in question is that the result of 'GROUP_CONCAT' cannot exceed a character limit of 1024 characters. The only accurate/reliable solution is:

First we count the amount of rows being returned so we can use that result, minus 1, to account for the / (slashes) that 'GROUP_CONCAT' adds to concatenate row Titles.

Then we need to count the character length of all returned rows and add that to the number of / (slashes) that 'GROUP_CONCAT' will add. That will give us the total character count of the 'GROUP_CONCAT' result set.

If the total character count exceeds the 'GROUP_CONCAT' character limit of 1024 then we throw an exception.

I've created a Unit Tests for this exception and the tests pass for both MySQL and Sqlite.

I do understand that the added queries do add a little more overhead, but it was negligible on my production server, and it's also the only way we can accurately define that there is an error and properly throw an exception if needed.

If this solution is satisfactory then we can close this Issue.

—
Reply to this email directly or view it on GitHub.

[jburns131]

That was my first thought too.

But if you look at the query, mainly Lines 104 and 109,

https://github.com/OWASP/rbac/blob/dev/PSR/Unit_Tests/PhpRbac/src/PhpRbac/core/lib/rbac.php#L104

you'll see that 'GROUP_CONCAT' is pulling every permission/role title in the table and concatenating them with a slash. Then the rest of the query checks to see if the path is inside that result string and if so finding and sending the entity type's ID back as a result.

So the 'GROUP_CONCAT' result set can be much longer than the $path, even if the path is only a few lengths deep, i.e. < 100 characters.

[abiusx]

I think a simpler solution is the better idea, like try...catch ing the query and returning some code when it fails. We have mentioned this limit, and there's nothing that can be done regarding it!

The whole purpose of PathID is to make it a single query, otherwise we could run a bunch of queries to retrieve the ID, which you are adding as an overhead now. I believe we can make it an option, so that if its true, this check is performed and if it fails, a loop of queries will be executed, and when its false, it simply errors.

What do you think?

[jburns131]

That should work fine.

i don't remember what type of error is thrown (PHP vs PDO exception), but I'm sure we can make it fail gracefully by introducing an exception if there isn't one thrown already.

Just to make sure we're on the same page, this is my understanding of the fix we're talking about:

• Add a parameter to the method, 'true' means run the test for character limit, 'false' means do not run test.
  • I propose that the parameter have a default of 'false'.
• If the 'char limit' test parameter is 'false', do not run the tests, but if the char count is exceeded, throw an exception so developers can implement proper error handling
  • If the char limit is exceeded then it might already be throwing an exception, but if not I propose we introduce and throw an exception

Do you want me to work on this, or do you have the time for it?

[abiusx]

This is correct.
We need to throw our own exception, cuz they have no idea whats heppening inside to catch.

If it checks for length, and length exceeds, its agood idea to failback to a method that retrives ID using multiple queries (tree lookup), just for reliability.

The TRUE/FALSE thing shouldn't be a n input to the method, but an option (e.g public variable in the class) that can be set by developers.
-Abbas

---

Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Oct 1, 2013, at 9:47 PM, Jesse Burns [email protected] wrote:

That should work fine.

i don't remember what type of error is thrown (PHP vs PDO exception), but I'm sure we can make it fail gracefully by introducing an exception if there isn't one thrown already.

Just to make sure we're on the same page, this is my understanding of the fix we're talking about:

Add a parameter to the method, 'true' means run the test for character limit, 'false' means do not run test.

I propose that the parameter have a default of 'false'.
If the 'char limit' test parameter is 'false', do not run the tests, but if the char count is exceeded, throw an exception so developers can implement proper error handling

If the char limit is exceeded then it might already be throwing an exception, but if not I propose we introduce and throw an exception
Do you want me to work on this, or do you have the time for it?

—
Reply to this email directly or view it on GitHub.

[jburns131]

See, I did get you wrong.

If the flag is true we check for an error. If there is an error, then we run a smaller set of queries (that also means more overhead) to get the requested pathId.

If the flag is true, we throw an error. I still propose that we find out if an exception is thrown, and if not we throw one.

[jburns131]

The only thing about the true/false being an class property vs a method parameter is that the property is specific to just this one method. I don't see where other methods would be able to take advantage or have use for this property.

If it was the last parameter in the method call then we can give it a default value (most likely 'false' since it's more efficient), and then devs would only need to directly manipulate this value if their path hierarchy becomes large enough to warrant the check.

Let me know if you see any flaws in my reasoning.

[jburns131]

Altered Jf::sqlPdo and Jf::sqlMysqli. Will now execute "SET SESSION group_concat_max_len = 1000000" if pathId is going to be used.

I believe this Issue is resolved.

I'm keeping this Issue open till I finish creating Unit Tests and they pass with flying colors.

[jburns131]

Unit Test completed. Closing Issue.