Skip to content

Unwrap PermDetails for canChgrp/canChown detection#5437

Merged
joshmoore merged 1 commit intoome:developfrom
joshmoore:perm-details-fix
Aug 16, 2017
Merged

Unwrap PermDetails for canChgrp/canChown detection#5437
joshmoore merged 1 commit intoome:developfrom
joshmoore:perm-details-fix

Conversation

@joshmoore
Copy link
Copy Markdown
Member

PermDetails objects are being passed to the BasicACLVoter.postProcess
method when new map(x_details_permissions) is used in the HQL query.
Not having access to the original object confuses the security system
leading to incorrect can* values. This permits unwrapping of internal
PermDetails instances.

See:

What this PR does

  • Makes the permission flags of IQuery.projection(new map(...)) match those of IQuery.get(...), e.g.
In [27]: q.get("Project", 1).getDetails().getPermissions().canChgrp()
Out[27]: True

In [28]: q.projection("select new map(p as p_details_permissions) from Project p where p.id = 1", None)[0][0]._val.values()[0]._val["canChgrp"]._val
Out[28]: True

Testing this PR

  1. create a single object (e.g. Project) belonging to root
  2. load canChrp in the two ways shown above
  3. compare that True is returned in both cases

Related reading

  1. https://trello.com/c/QbdB9M8k/115-canchgrp-canchown-bug
  2. enhance setting canChgrp, canChown permissions bits #5431

PermDetails objects are being passed to the BasicACLVoter.postProcess
method when `new map(x_details_permissions)` is used in the HQL query.
Not having access to the original object confuses the security system
leading to incorrect can* values. This permits unwrapping of internal
PermDetails instances.

See:

 * ome#3910
 * https://trello.com/c/QbdB9M8k/115-canchgrp-canchown-bug
@joshmoore joshmoore mentioned this pull request Aug 15, 2017
@mtbc
Copy link
Copy Markdown
Member

mtbc commented Aug 15, 2017

@will-moore should review too with respect to #5352.

@jburel jburel added the develop label Aug 15, 2017
@will-moore
Copy link
Copy Markdown
Member

@joshmoore
Copy link
Copy Markdown
Member Author

Merging this so that latest remains green. Happy to discuss more complete solutions if anyone foresees issues (i.e. other checks that need the correct class)

@joshmoore joshmoore merged commit 638211e into ome:develop Aug 16, 2017
@joshmoore joshmoore deleted the perm-details-fix branch August 16, 2017 09:32
@sbesson sbesson added this to the 5.4.0 milestone Oct 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants