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

[Dashboard/Study Progression -- View recruitment per site] Restricting users to see statistics only from sites they have access to. #5861

Conversation

racostas
Copy link
Contributor

Brief summary of changes

In dashboard-> Study Progression-> View recruitment per site users were able to see statistics from sites they do not belong/have access

Testing instructions

Now each user should be able to see the view recruitment per site only for the sites it belong/have access

Notes

This is a fix for 22.0.0, major design changes as creating a permission dedicated to statistics still pending.

Notes relative to the code-set:

The structure:

$recruitmentStartDate = $DB->pselectOne(
    "SELECT MIN(Date_registered)
     FROM candidate
     WHERE RegistrationCenterID IN (:Sites)",
    array(*'Sites' => implode(",", array_keys($list_of_sites))*)
);

Doesn't seem to be working for me. Only the first element of the array $list_of_sites is been passed to the IN SQL clause. Not sure if I'm using correctly this way. For the moment I write it as:

$recruitmentStartDate = $DB->pselectOne(
    "SELECT MIN(Date_registered)
     FROM candidate
     WHERE RegistrationCenterID IN (" . $sitesString . ")",
    array()
);

(Please see additional related code notes on #5847)

Links to related tickets (GitHub, Redmine, ...)

#5847

…g users to see statistics only from sites they have access to.
@johnsaigle johnsaigle added Category: Bug PR or issue that aims to report or fix a bug Category: Security PR or issue that aims to improve security Area: UI PR or issue related to the user interface labels Dec 17, 2019
@johnsaigle
Copy link
Contributor

array(*'Sites' => implode(",", array_keys($list_of_sites))*)

I'm a bit confused about the *s in the example. Are these meant for Markdown emphasis or are they actually in the code?

@racostas
Copy link
Contributor Author

Hi @johnsaigle. Thanks. No, sorry, the *s was trying to put that part of the text in bold here in github. My bad.

In the actual code is without it, just: array('Sites' => implode(",", array_keys($list_of_sites))).

As mentioned before, it is able to retrieve the first element in the array but only the the first. So the behaviour I'm seeing is that the query does IN ('FIRST_ELEMENT', 'ignore', 'the', 'rest'). It does not break but just ignore the rest of the elements acting more like an "=".

Cheers,
Rolando.

@johnsaigle
Copy link
Contributor

Thanks for the feedback.

I'm not sure why the code is behaving that way. I think it would be good to dig into why that's happening because it's not usually a good idea to take strings and put them directly into SQL statements. It's likely safe in this case but it would be better to use prepared statements if possible.

I don't have time to look into it right now but I can look at it soon and try to find a solution if no one else weighs in first.

if ($currentUser->hasPermission('access_all_profiles')) {
$list_of_sites = \Utility::getSiteList();
} else {
$site_id_arr = $currentUser->getCenterIDs();
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to remove DCC center (ID=1) to follow current implementation

@racostas
Copy link
Contributor Author

racostas commented Jan 7, 2020

I'm aborting this PR in favour of the great major redesign proposed on #5896. Nice!

@johnsaigle johnsaigle closed this Jan 7, 2020
@driusan driusan reopened this Jan 8, 2020
@driusan driusan added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Jan 8, 2020
@racostas
Copy link
Contributor Author

Deprecated by #5917

@racostas racostas closed this Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: UI PR or issue related to the user interface Category: Bug PR or issue that aims to report or fix a bug Category: Security PR or issue that aims to improve security State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants