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

Deprecate logged-in-user magic from permission checks #8953

Closed
juho-jaakkola opened this issue Sep 17, 2015 · 9 comments
Closed

Deprecate logged-in-user magic from permission checks #8953

juho-jaakkola opened this issue Sep 17, 2015 · 9 comments

Comments

@juho-jaakkola
Copy link
Member

Most of our permission checks default to the logged in user in case a user is not explicitly passed into the permission check functions.

Deprecating this feature will:

  • Make permission checks safer as it forces devs to think what they're doing instead of relying on magic
  • Reduce the complexity of core
@juho-jaakkola
Copy link
Member Author

We might also type hint ElggUser to further reduce boilerplate.

@mrclay
Copy link
Member

mrclay commented Sep 18, 2015

Transition might be tricky. We'd have to leave the default as 0 and interpret null as logged out, which might present a BC break.

@juho-jaakkola
Copy link
Member Author

I forgot that sometimes we may need to check also non-logged-in users. Could an empty ElggEntity object represent a non-logged in user?

$container->canWriteToContainer(new ElggUser());

@mrclay
Copy link
Member

mrclay commented Sep 19, 2015

A long time ago I proposed a UserState value object with just a few methods (getUser, getGuid, isLoggedIn). The value is in eliminating ambiguity. null means you don't have it, and the API can fetch it for you. If you pass it in, the API knows you intend for that state to be used.

Using a new ElggUser to mean logged out user I think would be dangerous as it leaked into other areas of the API. E.g. so many places we use a boolean check to see if there's an object or falsey.

Something like this would be BC safe:

function canEdit($user = ElggUser::NOT_GIVEN) {
    // here we can safely distinguish between these values:
    // ElggUser = logged in
    // null = logged out
    // ElggUser::NOT_GIVEN ('NOT_GIVEN') = pull it from session
    // anything else = throw exception
}

@mrclay
Copy link
Member

mrclay commented Sep 19, 2015

Eh, that's no good either, same problem as using 0.

@mrclay
Copy link
Member

mrclay commented Mar 3, 2016

Using entityTable->getUserForPermissionsCheck() internally, this seems no longer a big problem. If it's really worth breaking a ton of API to be more strict, we can use something like this, which can unambiguously represent a logged out user.

@mrclay mrclay removed this from the Elgg 2.x milestone May 12, 2016
@jdalsem
Copy link
Member

jdalsem commented Jan 22, 2018

Going to leave this as is

@jdalsem jdalsem closed this as completed Jan 22, 2018
@hypeJunction
Copy link
Contributor

I don't think we should leave it as is

@jdalsem
Copy link
Member

jdalsem commented Jan 22, 2018

for now we will... both ideas were not solving the issue. Can reopen this when there is something better

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

No branches or pull requests

4 participants