Skip to content
This repository was archived by the owner on Nov 4, 2021. It is now read-only.

Conversation

@Twixes
Copy link
Member

@Twixes Twixes commented May 18, 2021

Changes

#381 made changing an event's team ID by plugins an illegal operation, which is good for security. Here I propose that instead of throwing an error, we just ensure that the ID is never changed, in the background – the former option is especially troublesome because in the current approach it'd be hard for a user to identify which plugin is causing the issue, as the error is thrown after all plugins have ran.

Also, this fixes the behavior for batch processing, which did not gain this protection in #381.

Checklist

  • Jest tests

@Twixes Twixes requested review from mariusandra and neilkakkar May 18, 2021 23:34
@yakkomajuri
Copy link
Contributor

I do prefer the approach of handling things in the background, but #381 has a bit of discussion about this:

Do wonder though about an approach where we don't even expose team ID to plugins, therefore preventing plugin devs from shooting themselves in the foot by changing a prop that was indeed exposed to them.

I think it can be a bit annoying as a plugin dev if I'm given an object that I'm supposedly able to modify but suddenly my change doesn't work and I have to spend some time figuring out why.

Docs can ofc go some way here but anyway.

Not strongly opposed to this (or opposed at all actually), but just leaving some thoughts

@neilkakkar
Copy link
Contributor

Interesting! Regarding inability to figure out which plugin caused the issue: this isn't actually true. The error is thrown/processed on the pluginConfig, which means users can know exactly which plugin threw the error:

image

image

.. About the batch, definitely my bad, I assumed that had reached end of life, but it has only been deprecated.

@Twixes Twixes force-pushed the no-error-team-id-preservation branch from a620ed2 to c3e18da Compare May 19, 2021 13:32
@neilkakkar
Copy link
Contributor

Thinking this through: the part that seems weird is the order in which plugins run: since team_id is exposed as a readable property, and we have a plugin in between that changes the team_id, any plugin afterwards that depends/ wants to read it is borked.

@Twixes
Copy link
Member Author

Twixes commented May 19, 2021

Ah, that's true, may be good to leave the error in then @neilkakkar I made #396 as an alternative.

@Twixes Twixes closed this in #396 May 19, 2021
@Twixes Twixes deleted the no-error-team-id-preservation branch June 18, 2021 12:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants