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

Generate JWT per user and encode username; add /meet command #26

Closed

Conversation

bitmeal
Copy link

@bitmeal bitmeal commented Mar 29, 2020

Features

  • Generate JWT per user and encode username, from client side (+ mobile app fallback)
  • add /meet command; allowing
    • mobile app users to start a meeting
    • defining a meeting name /meet [optional: my meeting name, even with spaces]

additionally:

  • render "token valid until"-time client-side to display time in users timezone (+ mobile app fallback)
  • clean code and dependencies to allow build on clean system (in docker container) and pass all style checking!

Implementation

Meeting invitations for meetings requiring a token are sent without a token (not really, see below). Upon reception of the invitation a token is requested by the client side for the current user. The token is sent back and appended to the meeting url. (While waiting for the token, a message and loading indicator are displayed.)
Tokens are regenerated when re-rendering a post! The tokens validity time is now the lifetime of the token itself, NOT the time a user has to join a meeting - as tokens can be regenerated.
To be compatible with mobile users, a static, non-customized link is embedded in the messages plain text. The lifetime of the token is expressed in minutes, as the timezone can not be taken into account when falling back to plain text.

Security considerations

It would in theory be possible to request token generation for arbitrary usernames and channels, when forging a request against the endpoint including a full, valid "mattermost-header" and knowing a valid userID and channelID that user is part of.


"major" rework

Split handleStartMeeting into:

  • handleStartMeeting
  • startMeeting
  • handleJWTRequest
  • generateJWTToken

Add new api endpoint to generate tokens at /api/v1/meetings/gentoken

minor cleanup and modifications:

  • plugin.json cleanup
  • package.json added build dependencies, necessary for build on clean system or in docker container
  • webapp client: removed superagent dependency and changed to using redux client (already present)
  • plugin.go, clean unused:
    • POST_MEETING_KEY
    • unused storage of meeting id in mattermost key-value-store
  • Update npm version in docker container
  • remove sudo directives from docker-make as these pose a security risk and should not be necessary when user is configured correctly and added to docker-group

- Use "anonymous"-token in mobile app, as custom message types are not supported
- render token validity time on client side to get user timezone right
- change client for http requests in frontend to redux client
- remove sudo in dockerfile as it poses security risk: script can be run with sudo if desired and user running the script should be added to docker group anyways
- make code compile and pass style checks on a clean installation (e.g. docker image) and pull in some missing node build dependencies

User specific tokens are requested, once the message renders. A new endpoint is exposed at /api/v1/meetings/gentoken to request tokens. Tokens are regenerated on every new reder fo the site! The tokens validity time thus does not double as a limit for the time a user has to join the meeting anymore, but the actual validity time of a generated token! For mobile users the old behavior applies.
Security considderations: It would in theory be possible to request token generation for arbitrary users and channels, when forging a request against the endpoint including a full, valid "mattermost-header" and knowing a valid userID and channelID that user is part of.
@corporalaris
Copy link

Very excited about this. :)

@hanzei
Copy link
Contributor

hanzei commented May 21, 2020

@bitmeal @jespino With #40 merged, is this PR valid any longer?

@jespino
Copy link
Contributor

jespino commented May 21, 2020

@hanzei this still makes sense but is going to be fixed with #35

@jespino
Copy link
Contributor

jespino commented May 24, 2020

I think this is already applied through other PRs, so I going to close it.

@jespino jespino closed this May 24, 2020
@bitmeal
Copy link
Author

bitmeal commented May 24, 2020

I don't know if all of this PR is covered by the others.

To be fair, I don't really care anymore:
This was intended to give everybody the tools they need to effectively use mattermost together with jitsi in the then recent situation, and make it look like there is at least some sort of "integration". Letting this sit idly for 2 month and then failing to give credit for any work is nothing I approve of. So, go figure if all this PR fixes is integrated.

Spoiler: Not all is covered by the other PRs.
I did only look into one change of the PR, so I don't know about the rest, but: Why does anybody still think rendering dates shall be done server side?

Ohh, and there could have been a discussion and with that discussion a documentation of a decision making process for several features.

@jespino
Copy link
Contributor

jespino commented May 24, 2020

Hi @bitmeal, we recently started to work on the plugin because we wanted to improve it, I'm sorry if you feel like we are trying to retrieve credit from your work. That wasn't my intention at all, actually you can see, for example in this PR #39 I explicitly mention you, and in other PR (#40 I mention the original author of the changes). The changes to support JWT extra data related to user data was written by my from scratch, only after that I realized that there was already a PR with that changes, and was easier for me to merge my changes, instead of trying to understand and separate each change from the previous PR.

I'll give to the PR a second review to verify that I have integrated all the changes, how do you would feel more confortable including changes that are in your PR, for example, If I add you as author of the commit?

By the way, I think is another options adding a CONTRIBUTORS file, and list everybody there, of course, you included.

One more time, sorry if you felt that way, for sure it wasn't my intention and I really appreciate your effort.

@bitmeal
Copy link
Author

bitmeal commented May 28, 2020

I greatly approve of your work of improving the plugin and getting it more actively maintained! Please don't get me wrong here.

For the PR, I understand that the introduction of multiple features and making multiple non-minor changes to the code base with one PR was not a necessarily "good" decision; but it felt like the only way to get the plugin to a desirable state of usability within a timely manner (where time was perceived as the main factor in the, then current, situation from my side). There is a lot of valid criticism regarding this that I absolutely accept, and approve of your decision to implement features and changes in chunks!

[...] how do you would feel more confortable including changes that are in your PR, for example, If I add you as author of the commit?
By the way, I think is another options adding a CONTRIBUTORS file, and list everybody there, of course, you included.
I am not here for the internet points, and as long as I did not contribute anything I do not see any reasons to be listed as a contributor, as this is not true in that case.

To get back on topic:
What I do not approve of is the way this went; there is nothing to be gained in the way it happened, for all sides involved.
A way to get the most out of it, for the actual quality of the plugin, would have been to have a discussion, cooperate, and combine everybody's efforts. The point here is missing out on actual improvements of the code/features and a discussion of what thoughts went into the implementation - thereby not combining the works and not getting an even better thought out implementation/feature. Apart from implementing and merging features (on a code base you maintain) while adding a comment "I did it like he proposed/or did - cool", not looking good, it prevents an actual discussion. I for myself do not have the time nor interest in digging through someone else's code and checking if it does the same as what I proposed; where all of this could have happened in some dialogue. And (as I believe) resulted in an even better implementation. (And the focus is on checking if it does the same, after it has been implemented/merged.)

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

4 participants