crewjam / saml

SAML library for go
BSD 2-Clause "Simplified" License
969 stars 435 forks source link

Fix in #322 possibly not a fix for specific issue in #300 #334

Open olliephillips opened 3 years ago

olliephillips commented 3 years ago

I'm not pushing here at all. Added here as commenting on closed issue not ideal.

Please close if invalid.

Comment text on #300 below

===

Hey there! I wonder if the fix in #322 fixes the specific issue mentioned by the OP in this ticket?

I have a test SP setup using this package for testing IDP initiated assertions.

I was seeing the same error as reported. After double checking I was using the latest patched code, I could not resolve it.

Eventually I wrapped the middleware so that I could inspect the methods in play.

I had to make this change to the createSessionFromAssertion method mentioned by the OP to get this working (basically turned off looking for a tracked request?)

func (m *inspect) CreateSessionFromAssertion(w http.ResponseWriter, r *http.Request, assertion *saml.Assertion) {
    fmt.Println("CreateSessionFromAssertion inspection...")
    redirectURI := "/"

    if !m.ServiceProvider.AllowIDPInitiated {
        if trackedRequestIndex := r.Form.Get("RelayState"); trackedRequestIndex != "" {
            trackedRequest, err := m.RequestTracker.GetTrackedRequest(r, trackedRequestIndex)
            if err != nil {
                fmt.Println("error here")
                m.OnError(w, r, err)
                return
            }
            m.RequestTracker.StopTrackingRequest(w, r, trackedRequestIndex)

            redirectURI = trackedRequest.URI
        }
    } else {
        if uri := r.Form.Get("RelayState"); uri != "" {
            redirectURI = uri
        }
    }

    if err := m.Session.CreateSession(w, r, assertion); err != nil {
        m.OnError(w, r, err)
        return
    }

    http.Redirect(w, r, redirectURI, http.StatusFound)
}

I'm aware that the check/test isn't ideal, AllowIDPInitiated does not mean that the request was IDP initiated but the amend above does work, moving me past the error, with auth & redirection working.

I have no expertise with SAML other than I'm currently working with it, so I could be missing something, but I have had my IDP code set up and working (with RelayState) against samltest.id

If this is valid, I will open an issue for tracking.

===

Originally posted by @olliephillips in https://github.com/crewjam/saml/issues/300#issuecomment-775852327

jani123 commented 3 years ago

Pumped to this very issue myself too :/

DonovanCharpin commented 3 years ago

Hi guys,

Same issue on my side, when testing IDP initiated, I have the right RelayState and Response Status but an issue with named cookie not present. That would be nice to know if we could create a PR with @olliephillips fix.

chrisrollins65 commented 2 years ago

Thanks for this @olliephillips, but it's not enough in my case. Your changes effectively always ignore request tracking if AllowIDPInitiated is enabled. But AllowIDPInitiated does not mean all requests will always be IDP initiated.

I've created a PR #463 with changes that work for my case. First I check if the request is tracked with a cookie (the way the original code does). Then if that cookie doesn't exist, then I do what you do, ignoring it and setting the redirect URI based on the Relay State. This way the code runs as intended for SP initiated logins, but also works for IDP initiated logins.