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

Added ability to set a moderator via JWT and added a Clear tool for moderators #249

Merged
merged 8 commits into from
Jul 26, 2022

Conversation

jamesdeacon
Copy link
Contributor

@jamesdeacon jamesdeacon commented Jul 21, 2022

You can now add a moderator key within the jwt. If this is set to true then the user will have access to a Clear tool, which will wipe the entire contents of the board.

This fixes issues #92 and #212

By opening a pull request, I certify that I hold the intellectual property of the code I am submitting, and I am granting the initial authors of WBO a perpetual, worldwide, non-exclusive, royalty-free, and irrevocable license to this code.

Copy link
Owner

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for the PR !

A few remarks :

  • It looks like the server accepts "clear" requests without checking the moderator role.
  • Can we use the standard role claim in th JWT instead of a custom "moderator" claim ?
  • Where does the SVG come from ?

@jamesdeacon
Copy link
Contributor Author

Not sure why the build test is failing. It completes all tests when I run locally and even in the test itself it is passing most of the tests, it's just the circle test that is failing, and I've not touched that code. Are you able to re-run the test?

In response to your requests

  • Server checks the token before running the clear command.
  • Using standard role claim now
  • The SVG is from FlatIcon. I have an unlimited license to use it on websites and in software applications.

Copy link
Owner

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great ! I'm ready to merge this, but you'll need to change the illustration. You need to have the right of redistributing it under WBO's license.

You can create your own svg icon, or use an existing true open-source icon, such as one from remixicon for example

server/server.js Outdated
Comment on lines 110 to 116
var payload = jsonwebtoken.verify(token, config.AUTH_SECRET_KEY);
var roles = payload.roles;
if(roles) {
return roles.includes("moderator");
} else {
return false;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ca we avoid code duplication and use isModerator here ?

@jamesdeacon
Copy link
Contributor Author

I've made the code change requested and have changed the icon to one I've created

@lovasoa
Copy link
Owner

lovasoa commented Jul 26, 2022

The code is still duplicated between checkUserPermission in server.js and isModerator in board.js, isn't it ?

@jamesdeacon
Copy link
Contributor Author

Sorry, got confused. I've removed the duplication now. Both server.js and boardData.js now reference the new jwtauth.js file

@lovasoa lovasoa merged commit 34ea003 into lovasoa:master Jul 26, 2022
@lovasoa
Copy link
Owner

lovasoa commented Jul 26, 2022

Great, thank you for your contribution!

@jamesdeacon
Copy link
Contributor Author

That's great. Thanks so much for the brilliant whiteboard! Happy to contribute

@mwllgr
Copy link

mwllgr commented Jul 27, 2022

Will this get published on Docker Hub, @lovasoa? Thanks for the nice feature, @jamesdeacon!

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.

3 participants