april-knights / Squire

Core files for the Squire 2 web application.
GNU General Public License v3.0
2 stars 0 forks source link

User access to First Event value #72

Closed Gryph667 closed 2 years ago

Gryph667 commented 2 years ago

Describe the bug Non leadership users should not be able to change their own "First Event" value

To Reproduce Steps to reproduce the behavior:

  1. Have a Commander or lower security profile
  2. Navigate to own profile page
  3. Click the Edit icon
  4. User is able to select a different "First Event"

Expected behavior Value should not be present in the edit screen for Commander or lower security profile

Additional context This would not relate to the New Profile page. Anyone able to add users should be able to input that value.

Palpiter commented 2 years ago

From what I can tell the Profile-Edit page already has a check if "firstevent" is in the list of editable fields. https://github.com/april-knights/Squire/blob/779f854b68de22e29f201b4062eb5113c1b2fa2d/resources/views/profile/edit.blade.php#L120 So it's likely an issue of that boolean being set incorrectly on some (all?) permission-ranks.

I haven't figured out yet where the "layout.app" class gets defined to double check that it's not a bug with how those permissions get checked, but I'd assume that part works since other fields are correctly set as non-editable based on perms.

Gryph667 commented 2 years ago

@Palpiter I'm looking at the profile controller now.

Palpiter commented 2 years ago

ah that's the spot, thanks for linking it. Looks like this is the culprit then: https://github.com/april-knights/Squire/blob/779f854b68de22e29f201b4062eb5113c1b2fa2d/app/Http/Controllers/ProfileController.php#L199

Gryph667 commented 2 years ago

I was just looking at that exact line. You'll have to show me how to link the lines like that.

Palpiter commented 2 years ago

I was just looking at that exact line. You'll have to show me how to link the lines like that.

If you click on a line, you get three dots to the left of it, one of the options there is "Copy Permalink". That link automatically gets turned into a preview as seen above.

Palpiter commented 2 years ago

I'm not sure if this is out of scope of this issue, but wouldn't it be easier to maintain if the checks for editable fields always check the central "security" table where these permissions are supposed to be defined for each permission-rank instead of only checking one permission and then assigning an array of fields as editable?

So instead of the current "if X, return array (y, z, a, b)" it would be:

$editable_fields = DB::select('SELECT * FROM security WHERE edit = true'); Obviously pseudocode since the current security-table uses different fieldnames and might not even allow for a query like that at the moment.

LeoVerto commented 2 years ago

You're totally right, that would be a much better solution! Might be a bit harder to set up when you also consider making the same change for battalion and division fields.

Right now the security table containers three columns for each entity type, for example for user cvuser, cmuser and cduser to view, modify an delete users respectively.

cmbattuser would allow first seargants and higher ranks to edit users within their own battalion but this hasn't been implemented yet.