Comments (18)
I think the biggest downstream consumer is devise, so we'd wanna look if/how devise uses the builder and try and at least keep backwards compatibility for that
from omniauth.
It's also not necessary to redefine #call
since it's the same as the superclass.
If OmniAuth::Builder is intended to be used as shown in the README, maybe overridding ::new
would be a low-impact way of resolving this?
module OmniAuth
class Builder < ::Rack::Builder
def self.new(default_app = nil, &block)
builder = super
block ? builder.to_app : builder
end
end
end
from omniauth.
I searched the devise code. They don't use Builder. The only reference to "builder" is a form builder. I also ran their test suite against the code in the PR just in case, and everything passes.
from omniauth.
I opened another PR, with the Builder-as-a-module idea. I don't really have a preference between them, but the module version means there won't be an extra do-nothing piece of middleware in the stack since it just returns the built app.
from omniauth.
Thanks @speckins I will take a look as soon as I have some time, I've been very busy
from omniauth.
Thanks for the report, this is an area of the library I haven't looked much into. Would memoizing the to_app call be sufficient?
from omniauth.
I think you'd want to do it at build time because memoization is not thread safe.
from omniauth.
module OmniAuth
module Builder
def self.new(default_app = nil, &block)
::Rack::Builder.new(default_app, &block).to_app
end
end
end
maybe consider this???
from omniauth.
Except that OmniAuth::Builder
adds a few methods of its own, of which, the #provider
method is probably the main reason for using it.
omniauth/lib/omniauth/builder.rb
Line 29 in 7d90ba2
What about this:
module OmniAuth
class Builder < Rack::Builder
def initialize default_app = nil, &block
super
@app = to_app if default_app
end
def call env
(@app || to_app).call(env)
end
# The rest same as current OmniAuth::Builder class,
# not duplicated here....
end
end
It ought to cover people who are currently using it in both ways, as middleware as suggested in the docs or as Rack::Builder
is expected to be used.
from omniauth.
I don't think side effects in #initialize
are a good idea.
I like your original proposal: #1083 (comment)
I'd even suggest just doing it like this:
module OmniAuth
module Builder
def self.build(default_app = nil, &block)
new(default_app, &block).to_app
end
# All the existing code.
end
end
Then use OmniAuth::Builder.build(...)
.
from omniauth.
I'm not opposed to a major bump if need be, so the change can be as breaking as needed
from omniauth.
The problem is that omniauth has been telling people to use OmniAuth::Builder
as middleware. E.g.,
use OmniAuth::Builder do
provider :developer
end
Any Rack-like use(middleware, app, *args, &block)
implementation is going to push those arguments onto a stack, and when it finally assembles the Rack app, it's going to call OmniAuth::Builder.new(app, *args, &block)
. So that means that OmniAuth has no control over which class method is called, and the only two places to fix this in existing applications written this way are ::new
or #initialize
. (And Rack::Builder already has a class method, ::app
, that calls #to_app
.)
What about...
module OmniAuth
module Builder
def self.new(app, *args, &block)
builder = Rack::Builder.new(app).extend(self)
builder.instance_eval(&block) if block_given?
builder.to_app
end
def provider(klass, *args, **opts, &block)
# ...
end
def options(options = false)
# ...
end
# ...etc
end
end
This would work for applications using OmniAuth::Builder
as middleware with no changes needed. It would break for people (like me) who have been subclassing Builder, but at least it wouldn't be subtle (TypeError (superclass must be a Class (Module given))
) and could be resolved by subclassing Rack::Builder
and including the OmniAuth::Builder
module.
It would break for people who have been instantiating Builder and using the instance directly, though.
from omniauth.
One more option -- rename the current Builder class to something else and define a new Builder to use it internally.
module OmniAuth
class AuthBuilder < Rack::Builder
# ...same as current OmniAuth::Builder, except no #call
undef_method :call
end
class Builder
def initialize app, &block
@app = AuthBuilder.app(app, &block)
end
def call env
@app.call(env)
end
end
end
from omniauth.
If you had a preference on how you wanted this resolved (or the priorities to take into consideration, e.g., what level of backward-compatibility, etc), I'd be willing to take a crack at it and submit a PR.
from omniauth.
If you want an easy way to test external dependencies, you might like https://github.com/ioquatix/bake-test-external
from omniauth.
I tripped on this earlier today. How can I help get #1094 merged?
from omniauth.
I've just not had the time to manage a large refactor at the moment, I'd love to be able to take care of a few different long standing issues w/ omniauth in the next major release, I just haven't had the time to work on OmniAuth outside of critical maintenance. Namely I'd like to resolve this issue, completely remove the hashie dependency, not dup the entire strategy w/ every request, and shortcircuit out of omniauth early when the request is not on a path that omniauth cares about.
from omniauth.
I've just not had the time to manage a large refactor at the moment, I'd love to be able to take care of a few different long standing issues w/ omniauth in the next major release, I just haven't had the time to work on OmniAuth outside of critical maintenance. Namely I'd like to resolve this issue, completely remove the hashie dependency, not dup the entire strategy w/ every request, and shortcircuit out of omniauth early when the request is not on a path that omniauth cares about.
Makes sense! What do you think of doing those changes incrementally? I can help.
from omniauth.
Related Issues (20)
- Really confused about implementation HOT 4
- How to configure OmniAuth::AuthenticityTokenProtection for Hanami HOT 1
- Loading strategies from database HOT 1
- Feature Request: sign in with Ethereum HOT 3
- Omniauth failed to detect the url -> Authentication passthru HOT 1
- nil error in `callback_path` after omniauth 2 update HOT 8
- Programatically invoke an omniauth strategy HOT 3
- CSRF in Rack for callbacks requests
- Compatibility issues with rack-protection 3.x HOT 5
- Add Bake test for devise
- SSO Attlassian -> Gitlab (new user) HOT 1
- Memory leak possible HOT 2
- Allow customization of the login form? HOT 2
- Issues with oauth2 and doorkeeper on callback phase (invalid_request Missing required parameter: client_id) HOT 5
- Request phase - ActionController::InvalidAuthenticityToken even after skip_before_action HOT 1
- why user_response_structure's image have no effect HOT 2
- Need to update rack configuration from 2.2.3 to 2.2.3.1 HOT 1
- rack's executable "rackup" conflicts with rackup HOT 4
- Authentication failure! undefined method 'headers' HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from omniauth.