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] Permission view-all-teams is not checked in the teams.info endpoint #25841

Merged
merged 25 commits into from
Aug 17, 2022

Conversation

LucianoPierdona
Copy link
Contributor

@LucianoPierdona LucianoPierdona commented Jun 13, 2022

Proposed changes (including videos or screenshots)

Previously any authenticated user was able to access the teams.info endpoint, this PR updates this so only users with the view-all-teams permission or team members can access it.

Issue(s)

Steps to test or reproduce

User 1

  • Create a private team with only User 1 added to it.

User 2

  • Access the above team through curl
curl  -H "X-Auth-Token: {authToken}" \
      -H "X-User-Id: {userId}" \
      -H "Content-type: application/json" \
      http://localhost:3000/api/v1/teams.info?teamName=Team1

Expected result

  • If User 2 doesn't have the view-all-teams permission, the response must be unauthorized;
  • If User 2 has the view-all-teams permission, the response must be succesful;
  • For User 1, the response must be succesful;
  • If the team is changed to public, the response must be succesful for User 1 and User 2 regardless of the view-all-teams permission.

Further comments

@LucianoPierdona LucianoPierdona changed the title Fix/view teams info permission [FIX] view teams info permission Jun 13, 2022
@LucianoPierdona LucianoPierdona self-assigned this Jun 13, 2022
@LucianoPierdona LucianoPierdona requested a review from a team June 13, 2022 12:43
apps/meteor/app/api/server/v1/teams.ts Outdated Show resolved Hide resolved
apps/meteor/app/api/server/v1/teams.ts Outdated Show resolved Hide resolved
@LucianoPierdona LucianoPierdona changed the title [FIX] view teams info permission [FIX][BREAK] view teams info permission Jun 13, 2022
@LucianoPierdona LucianoPierdona changed the title [FIX][BREAK] view teams info permission [BREAK] view teams info permission Jun 13, 2022
@LucianoPierdona LucianoPierdona marked this pull request as ready for review June 22, 2022 16:44
@ggazzo ggazzo changed the title [BREAK] view teams info permission [FIX] View teams info permission Jun 27, 2022
@ggazzo ggazzo added this to the 5.0.0 milestone Jun 27, 2022
ggazzo
ggazzo previously approved these changes Jun 29, 2022
@casalsgh casalsgh removed this from the 5.0.0 milestone Jun 30, 2022
@LucianoPierdona LucianoPierdona requested a review from a team as a code owner August 15, 2022 13:52
apps/meteor/tests/end-to-end/api/25-teams.js Outdated Show resolved Hide resolved
apps/meteor/tests/end-to-end/api/25-teams.js Outdated Show resolved Hide resolved
LucianoPierdona and others added 2 commits August 16, 2022 12:34
Co-authored-by: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com>
@matheusbsilva137 matheusbsilva137 changed the title [FIX] View teams info permission [FIX] Permission view-all-teams is not checked in the teams.info endpoint Aug 16, 2022
Copy link
Member

@matheusbsilva137 matheusbsilva137 left a comment

Choose a reason for hiding this comment

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

LGTM. We just need a review from the chat-engine team for the new tests.

@LucianoPierdona LucianoPierdona requested a review from ggazzo August 16, 2022 17:19
@ggazzo ggazzo added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Aug 17, 2022
@kodiakhq kodiakhq bot merged commit 22f209d into develop Aug 17, 2022
@kodiakhq kodiakhq bot deleted the fix/view-teams-info-permission branch August 17, 2022 18:05
csuadev pushed a commit that referenced this pull request Aug 26, 2022
…endpoint (#25841)

Co-authored-by: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com>
@murtaza98 murtaza98 mentioned this pull request Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad: team-collab stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants