Skip to content

Restrict user's permissions for adding images. #382

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

Closed

Conversation

cssru
Copy link
Contributor

@cssru cssru commented Apr 11, 2016

This change is Reviewable

@php-coder
Copy link
Owner

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


src/main/java/ru/mystamps/web/controller/SeriesController.java, line 256 [r1] (raw file):
Don't use exceptions for a normal cases, like this. I suggest to return 403 status.


src/main/java/ru/mystamps/web/controller/SeriesController.java, line 382 [r1] (raw file):
Why did you change it?


src/main/java/ru/mystamps/web/controller/SeriesController.java, line 402 [r1] (raw file):
static


src/main/java/ru/mystamps/web/controller/SeriesController.java, line 414 [r1] (raw file):
static


src/main/java/ru/mystamps/web/controller/SeriesController.java, line 418 [r1] (raw file):
If anonymous user tries to add images to series that (by mistake) don't have an owner, then it will return true right?


src/main/webapp/WEB-INF/views/series/info.html, line 298 [r1] (raw file):
Why did you remove togglz?


Comments from Reviewable

@cssru
Copy link
Contributor Author

cssru commented Apr 12, 2016

src/main/java/ru/mystamps/web/controller/SeriesController.java, line 256 [r1] (raw file):
It is not normal case. It's suspicious activity, isn'it?
'Cause in normal app execution user will not see interface for adding images.


Comments from Reviewable

@cssru
Copy link
Contributor Author

cssru commented Apr 12, 2016

src/main/java/ru/mystamps/web/controller/SeriesController.java, line 382 [r1] (raw file):
Less or equals allows to add quantity+1 image. Equals only does not.


Comments from Reviewable

@cssru
Copy link
Contributor Author

cssru commented Apr 12, 2016

src/main/java/ru/mystamps/web/controller/SeriesController.java, line 418 [r1] (raw file):
Yes. But describe situation when series has no owner. Just for me, please.


Comments from Reviewable

@cssru
Copy link
Contributor Author

cssru commented Apr 12, 2016

src/main/webapp/WEB-INF/views/series/info.html, line 298 [r1] (raw file):
'Cause you asked for me. Or not? )


Comments from Reviewable

@php-coder
Copy link
Owner

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


src/main/java/ru/mystamps/web/controller/SeriesController.java, line 256 [r1] (raw file):
Yes, and it will be recorded when we will log 403 errors to db (#124). But it's another story.

Right now, please, return 403 status. And I don't mind about writing WARNING to the logs. In this case it also useful to provide user id (and don't forget that it might be null).


src/main/java/ru/mystamps/web/controller/SeriesController.java, line 382 [r1] (raw file):
Yes, I know that :) But it wasn't in the task and even if you want to fix it, it should be a separate fix. But you shouldn't fix it, because it's a feature, not a bug: #108


src/main/java/ru/mystamps/web/controller/SeriesController.java, line 418 [r1] (raw file):
I couldn't imagine something realistic, so, I don't mind. But I'm still thinking about corner cases and I'm trying to be defensive against even unknown possible outcomes.


src/main/java/ru/mystamps/web/support/spring/security/SecurityContextUtils.java, line 47 [r1] (raw file):
Maybe replace last 2 lines to simple authorities.contains(authority)? Or it's not the same?

P.S. Aha... I see. Why do we need another one method of checking that user has authority? I suggest to use existing, if you need to access HttpServletRequest then declare it as and argument of the controller's method.


src/main/webapp/WEB-INF/views/series/info.html, line 298 [r1] (raw file):
I don't remember that I asked you to remove togglz feature. Could you point me to my request? (Even if it's true, then you missed to remove this feature from Togglz config).


Comments from Reviewable

@cssru
Copy link
Contributor Author

cssru commented Apr 12, 2016

src/main/java/ru/mystamps/web/support/spring/security/SecurityContextUtils.java, line 47 [r1] (raw file):
Of course, i saw old method. I just suggest to use method, which doesn't use unnecessary parameter and doesn't create new objects.


Comments from Reviewable

@cssru
Copy link
Contributor Author

cssru commented Apr 16, 2016

src/main/java/ru/mystamps/web/support/spring/security/SecurityContextUtils.java, line 47 [r1] (raw file):
List.contains(String)? How it is possible?


Comments from Reviewable

@cssru
Copy link
Contributor Author

cssru commented Apr 16, 2016

@php-coder
Copy link
Owner

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


src/main/java/ru/mystamps/web/support/spring/security/SecurityContextUtils.java, line 47 [r1] (raw file):
Could you add type to a in anyMatch()?


Comments from Reviewable

@cssru cssru force-pushed the gh166_restrict_users_permissions branch from d914261 to 31b87c3 Compare April 18, 2016 09:34
@php-coder
Copy link
Owner

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@cssru cssru force-pushed the gh166_restrict_users_permissions branch from 31b87c3 to a4afd8c Compare April 18, 2016 10:39
@php-coder
Copy link
Owner

In current implementation admin can't add more images. For example, create series with 1 stamp, add 2 images and try to add another one.

@cssru
Copy link
Contributor Author

cssru commented Apr 19, 2016

Ah...I see. Soon it all will be ok )

@cssru cssru force-pushed the gh166_restrict_users_permissions branch from a4afd8c to 41a120c Compare April 19, 2016 21:15
@php-coder
Copy link
Owner

Merged in ae08224 commit. Thank you!

@php-coder php-coder closed this Apr 20, 2016
@cssru cssru deleted the gh166_restrict_users_permissions branch April 24, 2016 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants