-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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: prevent un-authorized access to workspace pages #20215
Conversation
@yuwenmemon @mananjadhav One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
src/languages/es.js
Outdated
@@ -1076,6 +1076,7 @@ export default { | |||
growlMessageOnDeleteError: 'No se puede eliminar el espacio de trabajo porque tiene informes que están siendo procesados', | |||
unavailable: 'Espacio de trabajo no disponible', | |||
memberNotFound: 'Miembro no encontrado. Para invitar a un nuevo miembro al espacio de trabajo, por favor, utiliza el botón Invitar que está arriba.', | |||
notAuthorized: `You do not have access to this page. Are you trying to join the workspace? Please reach out to the owner of this workspace so they can add you as a member! Something else? Reach out to ${CONST.EMAIL.CONCIERGE}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allroundexperts Can you post in on the open-source channel for the Spanish translation?
@@ -1071,6 +1071,7 @@ export default { | |||
growlMessageOnDeleteError: 'This workspace cannot be deleted right now because reports are actively being processed', | |||
unavailable: 'Unavailable workspace', | |||
memberNotFound: 'Member not found. To invite a new member to the workspace, please use the Invite button above.', | |||
notAuthorized: `You do not have access to this page. Are you trying to join the workspace? Please reach out to the owner of this workspace so they can add you as a member! Something else? Reach out to ${CONST.EMAIL.CONCIERGE}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you trying to join the workspace? Please reach out to the owner of this workspace so they can add you as a member!
@youssef-lr @sonialiap Looking at the test steps, if the user B is already a part of the workspace, does this statement make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay this could be a generic message, that shows up even for the user who is not a part of the workspace, we can keep it as is.
@@ -56,7 +57,10 @@ const BlockingView = (props) => ( | |||
height={props.iconHeight} | |||
/> | |||
<Text style={[styles.notFoundTextHeader]}>{props.title}</Text> | |||
<Text style={[styles.textAlignCenter]}>{props.subtitle}</Text> | |||
<AutoEmailLink |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allroundexperts what is the need for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message contains an email which needs to be shown as a link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pardon me for my ignorance, but can you please share the context of why this was needed? I cannot find it in the issue comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither I can find the screenshots/test steps related to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mananjadhav If you look at the English translation message, it contains concierge email address. We need a way to show it as a hyperlink.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Thanks for clarifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the comment, I think it would be better to add a description at the top of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handled.
@allroundexperts Can you please resolve the conflicts? |
@allroundexperts here's the spanish translation.
|
Handled. |
Thanks for that. I was away and wasn't able to ask in the channel. I've updated Spanish translations. |
@allroundexperts You missed checking this off. Can you please take care of it? |
Reviewer Checklist
Screenshots/Videos |
Thanks for taking care of the last bit. @yuwenmemon All yours. |
Bump @yuwenmemon! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* - Checks if a text contains any email. If it does, render it as a mailto: link | ||
* - Else just render it inside `Text` component | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: Remove this blank space
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/yuwenmemon in version: 1.3.26-0 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.26-4 🚀
|
Details
This PR adds checks to prevent un-authorised access to the workspace pages.
Fixed Issues
$ #18910
PROPOSAL: #18910 (comment)
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android