Changes required for signup flow #62
Conversation
nicoknoll
force-pushed
the
191-signup-flow-schulen-und-lehrer
branch
from
2b3b355
to
4849562
Compare
Current coverage is 86.83% (diff: 92.95%)@@ master #62 diff @@
==========================================
Files 38 38
Lines 506 471 -35
Methods 44 35 -9
Messages 0 0
Branches 52 50 -2
==========================================
- Hits 458 409 -49
- Misses 48 62 +14
Partials 0 0
|
| } | ||
| }); | ||
| }, | ||
| local.hooks.hashPassword({ passwordField: 'password' }) |
There was a problem hiding this comment.
Please don't use inline hooks of more than 2 lines
| class MoodleLoginStrategy extends AbstractLoginStrategy { | ||
| constructor(app) { | ||
| super(); | ||
| this.app = app; | ||
| } | ||
|
|
||
| /* | ||
| returns a promise with an authenticated client object, or the sign-in error | ||
| */ |
| exports.before = { | ||
| create: [ | ||
| auth.hooks.authenticate(['local', 'jwt']), | ||
| (hook) => { |
| local: new LocalLoginStrategy() | ||
| }; | ||
|
|
||
| const docs = { |
There was a problem hiding this comment.
Maybe we should include these somewhere in the new file (although feathers-swagger might already provide useful information)
| auth.populateUser(), | ||
| auth.restrictToAuthenticated()*/ | ||
| ], | ||
| all: [auth.hooks.authenticate('jwt')], |
There was a problem hiding this comment.
We should check if authentication should be required for getting information about roles
| return hook; | ||
| hook.result.displayName = displayName; | ||
| hook.result.permissions = permissions; | ||
| resolve(hook); |
There was a problem hiding this comment.
I'd remove the outer Promise(resolve => ... and just do .then( _ => Promise.resolve(hook))
|
|
||
| birthday: {type: Date} | ||
| system: {type: Object} // blackbox for frontend stuff like "cookies accepted" |
There was a problem hiding this comment.
I don't get the reason for this, let's talk about it
| done(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
What changed in this file?
| }); | ||
| }); | ||
|
|
||
| it('should return an error if no password is set', () => { |
There was a problem hiding this comment.
What happened to the case that no password is set? Was this test obsolete?
|
|
||
| it('should throw an error if there are two accounts per email, but no systemId is specified', () => { |
There was a problem hiding this comment.
What happened to this test case?
Lot of changes here:
Updated all NPM modules (especially
feathers-authenticationwith breaking changes)Implemented
displayNamefor usersImplemented
systemfield for user model to store fronend stuff like "cookies accepted"Refactored
system strategiesfrom "authentication" to "accounts` (including tests)Removed custom
AuthenticationServiceRemoved
schoolIdfrom account modelRemoved
accountIdsfrom user model (userId is already includes in account model)Notes about refactored authentication:
I refactored it so you can login with an account even if no user is associated. If the account doesn't have a user the user will be redirected to the signup page in frontend to create a user. The userId of the newly created user will be added to the account afterwards and the JWT will be refreshes to include the userId as well.