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

Update Reaction.hasPermission's owner pass and replace instances of Roles.userIsInRole #2895

Closed
impactmass opened this issue Sep 19, 2017 · 6 comments
Assignees
Labels
chore For issues that describe a task that does not affect the release, such as tests or CI configuration

Comments

@impactmass
Copy link
Contributor

Reaction.hasPermission currently gives global access for users with "owner" role (i.e it always returns true for owners). This means that a check like Reaction.hasPermission("anonymous") will return true when we want it to return false.

This happens because Reaction.hasPermission joins "owner" to the roles passed to it. We need to remove that join, so that when we check for particular role(s), we are sure that the response is really based on if user has that role.

Then we need to ensure that all current usage of Reaction.hasPermission still works with this change.

After that, we proceed to replace instances of Roles.userIsInRole with Reaction.hasPermission.

@aaronjudd
Copy link
Contributor

aaronjudd commented Sep 22, 2017

Related to, extends #2800

@aaronjudd aaronjudd added the chore For issues that describe a task that does not affect the release, such as tests or CI configuration label Sep 25, 2017
@aaronjudd aaronjudd reopened this Sep 25, 2017
@foladipo foladipo self-assigned this Oct 27, 2017
@impactmass
Copy link
Contributor Author

Comment arising from PR:

I think there's a bit more planning to be done about how this will get merged in, because this might cause some side effects in places where hasPermission is in use.

  • how we can test that there is no breaking change in other parts of core using this method
  • how we plan to release this out. considering other folks might have built on hasPermission, and this might be a breaking change for them.

@brent-hoover
Copy link
Collaborator

@impactmass Since @foladipo is no longer here can you take this over?

@impactmass
Copy link
Contributor Author

I can't at this moment, but maybe sometime in coming sprints

@spencern
Copy link
Contributor

spencern commented Oct 2, 2018

@impactmass @ticean should we continue to work on this issue in the near future?

@spencern spencern removed this from the North Maroon milestone Oct 2, 2018
@impactmass
Copy link
Contributor Author

We can close this. We were working on it in relation to #4525.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore For issues that describe a task that does not affect the release, such as tests or CI configuration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants