Add dict instance check before accessing it

[rajuniit]

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[erikjohnston]

@rajuniit Thanks for this. I'm curious what the rationale for this change was? I'm just wondering if it would be better to give a better error message instead, rather than ignore a malformed body?

[rajuniit]

@rajuniit Thanks for this. I'm curious what the rationale for this change was? I'm just wondering if it would be better to give a better error message instead, rather than ignore a malformed body?

Thank you @erikjohnston for your feedback. I think there is already an implementation where if there is no 'sid', it will create a new 'sid'. That's why I believe we shouldn't block this execution, like this.

if not sid:
            new_session_data = get_new_session_data() if get_new_session_data else {}

            session = await self.store.create_ui_auth_session(
                clientdict, uri, method, description
            )

            for k, v in new_session_data.items():
                await self.set_session_data(session.session_id, k, v)

Also, we have a check in the later code

if not authdict:
            raise InteractiveAuthIncompleteError(
                session.session_id, self._auth_dict_for_flows(flows, session.session_id)
            )

Maybe we can just log the error into the logger. what do you think?

[clokep]

It sounds like a major error if authdict isn't a dictionary -- how can this happen? Is it from client data? If so, we should send a good error message back to the user saying to make that a dictionary.

[rajuniit]

It sounds like a major error if authdict isn't a dictionary -- how can this happen? Is it from client data? If so, we should send a good error message back to the user saying to make that a dictionary.

Yes, it is from client data. And I think we are sending an error message back to the user if authdict is not defined.

From this code block

if not authdict:
            raise InteractiveAuthIncompleteError(
                session.session_id, self._auth_dict_for_flows(flows, session.session_id)
            )

The new check has been added to handle 500 errors only.

[rajuniit]

Is there anything I need to change to approve it?

[clokep]

Is there anything I need to change to approve it?

I think we want to send a similar error back if the authdict is not a dictionary. Right now we just ignore that it is the wrong data and attempt to continue processing.

[rajuniit]

Is there anything I need to change to approve it?

I think we want to send a similar error back if the authdict is not a dictionary. Right now we just ignore that it is the wrong data and attempt to continue processing.

I have added an error response.

[rajuniit]

@clokep is this failed test case related to my changes? Because I don't find anything related to it. Also, I see the same failed test case in other pull requests.

[rajuniit]

@clokep Do I need to wait for the reviewer? Or anything I need to do from my side?

[clokep]

@clokep Do I need to wait for the reviewer? Or anything I need to do from my side?

Yes, this is in the review queue and the team will look at it when they have time.

[clokep]

I had a chance to look deeper at this; check_ui_auth has 3 callers:

• validate_user_via_ui_auth, this has a handful of callers.
• PasswordRestServlet.on_POST
• RegisterRestServlet.on_POST

Some of these properly validate that auth is None or a dictionary. Others don't have validation (see #13147). I'm wondering if we should do validation up front instead of at this stage.

@DMRobertson It looks like you started adding the UI Auth validation of REST endpoints. Did you have a plan here?

[DMRobertson]

I didn't have a specific plan, I'm afraid. I vaguely remember UI auth being confusing when I looked it it, but that was about a year ago.

My general plan was to do as much validation as possible in the rest layer, and pass a parsed Pydantic model instance down to the handlers. The two can be done separately, which makes it easier to validate one endpoint at a time.

[DMRobertson]

There is an vaguely open question about how we should be using Pydantic going forward. It has just made a 2.x release with a strict mode built-in (no more check_pydantic_models.py) and performance improvements. There are some breaking changes though, plus some of our packagers are presumably going to be shipping Pydantic 1.x for some time. #15979 has some more detailed discussion.

[prajjawal05]

Hi @clokep @DMRobertson, was some decision made on Pydantic? What should we do here for now?

If this function is called from every rest endpoint at the beginning should we keep it here, or move it upfront to rest layers which are using this.

[clokep]

@prajjawal05 I think adding the validation would be a good step forward. I don't agree there's an open question. Today we're using pydantic 1.x and should not block good work because we might one-day update to pydantic 2.x.

[clokep]

As mentioned, I think this is the wrong approach. We should be doing validation of the data at the REST layer to ensure that the expected fields exist.