[WIP] Allow Session triggers #4373
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4373 +/- ##
==========================================
- Coverage 92.69% 92.66% -0.04%
==========================================
Files 118 118
Lines 8353 8354 +1
==========================================
- Hits 7743 7741 -2
- Misses 610 613 +3
Continue to review full report at Codecov.
|
spec/CloudCode.spec.js
Outdated
| @@ -1780,12 +1780,6 @@ describe('afterFind hooks', () => { | |||
| }); | |||
|
|
|||
| it('should validate triggers correctly', () => { | |||
| expect(() => { | |||
There was a problem hiding this comment.
If we do this we should keep these, but check for a valid response rather than an exception. You can use .not.ToThrow() as seen just a bit below here.
What's the motivation behind it? It was restricted for what I believe to be good reasons. |
|
For issue #4020, I needed it for performing an action when a user logged on. You mentioned that we could lift the restriction for this purpose. Another user asked about the status of this recently and I'm sure he found a reason as well. Also look at #4027 for another reason. I understand that you can shoot yourself in the foot with this and it can be dangerous but I think the developer will know not to change session information. |
Nope, you can't trust devs on that, on certain issue we're taken the ground of hardening the security. and it requires a good evaluation. If there's any risk this may cause harm to the developer, we should not proceed. |
|
Fair enough. In issue #4020, you mentioned that we could lift the restrictions for my case. What did you mean there? First, I would like to ask what exactly can the user do to the Session class that will mess things up catastrophically? My first question is maybe we can restrict editing the Session class in the triggers. Second, if the Session triggers method isn't going to work, then what is a way to solve the issues I listed above? Should we just have special callback functions like on login, on logout, on email verification, etc? This is a bit different from triggers for classes. |
|
We could lift the restriction, the reason why I didn't open the PR in the first place, was that I was still pondering the 'could' not the 'can' part of it. For sure, the _Sessions objects are quite nice as they give good info about logins, signups etc... |
|
I think that's a good idea. Ill update the PR when I get a chance. I can't think of a potential case of why you would want to change the _Session so I think that's safe to do until someone provides reason to allow it. |
|
I like the idea of making it read-only. That would remove the potential maliciousness that could happen on an _Session object, decreasing the likelihood of an issue in hooks. |
Instead of removing the tests for validating _Session triggers, I restored them and made sure that having a _Session trigger does NOT throw an exception
…t and new object into one REST array and then convert to ParseObject. Previously, it would convert the extraData and original object into a ParseObject and copy data from new array over. This has the side effect of failing for _Session class because the attributes are read only.
Rename sanitizedData to sanitizeData because sanitizedData implies the data will be returned. Instead, sanitizeData mutates the data field to remove any sensitive information.
Create expandData function which mutates the data field and expands any keys that use dot notation ('x.y')
…changes since they wouldn't work
|
I really need this feature |
|
I haven't had time to finish this up. There is one more test that is failing that I need to look at. The issue is that the _Session class has read only attributes and when using triggers, it builds the _Session class by starting with an empty object and setting each value. This throws an error because they are meant to be read only. If you want, test out this code (npm test) and find the issue in my code. |
|
Hey, I need to delete Installation after Session was deleted (logout). If this PR is not merged, how to achieve my case? |
|
@addisonElliott sorry it took forever, I changed my mind, let's open it up simply. |
|
@flovilmart No problem at all! I'll resume working on this soon. I was having some issues with tests randomly failing when _Session triggers are eliminated... |
|
@addisonElliott Awesome! |
|
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. |
|
@addisonElliott Have you got somewhere with this one? Maybe we could fallback and "open it up" as @flovilmart proposed. I will probably be in huge need of a way to catch logins soon. |
|
Sorry, haven't made progress. No time. It's on my list of TODOs but feel free to make a PR if you've got time. I'm trying to get Parse setup for my Windows environment first. |
|
I inspected your changes, looks good to me. However I could not reproduce the failing test case you mentioned above because of another issue (#4370, on macOS though). |
Remove restriction on having beforeSave & afterSave triggers on _Session class.
Fixes issue #4020