Skip to content

Conversation

flelayo
Copy link

@flelayo flelayo commented Jul 24, 2025

download_group property behaviour is not what the documentation says it should do:
controls download options in UI for each user. If present and user is authenticated, this value is checked against user roles. If the download_group is present in user groups then download options are shown in UI, else it fallback to **skin.hide_download_controls**

It's only used to restrict people with a specific role to use the API at the moment.
But we really need the feature described by the documentation.

Describe changes proposed in this pull request:

  • Check the authorities against the download_group value when returning the clone of properties to the front.
  • If the user has the right force the skin_hide_download_controls to show otherwise keep the same value.

Checks

This Service already don't have test so I don't think it need one.

Notify reviewers

Ino de Bruijn [email protected] added the properties in exemple properties

@flelayo flelayo force-pushed the download-group-show-button branch from 02483c4 to 79abd10 Compare July 25, 2025 08:33
@alisman
Copy link
Contributor

alisman commented Aug 11, 2025

@haynescd @inodb @flelayo it strikes me that we should not make the frontend properties dependent on auth. it should just pass configuration through for the most part. maybe we expose the privs to the frontend and then do enforce the logic there. i.e. the condition (has download group permission) is in the frontend.

@flelayo
Copy link
Author

flelayo commented Aug 13, 2025

I did it the quickest way possible.
I can make the change to send the privs in a front property but afterward, you want:

  1. change the skin_hide_download_controls variable in front.
  2. change every if(!skin_hide_download_controls) by if(!skin_hide_download_controls || hasDownloadGroupPriv()) ?

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