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

GitHub Authentication #282

Merged
merged 14 commits into from Jun 14, 2018
Merged

GitHub Authentication #282

merged 14 commits into from Jun 14, 2018

Conversation

lightnet328
Copy link
Member

@lightnet328 lightnet328 commented Mar 22, 2018

Overview

I implemented a feature to log in to Crowi using GitHub account.

Features

  • Log in to Crowi using GitHub account
  • Login restrictions by GitHub Organization

Changed

  • Callback url when registering with a Google Account
    • before: /register
    • after: /register/google
    • It just behaves like a login callback

Screens

0001
0002
0003
0004

@sotarok
Copy link
Member

@sotarok sotarok commented Mar 27, 2018

Thank you for the great PR.
I'll check it.

@sotarok sotarok self-requested a review May 31, 2018
Copy link
Member

@sotarok sotarok left a comment

Codes are almost okay!
I'll do QA then.

delete req.session.socialId;
delete req.session.socialEmail;
delete req.session.socialName;
delete req.session.socialImage;
Copy link
Member

@sotarok sotarok May 31, 2018

Choose a reason for hiding this comment

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

👍

var loginSuccess = function(req, res, userData) {
req.user = req.session.user = userData;
if (!userData.password) {
return res.redirect('/me/password');
}

clearGoogleSession(req);
clearSession(req);
Copy link
Member

@sotarok sotarok May 31, 2018

Choose a reason for hiding this comment

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

👍

return res.redirect('/login?register=1');
}
if (githubOrganizations && !User.isGitHubAccountValid(githubOrganizations)) {
req.flash('registerWarningMessage', 'このアカウントはコネクトできません。');
Copy link
Member

@sotarok sotarok May 31, 2018

Choose a reason for hiding this comment

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

Can you make above of two messages English?

@sotarok
Copy link
Member

@sotarok sotarok commented May 31, 2018

@lightnet328 Can you merge current master to resolve conflicts?

lib/routes/me.js Outdated

var userData = req.user;

var toDisconnect = req.body.disconnectGitHub ? true : false;
Copy link
Member

@hiroppy hiroppy Jun 1, 2018

Choose a reason for hiding this comment

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

const toDisconnect = !!req.body.disconnectGitHub;

Copy link
Member

@sotarok sotarok left a comment

@lightnet328 I met some errors while QA.
Can you make sure?


<div class="col-sm-12">
<div class="text-center">
<input type="submit" name="connectGitHub" class="btn btn-github" value="GitHubコネクト">
Copy link
Member

@sotarok sotarok Jun 9, 2018

Choose a reason for hiding this comment

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

I found this button shows Japanese, should be translated.

Copy link
Member

@sotarok sotarok Jun 9, 2018

Choose a reason for hiding this comment

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

And, after click this button, I met an error below:

TypeError: Cannot set property 'callbackAction' of undefined
   at actions.authGitHub (/Users/sotarok/working/crowi/crowi/lib/routes/me.js:291:41)

Is this feature (link account after account created) working?

Copy link
Member Author

@lightnet328 lightnet328 Jun 11, 2018

Choose a reason for hiding this comment

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

Well, it seems that it was not in a state to work after refactoring...
I fixed and translated it now.

return loginFailure(req, res);
}

const { githubId: user_id } = tokenInfo;
Copy link
Member

@sotarok sotarok Jun 9, 2018

Choose a reason for hiding this comment

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

Here met an error:

(node:84115) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): ReferenceError: githubId is not defined

Is const { user_id: githubId } instead?

Copy link
Member

@sotarok sotarok left a comment

Approved! it worked.
I'll merge it after you fix the conflict.

@sotarok sotarok merged commit dd57d97 into crowi:master Jun 14, 2018
1 check passed
@sotarok sotarok mentioned this pull request Sep 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants