Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fixes authentication when user is registered via module API #10156

Closed
wants to merge 4 commits into from

Conversation

maheichyk
Copy link
Contributor

@maheichyk maheichyk commented Feb 13, 2023

Type: Defect
Related: https://github.com/matrix-org/matrix-react-sdk-module-api

When user is registered via module API (using ILAG module for example) the following error is received:

Invariant Violation: Dispatch.dispatch(...): Cannot dispatch in the middle of a dispatch.
    invariant invariant.js:40
    dispatch Dispatcher.js:179
    dispatch dispatcher.ts:45
    doSetLoggedIn Lifecycle.ts:604
    <anonymous> Lifecycle.ts:76
    _invokeCallback Dispatcher.js:214
    dispatch Dispatcher.js:189
    dispatch dispatcher.ts:45
    overwriteAccountAuth ProxiedModuleApi.ts:146
    _callee$ IlagModule.js:78

This PR provides a fix to resolve this problem.

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

🐛 Bug Fixes

  • Fixes authentication when user is registered via module API (#10156). Contributed by @maheichyk.

Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
@@ -144,16 +144,13 @@ export class ProxiedModuleApi implements ModuleApi {
* @override
*/
public async overwriteAccountAuth(accountInfo: AccountAuthInfo): Promise<void> {
dispatcher.dispatch<OverwriteLoginPayload>(
await doSetLoggedIn(
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this was the only caller which used OverwriteLoginPayload so it should be cleaned up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, okay, I have removed OverwriteLoginPayload and it's usages from the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change actually breaks a lot of tests (approximately 10%). It looks not clear to me. It seems that some mocking is not working properly anymore. Is it valid to use Lifecycle this way? If yes maybe there is some idea or hint how the tests could be fixed? @t3chguy could you please help?

Copy link
Member

Choose a reason for hiding this comment

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

Given the method you are calling was not previously exported - it wasn't a designed use for it to be called like this.

As for the test failures, would need to step through the code to see what is wrong. Not something I'm able to dive into right now unfortunately

Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
@github-actions github-actions bot added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Feb 23, 2023
@germain-gg germain-gg removed their request for review February 28, 2023 08:13
@maheichyk
Copy link
Contributor Author

Created another PR that fixes this issue: #10257

@maheichyk
Copy link
Contributor Author

closing this PR in favor of #10257

@maheichyk maheichyk closed this Mar 7, 2023
@benparsons benparsons added the A-Timesheet-1 Log any time spent on this into the A-Timesheet-1 project label Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Timesheet-1 Log any time spent on this into the A-Timesheet-1 project T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants