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

[Security] Typos in Security's ExpressionLanguage #9597

Closed
wants to merge 2 commits into from

Conversation

ovrflo
Copy link
Contributor

@ovrflo ovrflo commented Nov 24, 2013

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #9593
License MIT
Doc PR none

Fixed the ExpressionLanguage and added the tests. It looks ok to me, but please, take a close look (some people will depend on this working right, including me :D ).

Thanks !

…\Authorization\ExpressionLanguage.

Changed some functions available in ExpressionLanguage:
is_authenticated() updated to account for null tokens (firewall security set to false).
Fix typos in is_fully_authenticated().
is_fully_authenticated() should not negate $trust_resolver->isFullFledged().
is_remember_me() should also not negate the result of $trust_resolver->isRememberMe().
@pborreli
Copy link
Contributor

thanks for you contribution ✨ you should respect coding standard in your test file (4 spaces indentation)

@ovrflo
Copy link
Contributor Author

ovrflo commented Nov 24, 2013

Sorry about the indentation. I'm not using my work IDE. I just fixed the indentation in the test. The other file looks ok.

Thanks !

@@ -31,21 +31,21 @@ protected function registerFunctions()
});

$this->register('is_authenticated', function () {
return '!$trust_resolver->isAnonymous($token)';
return '$token && !$trust_resolver->isAnonymous($token)';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why $token && is needed? AFAIK, the check is already done in isAnonymous.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind, understood now.

@fabpot fabpot closed this in 419e10d Nov 24, 2013
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