openproject
openproject copied to clipboard
Published
20 hours ago •
opf
opf
[#43193] Remove OAuth cookie after successful Nextcloud authorization
https://community.openproject.org/work_packages/43193
ATTENTION: This PR refactors a method with a functional approach for demonstration purposes. It is fully encapsulated and locally done and does not interfere with any other code.
Please give your opinion on the change. Contact @apfohl for explanation if needed.
Fixed implementation to pass specs.
The failing feature specs are activity related.
One of the remarks from the demo was a question by Markus I believe regarding debuggability. I think the do notation really marks it hard to debug due to its method/block wrapping. This is the trace moving through the method until getting the result.
Frame number: 0/87
From: /home/oliver/openproject/dev/app/services/oauth_clients/redirect_uri_from_state_service.rb:46 OAuthClients::RedirectUriFromStateService#call:
44: def call
45: binding.pry
=> 46: process(@cookies, @state)
47: .fmap { |uri| ServiceResult.success(result: uri) }
48: .value_or ServiceResult.failure
49: end
[1] pry(#<OAuthClients::RedirectUriFromStateService>)> s
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/do.rb:134 #<Module:0x00007fd55869aae8>#process:
129:
130: # @api private
131: def wrap_method(target, method, visibility)
132: target.module_eval(<<-RUBY, __FILE__, __LINE__ + 1)
133: #{VISIBILITY_WORD[visibility]} def #{method}(#{DELEGATE}) # private def create_acccount(...)
=> 134: if block_given? # if block_given?
135: super # super
136: else # else
137: Do.() { super { |*ms| Do.bind(ms) } } # Do.() { super { |*ms| Do.bind(ms) } }
138: end # end
139: end # end
[1] pry(#<OAuthClients::RedirectUriFromStateService>):1> s
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/do.rb:137 #<Module:0x00007fd55869aae8>#process:
132: target.module_eval(<<-RUBY, __FILE__, __LINE__ + 1)
133: #{VISIBILITY_WORD[visibility]} def #{method}(#{DELEGATE}) # private def create_acccount(...)
134: if block_given? # if block_given?
135: super # super
136: else # else
=> 137: Do.() { super { |*ms| Do.bind(ms) } } # Do.() { super { |*ms| Do.bind(ms) } }
138: end # end
139: end # end
140: RUBY
141: end
142:
[1] pry(#<OAuthClients::RedirectUriFromStateService>)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/do/mixin.rb:40 Dry::Monads::Do::Mixin#call:
39: def call
=> 40: yield
41: rescue Halt => e
42: e.result
43: end
[1] pry(Dry::Monads::Do)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/do.rb:137 #<Module:0x00007fd55869aae8>#process:
132: target.module_eval(<<-RUBY, __FILE__, __LINE__ + 1)
133: #{VISIBILITY_WORD[visibility]} def #{method}(#{DELEGATE}) # private def create_acccount(...)
134: if block_given? # if block_given?
135: super # super
136: else # else
=> 137: Do.() { super { |*ms| Do.bind(ms) } } # Do.() { super { |*ms| Do.bind(ms) } }
138: end # end
139: end # end
140: RUBY
141: end
142:
[1] pry(#<OAuthClients::RedirectUriFromStateService>)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/openproject/dev/app/services/oauth_clients/redirect_uri_from_state_service.rb:54 #<Module:0x00007fd55869aae8>#process:
49: end
50:
51: private
52:
53: def process(cookies, state)
=> 54: uuid = yield state
55: uri = yield callback_uri_from_cookies cookies, "oauth_state_#{uuid}"
56: callback_uri = yield validate_callback_uri uri
57:
58: Some(callback_uri)
59: end
53: def process(cookies, state)
=> 54: uuid = yield state
55: uri = yield callback_uri_from_cookies cookies, "oauth_state_#{uuid}"
56: callback_uri = yield validate_callback_uri uri
57:
58: Some(callback_uri)
59: end
[1] pry(#<OAuthClients::RedirectUriFromStateService>)> s
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/do.rb:137 #<Module:0x00007fd55869aae8>#process:
132: target.module_eval(<<-RUBY, __FILE__, __LINE__ + 1)
133: #{VISIBILITY_WORD[visibility]} def #{method}(#{DELEGATE}) # private def create_acccount(...)
134: if block_given? # if block_given?
135: super # super
136: else # else
=> 137: Do.() { super { |*ms| Do.bind(ms) } } # Do.() { super { |*ms| Do.bind(ms) } }
138: end # end
139: end # end
140: RUBY
141: end
142:
[1] pry(#<OAuthClients::RedirectUriFromStateService>)> s
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/do/mixin.rb:47 Dry::Monads::Do::Mixin#bind:
46: def bind(monads)
=> 47: monads = Do.coerce_to_monad(Array(monads))
48: unwrapped = monads.map do |result|
49: monad = result.to_monad
50: monad.or { Do.halt(monad) }.value!
51: end
52: monads.size == 1 ? unwrapped[0] : unwrapped
53: end
[1] pry(Dry::Monads::Do)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/do.rb:156 Dry::Monads::Do.coerce_to_monad:
155: def coerce_to_monad(monads)
=> 156: return monads if monads.size != 1
157:
158: first = monads[0]
159:
160: case first
161: when ::Array
162: [List.coerce(first).traverse]
163: when List
164: [first.traverse]
165: else
166: monads
167: end
168: end
[1] pry(Dry::Monads::Do)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/do.rb:158 Dry::Monads::Do.coerce_to_monad:
155: def coerce_to_monad(monads)
156: return monads if monads.size != 1
157:
=> 158: first = monads[0]
159:
160: case first
161: when ::Array
162: [List.coerce(first).traverse]
163: when List
164: [first.traverse]
165: else
166: monads
167: end
168: end
[1] pry(Dry::Monads::Do)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/do.rb:160 Dry::Monads::Do.coerce_to_monad:
155: def coerce_to_monad(monads)
156: return monads if monads.size != 1
157:
158: first = monads[0]
159:
=> 160: case first
161: when ::Array
162: [List.coerce(first).traverse]
163: when List
164: [first.traverse]
165: else
166: monads
167: end
168: end
[1] pry(Dry::Monads::Do)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/do.rb:166 Dry::Monads::Do.coerce_to_monad:
155: def coerce_to_monad(monads)
156: return monads if monads.size != 1
157:
158: first = monads[0]
159:
160: case first
161: when ::Array
162: [List.coerce(first).traverse]
163: when List
164: [first.traverse]
165: else
=> 166: monads
167: end
168: end
[1] pry(Dry::Monads::Do)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/do/mixin.rb:48 Dry::Monads::Do::Mixin#bind:
46: def bind(monads)
47: monads = Do.coerce_to_monad(Array(monads))
=> 48: unwrapped = monads.map do |result|
49: monad = result.to_monad
50: monad.or { Do.halt(monad) }.value!
51: end
52: monads.size == 1 ? unwrapped[0] : unwrapped
53: end
[1] pry(Dry::Monads::Do)> n
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/do/mixin.rb:52 Dry::Monads::Do::Mixin#bind:
46: def bind(monads)
47: monads = Do.coerce_to_monad(Array(monads))
48: unwrapped = monads.map do |result|
49: monad = result.to_monad
50: monad.or { Do.halt(monad) }.value!
51: end
=> 52: monads.size == 1 ? unwrapped[0] : unwrapped
53: end
[1] pry(Dry::Monads::Do)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/openproject/dev/app/services/oauth_clients/redirect_uri_from_state_service.rb:55 #<Module:0x00007fd55869aae8>#process:
50:
51: private
52:
53: def process(cookies, state)
54: uuid = yield state
=> 55: uri = yield callback_uri_from_cookies cookies, "oauth_state_#{uuid}"
56: callback_uri = yield validate_callback_uri uri
57:
58: Some(callback_uri)
59: end
60:
[1] pry(#<OAuthClients::RedirectUriFromStateService>)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/openproject/dev/app/services/oauth_clients/redirect_uri_from_state_service.rb:56 #<Module:0x00007fd55869aae8>#process:
51: private
52:
53: def process(cookies, state)
54: uuid = yield state
55: uri = yield callback_uri_from_cookies cookies, "oauth_state_#{uuid}"
=> 56: callback_uri = yield validate_callback_uri uri
57:
58: Some(callback_uri)
59: end
60:
61: def callback_uri_from_cookies(cookies, name)
[1] pry(#<OAuthClients::RedirectUriFromStateService>)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/openproject/dev/app/services/oauth_clients/redirect_uri_from_state_service.rb:58 #<Module:0x00007fd55869aae8>#process:
53: def process(cookies, state)
54: uuid = yield state
55: uri = yield callback_uri_from_cookies cookies, "oauth_state_#{uuid}"
56: callback_uri = yield validate_callback_uri uri
57:
=> 58: Some(callback_uri)
59: end
60:
61: def callback_uri_from_cookies(cookies, name)
62: Maybe(cookies[name])
63: end
[1] pry(#<OAuthClients::RedirectUriFromStateService>)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/maybe.rb:131 Dry::Monads::Maybe::Some#fmap:
130: def fmap(*args, &block)
=> 131: next_value = bind(*args, &block)
132:
133: if next_value.nil?
134: if self.class.warn_on_implicit_nil_coercion
135: Core::Deprecations.warn(
136: "Block passed to Some#fmap returned `nil` and was chained to None. "\
137: "This is literally an unlawful behavior and it will not be supported in "\
138: "dry-monads 2. \nPlease, replace `.fmap` with `.maybe` in places where you "\
139: "expect `nil` as block result.\n"\
140: "You can opt out of these warnings with\n"\
141: "Dry::Monads::Maybe.warn_on_implicit_nil_coercion false",
142: uplevel: 0,
143: tag: :'dry-monads'
144: )
145: end
146: Monads.None()
147: else
148: Some.new(next_value)
149: end
150: end
[1] pry(#<Dry::Monads::Maybe::Some>)> n
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/maybe.rb:133 Dry::Monads::Maybe::Some#fmap:
130: def fmap(*args, &block)
131: next_value = bind(*args, &block)
132:
=> 133: if next_value.nil?
134: if self.class.warn_on_implicit_nil_coercion
135: Core::Deprecations.warn(
136: "Block passed to Some#fmap returned `nil` and was chained to None. "\
137: "This is literally an unlawful behavior and it will not be supported in "\
138: "dry-monads 2. \nPlease, replace `.fmap` with `.maybe` in places where you "\
139: "expect `nil` as block result.\n"\
140: "You can opt out of these warnings with\n"\
141: "Dry::Monads::Maybe.warn_on_implicit_nil_coercion false",
142: uplevel: 0,
143: tag: :'dry-monads'
144: )
145: end
146: Monads.None()
147: else
148: Some.new(next_value)
149: end
150: end
[1] pry(#<Dry::Monads::Maybe::Some>)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/maybe.rb:148 Dry::Monads::Maybe::Some#fmap:
130: def fmap(*args, &block)
131: next_value = bind(*args, &block)
132:
133: if next_value.nil?
134: if self.class.warn_on_implicit_nil_coercion
135: Core::Deprecations.warn(
136: "Block passed to Some#fmap returned `nil` and was chained to None. "\
137: "This is literally an unlawful behavior and it will not be supported in "\
138: "dry-monads 2. \nPlease, replace `.fmap` with `.maybe` in places where you "\
139: "expect `nil` as block result.\n"\
140: "You can opt out of these warnings with\n"\
141: "Dry::Monads::Maybe.warn_on_implicit_nil_coercion false",
142: uplevel: 0,
143: tag: :'dry-monads'
144: )
145: end
146: Monads.None()
147: else
=> 148: Some.new(next_value)
149: end
150: end
[1] pry(#<Dry::Monads::Maybe::Some>)> s
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/maybe.rb:113 Dry::Monads::Maybe::Some#initialize:
112: def initialize(value = Undefined)
=> 113: raise ArgumentError, "nil cannot be some" if value.nil?
114:
115: super()
116:
117: @value = Undefined.default(value, Unit)
118: end
[1] pry(#<Dry::Monads::Maybe::Some>)> n
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/maybe.rb:115 Dry::Monads::Maybe::Some#initialize:
112: def initialize(value = Undefined)
113: raise ArgumentError, "nil cannot be some" if value.nil?
114:
=> 115: super()
116:
117: @value = Undefined.default(value, Unit)
118: end
[1] pry(#<Dry::Monads::Maybe::Some>)> s
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-monads-1.4.0/lib/dry/monads/maybe.rb:117 Dry::Monads::Maybe::Some#initialize:
112: def initialize(value = Undefined)
113: raise ArgumentError, "nil cannot be some" if value.nil?
114:
115: super()
116:
=> 117: @value = Undefined.default(value, Unit)
118: end
[1] pry(#<Dry::Monads::Maybe::Some>)> s
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-core-0.7.1/lib/dry/core/constants.rb:66 self.default:
65: def undefined.default(x, y = self) # rubocop:disable Naming/MethodParameterName
=> 66: if equal?(x)
67: if equal?(y)
68: yield
69: else
70: y
71: end
72: else
73: x
74: end
75: end
[1] pry(#<Object>)> n
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/dry-core-0.7.1/lib/dry/core/constants.rb:73 self.default:
65: def undefined.default(x, y = self) # rubocop:disable Naming/MethodParameterName
66: if equal?(x)
67: if equal?(y)
68: yield
69: else
70: y
71: end
72: else
=> 73: x
74: end
75: end
[1] pry(#<Object>)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/3.1.0/ostruct.rb:127 OpenStruct#initialize:
126: def initialize(hash=nil)
=> 127: if hash
128: update_to_values!(hash)
129: else
130: @table = {}
131: end
132: end
[1] pry(#<Shared::ServiceState>)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/.rbenv/versions/3.1.2/lib/ruby/3.1.0/ostruct.rb:130 OpenStruct#initialize:
126: def initialize(hash=nil)
127: if hash
128: update_to_values!(hash)
129: else
=> 130: @table = {}
131: end
132: end
[1] pry(#<Shared::ServiceState>)>
<internal:warning>:51:in `warn': _pry_ is deprecated, use pry_instance instead (StructuredWarnings::BuiltInWarning)
From: /home/oliver/openproject/dev/app/services/service_result.rb:59 ServiceResult.failure:
53: def self.failure(errors: nil,
54: message: nil,
55: message_type: nil,
56: state: ::Shared::ServiceState.new,
57: dependent_results: [],
58: result: nil)
=> 59: new(success: false,
60: errors:,
61: message:,
62: message_type:,
63: state:,
64: dependent_results:,
65: result:)
66: end
[1] pry(ServiceResult)>
One of the remarks from the demo was a question by Markus I believe regarding debuggability. I think the do notation really marks it hard to debug due to its method/block wrapping. This is the trace moving through the method until getting the result.
Yes I see and know what you mean. I experienced this with other languages too. And there is no easy solution to that. At least I don't see one. Changing it to a chain of .bind()/.flat_map() calls will get you also only a little bit better debuggability, as you still have the blocks/lambdas that are given to .bind(). This would also be true for adding .bind() to ServiceResult.
What I used to do in this circumstance was to add breakpoints to the functions that are passed to .bind(). Once you reach the end of that function just continue to the next break point. As debugging the implementation of the gem is mostly never needed.
It is not as easy as having a more procedural approach. But it gains more expressiveness. I would also like an opinion on that, if you would like to write something about that.
@apfohl I converted this PR into a draft so nobody accidentally merges it.
@oliverguenther @wielinde I moved the real implementation to https://github.com/opf/openproject/pull/11008.