SP-initiated logout #23
Conversation
| else | ||
| flash[:alert] = t('omniauth.logout_fail') | ||
| render :failure | ||
| end |
There was a problem hiding this comment.
Looks like this is not tested. By the way, I can tell because I'm using Code Climate's awesome browser extension, which you have to sign up to get, but I got access pretty quickly. https://codeclimate.com/browser-extension
There was a problem hiding this comment.
To clarify, what's not tested is the failure part of this method.
There was a problem hiding this comment.
thanks, failure tests added.
pkarman
force-pushed
the
sp-initiated-slo
branch
2 times, most recently
from
ff23e32 to
e3e3686
Compare
|
@monfresh thanks for the review. I've added tests for the failure flows. |
pkarman
force-pushed
the
sp-initiated-slo
branch
3 times, most recently
from
a93df4b to
c6d3285
Compare
| @@ -0,0 +1,5 @@ | |||
| class UuiDtoString < ActiveRecord::Migration | |||
| def change | |||
| change_column :users, :uuid, :string, null: false | |||
There was a problem hiding this comment.
since the -idp seed users have UUID strings that are not actually valid UUIDs (not strictly base64-encoded), use a more generic string definition. As long as it is a MBUN we don't really care.
|
tests here will fail until saml-idp/saml_idp#50 is merged. |
pkarman
force-pushed
the
sp-initiated-slo
branch
2 times, most recently
from
79d82f2 to
27bf2ad
Compare
|
saml-idp/saml_idp#50 appears to have been abandoned. I have switched this PR to point at the 18F fork. |
pkarman
force-pushed
the
sp-initiated-slo
branch
from
27bf2ad to
a425316
Compare
|
@amoose now that 18F/saml_idp#3 is merged I have updated this PR to point at master. |
pkarman
force-pushed
the
sp-initiated-slo
branch
from
a425316 to
0c11fda
Compare
**Why**: Better UX, since we end up on dashboard site, where user clicked the Sign Out button.
pkarman
force-pushed
the
sp-initiated-slo
branch
from
0c11fda to
700eaab
Compare
|
LGTM 👍 |
| # expect we are logged out, on our site | ||
| expect(response).to redirect_to(root_url) | ||
| expect(flash[:notice]).to eq I18n.t('omniauth.logout_ok') | ||
| end |
There was a problem hiding this comment.
The only gap I see is IdP-initiated logout. (we can improve coverage later)
Why:
Better UX, since we end up on dashboard site, where user
clicked the Sign Out button.