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

Relax identity server discovery error handling #3588

Merged
merged 1 commit into from
Nov 18, 2019

Conversation

jryans
Copy link
Collaborator

@jryans jryans commented Nov 1, 2019

If discovery results in a warning for the identity server (as in can't be found
or is malformed), this allows you to continue signing in and shows the warning
above the form.

2019-11-01 at 12 29

Fixes element-hq/element-web#11102

If discovery results in a warning for the identity server (as in can't be found
or is malformed), this allows you to continue signing in and shows the warning
above the form.

Fixes element-hq/element-web#11102
@jryans jryans requested a review from turt2live November 1, 2019 12:35
@@ -33,6 +34,8 @@ export class ValidatedServerConfig {
isUrl: string;

isDefault: boolean;

warning: string;
Copy link
Member

Choose a reason for hiding this comment

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

ideally we don't put state flags here - this is just a configuration blob, not an object meant to carry state to other components.

hsNameIsDifferent arguably shouldn't be here either, but its role is slightly different.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree it's not great, but I also don't want throw and change control flow of callers... Should validation methods return multiple objects then to contain the warning...?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine for now, just highlighting that the original design of this didn't really anticipate this kind of flag being required.

// rewrite homeserver error if we don't care about problems
if (syntaxOnly) {
hsResult.error = AutoDiscovery.ERROR_INVALID_IDENTITY_SERVER;
// rewrite homeserver error since we don't care about problems
Copy link
Member

Choose a reason for hiding this comment

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

we do care about problems - this syntaxOnly flag is used during app startup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't follow your comment here... The current change intends that we only reach this point if we got FAIL_PROMPT. If so, we want to record a warning with or without syntaxOnly mode.

Copy link
Member

@turt2live turt2live Nov 1, 2019

Choose a reason for hiding this comment

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

I think you're right, but I am concerned that the app won't load if there's a warning. If the app loads fine with no identity server (or a dead identity server) while the user is logged in, then this is fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the app loads with a dead identity server while logged in.

@jryans jryans merged commit d5d2f7f into develop Nov 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the IS optional during discovery
2 participants