Allow CompositeAuth auth methods to use their own user if defined

[mtangoo]

Q	A
Is bugfix?	✔️
New feature?	❌
Breaks BC?	❌
Fixed issues	#20238

[codecov]

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 64.84%. Comparing base (fd866da) to head (e9f7a54).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
framework/filters/auth/CompositeAuth.php	60.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #20308      +/-   ##
============================================
- Coverage     64.84%   64.84%   -0.01%     
- Complexity    11424    11427       +3     
============================================
  Files           431      431              
  Lines         37172    37177       +5     
============================================
+ Hits          24106    24109       +3     
- Misses        13066    13068       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bizley]

I see this was discussed and proposed to be changed in 2.2. Are we doing this in 2.0 anyway?

[samdark]

@bizley I don't see any compatibility issues with this change. Are there any?

[mtangoo]

I see this was discussed and proposed to be changed in 2.2. Are we doing this in 2.0 anyway?

I think in 2.2 there will be a need to look at the API design itself and if necessary make breaking change. This is a "painkiller" approach to just fix the issue without any breaking changes

[bizley]

With this change user set in the inner auth method will be used for it, now we are using user set in the composite auth.

[mtangoo]

With this change user set in the inner auth method will be used for it, now we are using user set in the composite auth.

Only if is set. Otherwise it works as it used to!

[bizley]

With this change user set in the inner auth method will be used for it, now we are using user set in the composite auth.

Only if is set. Otherwise it works as it used to!

Yes, but now even if user is set in one of the methods it is ignored.

[mtangoo]

With this change user set in the inner auth method will be used for it, now we are using user set in the composite auth.

Only if is set. Otherwise it works as it used to!

Yes, but now even if user is set in one of the methods it is ignored.

Isn't that the purpose of defining user in auth method, that you want it to override anything else defined?

[bizley]

With this change user set in the inner auth method will be used for it, now we are using user set in the composite auth.

Only if is set. Otherwise it works as it used to!

Yes, but now even if user is set in one of the methods it is ignored.

Isn't that the purpose of defining user in auth method, that you want it to override anything else defined?

I agree but this is BC break as was stated in #20238

[mtangoo]

I take it as a bug, because otherwise what is the point of an API allowing devs to pass user object of their choice if it ends up being discarded?

IMO, this is a bug that wasn't uncovered for so long.

[samdark]

I view it as a bug as well. I can't imagine a good scenario where this override is defined but everything works because it's ignored.

[terabytesoftw]

In my opinion we should fix the bugs that appear in Yii2, what's the point of not doing BC and having bugs is my two cents.

[bizley]

I agreed with @schmunk42 on this being potentially problematic. I would like to have it change anyway. If you want to proceed with the change I don't mind.

[mtangoo]

I agreed with @schmunk42 on this being potentially problematic. I would like to have it change anyway. If you want to proceed with the change I don't mind.

I agree with you on this needing a change. But this can be discussed and changed in 2.2, I guess

[samdark]

Alright. Let's have it. Thanks, @mtangoo

[spmaxinc]

In my original message, I also mentioned $request and $response having the same fate as the $user. If I remember correctly (sorry, can't study the code in detail right now), $request and $response could also be configured for different auth methods, but they were ignored in CompositeAuth. Was this changed/fixed or pushed back? It was not an issue that I ran into. I just ran into the $user issue, but looking back then, both $request and $response were ignored if they were set/defined in authorization methods.

[mtangoo]

In my original message, I also mentioned $request and $response having the same fate as the $user. If I remember correctly (sorry, can't study the code in detail right now), $request and $response could also be configured for different auth methods, but they were ignored in CompositeAuth. Was this changed/fixed or pushed back? It was not an issue that I ran into. I just ran into the $user issue, but looking back then, both $request and $response were ignored if they were set/defined in authorization methods.

Can you explain how do you pass a different request or response object in Yii2. Can you provide a use case of that scenario?

[spmaxinc]

For example, let's say I want to use HttpBasicAuth and HttpBearerAuth in CompositeAuth. Each of HttpBasicAuth and HttpBearerAuth allows separate configuration of $response and $request components in themselves, which is expected. However, these $response and $request configured in HttpBasicAuth and HttpBearerAuth will not be respected by CompositeAuth. Their configuration is not passed into CompositeAuth, because CompositeAuth defines its own $request and $response (or null), and by default CompositeAuth will take $response and $request from the app config and not from Auth components passed into it.

[spmaxinc]

Side comment, which I want to make sure we do not overlook and kill this class all together. I don't think it is emphasized enough in the documentation.

The cool thing about CompositeAuth is not that it may be simplifying several methods of authentication, but most importantly it allows OR logic. If you were to simply list the AuthMethods in controller that will create AND logic, which is probably not what you want.

[mtangoo]

I will make a followup PR to finalize the overlooked part

[mtangoo]

Side comment, which I want to make sure we do not overlook and kill this class all together. I don't think it is emphasized enough in the documentation.

The cool thing about CompositeAuth is not that it may be simplifying several methods of authentication, but most importantly it allows OR logic. If you were to simply list the AuthMethods in controller that will create AND logic, which is probably not what you want.

Yii have a lot of parts that are cool and underutilized by devs. Partly is because many ofnits devs don't write or make a lot of videos tuts

[schmunk42]

Yii have a lot of parts that are cool and underutilized by devs. Partly is because many ofnits devs don't write or make a lot of videos tuts

Maybe we can have an AI make video tutorials from written documentation. 😆

[mtangoo]

Yii have a lot of parts that are cool and underutilized by devs. Partly is because many ofnits devs don't write or make a lot of videos tuts

Maybe we can have an AI make video tutorials from written documentation. 😆

May be. But a touch of humanity always beats the robots 😉