Skip to content

Conversation

hhamon
Copy link
Contributor

@hhamon hhamon commented May 12, 2011

[security] added a note to change the voter strategy in the AccessDecisionManager object.

@weaverryan
Copy link
Member

Hugo-

There may be a little more needed here. Noel wrote this entry, and I asked him to change it so it used a different strategy and was more straightforward. He did that, but then we found out that the new method I asked him to use would not work. That pull request and conversation is here: https://github.com/symfony/symfony-docs/pull/206/files

After reviewing that, let me know if this PR addresses the issue fully or if more changes need to be made so that the example in this entry is functional. It's been on my todo list for awhile, so I'm happy you're looking into this entry.

Thanks!

@hhamon
Copy link
Contributor Author

hhamon commented May 12, 2011

Hi Ryan,

The problem if you follow the tutorial, you will have a voter but you won't be able to see it in action as the access decision manager service is configured to grant access if one voter grants access. I was frustrated to not see this voter in action until I understand how the access decision manager works.

Maybe we should add a note to explain how to setup a basic authentication first and then how to change the access decision manager strategy to make the ClientIpVoter voter works.

@weaverryan
Copy link
Member

Hugo-

Ok, first - it does look like your new section takes care of the problem - meaning that this entry will now work if the user follows along. That's the big thing I was concerned about. So, I'm going to merge it in.

When we have more information about the decision manager somewhere, we could make a link to that section from here.

Thanks!

weaverryan added a commit that referenced this pull request May 12, 2011
[security] added a note to change the voter strategy in the AccessDecisio
@weaverryan weaverryan merged commit ccfd19f into symfony:master May 12, 2011
@hhamon
Copy link
Contributor Author

hhamon commented May 12, 2011

Thanks!

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.

2 participants