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

[Westminster] Add OpenID Connect single sign-on support #2523

Merged
merged 5 commits into from
Jul 11, 2019

Conversation

davea
Copy link
Member

@davea davea commented Jun 20, 2019

Extends social login functionality to authenticate against an (MS Active Directory B2C-specific) OpenID Connect identity provider.

@codecov
Copy link

codecov bot commented Jun 20, 2019

Codecov Report

Merging #2523 into westminster-reviewed will increase coverage by 0.03%.
The diff coverage is 79.5%.

Impacted file tree graph

@@                   Coverage Diff                    @@
##           westminster-reviewed    #2523      +/-   ##
========================================================
+ Coverage                 82.32%   82.35%   +0.03%     
========================================================
  Files                       211      214       +3     
  Lines                     13085    13187     +102     
  Branches                   2417     2439      +22     
========================================================
+ Hits                      10772    10860      +88     
- Misses                     1476     1480       +4     
- Partials                    837      847      +10
Impacted Files Coverage Δ
...erllib/FixMyStreet/App/Controller/Report/Update.pm 89.93% <100%> (+0.03%) ⬆️
perllib/FixMyStreet/DB/Result/User.pm 94.83% <100%> (+0.31%) ⬆️
perllib/FixMyStreet/App/Controller/Report/New.pm 87.58% <100%> (+0.01%) ⬆️
perllib/OIDC/Lite/Client/WebServer/Azure.pm 100% <100%> (ø)
perllib/OIDC/Lite/Client/IDTokenResponseParser.pm 56% <56%> (ø)
perllib/FixMyStreet/App/Controller/Auth.pm 82.12% <77.77%> (+0.54%) ⬆️
perllib/FixMyStreet/App/Controller/Auth/Social.pm 86.52% <80%> (-2.95%) ⬇️
perllib/FixMyStreet/Cobrand/Westminster.pm 85.71% <85.71%> (+60.71%) ⬆️
perllib/FixMyStreet/Cobrand/BathNES.pm 54.28% <0%> (-1.91%) ⬇️
perllib/FixMyStreet/Cobrand/UKCouncils.pm 86.3% <0%> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 412c270...8c4f606. Read the comment docs.

@zarino zarino force-pushed the issues/community/57-westminster-cobrand branch from 97456ae to 7b8d65c Compare June 25, 2019 15:25
@dracos dracos force-pushed the issues/community/57-westminster-cobrand branch from 7b8d65c to 385bf4e Compare June 27, 2019 08:39
@davea davea force-pushed the westminster-sso branch 2 times, most recently from 5e9c8f1 to d1cd01c Compare June 28, 2019 16:44
@davea davea marked this pull request as ready for review June 28, 2019 16:47
@davea davea requested a review from dracos June 28, 2019 16:47
Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

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

Have to dash, almost finished, just the OIDC::Lite ones to check over, but thought would submit now anyway. Looks good, main overall comment is can/should we refactor the different mechanisms together now we're at three. Plus other little bits.

perllib/FixMyStreet/Cobrand/Westminster.pm Outdated Show resolved Hide resolved
perllib/FixMyStreet/DB/Result/User.pm Show resolved Hide resolved
perllib/FixMyStreet/DB/Result/User.pm Outdated Show resolved Hide resolved
perllib/FixMyStreet/App/Controller/Auth.pm Outdated Show resolved Hide resolved
perllib/FixMyStreet/App/Controller/Report/New.pm Outdated Show resolved Hide resolved
perllib/FixMyStreet/App/Controller/Auth.pm Outdated Show resolved Hide resolved
$c->res->redirect($url);
}

sub oidc_callback: Path('/auth/OIDC') : Args(0) {
Copy link
Member

Choose a reason for hiding this comment

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

I realise they're all subtly different, but is there any worth in factoring the (now) three out to shared subclasses or something with variants for their differing parts?

perllib/FixMyStreet/App/Controller/Report/Update.pm Outdated Show resolved Hide resolved
perllib/FixMyStreet/App/Controller/Report/Update.pm Outdated Show resolved Hide resolved
Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

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

One question on setting the parser class is all.

perllib/OIDC/Lite/Client/WebServer/Azure.pm Show resolved Hide resolved
@dracos dracos force-pushed the issues/community/57-westminster-cobrand branch 4 times, most recently from 7a65687 to 6790083 Compare July 10, 2019 16:49
@dracos dracos changed the base branch from issues/community/57-westminster-cobrand to westminster-reviewed July 11, 2019 13:14
Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

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

Looks good, one suggested change is all!

perllib/FixMyStreet/App/Controller/Auth.pm Outdated Show resolved Hide resolved
@davea davea merged commit 8c4f606 into westminster-reviewed Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants