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.

Sign up for GitHub

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 SessionIndex to LogoutRequest if it's available #38

Merged
merged 2 commits into from
Sep 17, 2024
Merged

Add SessionIndex to LogoutRequest if it's available #38

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a0e4509
Set SessionIndex on LogoutRequest if it's available
mgyongyosi Sep 13, 2024
9fa8a46
fix
mgyongyosi Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion example/trivial/trivial.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func hello(w http.ResponseWriter, r *http.Request) {

func logout(w http.ResponseWriter, r *http.Request) {
nameID := samlsp.AttributeFromContext(r.Context(), "urn:oasis:names:tc:SAML:attribute:subject-id")
url, err := samlMiddleware.ServiceProvider.MakeRedirectLogoutRequest(nameID, "")
url, err := samlMiddleware.ServiceProvider.MakeRedirectLogoutRequest(nameID, "", "")
if err != nil {
panic(err) // TODO handle error
}
Expand Down
14 changes: 9 additions & 5 deletions service_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1192,7 +1192,7 @@ func (sp *ServiceProvider) SignLogoutRequest(req *LogoutRequest) error {
}

// MakeLogoutRequest produces a new LogoutRequest object for idpURL.
func (sp *ServiceProvider) MakeLogoutRequest(idpURL, nameID string) (*LogoutRequest, error) {
func (sp *ServiceProvider) MakeLogoutRequest(idpURL, nameID, sessionIndex string) (*LogoutRequest, error) {

req := LogoutRequest{
ID: fmt.Sprintf("id-%x", randomBytes(20)),
Expand All @@ -1210,6 +1210,10 @@ func (sp *ServiceProvider) MakeLogoutRequest(idpURL, nameID string) (*LogoutRequ
SPNameQualifier: sp.Metadata().EntityID,
},
}
if sessionIndex != "" {
req.SessionIndex = &SessionIndex{sessionIndex}
}
Comment on lines +1213 to +1215
Copy link

Choose a reason for hiding this comment

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

Can we get rid of the conditional statement? Can we get the same results if req.SessionIndex is nil vs ""?

Suggested change
if sessionIndex != "" {
req.SessionIndex = &SessionIndex{sessionIndex}
}
req.SessionIndex = &SessionIndex{sessionIndex}

Copy link
Author

Choose a reason for hiding this comment

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

I enclosed it in the if statement, because req.SessionIndex is a pointer and if that's nil then the xml marshaller skips the field. If I would instantiate SessionIndex regardless of the sessionIndex value then the SessionIndex would be marshalled to xml with an empty value in case of empty sessionIndex and that would change the current behaviour.


if len(sp.SignatureMethod) > 0 {
if err := sp.SignLogoutRequest(&req); err != nil {
return nil, err
Expand All @@ -1221,8 +1225,8 @@ func (sp *ServiceProvider) MakeLogoutRequest(idpURL, nameID string) (*LogoutRequ
// MakeRedirectLogoutRequest creates a SAML authentication request using
// the HTTP-Redirect binding. It returns a URL that we will redirect the user to
// in order to start the auth process.
func (sp *ServiceProvider) MakeRedirectLogoutRequest(nameID, relayState string) (*url.URL, error) {
req, err := sp.MakeLogoutRequest(sp.GetSLOBindingLocation(HTTPRedirectBinding), nameID)
func (sp *ServiceProvider) MakeRedirectLogoutRequest(nameID, relayState, sessionIndex string) (*url.URL, error) {
req, err := sp.MakeLogoutRequest(sp.GetSLOBindingLocation(HTTPRedirectBinding), nameID, sessionIndex)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1261,8 +1265,8 @@ func (r *LogoutRequest) Redirect(relayState string) *url.URL {
// MakePostLogoutRequest creates a SAML authentication request using
// the HTTP-POST binding. It returns HTML text representing an HTML form that
// can be sent presented to a browser to initiate the logout process.
func (sp *ServiceProvider) MakePostLogoutRequest(nameID, relayState string) ([]byte, error) {
req, err := sp.MakeLogoutRequest(sp.GetSLOBindingLocation(HTTPPostBinding), nameID)
func (sp *ServiceProvider) MakePostLogoutRequest(nameID, relayState, sessionIndex string) ([]byte, error) {
req, err := sp.MakeLogoutRequest(sp.GetSLOBindingLocation(HTTPPostBinding), nameID, sessionIndex)
if err != nil {
return nil, err
}
Expand Down
61 changes: 49 additions & 12 deletions service_provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,19 @@ func TestSPFailToProduceSignedRequestWithBogusSignatureMethod(t *testing.T) {
}

func TestSPCanProducePostLogoutRequest(t *testing.T) {
testCases := []struct {
name string
sessionIndex string
}{
{
name: "TestSPCanProducePostLogoutRequest_NoSessionIndex",
},
{
name: "TestSPCanProducePostLogoutRequest_SessionIndex",
sessionIndex: "session-123",
},
}

test := NewServiceProviderTest(t)
TimeNow = func() time.Time {
rv, _ := time.Parse("Mon Jan 2 15:04:05 UTC 2006", "Mon Dec 1 01:31:21 UTC 2015")
Expand All @@ -390,12 +403,30 @@ func TestSPCanProducePostLogoutRequest(t *testing.T) {
err := xml.Unmarshal(test.IDPMetadata, &s.IDPMetadata)
assert.Check(t, err)

form, err := s.MakePostLogoutRequest("[email protected]", "relayState")
assert.Check(t, err)
golden.Assert(t, string(form), t.Name()+"_form")
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
RandReader = &testRandomReader{}
form, err := s.MakePostLogoutRequest("[email protected]", "relayState", tc.sessionIndex)
assert.Check(t, err)
golden.Assert(t, string(form), tc.name+"_form")
})
}
}

func TestSPCanProduceRedirectLogoutRequest(t *testing.T) {
testCases := []struct {
name string
sessionIndex string
}{
{
name: "TestSPCanProduceRedirectLogoutRequest_NoSessionIndex",
},
{
name: "TestSPCanProduceRedirectLogoutRequest_SessionIndex",
sessionIndex: "session-123",
},
}

test := NewServiceProviderTest(t)
TimeNow = func() time.Time {
rv, _ := time.Parse("Mon Jan 2 15:04:05.999999999 UTC 2006", "Mon Dec 1 01:31:21.123456789 UTC 2015")
Expand All @@ -412,16 +443,22 @@ func TestSPCanProduceRedirectLogoutRequest(t *testing.T) {
err := xml.Unmarshal(test.IDPMetadata, &s.IDPMetadata)
assert.Check(t, err)

redirectURL, err := s.MakeRedirectLogoutRequest("[email protected]", "relayState")
assert.Check(t, err)
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
RandReader = &testRandomReader{}

decodedRequest, err := testsaml.ParseRedirectRequest(redirectURL)
assert.Check(t, err)
assert.Check(t, is.Equal("idp.testshib.org",
redirectURL.Host))
assert.Check(t, is.Equal("/idp/profile/SAML2/Redirect/SLO",
redirectURL.Path))
golden.Assert(t, string(decodedRequest), t.Name()+"_decodedRequest")
redirectURL, err := s.MakeRedirectLogoutRequest("[email protected]", "relayState", tc.sessionIndex)
assert.Check(t, err)

decodedRequest, err := testsaml.ParseRedirectRequest(redirectURL)
assert.Check(t, err)
assert.Check(t, is.Equal("idp.testshib.org",
redirectURL.Host))
assert.Check(t, is.Equal("/idp/profile/SAML2/Redirect/SLO",
redirectURL.Path))
golden.Assert(t, string(decodedRequest), tc.name+"_decodedRequest")
})
}
}

func TestSPCanProducePostLogoutResponse(t *testing.T) {
Expand Down
0 ...ta/TestSPCanProducePostLogoutRequest_form → ...ducePostLogoutRequest_NoSessionIndex_form
View file
Open in desktop
File renamed without changes.
1 change: 1 addition & 0 deletions testdata/TestSPCanProducePostLogoutRequest_SessionIndex_form
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<form method="post" action="https://idp.testshib.org/idp/profile/SAML2/POST/SLO" id="SAMLRequestForm"><input type="hidden" name="SAMLRequest" value="PHNhbWxwOkxvZ291dFJlcXVlc3QgeG1sbnM6c2FtbD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOmFzc2VydGlvbiIgeG1sbnM6c2FtbHA9InVybjpvYXNpczpuYW1lczp0YzpTQU1MOjIuMDpwcm90b2NvbCIgSUQ9ImlkLTAwMDIwNDA2MDgwYTBjMGUxMDEyMTQxNjE4MWExYzFlMjAyMjI0MjYiIFZlcnNpb249IjIuMCIgSXNzdWVJbnN0YW50PSIyMDE1LTEyLTAxVDAxOjMxOjIxWiIgRGVzdGluYXRpb249Imh0dHBzOi8vaWRwLnRlc3RzaGliLm9yZy9pZHAvcHJvZmlsZS9TQU1MMi9QT1NUL1NMTyI&#43;PHNhbWw6SXNzdWVyIEZvcm1hdD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOm5hbWVpZC1mb3JtYXQ6ZW50aXR5Ij5odHRwczovLzE1NjYxNDQ0Lm5ncm9rLmlvL3NhbWwyL21ldGFkYXRhPC9zYW1sOklzc3Vlcj48c2FtbDpOYW1lSUQgTmFtZVF1YWxpZmllcj0iaHR0cHM6Ly9pZHAudGVzdHNoaWIub3JnL2lkcC9zaGliYm9sZXRoIiBTUE5hbWVRdWFsaWZpZXI9Imh0dHBzOi8vMTU2NjE0NDQubmdyb2suaW8vc2FtbDIvbWV0YWRhdGEiIEZvcm1hdD0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6Mi4wOm5hbWVpZC1mb3JtYXQ6dHJhbnNpZW50Ij5yb3NAb2N0b2xhYnMuaW88L3NhbWw6TmFtZUlEPjxzYW1scDpTZXNzaW9uSW5kZXggeG1sbnM6c2FtbHA9InVybjpvYXNpczpuYW1lczp0YzpTQU1MOjIuMDpwcm90b2NvbCI&#43;c2Vzc2lvbi0xMjM8L3NhbWxwOlNlc3Npb25JbmRleD48L3NhbWxwOkxvZ291dFJlcXVlc3Q&#43;" /><input type="hidden" name="RelayState" value="relayState" /><input id="SAMLSubmitButton" type="submit" value="Submit" /></form><script>document.getElementById('SAMLSubmitButton').style.visibility="hidden";document.getElementById('SAMLRequestForm').submit();</script>
0 ...oduceRedirectLogoutRequest_decodedRequest → ...goutRequest_NoSessionIndex_decodedRequest
View file
Open in desktop
File renamed without changes.
1 change: 1 addition & 0 deletions testdata/TestSPCanProduceRedirectLogoutRequest_SessionIndex_decodedRequest
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<samlp:LogoutRequest xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" ID="id-00020406080a0c0e10121416181a1c1e20222426" Version="2.0" IssueInstant="2015-12-01T01:31:21.123Z" Destination="https://idp.testshib.org/idp/profile/SAML2/Redirect/SLO"><saml:Issuer Format="urn:oasis:names:tc:SAML:2.0:nameid-format:entity">https://15661444.ngrok.io/saml2/metadata</saml:Issuer><saml:NameID NameQualifier="https://idp.testshib.org/idp/shibboleth" SPNameQualifier="https://15661444.ngrok.io/saml2/metadata" Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient">[email protected]</saml:NameID><samlp:SessionIndex xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol">session-123</samlp:SessionIndex></samlp:LogoutRequest>