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

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

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 requested a review from turt2live November 18, 2019 16:46
@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

2 participants