Skip to content

Fix PHP notices#234

Closed
dregad wants to merge 2 commits into
dokuwiki:masterfrom
dregad:master
Closed

Fix PHP notices#234
dregad wants to merge 2 commits into
dokuwiki:masterfrom
dregad:master

Conversation

@dregad
Copy link
Copy Markdown
Contributor

@dregad dregad commented Jun 26, 2013

During tests following conversion of the old MantisBT authentication back-end to a plugin, I realized that DokuWiki triggers a number of PHP notices.

dregad added 2 commits June 26, 2013 18:27
Caused by accessing undefined index REMOTE_USER.
Failure to do this causes a PHP notice error, which can happen e.g. if a
plugin starts a session.
@dregad
Copy link
Copy Markdown
Contributor Author

dregad commented Jun 26, 2013

Hi Andreas,

Thanks for your feedback and guidance. Having the session opening is a good idea on principle, but it probably would not work for my specific case since the session is opened by the MantisBT API, which has to be included for the auth plugin to work.

So, on second thoughts, maybe what I need to do is tweak the auth plugin to selectively call the required MantisBT APIs instead of simply including all of them through the main core.php; that automatically starts a session, but in fact it's probably not needed for what the plugin needs to do. I'll think about it.

Do you have any comments or objections about the other commit ? If not, do you want to cherry-pick that, or should I submit a separate pull request ?

In any case, I think this one can be closed.

@Chris--S
Copy link
Copy Markdown
Collaborator

Chris--S commented Aug 3, 2013

I think the intention is correct, but the implementation above looks flawed. E.g. in auth.php it looks like your shifting the problem from $_SERVER['REMOTE_USER'] to $id and $rest. In this instance, maybe ensuring $_SERVER['REMOTE_USER'] is always initialized might be a better solution.

I'll close this PR, but we would accept something similar with a better implementation. Killing notices is good.

@Chris--S Chris--S closed this Aug 3, 2013
@Chris--S
Copy link
Copy Markdown
Collaborator

Chris--S commented Aug 3, 2013

my above comment isn't strictly correct :)

however, in checking the code referenced in auth.php, we've discovered a number of flaws...

  • rules should be skipped when they contain %USER% tokens and there is no logged in user (as happens for %GROUP%).
  • code needs to be reorganized to allow for rules which contain both %GROUP% and %USER% tokens.

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.

3 participants