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

Simplify the relationship between /welcome /login /register config.json .well-known and guest users #9290

Closed
12 tasks done
lampholder opened this issue Mar 26, 2019 · 10 comments
Closed
12 tasks done
Assignees
Labels
P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect

Comments

@lampholder
Copy link
Member

lampholder commented Mar 26, 2019

Things which I would like to be true:

  • /welcome /login and /register can be accessed without creating a guest user, and accessing them does not create a guest user
    • in fact, I'd like us to create guest users as late in the process as possible
  • config.json can only specify a default_server_name or a default_hs_url - if you specify both, we fail completely and obviously
  • we don't default to matrix.org. It's just an unexpected trap for people to fall in through misconfiguring
  • if the homeserver location has been in any way derived from user input (and we fail to reach the homeserver) we tell the user to check their input
  • if the homeserver location has been derived from admin configuration (and we fail to reach the homeserver) we tell the user to contact the administrator of the homeserver
  • whenever we have a homeserver name available, use it in the Sign into your Matrix account on ${homeserver_name} string

Outstanding items:

  • can we know the administrator of the homeserver?

Task list for Travis:

@turt2live
Copy link
Member

/welcome /login and /register can be accessed without creating a guest user, and accessing them does not create a guest user

This is problematic for /welcome because of the room directory button.

we don't default to matrix.org. It's just an unexpected trap for people to fall in through misconfiguring

We have to default at some point, unless the goal is to error obviously when neither a server_name/hs_url is specified?

@lampholder
Copy link
Member Author

lampholder commented Mar 26, 2019

This is problematic for /welcome because of the room directory button.

Ah yes, could we create the guest user upon first viewing the directory? Or could viewing the directory also not require a guest user?

We have to default at some point, unless the goal is to error obviously when neither a server_name/hs_url is specified?

Sounds good to me. We can (and should) ship a default config.json that sets the default_server_name to matrix.org, but having no option default to anything makes it too easy for it to look like it's working when it isn't.

This does raise a question about people migrating to the new version, but we can do a noisy blog post (and if it fails fast with easily remedyable config.json changes to fix then dilligent riot install managers won't get tripped up anyway)

@turt2live
Copy link
Member

Ah yes, could we create the guest user upon first viewing the directory? Or could viewing the directory also not require a guest user?

Not requiring a guest leads into the question of "which homeserver are you viewing the directory for?", and discovering that, at which point you'd might as well create a guest because people are likely to click on rooms. I'd generally recommend having a guest account be created because it can be slow (see also: matrix.org)

This does raise a question about people migrating to the new version, but we can do a noisy blog post (and if it fails fast with easily remedyable config.json changes to fix then dilligent riot install managers won't get tripped up anyway)

I think in practice most people are already using matrix.org as a default, or have changed the strings as appropriate. Screaming in bright red text saying "config.json error: no default homeserver specified" is probably okay. Would we want users to still be able to log in despite this error? (using full mxids, for instance, or changing the URLs manually).

@turt2live
Copy link
Member

Summarizing from the discussion on April 10th, UK Afternoon:

  • We're going to change the config to support a wider range of options
  • We'll block the UI on .well-known (with a relatively short timeout) before trying to render anything useful
  • We will be more aggressive about configuration errors, to the point of not defaulting to matrix.org if the config is set up appropriately.

New app logic:

  • If well known data is in the config...
    • Error if a server name or hs_url is set
    • Use the object
  • else if a server name is set...
    • error if a hs_url is set
    • 10s timeout on fetch
    • loud errors on failure
    • block ui
    • use the returned config, if valid. Otherwise error
  • else if a hs_url is set
    • do a proof of life call
    • if not dead, use it. If dead, error

when the user types a full mxid into login:

  • if the server name matches the config, use the config
  • else do lookup, normal process
  • same applies to custom hs_url options

@turt2live
Copy link
Member

also most (all?) of the issues listed above should be fixed with this new behaviour definition

@Ryonez
Copy link

Ryonez commented Apr 14, 2019

Would the config issues here affect integrations and lab settings setting in the config?

As far as I can tell, the only thing being respected is the "brand" variable.

@turt2live
Copy link
Member

Unlikely, that sounds like a different problem.

turt2live added a commit that referenced this issue Apr 16, 2019
Implements the process described here: #9290 (comment)

The expectation is that later layers (like the react-sdk) will make use of the `validated_discovery_config` option instead of interpreting the config themselves.

We intentionally block the UI from loading here to avoid races between discovery and the app loading.
turt2live added a commit that referenced this issue May 3, 2019
Implements the process described here: #9290 (comment)

The expectation is that later layers (like the react-sdk) will make use of the `validated_discovery_config` option instead of interpreting the config themselves.

We intentionally block the UI from loading here to avoid races between discovery and the app loading.
turt2live added a commit to matrix-org/matrix-react-sdk that referenced this issue May 13, 2019
TODO still remains about making ModularServerConfig extend ServerConfig instead of duplicating everything.

See element-hq/element-web#9290
turt2live added a commit to matrix-org/matrix-react-sdk that referenced this issue May 13, 2019
turt2live added a commit to matrix-org/matrix-js-sdk that referenced this issue May 14, 2019
Applies to verification of the homeserver, identity server, and fetching of the .well-known objects. Does not affect other HTTP requests.

See element-hq/element-web#9290
turt2live added a commit that referenced this issue May 14, 2019
We don't actually need to do anything because the app transparently handles this.

See #9290
turt2live added a commit to matrix-org/matrix-react-sdk that referenced this issue May 14, 2019
The app is expected to flag a particular config themselves as default. This is primarily intended so that other parts of the app can determine what to do based on whether or not the config is a default config.

See element-hq/element-web#9290
turt2live added a commit to matrix-org/matrix-react-sdk that referenced this issue May 14, 2019
turt2live added a commit that referenced this issue May 14, 2019
For use in the rest of the app.

See #9290
turt2live added a commit to matrix-org/matrix-react-sdk that referenced this issue May 14, 2019
turt2live added a commit that referenced this issue May 14, 2019
This doesn't cover default_server_name because that pulls in a questionable amount of JS.

See #9290
turt2live added a commit that referenced this issue May 14, 2019
This doesn't cover default_server_name because that pulls in a questionable amount of JS.

See #9290
@turt2live
Copy link
Member

After weeks (months?) of work, it's now finally up for review: matrix-org/matrix-react-sdk#3001 and co.

@turt2live turt2live moved this from In Progress to In Review in Web App Team May 21, 2019
@turt2live
Copy link
Member

Fixed by matrix-org/matrix-react-sdk#3001 - it won't make it into 1.2.0, but should be in the next regularly scheduled release.

Web App Team automation moved this from In Review to In Test May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect
Projects
None yet
Development

No branches or pull requests

3 participants