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

fix conditionals for permissions check #212

Merged
merged 2 commits into from
Feb 12, 2021
Merged

fix conditionals for permissions check #212

merged 2 commits into from
Feb 12, 2021

Conversation

vpchung
Copy link
Member

@vpchung vpchung commented Feb 11, 2021

  • Did you run your tests locally?
  • Did you add a good description about your changes or additions?

The current issue was that using the set comparison made it so the permissions had to be just READ and DOWNLOAD. If someone had additional permissions, e.g. UPDATE, validation would fail (even though they would technically have access to the project).

@vpchung vpchung requested a review from thomasyu888 February 11, 2021 07:29
@vpchung vpchung changed the title fix conditionals for permissions fix conditionals for permissions check Feb 11, 2021
@coveralls
Copy link

Pull Request Test Coverage Report for Build 557116073

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 78.264%

Totals Coverage Status
Change from base Build 530072008: 0.0%
Covered Lines: 1001
Relevant Lines: 1279

💛 - Coveralls

Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

Ah interesting, I never thought that people would give the synapse authenticated users group, but I can definitely see how people could give more than download permissions to the admin group.

Thanks!

@thomasyu888 thomasyu888 changed the base branch from master to develop February 11, 2021 07:43
@thomasyu888
Copy link
Member

@vpchung, did you need a patch release?

@vpchung
Copy link
Member Author

vpchung commented Feb 11, 2021

No, that was my bad. I was the one who implemented those conditionals haha.

And yes, a patch release would be great, thanks!

@vpchung vpchung merged commit 6b49c30 into develop Feb 12, 2021
@vpchung vpchung deleted the writeup-fix branch February 12, 2021 07:53
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