Copy audience claims to user roles

[PBXg33k]

This PR will copy the audience claims to the user if the user object has a method "addRole(string $role)"

Added a slightly extended class of symfony's User class (which is final) which has the addRole() method for testing the new method in the Authenticator.

[coveralls]

Coverage increased (+0.1%) to 98.295% when pulling 1d5c3ca on PBXg33k:feature/user-roles into cdd018c on kleijnweb:master.

[kleijnweb]

Hi Oguz, thanks for the PR. It looks good, but I have 2 issues with it:

1. I am not a fan of duck typing in PHP, using interfaces is preferred
2. The PR is not strictly backwards compatible: an object that already has the method will potentially change security behavior

Both can be solved by using a type check with a DIP interface.

[PBXg33k]

Hey John,

Thanks for the feedback. The reasoning behind this approach was because i couldn't find the setters for the roles in the common interfaces for the Symfony framework.

I will look for a more proper solution in the meantime.

[kleijnweb]

Applying the DIP would mean creating a new interface, adding it to JwtBundle, then implement in identity (User) classes that you want to apply this behavior to. Then instead of checking if the method exists (duck typing), you do a type assertion.

[coveralls]

Coverage decreased (-1.0%) to 97.191% when pulling 982545a on PBXg33k:feature/user-roles into cdd018c on kleijnweb:master.

[coveralls]

Coverage increased (+0.2%) to 98.315% when pulling 3940c39 on PBXg33k:feature/user-roles into cdd018c on kleijnweb:master.

[coveralls]

Coverage increased (+0.2%) to 98.315% when pulling e1f2a33 on PBXg33k:feature/user-roles into cdd018c on kleijnweb:master.

[PBXg33k]

Hey John,

The PR has been updated but for some unknown reason Scrutinizer has failed on PHP Analyzer: Determining Dependencies, mind running it again or should i make an empty commit? 😝

[kleijnweb]

Just add the missing newline to UserInterface.php and it will trigger again ;)

[coveralls]

Coverage increased (+0.2%) to 98.315% when pulling ea78cf8 on PBXg33k:feature/user-roles into cdd018c on kleijnweb:master.

[kleijnweb]

One last thing: role names in Symfony are by convention upper case and prefixed with "ROLE_". Lets coax audience claims into that format. In future we may want to use something like a RoleFormatter type to define different strategies, but for now, just always upper-casing and prefixing (unless the prefix is already present) will do.

[PBXg33k]

Meaning we'd prefix the claims with "ROLE_" and convert them to uppercase?

[kleijnweb]

Indeed.

[coveralls]

Coverage increased (+0.2%) to 98.315% when pulling 1f18305 on PBXg33k:feature/user-roles into cdd018c on kleijnweb:master.

[kleijnweb]

Released as v0.12.0.

[PBXg33k]

Thanks 👍