Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
30 changes: 27 additions & 3 deletions client/modules/User/components/SocialAuthButton.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,17 @@ const labels = {
google: 'Login with Google'
};

const linkLabels = {
github: {
connect: 'Connect GitHub Account',
connected: 'Re-link GitHub Account'
},
google: {
connect: 'Connect Google Account',
connected: 'Re-link Google Account'
}
};

const icons = {
github: GithubIcon,
google: GoogleIcon
Expand All @@ -31,22 +42,35 @@ const StyledButton = styled(Button)`
width: ${remSize(300)};
`;

function SocialAuthButton({ service }) {
function SocialAuthButton({ service, linkStyle, isConnected }) {
const ServiceIcon = icons[service];
let label;
if (linkStyle) {
label = isConnected ? linkLabels[service].connected : linkLabels[service].connect;
} else {
label = labels[service];
}
return (
<StyledButton
iconBefore={<ServiceIcon aria-label={`${service} logo`} />}
href={authUrls[service]}
>
{labels[service]}
{label}
</StyledButton>
);
}

SocialAuthButton.services = services;

SocialAuthButton.propTypes = {
service: PropTypes.oneOf(['github', 'google']).isRequired
service: PropTypes.oneOf(['github', 'google']).isRequired,
linkStyle: PropTypes.bool,
isConnected: PropTypes.bool
};

SocialAuthButton.defaultProps = {
linkStyle: false,
isConnected: false
};

export default SocialAuthButton;
22 changes: 19 additions & 3 deletions client/modules/User/pages/AccountView.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,37 @@ import APIKeyForm from '../components/APIKeyForm';
import Nav from '../../../components/Nav';

function SocialLoginPanel(props) {
const { user } = props;
return (
<React.Fragment>
<AccountForm {...props} />
<h2 className="form-container__divider">Social Login</h2>
<p className="account__social-text">
Use your GitHub or Google account to log into the p5.js Web Editor.
Use your GitHub or Google account to login to the p5.js Web Editor.
</p>
<div className="account__social-stack">
<SocialAuthButton service={SocialAuthButton.services.github} />
<SocialAuthButton service={SocialAuthButton.services.google} />
<SocialAuthButton
service={SocialAuthButton.services.github}
linkStyle
isConnected={!!user.github}
/>
<SocialAuthButton
service={SocialAuthButton.services.google}
linkStyle
isConnected={!!user.google}
/>
</div>
</React.Fragment>
);
}

SocialLoginPanel.propTypes = {
user: PropTypes.shape({
github: PropTypes.string,
google: PropTypes.string
}).isRequired
};

class AccountView extends React.Component {
componentDidMount() {
document.body.className = this.props.theme;
Expand Down
32 changes: 21 additions & 11 deletions server/config/passport.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ import { BasicStrategy } from 'passport-http';

import User from '../models/user';

function generateUniqueUsername(username) {
const adj = friendlyWords.predicates[Math.floor(Math.random() * friendlyWords.predicates.length)];
return slugify(`${username} ${adj}`);
}

passport.serializeUser((user, done) => {
done(null, user.id);
});
Expand Down Expand Up @@ -108,14 +113,20 @@ passport.use(new GitHubStrategy({
existingEmailUser.verified = User.EmailConfirmation.Verified;
existingEmailUser.save(saveErr => done(null, existingEmailUser));
} else {
const user = new User();
user.email = primaryEmail;
user.github = profile.id;
user.username = profile.username;
user.tokens.push({ kind: 'github', accessToken });
user.name = profile.displayName;
user.verified = User.EmailConfirmation.Verified;
user.save(saveErr => done(null, user));
let { username } = profile;
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused about when this flow will be triggered? If the user does not have an Editor account with the same email as their Github account?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's expected that this would trigger when a user is registering for the Editor with a GitHub account, i.e. they clicked "Login with GitHub" from the Signup Page.

This just reminded me that I should test if linking social accounts works properly if the email they registered with for the Editor is different from the email for their GItHub/Google Account.

User.findByUsername(username, { caseInsensitive: true }, (findByUsernameErr, existingUsernameUser) => {
if (existingUsernameUser) {
username = generateUniqueUsername(username);
}
const user = new User();
user.email = primaryEmail;
user.github = profile.id;
user.username = profile.username;
user.tokens.push({ kind: 'github', accessToken });
user.name = profile.displayName;
user.verified = User.EmailConfirmation.Verified;
user.save(saveErr => done(null, user));
});
}
});
});
Expand All @@ -141,10 +152,9 @@ passport.use(new GoogleStrategy({

User.findByEmail(primaryEmail, (findByEmailErr, existingEmailUser) => {
let username = profile._json.emails[0].value.split('@')[0];
User.findByUsername(username, (findByUsernameErr, existingUsernameUser) => {
User.findByUsername(username, { caseInsensitive: true }, (findByUsernameErr, existingUsernameUser) => {
if (existingUsernameUser) {
const adj = friendlyWords.predicates[Math.floor(Math.random() * friendlyWords.predicates.length)];
username = slugify(`${username} ${adj}`);
username = generateUniqueUsername(username);
}
// what if a username is already taken from the display name too?
// then, append a random friendly word?
Expand Down
11 changes: 4 additions & 7 deletions server/controllers/user.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ export function userResponse(user) {
apiKeys: user.apiKeys,
verified: user.verified,
id: user._id,
totalSize: user.totalSize
totalSize: user.totalSize,
github: user.github,
google: user.google
};
}

Expand Down Expand Up @@ -93,12 +95,7 @@ export function createUser(req, res, next) {
export function duplicateUserCheck(req, res) {
const checkType = req.query.check_type;
const value = req.query[checkType];
const query = {};
query[checkType] = value;
// Don't want to use findByEmailOrUsername here, because in this case we do
// want to use case-insensitive search for usernames to prevent username
// duplicates, which overrides the default behavior.
User.findOne(query).collation({ locale: 'en', strength: 2 }).exec((err, user) => {
User.findByEmailOrUsername(value, { caseInsensitive: true }, (err, user) => {
if (user) {
return res.json({
exists: true,
Expand Down
29 changes: 24 additions & 5 deletions server/models/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ const userSchema = new Schema({
verifiedToken: String,
verifiedTokenExpires: Date,
github: { type: String },
google: { type: String },
email: { type: String, unique: true },
tokens: Array,
apiKeys: { type: [apiKeySchema] },
Expand Down Expand Up @@ -170,14 +171,21 @@ userSchema.statics.findByEmail = function findByEmail(email, cb) {
* Queries User collection by username and returns one User document.
*
* @param {string} username - Username string
* @param {Object} [options] - Optional options
* @param {boolean} options.caseInsensitive - Does a caseInsensitive query, defaults to false
* @callback [cb] - Optional error-first callback that passes User document
* @return {Promise<Object>} - Returns Promise fulfilled by User document
*/
userSchema.statics.findByUsername = function findByUsername(username, cb) {
userSchema.statics.findByUsername = function findByUsername(username, options, cb) {
const query = {
username
};
return this.findOne(query, cb);
if ((arguments.length === 3 && options.caseInsensitive)
|| (arguments.length === 2 && typeof options === 'object' && options.caseInsensitive)) {
return this.findOne(query).collation({ locale: 'en', strength: 2 }).exec(cb);
}
const callback = typeof options === 'function' ? options : cb;
return this.findOne(query, callback);
};

/**
Expand All @@ -187,15 +195,26 @@ userSchema.statics.findByUsername = function findByUsername(username, cb) {
* a username or email.
*
* @param {string} value - Email or username
* @param {Object} [options] - Optional options
* @param {boolean} options.caseInsensitive - Does a caseInsensitive query rather than
* default query for username or email, defaults
* to false
* @callback [cb] - Optional error-first callback that passes User document
* @return {Promise<Object>} - Returns Promise fulfilled by User document
*/
userSchema.statics.findByEmailOrUsername = function findByEmailOrUsername(value, cb) {
userSchema.statics.findByEmailOrUsername = function findByEmailOrUsername(value, options, cb) {
// do the case insensitive stuff
const isEmail = value.indexOf('@') > -1;
Copy link
Member

Choose a reason for hiding this comment

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

We validate usernames not containing "@" chars in the client but I wonder whether this should also happen in the mongoose User model?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean validating a username when saving a (new or existing) User document?

Copy link
Member

Choose a reason for hiding this comment

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

Yes! Just to ensure these username rules are respected at the core of the app.

if ((arguments.length === 3 && options.caseInsensitive)
|| (arguments.length === 2 && typeof options === 'object' && options.caseInsensitive)) {
const query = isEmail ? { email: value } : { username: value };
return this.findOne(query).collation({ locale: 'en', strength: 2 }).exec(cb);
}
const callback = typeof options === 'function' ? options : cb;
if (isEmail) {
return this.findByEmail(value, cb);
return this.findByEmail(value, callback);
}
return this.findByUsername(value, cb);
return this.findByUsername(value, callback);
};

/**
Expand Down