Readonly trigger for _Session hook #5155
Conversation
| }; | ||
|
|
||
| // Returns an updated copy of the object | ||
| RestWrite.prototype.buildUpdatedObject = function(extraData) { | ||
| const updatedObject = triggers.inflate(extraData, this.originalData); | ||
| let updatedObject = triggers.inflate(extraData, this.originalData); | ||
| Object.keys(this.data).reduce(function(data, key) { |
There was a problem hiding this comment.
I did not really understand what is the purpose of this reduce.
Didn't touch it, but RestWrite.prototype.buildUpdatedObject = function(extraData) { could be fixed to have less duplicated code (like inflating twice in the case of session)
Codecov Report
@@ Coverage Diff @@
## master #5155 +/- ##
==========================================
- Coverage 93.93% 93.81% -0.13%
==========================================
Files 123 123
Lines 8975 8954 -21
==========================================
- Hits 8431 8400 -31
- Misses 544 554 +10
Continue to review full report at Codecov.
|
src/RestWrite.js
Outdated
| @@ -1482,20 +1482,25 @@ RestWrite.prototype.objectId = function() { | |||
| }; | |||
|
|
|||
| // Returns a copy of the data and delete bad keys (_auth_data, _hashed_password...) | |||
| RestWrite.prototype.sanitizedData = function() { | |||
| RestWrite.prototype.sanitizedData = function(decodeData = true) { | |||
There was a problem hiding this comment.
I don’t recommend passing default paramètres as truths, as different implementations with bottom values will produce different results (undefined, null,false)
There was a problem hiding this comment.
i will set it to false by default, but didn't know if its being used somewhere else. Was afraid from breaking something. Tests will show anyhow.
There was a problem hiding this comment.
False / undefined /null should be the default behavior. If true is passed, then we should use the new code path
| }; | ||
|
|
||
| // Returns an updated copy of the object | ||
| RestWrite.prototype.buildUpdatedObject = function(extraData) { | ||
| const updatedObject = triggers.inflate(extraData, this.originalData); | ||
| let updatedObject = triggers.inflate(extraData, this.originalData); | ||
| Object.keys(this.data).reduce(function(data, key) { |
src/RestWrite.js
Outdated
| // This is okay, since sessions are mostly never updated, only created or destroyed. | ||
| // Additionally sanitizedData should be kept in json, not decoded. | ||
| // because '.inflate' internally uses '.fromJSON' so it expects data to be JSON to work properly. | ||
| updatedObject = triggers.inflate(extraData, this.sanitizedData(false)); |
There was a problem hiding this comment.
Please do an early return instead
src/triggers.js
Outdated
| @@ -31,7 +37,7 @@ const baseStore = function() { | |||
| }; | |||
|
|
|||
| function validateClassNameForTriggers(className, type) { | |||
| const restrictedClassNames = ['_Session']; | |||
| const restrictedClassNames = []; | |||
| if (restrictedClassNames.indexOf(className) != -1) { | |||
There was a problem hiding this comment.
What’s the point of keeping that? Perphas use ‘readOnlyClassNames’ instead
There was a problem hiding this comment.
no point, will remove it
src/triggers.js
Outdated
| // handle special readOnlyTriggers cases | ||
| if (isReadOnlyTrigger) { | ||
| // Ignore thrown errors in beforeDelete & during login. | ||
| // We should prevent login from breaking if an error is thrown during the process, |
There was a problem hiding this comment.
If it’s a readOnlyTrigger, why would we perform additional checks?
If the trigger is a simple event based one errors should simply get ignored. So it should resolved always.
There was a problem hiding this comment.
I want to give the possibility to reject a login, rejecting all readOnlyTrigger events would get in the way of that.
I am thinking now of refactoring the name to sessionTrigger rather than readonlyTrigger, it seems more appropriate.
Rejecting a session during signup will not prevent the RestWrite from creating the user, it will require a bit more refactoring to achieve this, since a user is created before the session. Rejecting a signup can be done on user triggers, while a session could be used to track logins and reject them for existing users, and maybe later on use them for 2fa capability. Once a user is signed up (and an account is created _User) there seems to be no point in rejecting a session. |
|
But still creating a user is one thing, creating a session is another thing. Either it is fully ignore or it isn’t. |
Preventing a logIn? Ensuring users have gone through a verification step before actually have credentials etc... lots of reasons actually to prevent session creation. Whicj is exactly the point no? |
|
So you think both should be independent ?
|
|
What do you think? |
|
And what are you trying to achieve with the PR, because if it’s simply having hooks in session, I believe there is an ulterior motive |
|
I think its good, i will look into it. Sent with GitHawk |
|
Yea, just that i need to cancel some logins easily. There are other ways though, but a session hook is the most direct way. Sent with GitHawk |
|
How about something like this ? The only error that gets ignored is in beforeDelete. |
|
Sorry for the late response! Sounds good then. If errors are thrown in session’s before Delete, then we should probably indicate by a log that it will continue and proceed |
spec/CloudCodeSessionTrigger.spec.js
Outdated
| user.setUsername('some-user-name'); | ||
| user.setPassword('password'); | ||
| await user.signUp(); | ||
| } catch (error) { |
There was a problem hiding this comment.
This try / catch is not necessary
spec/CloudCodeSessionTrigger.spec.js
Outdated
| user.setUsername('user-name'); | ||
| user.setPassword('user-password'); | ||
| await user.signUp(); | ||
| } catch (error) { |
spec/CloudCodeSessionTrigger.spec.js
Outdated
| Parse.Cloud.afterDelete('_Session', function() { | ||
| const someObject = new Parse.Object('Test'); | ||
| someObject.set('key', 'value'); | ||
| someObject.save(); |
There was a problem hiding this comment.
You can also use a simple spy or counter to ensure the function has been called. This makes the test suite faster :)
| @@ -387,6 +394,11 @@ function mockShortLivedAuth() { | |||
| return auth; | |||
| } | |||
|
|
|||
| // delay | |||
| function delay(millis) { | |||
There was a problem hiding this comment.
I believe this function is declared at many places already, do you plan to reuse it everywhere?
There was a problem hiding this comment.
will check if i can consolidate them all here.
| error: function (error) { | ||
| // ignore errors thrown in before-delete (during logout for example) | ||
| if (isSessionTrigger && request.triggerName === Types.beforeDelete) { | ||
| return resolve(undefined, error); |
There was a problem hiding this comment.
Not sure resolve supports two arguments as it is a native promise IIRC.
There was a problem hiding this comment.
Resolve here is a custom function, defined here. Its not the the promise resolve.
Line 560 in 56dc2e4
src/triggers.js
Outdated
| @@ -524,9 +552,15 @@ export function maybeRunTrigger( | |||
| config, | |||
| context | |||
| ); | |||
| var { success, error } = getResponseObject( | |||
| var { | |||
There was a problem hiding this comment.
If this line has changed, let’s replace var by const
src/triggers.js
Outdated
| @@ -582,7 +616,9 @@ export function maybeRunTrigger( | |||
| // Converts a REST-format object to a Parse.Object | |||
| // data is either className or an object | |||
| export function inflate(data, restObject) { | |||
| var copy = typeof data == 'object' ? data : { className: data }; | |||
| var copy = typeof data == 'object' ? data : { | |||
There was a problem hiding this comment.
Use const if we change this line as well
|
Will look into the requested changes as soon as i can. |
|
@flovilmart i think I got them all. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Allowing a ReadOnly trigger on the session targetting this workflow:
beforeSave: triggered whenever a session is created.
Thrown errors during signup are ignored; to cancel signup a hook on _User can be used.
Thrown errors during login prevent the user from login in, a session will no be created.
Any edits to the session object will be discarded.
beforeDelete: thrown errors are discarded.
no restrictions on afterSave or afterDelete.
beforeFind and afterFind are not allowed on _Session. (not changed from before)
@flovilmart does this seem okay?