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

restrict administration tab #242

Merged
merged 3 commits into from
Apr 1, 2024
Merged

restrict administration tab #242

merged 3 commits into from
Apr 1, 2024

Conversation

lakikowolfe
Copy link
Collaborator

Restrict administration tab to Data Flow App users who do not have administration access to the synapse folder holding the data flow manifest.

shinyjs::hide(selector = ".sidebar-menu")
shinyjs::hide(selector = "a[data-value='tab_administrator']")
Copy link
Collaborator Author

@lakikowolfe lakikowolfe Mar 22, 2024

Choose a reason for hiding this comment

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

I've been using shinyjs hide and show functions to manage when tabs are visible to the user. Through some googling it was brought to my attention that this isn't a very secure method. A user can easily bypass the hidden tab (see for more info).

I'm not really too worried about users "breaking in" to the data flow manifest, but there is a solution if we wanted to make this more secure. For example, using shinyjs::renderMenu() to dynamically render the menu rather than just hiding the tab.

I didn't come across this problem/solution until after implementing the hide/show method. So I figured it was worth putting it up to review to determine if this extra step is necessary.

Curious what you think @afwillia @milen-sage @mialy-defelice

@milen-sage
Copy link
Collaborator

Thanks for flagging this @lakikowolfe ! Could you start a Jira issue? We can prioritize (I don't think it needs to block work now, but we can decide whether we want to address before going to production).

# show admin tab if user has ADMIN access to data flow manifest
manifest_admin_perm <- dfamodules::synapse_access(
id = selected_dcc_config()$dcc$manifest_dataset_id,
access = "CHANGE_PERMISSIONS",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed with Jay that using CHANGE_PERMISSIONS would reliably identify who is an administrator

@lakikowolfe lakikowolfe merged commit 1175b90 into dev Apr 1, 2024
1 check passed
@lakikowolfe lakikowolfe deleted the fds-1881-restrict-admin-tab branch April 1, 2024 22:46
@lakikowolfe lakikowolfe mentioned this pull request May 16, 2024
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.

2 participants