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

The 'jf::CurrentUser()' method does not exist #2

Closed
jburns131 opened this issue Aug 21, 2013 · 4 comments
Closed

The 'jf::CurrentUser()' method does not exist #2

jburns131 opened this issue Aug 21, 2013 · 4 comments
Assignees

Comments

@jburns131
Copy link
Collaborator

The 'jf::CurrentUser()' method does not exist, causing errors:

  • $rbac->Users->HasRole() // Complete
  • $rbac->Users->Assign() // Complete
  • $rbac->Users->Unassign() // Complete
  • $rbac->Users->AllRoles() // Complete
  • $rbac->Users->RoleCount() // Complete
  • $rbac->Users->Enforce() // Complete

All of the above throw PHP errors if no '$User/$UserID' parameter is provided ($User/$UserID = null). The proper behavior is to check 'jf::CurrentUser()' for current user id, but if 'jf::CurrentUser()' does not exist we get an error stating that fact.

I'm assuming that 'CurrentUser' is an interface that users of PhpRbac needs to implement, but wanted to verify that.

I was wondering if it might be a good idea to implement CurrentUser() with a default of 0 (zero, for guest), and either allow the user of PhpRbac to override it, or have a setter so users can dynamically update the current UserID.

I've added the following code to the jf class (jf.php) as a temporary fix so I can continue working on Unit Tests:

static function CurrentUser() {
    return 0; // Guest
}
jburns131 added a commit that referenced this issue Aug 22, 2013
Added temporary jf::CurrentUser() static method. See Issue #2.
@abiusx
Copy link
Contributor

abiusx commented Aug 22, 2013

it is a means of retriving the current (logged in) user ID to check for permissions, cuz its the most common case.
it is left there to be implemented by the developer, but marking it out in a PSR implementatin woul dbe a good idea. Its authentication domain and should not be included in authorization.
-A


Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Mordad 31, 1392, at 1:48 AM, Jesse Burns notifications@github.com wrote:

The 'jf::CurrentUser()' method does not exist, causing errors:

$rbac->Users->HasRole()
$rbac->Users->Assign()
$rbac->Users->Unassign()
$rbac->Users->AllRoles()
$rbac->Users->RoleCount()
$rbac->Users->Enforce()
All of the above throw PHP errors if no '$User/$UserID' parameter is provided ($User/$UserID = null). The proper behavior is to check 'jf::CurrentUser()' for current user id, but if 'jf::CurrentUser()' does not exist we get an error stating that fact.

I'm assuming that 'CurrentUser' is an interface that users of PhpRbac needs to implement, but wanted to verify that.

I was wondering if it might be a good idea to implement CurrentUser() with a default of 0 (zero, for guest), and either allow the user of PhpRbac to override it, or have a setter so users can dynamically update the current UserID.

I've added the following code to the jf class (jb.php) as a temporary fix so I can continue working on Unit Tests:

static function CurrentUser() {
return 0;
}

Reply to this email directly or view it on GitHub.

@jburns131
Copy link
Collaborator Author

Thank you. If it's ok with you I will remove any reference to jf::Current user, thus making the user-id mandatory.

I will also update the above mentioned methods and create/throw an exception if the user-id argument is empty.

Sound good?

@abiusx
Copy link
Contributor

abiusx commented Aug 23, 2013

gr8


Notice: This message is digitally signed, its source and integrity are verifiable.
If you mail client does not support S/MIME verification, it will display a file (smime.p7s), which includes the X.509 certificate and the signature body. Read more at Certified E-Mail with Comodo and Thunderbird in AbiusX.com

On Shahrivar 1, 1392, at 7:20 AM, Jesse Burns notifications@github.com wrote:

Thank you. If it's ok with you I will remove any reference to jf::Current user, thus making the user-id mandatory.

I will also update the above mentioned methods and create/throw an exception if the user-id argument is empty.

Sound good?


Reply to this email directly or view it on GitHub.

jburns131 added a commit that referenced this issue Aug 24, 2013
…ge for further details.

Resolves Issue #2:

* I have removed all references of 'jf::CurrentUser()', made $UserID mandatory, and have added an exception if the $UserID is not provided. This exception will add flexibility to PhpRbac's testability and error handling capabilities.
* I have updated all affected Unit Tests associated with these changes, while adding Unit Tests as needed

Resolves Issue #3:

* Along with the changes from Issue #2, I have removed the references to jb::run and added the 403 header along with a die which displays a 'Forbidden' message if the check in $rbac->enforce fails
* If the check in $rbac->enforce is successful, $rbac->enforce will return true for testing and error handling purposes

Resolves #6:

* UserID is now mandatory and will throw an exception if the parameter is empty
@jburns131
Copy link
Collaborator Author

I've addressed this Issue in commit aebafee

Please review and close Issue if everything looks good to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants