Handle response_type as an array #38
Conversation
|
Note: |
| @@ -312,13 +317,36 @@ def id_token_callback_phase | |||
| call_app! | |||
| end | |||
|
|
|||
| def type_response?(type) | |||
There was a problem hiding this comment.
What is the purpose of this method? I'm curious why did you choose this name "type_response?"
There was a problem hiding this comment.
Previously I had id_token_response? and I changed it to more generic
type_response?(type). Basically, it checks if type is specified in options.response_type.
We can check the presence of code only if code has been requested.
We can check the presence of id_token only if id_token has been requested.
We can verify id_token only if id_token has been requested.
We can request for access tokens only if code has been requested.
That's why we need to check it.
The name of this method can be improved.
There was a problem hiding this comment.
Gotcha, what would you say if we rename the method to something like configured_response_type?(type) or known_response_type?(type) ? I think it'd make it more clear what's the purpose of this method.
|
Also, could you add few words of description to this PR ? e.g. what's your motivation, what does this PR add/change/remove? |
| true | ||
| end | ||
|
|
||
| def verify_id_token(id_token) |
There was a problem hiding this comment.
what's the point to pass if_token as argument here? can't you access it through params?
| @@ -185,6 +182,14 @@ def public_key | |||
|
|
|||
| private | |||
|
|
|||
| def only_id_token? | |||
| if options.response_type.is_a?(Array) | |||
There was a problem hiding this comment.
please consider following:
types = Array(options.response_type)
types.first.to_s == 'id_token' && types.size == 1I'd prefer to check length of array instead of using #one? which is intended to check how much elements in array are truthy, not for measuring the length of enumerable.
There was a problem hiding this comment.
I changed this method.
There was a problem hiding this comment.
Why do you insist on this complex logic instead of memoizing of normalized response_types array?
There was a problem hiding this comment.
In the tests we assign response types like that: strategy.options.response_type = :id_token and I don't know if there are users who dynamically change the response_type. If so, I'm not sure if memoization could work.
There was a problem hiding this comment.
dynamically change the response_type
what? why?
There was a problem hiding this comment.
I don't if there is such a case. I can memoize it.
There was a problem hiding this comment.
Do you have experience of changing settings in runtime? Can you describe why anybody would do that?
Sorry but this doesn't make sense to me.
There was a problem hiding this comment.
No, I don't have.
| @@ -312,13 +317,36 @@ def id_token_callback_phase | |||
| call_app! | |||
| end | |||
|
|
|||
| def type_response?(type) | |||
| if options.response_type.is_a?(Array) | |||
There was a problem hiding this comment.
This code looks very familiar to me. Does it duplicate only_id_token? for at least 50%?
Please consider extracting response_type normalization to separate instance method and get rid of duplication:
def response_types
@response_types ||= Array(options.response_type).map(&:to_s)
end
# not sure if we really need this method now
def type_response?(type)
response_types.include?(type.to_s)
end
def only_id_token?
response_types == ['id_token']
# or
# response_types.include?('id_token') && response_types.size == 1
end
krzysiek1507
force-pushed
the
feature/handle-response-type-as-array
branch
from
e93ce39
to
d885b3d
Compare
krzysiek1507
changed the title
krzysiek1507
force-pushed
the
feature/handle-response-type-as-array
branch
from
d885b3d
to
f1b7278
Compare
No description provided.