Handle IdP-initiated SAML requests #1413
Conversation
The logic handling the case where we are given a relay state or not and what that relay state might contain has gotten a bit fiddly. Let's factor it out into its own function.
jordigh
force-pushed
the
jordigh/saml
branch
3 times, most recently
from
2d1eed8 to
f2e6acc
Compare
The `_getSession` method can use a better default of getting the session from the request instead of an empty object in case that the session has not been saved to the store yet.
We need to verify that redirect URLs go back to Grist, for safety, so let's factor that out into a new function.
Now we use the results of the utility function to determine what to do with the SAML request. This in particular will now handle IdP-initiated requests that are missing a `RelayState`.
app/server/lib/SamlConfig.ts
Outdated
|
|
||
| if (!relayState) { | ||
| // Presumably an IdP-inititated signin. | ||
| params.redirectUrl = checkRedirectUrl(relayState, req).href; |
There was a problem hiding this comment.
This is making some computation with relayState after checking that relayState is falsy. Possibly unintentional?
There was a problem hiding this comment.
Yes, sorry, you're right, I accidentally swapped around these two redirect URLs.
Just fixed it.
app/server/lib/SamlConfig.ts
Outdated
| const permitStore = this._gristServer.getExternalPermitStore(); | ||
| const state = await permitStore.getPermit(relayState); | ||
| if (!state) { | ||
| // Presumably an IdP-inititated signin without a relayState. |
There was a problem hiding this comment.
"without a relayState" seems to refer to the other clause (where !relayState is checked). Perhaps comments got mixed up?
There was a problem hiding this comment.
The second case is supposed to check that we do have a relay state but it doesn't contain a permit. Perhaps it contains a redirect URL? That is apparently one non-standard but de facto common use of RelayState for IdP-initiatied logins. I've modified the comments to reflect this intent.
jordigh
force-pushed
the
jordigh/saml
branch
2 times, most recently
from
f279a52 to
c396531
Compare
Context
Our SAML handling does not handle identity provider initiated requests (e.g. you have a dashboard of applications in your Okta or Google admin, and you want to click on one of those icons to log into Grist).
Original commit messages:
29a077f ScopedSession: default to request session instead of empty object
The
_getSessionmethod can use a better default of getting thesession from the request instead of an empty object in case that the
session has not been saved to the store yet.
3f97771 SamlConfig: new utility function, checkRedirectUrl
We need to verify that redirect URLs go back to Grist, for safety, so
let's factor that out into a new function.
f14b043 SamlConfig: new utility function, _processInitialRequest
The logic handling the case where we are given a relay state or not
and what that relay state might contain has gotten a bit fiddly. Let's
factor it out into its own function.
448a3b4 SamlConfig: defer the initial logic to _processInitialRequest
Now we use the results of the utility function to determine what to do
with the SAML request. This in particular will now handle
IdP-initiated requests that are missing a
RelayState.Proposed solution
This adds functionality for IdP-initiated requests by handling SAML requests that do not have a
RelayState.Has this been tested?
I need some help testing this correctly. We currently do not have any SAML tests at all, so I need to better understand the protocol before I can write the correct tests for it.