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
Generate JWT per user and encode username; add /meet command #26
Conversation
- 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.
…ing a meeting name
Very excited about this. :) |
I think this is already applied through other PRs, so I going to close it. |
I don't know if all of this PR is covered by the others. To be fair, I don't really care anymore: Spoiler: Not all is covered by the other PRs. Ohh, and there could have been a discussion and with that discussion a documentation of a decision making process for several features. |
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 One more time, sorry if you felt that way, for sure it wasn't my intention and I really appreciate your effort. |
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!
To get back on topic: |
Features
/meet
command; allowing/meet [optional: my meeting name, even with spaces]
additionally:
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
cleanuppackage.json
added build dependencies, necessary for build on clean system or in docker containersuperagent
dependency and changed to using redux client (already present)plugin.go
, clean unused:POST_MEETING_KEY
sudo
directives fromdocker-make
as these pose a security risk and should not be necessary when user is configured correctly and added todocker
-group