Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add controller for the multiauth-module #1182

Merged
merged 22 commits into from Jan 10, 2021
Merged

Add controller for the multiauth-module #1182

merged 22 commits into from Jan 10, 2021

Conversation

@tvdijen
Copy link
Member

@tvdijen tvdijen commented Aug 10, 2019

This depends on #1178

@tvdijen tvdijen added this to the 1.18 milestone Aug 26, 2019
@tvdijen tvdijen removed this from the 1.18 milestone Aug 29, 2019
@tvdijen tvdijen added this to the 1.19 milestone Aug 29, 2019
@tvdijen tvdijen force-pushed the routing-multiauth branch from 7cb92f0 to 33d8d85 Sep 24, 2019
@codecov
Copy link

@codecov codecov bot commented Oct 14, 2019

Codecov Report

Merging #1182 (d3efee7) into master (879588a) will increase coverage by 8.04%.
The diff coverage is 94.00%.

@@             Coverage Diff              @@
##             master    #1182      +/-   ##
============================================
+ Coverage     37.10%   45.15%   +8.04%     
- Complexity     3471     3474       +3     
============================================
  Files           139      140       +1     
  Lines         10337    10407      +70     
============================================
+ Hits           3836     4699     +863     
+ Misses         6501     5708     -793     

Loading

@tvdijen tvdijen removed this from the 1.19 milestone Dec 31, 2019
@tvdijen tvdijen added this to the 2.0 milestone Dec 31, 2019
@tvdijen tvdijen force-pushed the routing-multiauth branch 3 times, most recently from e5cdc9f to eecbd76 May 11, 2020
@tvdijen tvdijen force-pushed the routing-multiauth branch from eecbd76 to 5a719ef May 11, 2020
@tvdijen tvdijen force-pushed the routing-multiauth branch from ce7f025 to 94229ea May 13, 2020
@tvdijen tvdijen requested a review from jaimeperez May 13, 2020
@tvdijen tvdijen force-pushed the master branch 3 times, most recently from 1c686ab to eb20457 Aug 17, 2020
@tvdijen tvdijen requested a review from thijskh Aug 22, 2020
Copy link
Member

@thijskh thijskh left a comment

Just two comments (about things that existed before), but I’m not very familiar with the module so at this point would not explicitly ”approve” it.

Loading

$source = $request->get('source', null);
if (is_null($source)) {
foreach ($request->query->all() as $k => $v) {
$k = explode('-', $k, 2);
Copy link
Member

@thijskh thijskh Aug 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knows the code is preexisting, but I find it hard to follow what happens here. Partially because it’s in general unclear to me what the intention is (should have a comment), partially because $k is overwritten inside the loop.

Loading

Copy link
Member Author

@tvdijen tvdijen Aug 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a 'hack' to be able to work with authsource-names that are not url-friendly.. Instead of using the source query-param it will find any query-param that starts with src- and then decodes the part after de - to find the actual source name.. It should probably hit a break; on the first hit..

Loading

Copy link
Member Author

@tvdijen tvdijen Aug 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only clue I can find is this one; 2142cfe

This part of the code doesn't really seem related to the commit description though.
If it is indeed a workaround for old IE-versions back in 2012, we could probably revert it by now

Loading

Copy link
Member

@thijskh thijskh Aug 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I propose to drop it indeed

Loading

Copy link
Member Author

@tvdijen tvdijen Aug 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I'm taking a closer look at this, I think I can understand why this was needed... Every tag would have the same name="source" in the old situation.. I can imagine older IE-versions not dealing with that very well..

I've dropped the IE-fix, but we need to proper test this to make sure nothing is broken

Loading


$t->data['authstate'] = $authStateId;
$t->data['sources'] = $sources;
$t->data['selfUrl'] = $_SERVER['PHP_SELF'];
Copy link
Member

@thijskh thijskh Aug 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this used? It might not be fully trusted content?

Loading

Copy link
Member Author

@tvdijen tvdijen Aug 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selfUrl is used in the template and it's value is being escaped there, although the PHP_SELF-variable should be trustworthy because it's set by the webserver (it's not request data).

Could probably replace it with a call to Utils\HTTP::getSelfURLNoQuery() if you like

Loading

Copy link
Member

@thijskh thijskh Aug 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That definitely seems like an improvement

Loading

Copy link
Member Author

@tvdijen tvdijen Aug 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too sure it is.. I couldn't use the utility because we need the relative url, so I had to go with the manual approach..

Loading

modules/multiauth/lib/Controller/DiscoController.php Outdated Show resolved Hide resolved
Loading
@tvdijen tvdijen merged commit 8ed5384 into master Jan 10, 2021
8 of 9 checks passed
Loading
@tvdijen tvdijen deleted the routing-multiauth branch Jan 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants