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

Remove the need for CSRF check on ocs::getCurrentUser #7798

Closed
wants to merge 1 commit into from

Conversation

pierreozoux
Copy link
Member

Fixes #5694

I tested on my server, and worked like a charm :)

I think in term of security it is fine to open this route. What do you think?

Fixes #5694

I tested on my server, and worked like a charm :)

I think in term of security it is fine to open this route. What do you think?
@rullzer
Copy link
Member

rullzer commented Jan 11, 2018

Well yes this fixes it but it removes the CSRF protection... also it is not a generic approach we basically need to fix the middleware to not check for CSRF if the bearer auth is set much like the OCS-APIREQUEST header

@MorrisJobke
Copy link
Member

The proper fix seems to be #7873 - @pierreozoux could you check if this works for you?

@rullzer
Copy link
Member

rullzer commented Jan 24, 2018

Yes lets do it in #7873

@rullzer rullzer closed this Jan 24, 2018
@rullzer rullzer deleted the nocsrfcheck-ocs-getcurrentuser branch January 24, 2018 07:49
@Dagefoerde
Copy link
Member

Dagefoerde commented Jan 26, 2018

@NoCSRFRequired would be fine here, though, wouldn't it? After all, that endpoint is purely informational and does not cause any side effects.
Nevertheless, for #5694 this fix wouldn't be sufficient. #7873, as a more general approach, looks more suited to me. (Haven't gotten around to testing that one against Moodle, yet, though. It's on my agenda.)

@rullzer
Copy link
Member

rullzer commented Jan 27, 2018

@Dagefoerde well yes. However I'm not a fan of multiple hacks to fix the same issue ;) Also since it is not enough for #5694 fixing it properly and making sure if you chose to extend NC support in moodle with other endpoints this just works makes more sense imo :).

Looking forward to your review of #7873, THNX :)

@Dagefoerde
Copy link
Member

not a fan of multiple hacks to fix the same issue
I agree, me neither ☺. My point is rather that this PR fixes an entirely different issue. It is just labeled as fixing the same one.

Regardless, for my (and Moodle's) needs the fix of #7873 is exactly the way to go, so thanks (again) for that, @rullzer.

In the present PR the real issue is that the @NoCSRFRequired is redundant here (in my opinion) and therefore might harm understanding of what @NoCSRFRequired is good for. But since functionality is unharmed it's at your discretion whether you incorporate this fix as well, or maybe even look for other instances of unneeded annotations.

@rullzer
Copy link
Member

rullzer commented Jan 29, 2018

The CSRF protection is on by default. You have to add the annotation to disable it ;). So there is no @CSRFrequired annotation.

@Dagefoerde
Copy link
Member

A right, it was the other way round. Sorry for that. So I have to revise: You could consider adding the @NoCSRFRequired here because it makes sense. But, as I said, it's not exactly necessary because everything works regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants