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

Two Factor Authentication #285

Merged
merged 49 commits into from
Nov 17, 2021
Merged

Two Factor Authentication #285

merged 49 commits into from
Nov 17, 2021

Conversation

ricknjacky
Copy link
Contributor

@ricknjacky ricknjacky commented Aug 24, 2021

  • Enable/Disable TOTP
  • Verification of TOTP entered
  • Login Verification
  • Unit Tests
  • Documentation

@llaske
Copy link
Owner

llaske commented Oct 3, 2021

I can't see the new Enable 2FA button on the user profile.
Is there something special to configure?

image

@ricknjacky
Copy link
Contributor Author

ricknjacky commented Oct 3, 2021

Right, the settings is in the profile view (the one we access from top right corner menu) I rechecked, with this branch's file changes, It works.

^^Do test this at your end too and let me know if any issues occur here.

This is because the view is edit view and unlike password, we are restricting access to making changes to 2FA's state and enable/disable to the user only (account holder). This is the same logic in SSP-SERVER too.

@ricknjacky ricknjacky changed the title Two Factor Authentication - Enable/Disable Two Factor Authentication Oct 3, 2021
@llaske
Copy link
Owner

llaske commented Oct 30, 2021

Nice @ricknjacky. It works now.

Few remarks:

  • Buttons alignment is not good. I think it should be margin-top: -63px for Enable button and margin-top: -68px for Disable button.
  • Put 2FA tutorial popup before the created time popup. Popup should appear in the field order.
  • Remove commented code here.
  • Add a white space between Sugarizer and Server on the default service name here.

api/controller/users.js Outdated Show resolved Hide resolved
api/controller/users.js Outdated Show resolved Hide resolved
api/controller/auth.js Outdated Show resolved Hide resolved
api/controller/auth.js Outdated Show resolved Hide resolved
api/controller/auth.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@llaske
Copy link
Owner

llaske commented Nov 11, 2021

Thanks @ricknjacky for fixing PR feedback.

Two things left to do :

  • Add a warning message when a teacher enable 2FA
  • Update unit tests due to 2FA changes

@ricknjacky
Copy link
Contributor Author

Thanks @ricknjacky for fixing PR feedback.

Two things left to do :

  • Add a warning message when a teacher enable 2FA
  • Update unit tests due to 2FA changes

Thank-you for the update!
Adding warning message asap.
As discussed, once this PR is merged into dev, I'll rebase and send in a new PR for tests.

@llaske
Copy link
Owner

llaske commented Nov 11, 2021

Regarding unit test if it's not too complex it will be better to report your changes here so everything will be at the same place

@ricknjacky
Copy link
Contributor Author

@llaske
Screenshot from 2021-11-13 07-22-55
Have added all the relevant test cases and they all pass successfully. Kindly review, Thanks.

@llaske
Copy link
Owner

llaske commented Nov 13, 2021

Nice @ricknjacky.
Could you also update the Server Settings in README file to explain new parameters meaning in .INI file?

@ricknjacky
Copy link
Contributor Author

@llaske done. Added relevant comments and information via comments too.

@llaske llaske merged commit 377e597 into llaske:dev Nov 17, 2021
@llaske
Copy link
Owner

llaske commented Nov 17, 2021

Good job @ricknjacky . Thanks for this contribution.

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