Skip to content

Fix search on private worksheets #4432

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.

Sign up for GitHub

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

Open
AndrewJGaut wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Fix search on private worksheets #4432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
bd55d25
just deubgging
Mar 17, 2023
d50713f
Merge branch 'master' of github.com:codalab/codalab-worksheets into f…
Mar 22, 2023
419a965
Got the tests passing without group search
Mar 31, 2023
0f3a914
works with root user. non-root just fails when it comes to running a …
Mar 31, 2023
1157741
Fixed issue with permissions
Mar 31, 2023
a1df240
Fixed the issues.
Mar 31, 2023
94c92dd
Merge branch 'master' of github.com:codalab/codalab-worksheets into f…
Mar 31, 2023
1f26d89
Fix up styling
Mar 31, 2023
ec82dab
minor change
Mar 31, 2023
eda85dd
Merge branch 'master' into fix-search-on-private-worksheets
AndrewJGaut Apr 10, 2023
01c2602
Fix bug and styling
Apr 11, 2023
c54c06b
Merge branch 'fix-search-on-private-worksheets' of github.com:codalab…
Apr 11, 2023
de4f233
Fix minor column name bug
Apr 12, 2023
570afce
Merge branch 'master' of github.com:codalab/codalab-worksheets into f…
Sep 24, 2023
513e416
Add in a few more tests and the extra check Yifan asked for
Sep 24, 2023
9f9e68b
Fix protected mode issue
Sep 24, 2023
7f38e55
Fix another protected_mode issue.
Sep 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions codalab/model/bundle_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,20 +642,29 @@ def add_join(table: Table, condition: Any, left_outer_join: bool = False):

if user_id != self.root_user_id:
# Restrict to the bundles that we have access to.
aliased_group_bundle_permission = aliased(cl_group_bundle_permission)
add_join(
cl_group_bundle_permission,
cl_bundle.c.uuid == cl_group_bundle_permission.c.object_uuid,
aliased_group_bundle_permission,
cl_bundle.c.uuid == aliased_group_bundle_permission.c.object_uuid,
left_outer_join=True,
)
aliased_cl_user_group = aliased(cl_user_group)
add_join(
aliased_cl_user_group,
aliased_cl_user_group.c.user_id == user_id,
left_outer_join=True,
Comment on lines +645 to +655
Copy link
Copy Markdown
Contributor Author

@AndrewJGaut AndrewJGaut Mar 31, 2023

Choose a reason for hiding this comment

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

We use aliased here to account for the case when we've already done a join on the group_bundle_permission table or the user_group table. (If you do that twice without aliasing one of them, the query is invalid according to MySQL syntax rules.)

Copy link
Copy Markdown
Contributor

@yifanmai yifanmai Apr 10, 2023

Choose a reason for hiding this comment

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

Does aliased automatically pick a unique name, or do you need to explicitly pick a name?

In a different PR, we should probably add validation when we do the joins to make sure that we're not joining onto the same table twice.

)
add_join(cl_user_group, cl_user_group.c.user_id == user_id, left_outer_join=True)
access_via_owner = cl_bundle.c.owner_id == user_id
access_via_group = and_(
or_( # Join constraint (group)
cl_group_bundle_permission.c.group_uuid
aliased_group_bundle_permission.c.group_uuid
Copy link
Copy Markdown
Contributor

@yifanmai yifanmai Apr 10, 2023

Choose a reason for hiding this comment

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

Actually, is there a potential false positive case here (access_via_group is incorrectly true) if aliased_group_bundle_permission.c.group_uuid and self.public_group_uuid are both None?

Copy link
Copy Markdown
Contributor

@yifanmai yifanmai Apr 10, 2023

Choose a reason for hiding this comment

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

Thinking about this a bit more, this logic seems incorrect?

  • aliased_group_bundle_permission is all the group bundle permissions for the object
  • aliased_cl_user_group is all the group memberships for the user
  • aliased_cl_user_group.c.user_id == user_id doesn't do anything because this is already enforced by the join.
  • So it seems like if the bundle grants access to some group A, and the user X belongs to some group B, then user X incorrectly gets access to the bundle?
  • Seems like what we're missing here is we need to check that aliased_group_bundle_permission.c.group_id == aliased_cl_user_group.c.group_id?
  • Alternatively you can push this constraint into the join condition i.e. when you in onto aliased_group_bundle_permission, join using the condition that the bundle permission group is the user's groups or the public group only?

Copy link
Copy Markdown
Contributor Author

@AndrewJGaut AndrewJGaut Apr 11, 2023

Choose a reason for hiding this comment

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

Yes, you're totally correct. Thanks for catching this! I updated it. I believe it should be correct now.

== self.public_group_uuid, # Public group
cl_user_group.c.user_id == user_id,
aliased_group_bundle_permission.c.group_uuid
== aliased_cl_user_group.c.group_uuid, # Private group
Comment on lines +662 to +663
Copy link
Copy Markdown
Contributor

@yifanmai yifanmai May 23, 2023

Choose a reason for hiding this comment

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

I believe you have to also check that aliased_group_bundle_permission.c.group_uuid is not NULL.

Otherwise this could be triggered if the user is not in any groups, and the bundles has no group permissions. Could you check if this is the case?

Copy link
Copy Markdown
Contributor Author

@AndrewJGaut AndrewJGaut Sep 27, 2023

Choose a reason for hiding this comment

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

OK, I added that check and some more tests to make sure that's not a problem anymore.

I think it looks good now?

),
cl_group_bundle_permission.c.permission
aliased_group_bundle_permission.c.permission
>= GROUP_OBJECT_PERMISSION_READ, # Match the uuid of the parent
aliased_group_bundle_permission.c.group_uuid is not None,
)

where_clause = and_(where_clause, or_(access_via_owner, access_via_group))
Expand Down
160 changes: 101 additions & 59 deletions tests/cli/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1872,50 +1872,11 @@ def test_perm(ctx):

@TestModule.register('search')
def test_search(ctx):
name = random_name()
uuid1 = _run_command([cl, 'upload', test_path('a.txt'), '-n', name])
uuid2 = _run_command([cl, 'upload', test_path('b.txt'), '-n', name])
check_equals(uuid1, _run_command([cl, 'search', uuid1, '-u']))
check_equals(
uuid1[:8], _run_command([cl, 'search', 'uuid=' + uuid1, '-f', 'uuid']).split("\n")[2]
)
check_equals(uuid1, _run_command([cl, 'search', 'uuid=' + uuid1, '-u']))
check_equals('', _run_command([cl, 'search', 'uuid=' + uuid1[0:8], '-u']))
check_equals(uuid1, _run_command([cl, 'search', 'uuid=' + uuid1[0:8] + '.*', '-u']))
check_equals(uuid1, _run_command([cl, 'search', 'uuid=' + uuid1[0:8] + '%', '-u']))
check_equals(uuid1, _run_command([cl, 'search', 'uuid=' + uuid1, 'name=' + name, '-u']))
check_equals(
uuid1 + '\n' + uuid2, _run_command([cl, 'search', 'name=' + name, 'id=.sort', '-u'])
)
check_equals(
uuid1 + '\n' + uuid2,
_run_command([cl, 'search', 'uuid=' + uuid1 + ',' + uuid2, 'id=.sort', '-u']),
)
check_equals(
uuid2 + '\n' + uuid1, _run_command([cl, 'search', 'name=' + name, 'id=.sort-', '-u'])
)
check_equals('2', _run_command([cl, 'search', 'name=' + name, '.count']))
size1 = float(_run_command([cl, 'info', '-f', 'data_size', uuid1]))
size2 = float(_run_command([cl, 'info', '-f', 'data_size', uuid2]))
check_equals(
size1 + size2, float(_run_command([cl, 'search', 'name=' + name, 'data_size=.sum']))
)
# Check floating
check_equals('', _run_command([cl, 'search', '.floating', '-u']))
uuid3 = _run_command([cl, 'upload', test_path('a.txt')])
_run_command([cl, 'detach', uuid3], 0)
check_equals(uuid3, _run_command([cl, 'search', '.floating', '-u']))
_run_command([cl, 'rm', uuid3]) # need to remove since not on main worksheet
# Check search when groups empty
check_equals('', _run_command([cl, 'search', '.shared']))
# Check search with non-root user.
if not os.getenv('CODALAB_PROTECTED_MODE'):
# This test does not work when protected_mode is True.
_, current_user_name = current_user()
user_name = 'non_root_user_' + random_name()
create_user(ctx, user_name, disk_quota='2000')
switch_user(user_name)
check_equals(uuid1, _run_command([cl, 'search', 'uuid=' + uuid1, '-u']))
def test_search_helper(ctx):
Copy link
Copy Markdown
Contributor Author

@AndrewJGaut AndrewJGaut Mar 31, 2023

Choose a reason for hiding this comment

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

define a new helper function (so we can run the same tests multiple times)

# Basic Search and info
name = random_name()
uuid1 = _run_command([cl, 'upload', test_path('a.txt'), '-n', name])
uuid2 = _run_command([cl, 'upload', test_path('b.txt'), '-n', name])
check_equals(uuid1, _run_command([cl, 'search', uuid1, '-u']))
check_equals(
uuid1[:8], _run_command([cl, 'search', 'uuid=' + uuid1, '-f', 'uuid']).split("\n")[2]
Expand All @@ -1925,22 +1886,103 @@ def test_search(ctx):
check_equals(uuid1, _run_command([cl, 'search', 'uuid=' + uuid1[0:8] + '.*', '-u']))
check_equals(uuid1, _run_command([cl, 'search', 'uuid=' + uuid1[0:8] + '%', '-u']))
check_equals(uuid1, _run_command([cl, 'search', 'uuid=' + uuid1, 'name=' + name, '-u']))
check_equals(
uuid1 + '\n' + uuid2, _run_command([cl, 'search', 'name=' + name, 'id=.sort', '-u'])
)
check_equals(
uuid1 + '\n' + uuid2,
_run_command([cl, 'search', 'uuid=' + uuid1 + ',' + uuid2, 'id=.sort', '-u']),
)
check_equals(
uuid2 + '\n' + uuid1, _run_command([cl, 'search', 'name=' + name, 'id=.sort-', '-u'])
)
check_equals('2', _run_command([cl, 'search', 'name=' + name, '.count']))
size1 = float(_run_command([cl, 'info', '-f', 'data_size', uuid1]))
size2 = float(_run_command([cl, 'info', '-f', 'data_size', uuid2]))
check_equals(
size1 + size2, float(_run_command([cl, 'search', 'name=' + name, 'data_size=.sum']))
)

# Search for floating bundles
check_equals('', _run_command([cl, 'search', '.floating', '-u']))
uuid3 = _run_command([cl, 'upload', test_path('a.txt')])
_run_command([cl, 'detach', uuid3], 0)
check_equals(uuid3, _run_command([cl, 'search', '.floating', '-u']))
_run_command([cl, 'rm', uuid3]) # need to remove since not on main worksheet

# Search bundles on private worksheets
wuuid = _run_command([cl, 'work', '-u'])
new_wuuid = _run_command([cl, 'new', random_name()])
_run_command([cl, 'wperm', new_wuuid, 'public', 'n']) # make worksheet private
_run_command([cl, 'work', new_wuuid]) # switch to worksheet
uuid = _run_command([cl, 'upload', test_path('a.txt')])
check_equals(
uuid, _run_command([cl, 'search', uuid, '-u'])
) # Make sure this user can access bundle

# Make sure other users can't access that bundle on the private worksheet.
if not os.getenv('CODALAB_PROTECTED_MODE'):
# Make sure other users can't access it.
_, current_user_name = current_user()
user_name = random_name()
create_user(ctx, user_name, disk_quota='1000000')
switch_user(user_name)
check_equals('', _run_command([cl, 'search', uuid, '-u']))
switch_user(current_user_name)

# Switch back to public worksheet.
_run_command([cl, 'work', wuuid])

# Make sure bundle that has no permissions on the public worksheet cannot be searched.
if not os.getenv('CODALAB_PROTECTED_MODE'):
uuid = _run_command([cl, 'upload', test_path('a.txt')])
_run_command([cl, 'perm', uuid, 'public', 'none'])
_, current_user_name = current_user()
user_name = 'non_root_user_' + random_name()
create_user(ctx, user_name, disk_quota='1000000')
switch_user(user_name)
check_equals('', _run_command([cl, 'search', uuid]))
switch_user(current_user_name)

# Search with groups
# Empty group
check_equals('', _run_command([cl, 'search', '.shared']))
group_bundle_uuid = _run_command([cl, 'upload', test_path('a.txt')])
ctx.collect_bundle(group_bundle_uuid)
user_id, user_name = current_user()
# Create new group
group_name = random_name()
group_uuid = create_group(ctx, group_name)
# Make bundle unavailable to public but available to the group
_run_command([cl, 'perm', group_bundle_uuid, 'public', 'n'])
_run_command([cl, 'perm', group_bundle_uuid, group_name, 'r'])
check_contains(group_bundle_uuid[:8], _run_command([cl, 'search', '.shared']))
check_contains(
group_bundle_uuid[:8], _run_command([cl, 'search', 'group={}'.format(group_uuid)])
)
check_contains(
group_bundle_uuid[:8], _run_command([cl, 'search', 'group={}'.format(group_name)])
)
# Check other groups do not have access to it
other_group_name = random_name()
other_group_uuid = create_group(ctx, other_group_name)
check_equals('', _run_command([cl, 'search', 'group={}'.format(other_group_uuid)]))

# Test with root user.
test_search_helper(ctx)

# Test with non-root user.
# When protected_mode is True, only root can run many of these commands, so we
# can't run the test with another user.
if not os.getenv('CODALAB_PROTECTED_MODE'):
_, current_user_name = current_user()
user_name = 'non_root_user_' + random_name()
create_user(ctx, user_name, disk_quota='1000000')
switch_user(user_name)
new_wuuid = _run_command([cl, 'new', random_name()])
_run_command([cl, 'work', new_wuuid])
test_search_helper(ctx)
switch_user(current_user_name)
# Check search by group
group_bname = random_name()
group_buuid = _run_command([cl, 'run', 'echo hello', '-n', group_bname])
wait(group_buuid)
ctx.collect_bundle(group_buuid)
user_id, user_name = current_user()
# Create new group
group_name = random_name()
group_uuid = create_group(ctx, group_name)
# Make bundle unavailable to public but available to the group
_run_command([cl, 'perm', group_buuid, 'public', 'n'])
_run_command([cl, 'perm', group_buuid, group_name, 'r'])
check_contains(group_buuid[:8], _run_command([cl, 'search', '.shared']))
check_contains(group_buuid[:8], _run_command([cl, 'search', 'group={}'.format(group_uuid)]))
check_contains(group_buuid[:8], _run_command([cl, 'search', 'group={}'.format(group_name)]))


@TestModule.register('search_time')
Expand Down