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

MSC2284: Making the identity server optional during discovery #2284

Merged
merged 3 commits into from Nov 18, 2019

Conversation

@turt2live
Copy link
Member

turt2live commented Sep 6, 2019

Rendered

@turt2live turt2live added the proposal label Sep 6, 2019
@turt2live turt2live changed the title Proposal to make the identity server optional during discovery MSC2284: Making the identity server optional during discovery Sep 6, 2019
@jryans
jryans approved these changes Sep 6, 2019
Copy link
Member

jryans left a comment

This seems reasonable to me from a client perspective... The IS should (soon) be optional for all authentication flows, so it's only needed lazily for lookups and binding.

Clients can end up being configured with an invalid or inoperable identity server. This is
considered a feature by this proposal to allow for less intelligent clients to have their
identity server disabled. Intelligent clients could interpret the lack of identity server
as the homeserver/user asking that identity server functionality be disabled in the client.

This comment has been minimized.

Copy link
@joepie91

joepie91 Sep 6, 2019

I feel like in the interest of infrastructure debuggability and general client UX, there should be a flag that indicates whether the identity server is intentionally disabled, so that intelligent clients can distinguish between this and an erroneous misconfiguration (or transient unavailability due to infrastructural issues).

This comment has been minimized.

Copy link
@turt2live

turt2live Nov 1, 2019

Author Member

I'm not sure I agree. In terms of transient unavailability, users won't be able to log in in the first place because the homeserver information will also be missing. Spelling errors, structural problems, etc are easily debugged by a community member in a support channel - the problem should be obvious. For instance, if you end up with a user experience that doesn't involve the identity server you were expecting, you can probably guess that it has to do with your config. Hopefully people these days are still testing their configuration changes before going live.

The other point for defaulting to no identity server instead of wanting an explicit flag is to protect user privacy. Where possible, the spec should be heavily suggesting to clients that a default identity server not be used. When the spec says that lack of useful identity server information means that none is selected, the client can't reasonably default to an identity server which might be risky to the user. Not to mention a flag saying "I absolutely don't want to use an identity server" is more fiddly to implement, particularly when the interaction between users and homeservers is already insanely complicated during login.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Sep 6, 2019

This feels very sane indeed to me - we've been bitten multiple times by inexplicable client failures just because the IS was unavailable, and given an IS isn't even compulsory, an invalid IS shouldn't block anything.

@JonahAragon JonahAragon mentioned this pull request Oct 8, 2019
1 of 4 tasks complete
jryans added a commit to matrix-org/matrix-js-sdk that referenced this pull request Nov 1, 2019
As discussed in MSC2284, this relaxes the identity server discovery to a
`FAIL_PROMPT` state so that clients can choose to warn and continue.

Part of vector-im/riot-web#11102
Implements matrix-org/matrix-doc#2284
jryans added a commit to matrix-org/matrix-js-sdk that referenced this pull request Nov 1, 2019
As discussed in MSC2284, this relaxes the identity server discovery to a
`FAIL_PROMPT` state so that clients can choose to warn and continue.

Part of vector-im/riot-web#11102
Implements matrix-org/matrix-doc#2284
@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Nov 1, 2019

This seems largely accepted by the spec core team and surrounding teams, so to try and push it forward/get more feedback:

@mscbot fcp merge

@mscbot

This comment has been minimized.

Copy link
Collaborator

mscbot commented Nov 1, 2019

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

No concerns currently listed.

Once at least 75% of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

Copy link
Member

KitsuneRal left a comment

I'm generally not a fan of adding prompts to UX but I agree that FAIL_ERROR is a bit too harsh and that client applications can do some further intelligence on whether/how they should interact with the user on the matter.

@mscbot

This comment has been minimized.

Copy link
Collaborator

mscbot commented Nov 13, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Nov 18, 2019

hello @mscbot, it is time.

@mscbot

This comment has been minimized.

Copy link
Collaborator

mscbot commented Nov 18, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@turt2live turt2live merged commit f1ff31d into master Nov 18, 2019
8 checks passed
8 checks passed
buildkite/matrix-doc Build #1024 passed (1 minute, 19 seconds)
Details
ci/circleci: build-dev-scripts Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
ci/circleci: check-docs Your tests passed on CircleCI!
Details
ci/circleci: validate-docs Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
Details
swagger Click to preview the swagger build.
Details
@turt2live turt2live deleted the travis/msc/optional-is-discover branch Nov 18, 2019
turt2live added a commit to matrix-org/matrix-js-sdk that referenced this pull request Nov 26, 2019
As discussed in MSC2284, this relaxes the identity server discovery to a
`FAIL_PROMPT` state so that clients can choose to warn and continue.

Part of vector-im/riot-web#11102
Implements matrix-org/matrix-doc#2284
turt2live added a commit to matrix-org/matrix-js-sdk that referenced this pull request Nov 26, 2019
As discussed in MSC2284, this relaxes the identity server discovery to a
`FAIL_PROMPT` state so that clients can choose to warn and continue.

Part of vector-im/riot-web#11102
Implements matrix-org/matrix-doc#2284
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.