Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix useless use of $auth->isCaseSensitive() in auth_aclcheck() #146

Closed
wants to merge 2 commits into from

Conversation

kazmiya
Copy link
Contributor

@kazmiya kazmiya commented Nov 24, 2012

ACL matching of DokuWiki is currently always case-sensitive
regardless of auth backend setting ($auth->isCaseSensitive).

These commits enable case-insensitive matching in the same way
of auth_isMember().

ACL matching of DokuWiki is currently always case-sensitive
regardless of auth backend setting ($auth->isCaseSensitive).

This commit enables case-insensitive matching in the same way
of auth_isMember().
@dom-mel
Copy link
Collaborator

dom-mel commented Nov 24, 2012

That isn't true. The current code uses a case insensitive RegEx (the "i" flag).

see http://php.net/manual/en/function.preg-match.php Example 1.

@dom-mel dom-mel closed this Nov 24, 2012
@splitbrain splitbrain reopened this Nov 24, 2012
@splitbrain
Copy link
Collaborator

Reopened, since I guess @kazmiya was trying to fix a problem he had here. I could imagine that preg_match's idea of case insensitivity differs from what a utf8_strtolower does. Especially when non-ASCII chars are involved and the PREG module is missing some compile time options...

@kazmiya can you please provide some more details why the original code fails for you? A unit test would be helpful as well.

@michitux
Copy link
Collaborator

From looking at the code I get the impression that we matched the id of the page case-insensitively against the ACL rules when the authentication system indicated that the user/group should be matched case-insensitively. The comparison of the group/user happens not in the regular expression but later in the if(!in_array($acl[1], $groups)) { statement, that's why @kazmiya added the utf8_strtolower here.

For me this looks good, I vote for merging though having unit tests would be great of course.

@kazmiya
Copy link
Contributor Author

kazmiya commented Nov 25, 2012

@michitux correctly explained what I was thinking about.

The $auth->isCaseSensitive() is for user IDs, not for page IDs. The current code uses case-insensitive preg_match against page IDs which appears on acl.auth.php file.

The problem is:

  1. Let's say you have "MyUser" in your LDAP server.
  2. And you also have "* myuser 1" entry in your acl.auth.php.
  3. When you log in as MyUser, acl matching against myuser will fail.
  4. You cannot get read permission on the root namespace.

But after I submitted this pull request, I noticed that I should give special consideration for the @ALL group.

i.e. I guess this line...

if(!$auth->isCaseSensitive()) $acl[1] = utf8_strtolower($acl[1]);

should be:

if(!$auth->isCaseSensitive() && $acl[1] !== '@ALL') {
    $acl[1] = utf8_strtolower($acl[1]);
}

($acl[1] here stores an userID or a groupID set in acl.auth.php)

Any ideas?

@dom-mel
Copy link
Collaborator

dom-mel commented Nov 25, 2012

sorry didn't see that last night ;-)
@michitux +1 for unit tests

@michitux
Copy link
Collaborator

I think this exception for @ALL makes sense and is needed as otherwise rules for @ALL are never matched. I think this should be covered in a unit test, too.

@kazmiya
Copy link
Contributor Author

kazmiya commented Nov 25, 2012

Closed. Please see #147.

@kazmiya kazmiya closed this Nov 25, 2012
splitbrain added a commit that referenced this pull request Apr 9, 2020
* master:
  media fields need unique IDs. fixes #152
  fixed references to old Integer type
  removed Integer type
  Version upped
  fixed problem with multicolumns and made the param name clearer
  added missing schema file
  when Page uses titles, sorting should use title #146
  Decimal type needs to type cast for order and compare #146
  make sure pages are assigned in tests
  give types a way to influence sorting
  Version upped
  diff needs to compare raw values
  test wildcard adding in Search
splitbrain added a commit that referenced this pull request Apr 9, 2020
* master:
  output the key not value for unknown option in ConfigParser
  media fields need unique IDs. fixes #152
  fixed references to old Integer type
  removed Integer type
  Version upped
  fixed problem with multicolumns and made the param name clearer
  added missing schema file
  when Page uses titles, sorting should use title #146
  Decimal type needs to type cast for order and compare #146
  make sure pages are assigned in tests
  give types a way to influence sorting
  add slight shadow for aggregation images
  Fixed multi fields for inline editor
  test wildcard adding in Search
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants