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

Bug with handling of admin context #701

Open
jettero777 opened this issue Dec 17, 2021 · 3 comments
Open

Bug with handling of admin context #701

jettero777 opened this issue Dec 17, 2021 · 3 comments

Comments

@jettero777
Copy link
Contributor

jettero777 commented Dec 17, 2021

Hello,

I'm having issue when I need to access some models from inside of EVENT_AFTER_NGREST_FIND event handler.
When I do SomeModel::find() I expect to get model' fields as when I do find() in not admin context, but instead fields being processed by plugins and prepared for admin backend.
It is because of these parts of code, where context being determined solely by global GET vars:

    public function afterFind()
    {
        if ($this->getNgRestCallType()) {
            if ($this->getNgRestCallType() == 'list') {
                $this->trigger(self::EVENT_AFTER_NGREST_FIND);
            }
            if ($this->getNgRestCallType() == 'update') {
                $this->trigger(self::EVENT_AFTER_NGREST_UPDATE_FIND);
            }
        } else {
            return parent::afterFind();
        }
    }
    public function getNgRestCallType()
    {
        if ($this->_ngrestCallType === null) {
            $this->_ngrestCallType = (!Yii::$app instanceof \yii\web\Application) ? false : Yii::$app->request->get('ngrestCallType', false);
        }

        return $this->_ngrestCallType;
    }

As a result finding any model from inside admin context will call EVENT_AFTER_NGREST_FIND or EVENT_AFTER_NGREST_UPDATE_FIND though I'm not going to use model for REST and want to have raw field values and not processed (namely I need raw foreign keys but they are replaced with labels by plugin).

Furthermore it leads to more significant bug.

If you add to website url '?ngrestCallType=update' or '?ngrestCallType=list' or '?ngrestCallType=1' then website will become broken because all models will be processed as in admin context.

For example will be shown all translations in json format instead of only needed one, foreign keys will be replaces with fields names in case of selectModel plugin, etc.
In some cases will be just fatal error shown because some fields of a model contains arrays there should be strings (because it was decoded by plugin for admin context).

It is not expected behavior for a website.
I suppose getNgRestCallType context shouldn't not be set globally for all models but only for models explicitly specified in REST call.

@nadar
Copy link
Contributor

nadar commented Dec 17, 2021

yes we are aware of that, it will throw an exception which is bad, but nothing more will happen. we could ensure admin context with https://github.com/luyadev/luya/blob/master/core/web/Request.php#L60 but not sure this will break backwards compatibility.

@jettero777
Copy link
Contributor Author

jettero777 commented Dec 17, 2021

yes we are aware of that, it will throw an exception which is bad, but nothing more will happen.

In my tests I'm more often getting broken output, not just exception, for example instead of text I'm getting {"en":"text", "de":"text", ....} in frontend

Also because model' fields in unexpected state it can lead to wrong processing by business logic and can lead to more serious problems and vulnerabilities.

we could ensure admin context with https://github.com/luyadev/luya/blob/master/core/web/Request.php#L60 but not sure this will break backwards compatibility.

This will work for frontend but in admin all models still will be processed as for REST output, maybe to add some static property to NgRestModel to be able to override global getNgRestCallType context?

Is EVENT_AFTER_NGREST_FIND really supposed to be called for any model find() in admin by design? I think it is not needed if model is not going to be displayed via REST call.

@jettero777
Copy link
Contributor Author

jettero777 commented Dec 18, 2021

Here results of tests in frontend:

  1. Frontend guest user
  • ?ngrestCallType=update – exception (array to string conversion)
  • ?ngrestCallType=list – no exception, seems ok at first, but really models are broken, foreign keys are replaced with label instead of id and because of it lazy relations to other models is not working and business logic using relations would fail.
  • ?ngrestCallType=1 – no exception, i18n text string not decoded properly and shown in json format {"en":.., "de":.., ...}
  1. Logged-in user in frontend.
  • ?ngrestCallType=update – exception (Getting unknown property: luya\web\Application::adminLanguage)
  • ?ngrestCallType=list – exception (Getting unknown property: luya\web\Application::adminLanguage)
  • ?ngrestCallType=1 – no exception, i18n text string not decoded properly and shown in json format {"en":.., "de":.., ...}

I think I will fix it temporally with custom base class with replaced afterFind() method. But in future I think it better to be fixed, in some major version maybe where BC can be infringed.

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

No branches or pull requests

2 participants