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

Fixes #64 inGroup method to check with OR instead of AND #69

Merged
merged 3 commits into from
Aug 8, 2019
Merged

Fixes #64 inGroup method to check with OR instead of AND #69

merged 3 commits into from
Aug 8, 2019

Conversation

fefo-p
Copy link
Contributor

@fefo-p fefo-p commented Aug 7, 2019

Rolefilter method inGroup() used to do an AND comparison to check if user belonged to all groups.
Now, it's changed to an OR comparison, ie: if user belongs to any group passed as parameters, then it returns TRUE.
Regarding the ANDcheck, if needed it could be in a new method named inGroups() that would make more sense

Rolefilter method inGroup()used to do an AND comparison to check if user belonged to all groups.
Now, it's changed to an OR comparison, ie: if user belongs to any group passed as parameters, then it returns TRUE.
@fefo-p fefo-p changed the title inGroup method to check with OR instead of AND Fixes #64 inGroup method to check with OR instead of AND Aug 8, 2019

// Check each requested permission
foreach ($params as $group)
{
$result = $result && $authorize->inGroup($group, $authenticate->id());
if(! $result)
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of continuing to loop over results, we should break out of the loop as soon as we a have a match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will look into it

@MGatner
Copy link
Collaborator

MGatner commented Aug 8, 2019

Maybe, but I think like you said it will always be true, no matter what $result ends up being.
But I get what you mean. Will look into it later when I can test. Right now I'm at work and don't have my computer with me.

Oh right! I forgot we need to be returning null. No need to track $result at all then. Here's my recommendation:

		$authorize = Services::authorization();
		
		// Check each requested permission
		foreach ($params as $group)
		{
				 if ($authorize->inGroup($group, $authenticate->id())
				{
						return;
				}
		}

		if ($authenticate->silent())
		{
				$redirectURL = session('redirect_url') ?? '/';
				unset($_SESSION['redirect_url']);
				return redirect()->to($redirectURL)->with('error', lang('Auth.notEnoughPrivilege'));
		}
		else {
				throw new \RuntimeException(lang('Auth.notEnoughPrivilege'));
		}

@fefo-p
Copy link
Contributor Author

fefo-p commented Aug 8, 2019

Looks good

=> Got rid of $result as it's no longer needed.
@lonnieezell lonnieezell merged commit d37258a into lonnieezell:develop Aug 8, 2019
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