-
Notifications
You must be signed in to change notification settings - Fork 785
filtering the xref of '/' based on authorization #1618
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
Conversation
* remove all which aren't among the filtered set of | ||
* projects. | ||
*/ | ||
list = new ArrayList<>(list); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather have the list
variable named differently than the original, will make it easier to debug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed that
*/ | ||
list = new ArrayList<>(list); | ||
list.removeIf((t) -> { | ||
return !getProjectHelper().getAllProjects().stream().anyMatch((p) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if projects are disabled ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good question also for the whole auth framework
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed here
if (getPath().isEmpty()) { | ||
/** | ||
* This denotes the source root directory, we need to filter | ||
* projects which aren't allowed by the authorization. E. g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adjust the comment explaining what method called below actually performs the authorization check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
List<String> list = Arrays.asList(files); | ||
if (getPath().isEmpty()) { | ||
/** | ||
* This denotes the source root directory, we need to filter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls explain in the comment why this only has to be done for the root dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -377,7 +377,8 @@ public String canProcess() { | |||
* | |||
* @return an empty list, if the resource does not exist, is not a directory | |||
* or an error occurred when reading it, otherwise a list of filenames in | |||
* that directory, sorted alphabetically | |||
* that directory, sorted alphabetically and filtered based on authorization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not precise I think as the authorization is performed only for root dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
* | ||
* <p> | ||
* For the root directory (/xref/) an authorization is performed for each of | ||
* the project in case that projects are used in OpenGrok.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'each project' or 'each of the projects'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'in OpenGrok' is redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Is it possible to unit test this ? |
I added a simple test case for this |
fixes #1610