Make omniauth use post by default #1010
Conversation
BobbyMcWho
added
the
Breaking Change
|
Love the progress and direction! Why not stick the rails specific configuration in a railtie so apps don't have to do anything? |
I'm not particularly fond of including Rails specific code in libraries, I know that is also a position held by past maintainers of this project as well. If rails changes the internals of the protect_from_request_forgery classes, that means that omniauth is now required to update and write version-specific implementations. I'd much rather leave it to the application owners to configure, or have the frequently used cookpad/omniauth-rails_csrf_protection updated post release of OmniAuth 2.0 to configure it. |
BobbyMcWho
dismissed stale reviews from suprnova32 and danielpaul
via
ad6228c
BobbyMcWho
force-pushed
the
make-omniauth-use-post-by-default
branch
from
08f9cce
to
ad6228c
Compare
|
Going to merge this into a 2_0_indev branch and see if there are any other breaking change chores that we wrap in while we're releasing a major version bump. |
BobbyMcWho
changed the title
| @@ -180,6 +180,8 @@ def call!(env) # rubocop:disable CyclomaticComplexity, PerceivedComplexity | |||
| raise(error) | |||
| end | |||
|
|
|||
| warn_if_using_get | |||
There was a problem hiding this comment.
@BobbyMcWho It seems that this warning will be caused by any GET requests that reached the application no matter the path. Am I missing something?
There was a problem hiding this comment.
Hmm, I see what you're saying, however it will only log in the case that OmniAuth.config.allowed_request_methods.include?(:get) resolves to true. (See line 198)
There was a problem hiding this comment.
Yeah, correct. During the upgrade process, we wanted to track down any potential cases where GET is used instead of the legit POST. We relied on the https://github.com/omniauth/omniauth/wiki/Upgrading-to-2.0#get that says
Because using GET for login poses concerns (see OWASP Cheatsheet), after upgrading OmniAuth will log a :warn level log with every GET request to a login path when your OmniAuth.config.allowed_request_methods include :get.
Instead, we've uncovered that the warning is produced to any GET request to our app and the log stream is getting bloated pretty fast.
Should the warn_if_using_get be guarded with on_auth_path? check? We can work on the PR if you confirm.
There was a problem hiding this comment.
I think that would be a reasonable resolution, yes. A PR would be appreciated and I'll hopefully have some time soon to go through the release w/ a patch bump for you.
There was a problem hiding this comment.
@BobbyMcWho Hi, I've prepared a PR for this issue #1041
|
HI! Something I was not able to find anywhere is any comments on whether enforcing POST for request phase implies that Single Sign On aka SSO would be affected: As far as I understand, switching to POST only does make sense if the request phase form submission itself is explicit and imposed to user – it would not make sense to have some JS that submits the form automatically. Imagine a situation where single auth provider is being used by ecosystem of apps – let's say each of these apps is a tab in a navigation bar of a portal. |
Previously any request to `/users/<id>` would trigger a log in flow if you're not logged in. In omniauth/omniauth#1010 (or somewhere around there) omniauth removed the ability to use a GET request to log in at all. Now all login requests must be done via GET. This means you cannot redirect to a login endpoint, you must drop the user off somewhere there is a form and they must manually click it to trigger a POST. That's what this PR does. If you try to edit or view your user account without being logged in today: it will give you a 404 error page with a cryptic error message. ``` Not found. Authentication passthru. ``` (Which I think omniauth could improve A LOT, FWIW). Anywhoo, to move forward I'm replacing the prior `authenticate_user!` before filter with one that checks if the request is a post. If it is, then it will directly authenticate the user. Otherwise we drop them on a "soft" page that tells them they must first login. It's not perfect, I'm sure there will be edge cases, but it works.
Previously any request to `/users/<id>` would trigger a log in flow if you're not logged in. In omniauth/omniauth#1010 (or somewhere around there) omniauth removed the ability to use a GET request to log in at all. Now all login requests must be done via GET. This means you cannot redirect to a login endpoint, you must drop the user off somewhere there is a form and they must manually click it to trigger a POST. That's what this PR does. If you try to edit or view your user account without being logged in today: it will give you a 404 error page with a cryptic error message. ``` Not found. Authentication passthru. ``` (Which I think omniauth could improve A LOT, FWIW). Anywhoo, to move forward I'm replacing the prior `authenticate_user!` before filter with one that checks if the request is a post. If it is, then it will directly authenticate the user. Otherwise we drop them on a "soft" page that tells them they must first login. It's not perfect, I'm sure there will be edge cases, but it works.
Re:
#960
#809
Shoutout @pat, this work was influenced by #963
This would obviously need to be a major version bump due to breaking changes.
This is based on the ideas of a few others, with some things I added in myself.
The changes include:
request_validation_phasespecific to this type of workflow.Example apps available here.
Rails apps will require a special verification phase that they config in an initializer (or elsewhere if using devise).
Seeking feedback and reviews, hoping I didn't miss anything glaringly obvious here.