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

Bring back lost functionality on login/register/password-reset screens #200

Merged
merged 5 commits into from
Mar 15, 2016
Merged

Bring back lost functionality on login/register/password-reset screens #200

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0bb58dd
brings back the functionality in login/register/screens that got lost…
ara4n Mar 6, 2016
a82d371
provide sensibile defaults for PasswordLogin
ara4n Mar 6, 2016
893e338
incorporate keganfeedback
ara4n Mar 12, 2016
438558d
PR feedback
ara4n Mar 15, 2016
59866a2
final PR feedback
ara4n Mar 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/MatrixClientPeg.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ var matrixClient = null;
var localStorage = window.localStorage;

function deviceId() {
// XXX: is Math.random()'s deterministicity a problem here?
var id = Math.floor(Math.random()*16777215).toString(16);
id = "W" + "000000".substring(id.length) + id;
if (localStorage) {
Expand Down Expand Up @@ -101,10 +102,12 @@ class MatrixClient {
// -matthew

replaceUsingUrls(hs_url, is_url) {
// ...not to be confused with MatrixClientPeg's createClient...
matrixClient = Matrix.createClient({
baseUrl: hs_url,
idBaseUrl: is_url
});

// XXX: factor this out with the localStorage setting in replaceUsingAccessToken
if (localStorage) {
try {
Expand All @@ -115,18 +118,19 @@ class MatrixClient {
}
} else {
console.warn("No local storage available: can't persist HS/IS URLs!");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

A wild space monster appears !

}

replaceUsingAccessToken(hs_url, is_url, user_id, access_token, isGuest) {
if (localStorage) {
try {
localStorage.clear();
} catch (e) {
console.warn("Error using local storage");
console.warn("Error clearing local storage", e);
}
}
this.guestAccess.markAsGuest(Boolean(isGuest));
// ...not to be confused with Matrix.createClient()...
createClient(hs_url, is_url, user_id, access_token, this.guestAccess);
Copy link
Member

Choose a reason for hiding this comment

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

Given createClient on React SDK isn't called in many places, I would be tempted to rename it to avoid all confusion (and then the comments are obsolete!).

if (localStorage) {
try {
Expand All @@ -136,7 +140,7 @@ class MatrixClient {
localStorage.setItem("mx_access_token", access_token);
console.log("Session persisted for %s", user_id);
} catch (e) {
console.warn("Error using local storage: can't persist session!");
console.warn("Error using local storage: can't persist session!", e);
}
} else {
console.warn("No local storage available: can't persist session!");
Expand Down
73 changes: 57 additions & 16 deletions src/components/structures/MatrixChat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,15 +85,41 @@ module.exports = React.createClass({
};
},

getCurrentHsUrl: function() {
if (MatrixClientPeg.get()) {
return MatrixClientPeg.get().getHomeserverUrl();
}
else if (window.localStorage && window.localStorage.getItem("mx_hs_url")) {
return window.localStorage.getItem("mx_hs_url");
}
else if (this.props.config) {
return this.props.config.default_hs_url
}
return "https://matrix.org";
},

getCurrentIsUrl: function() {
if (MatrixClientPeg.get()) {
return MatrixClientPeg.get().getIdentityServerUrl();
}
else if (window.localStorage && window.localStorage.getItem("mx_is_url")) {
return window.localStorage.getItem("mx_is_url");
}
else if (this.props.config) {
return this.props.config.default_is_url
}
return "https://matrix.org";
},

componentWillMount: function() {
this.favicon = new Favico({animation: 'none'});
},

componentDidMount: function() {
this._autoRegisterAsGuest = false;
if (this.props.enableGuest) {
if (!this.props.config || !this.props.config.default_hs_url) {
console.error("Cannot enable guest access: No supplied config prop for HS/IS URLs");
if (!this.getCurrentHsUrl() || !this.getCurrentIsUrl()) {
console.error("Cannot enable guest access: can't determine HS/IS URLs to use");
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need an IS URL specified to register as a guest? I think you'll find we don't.

}
else if (this.props.startingQueryParams.client_secret && this.props.startingQueryParams.sid) {
console.log("Not registering as guest; registration.");
Expand Down Expand Up @@ -155,19 +181,19 @@ module.exports = React.createClass({

_registerAsGuest: function() {
var self = this;
var config = this.props.config;
console.log("Doing guest login on %s", config.default_hs_url);
console.log("Doing guest login on %s", this.getCurrentHsUrl());
MatrixClientPeg.replaceUsingUrls(
config.default_hs_url, config.default_is_url
this.getCurrentHsUrl(),
this.getCurrentIsUrl()
);
MatrixClientPeg.get().registerGuest().done(function(creds) {
console.log("Registered as guest: %s", creds.user_id);
self._setAutoRegisterAsGuest(false);
self.onLoggedIn({
userId: creds.user_id,
accessToken: creds.access_token,
homeserverUrl: config.default_hs_url,
identityServerUrl: config.default_is_url,
homeserverUrl: self.getCurrentHsUrl(),
identityServerUrl: self.getCurrentIsUrl(),
Copy link
Member

Choose a reason for hiding this comment

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

I really don't like how this is structured :( - The figurative rug can easily be pulled from under you between the call to registerGuest() and its completion, resulting in a different HS/IS URL to the ones defined in :186. Can you please yank the HS/IS URLs out to a separate variable rather than assuming getCurrentHsUrl() and co will be consistent between calls.

guest: true
});
}, function(err) {
Expand All @@ -188,7 +214,12 @@ module.exports = React.createClass({
switch (payload.action) {
case 'logout':
if (window.localStorage) {
// preserve our HS & IS URLs for convenience
var hsUrl = this.getCurrentHsUrl();
var isUrl = this.getCurrentIsUrl();
window.localStorage.clear();
window.localStorage.setItem("mx_hs_url", hsUrl);
window.localStorage.setItem("mx_is_url", isUrl);
Copy link
Member

Choose a reason for hiding this comment

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

Tempted to inline this: The variable name isn't giving you any more information than the key name here.

}
Notifier.stop();
UserActivity.stop();
Expand Down Expand Up @@ -711,7 +742,7 @@ module.exports = React.createClass({
}
}
else {
console.error("Unknown screen : %s", screen);
console.info("Ignoring showScreen for '%s'", screen);
}
},

Expand Down Expand Up @@ -875,6 +906,8 @@ module.exports = React.createClass({
var NewVersionBar = sdk.getComponent('globals.NewVersionBar');
var ForgotPassword = sdk.getComponent('structures.login.ForgotPassword');

// work out the HS URL prompts we should show for

// needs to be before normal PageTypes as you are logged in technically
if (this.state.screen == 'post_registration') {
return (
Expand Down Expand Up @@ -970,26 +1003,34 @@ module.exports = React.createClass({
username={this.state.upgradeUsername}
disableUsernameChanges={Boolean(this.state.upgradeUsername)}
guestAccessToken={this.state.guestAccessToken}
hsUrl={this.props.config.default_hs_url}
isUrl={this.props.config.default_is_url}
defaultHsUrl={this.props.config.default_hs_url}
defaultIsUrl={this.props.config.default_is_url}
initialHsUrl={this.getCurrentHsUrl()}
initialIsUrl={this.getCurrentIsUrl()}
registrationUrl={this.props.registrationUrl}
onLoggedIn={this.onRegistered}
onLoginClick={this.onLoginClick} />
onLoginClick={this.onLoginClick}
onRegisterClick={this.onRegisterClick} />
);
} else if (this.state.screen == 'forgot_password') {
return (
<ForgotPassword
homeserverUrl={this.props.config.default_hs_url}
identityServerUrl={this.props.config.default_is_url}
onComplete={this.onLoginClick} />
defaultHsUrl={this.props.config.default_hs_url}
defaultIsUrl={this.props.config.default_is_url}
initialHsUrl={this.getCurrentHsUrl()}
initialIsUrl={this.getCurrentIsUrl()}
onComplete={this.onLoginClick}
onLoginClick={this.onLoginClick} />
);
} else {
return (
<Login
onLoggedIn={this.onLoggedIn}
onRegisterClick={this.onRegisterClick}
homeserverUrl={this.props.config.default_hs_url}
identityServerUrl={this.props.config.default_is_url}
defaultHsUrl={this.props.config.default_hs_url}
defaultIsUrl={this.props.config.default_is_url}
initialHsUrl={this.getCurrentHsUrl()}
initialIsUrl={this.getCurrentIsUrl()}
onForgotPasswordClick={this.onForgotPasswordClick}
onLoginAsGuestClick={this.props.enableGuest && this.props.config && this.props.config.default_hs_url ? this._registerAsGuest: undefined}
/>
Expand Down
27 changes: 21 additions & 6 deletions src/components/structures/login/ForgotPassword.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,12 @@ module.exports = React.createClass({
displayName: 'ForgotPassword',

propTypes: {
homeserverUrl: React.PropTypes.string,
identityServerUrl: React.PropTypes.string,
defaultHsUrl: React.PropTypes.string,
defaultIsUrl: React.PropTypes.string,
initialHsUrl: React.PropTypes.string,
initialIsUrl: React.PropTypes.string,
onLoginClick: React.PropTypes.func,
onRegisterClick: React.PropTypes.func,
onComplete: React.PropTypes.func.isRequired
},

Expand Down Expand Up @@ -152,8 +156,9 @@ module.exports = React.createClass({
else {
resetPasswordJsx = (
<div>
To reset your password, enter the email address linked to your account:
<br />
<div className="mx_Login_prompt">
To reset your password, enter the email address linked to your account:
</div>
<div>
<form onSubmit={this.onSubmitForm}>
<input className="mx_Login_field" ref="user" type="text"
Expand All @@ -175,11 +180,21 @@ module.exports = React.createClass({
</form>
<ServerConfig ref="serverConfig"
withToggleButton={true}
defaultHsUrl={this.props.homeserverUrl}
defaultIsUrl={this.props.identityServerUrl}
defaultHsUrl={this.props.defaultHsUrl}
defaultIsUrl={this.props.defaultIsUrl}
initialHsUrl={this.props.initialHsUrl}
initialIsUrl={this.props.initialIsUrl}
onHsUrlChanged={this.onHsUrlChanged}
onIsUrlChanged={this.onIsUrlChanged}
delayTimeMs={0}/>
<div className="mx_Login_error">
</div>
<a className="mx_Login_create" onClick={this.props.onLoginClick} href="#">
Return to login
</a>
<a className="mx_Login_create" onClick={this.props.onRegisterClick} href="#">
Create a new account
</a>
<LoginFooter />
</div>
</div>
Expand Down
50 changes: 34 additions & 16 deletions src/components/structures/login/Login.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,28 +30,29 @@ var ServerConfig = require("../../views/login/ServerConfig");
module.exports = React.createClass({displayName: 'Login',
propTypes: {
onLoggedIn: React.PropTypes.func.isRequired,
homeserverUrl: React.PropTypes.string,
identityServerUrl: React.PropTypes.string,

initialHsUrl: React.PropTypes.string,
initialIsUrl: React.PropTypes.string,
defaultHsUrl: React.PropTypes.string,
defaultIsUrl: React.PropTypes.string,

// login shouldn't know or care how registration is done.
onRegisterClick: React.PropTypes.func.isRequired,

// login shouldn't care how password recovery is done.
onForgotPasswordClick: React.PropTypes.func,
onLoginAsGuestClick: React.PropTypes.func,
},

getDefaultProps: function() {
return {
homeserverUrl: 'https://matrix.org/',
identityServerUrl: 'https://matrix.org'
};
},

getInitialState: function() {
return {
busy: false,
errorText: null,
enteredHomeserverUrl: this.props.homeserverUrl,
enteredIdentityServerUrl: this.props.identityServerUrl
enteredHomeserverUrl: this.props.initialHsUrl || this.props.defaultHsUrl,
enteredIdentityServerUrl: this.props.initialIsUrl || this.props.defaultIsUrl,

// used for preserving username when changing homeserver
username: "",
};
},

Expand All @@ -76,12 +77,26 @@ module.exports = React.createClass({displayName: 'Login',
});
},

onUsernameChanged: function(username) {
this.setState({ username: username });
},

onHsUrlChanged: function(newHsUrl) {
this._initLoginLogic(newHsUrl);
var self = this;
this.setState({
enteredHomeserverUrl: newHsUrl
}, function() {
self._initLoginLogic(newHsUrl);
});
},

onIsUrlChanged: function(newIsUrl) {
this._initLoginLogic(null, newIsUrl);
var self = this;
this.setState({
enteredIdentityServerUrl: newIsUrl
}, function() {
self._initLoginLogic(null, newIsUrl);
});
},

_initLoginLogic: function(hsUrl, isUrl) {
Expand Down Expand Up @@ -162,6 +177,8 @@ module.exports = React.createClass({displayName: 'Login',
return (
<PasswordLogin
onSubmit={this.onPasswordLogin}
initialUsername={this.state.username}
onUsernameChanged={this.onUsernameChanged}
onForgotPasswordClick={this.props.onForgotPasswordClick} />
);
case 'm.login.cas':
Expand Down Expand Up @@ -203,8 +220,10 @@ module.exports = React.createClass({displayName: 'Login',
{ this.componentForStep(this._getCurrentFlowStep()) }
<ServerConfig ref="serverConfig"
withToggleButton={true}
defaultHsUrl={this.props.homeserverUrl}
defaultIsUrl={this.props.identityServerUrl}
initialHsUrl={this.props.initialHsUrl}
initialIsUrl={this.props.initialIsUrl}
defaultHsUrl={this.props.defaultHsUrl}
defaultIsUrl={this.props.defaultIsUrl}
onHsUrlChanged={this.onHsUrlChanged}
onIsUrlChanged={this.onIsUrlChanged}
delayTimeMs={1000}/>
Expand All @@ -216,7 +235,6 @@ module.exports = React.createClass({displayName: 'Login',
Create a new account
</a>
{ loginAsGuestJsx }
<br/>
<LoginFooter />
</div>
</div>
Expand Down
Loading