#195 SSO and SLO additions #236

sfrunza13 · 2023-02-17T00:21:55Z

This Pull Request aims to close #195

I used the previously added IDP and configured Starchart as a service provider that requests login and logout to it using redirect and post bindings. The SP metadata is generated by Samlify methods and then exposed on the /sp route.

The SAML response will have its assertions signed by the IDP.

The "POST-BINDING" for the logout actually is a get request and the response is found in the url.

TO TEST THIS

You can take the code and pull it into a branch of your own or with gh cli or however you choose to do it and run the regular steps to run starchart. The login container should be up and running in docker and the login button will redirect to the IDP and the signout will also do the same. With the container running you can go here http://localhost:8081/simplesaml/saml2/idp/metadata.php?output=xhtml and see the IDP Metadata.

testing account for IDP:
username: user1
password: user1pass

You can also go into the network tab and check the flow of the SAML requests and responses through every HTTP request. The payload will have SAML response or SAML request and you can plug them into a decoder and get the XML of the request or response. In this way you can see that the response for example has signed assertions.

sirinoks

Some questions here and there, overall it makes sense.
I'm lacking instruction on how to test this, and confirm if it runs/works

app/routes/login.tsx

app/routes/login/callback.tsx

app/routes/logout/callback.tsx

config/saml20-idp-hosted.php

humphd

This is amazing. I'm thrilled to see SAML2 based SSO logins working for Starchart as part of 0.3! Really great work on this.

I'd like to see us improve this code a bunch, but I don't want to block it landing in 0.3 either. Can you please take a look at my feedback and decide how much you want to do before 0.3, and what you want to do in 0.4. Anything that will happen later should get filed as follow-up issues.

Also, this code is easily testable via Playwright, and I think that should be your primary task in 0.4, so we can always know that it works going forward. See my existing login/logout Playwright test code in Telescope for some ideas:

https://github.com/Seneca-CDOT/telescope/blob/master/src/api/sso/test/e2e/browser-util.js#L14-L57

I'm sure @Eakam1007 can provide guidance too.

humphd · 2023-02-17T14:37:21Z

.env.example

@@ -18,3 +18,7 @@ LOG_LEVEL=debug
 # Nodemailer config
 NOTIFICATIONS_EMAIL_USER="no-reply@senecacollege.ca"
 MAILHOG_SMTP_PORT=1025
+SAML_IDP_METADATA=http://localhost:8081/simplesaml/saml2/idp/metadata.php


Because Samlify can configure the IdP via its metadata, we should probably make this a path to a file that we load (i.e., readFileSync()) vs. a URL that we have to download async at startup:

const idp = new IdentityProvider({ // required metadata: readFileSync('./config/idp-metadata.xml'), });

The metadata files we need are:

Dev: http://localhost:8081/simplesaml/saml2/idp/metadata.php

Staging: https://login.microsoftonline.com/eb34f74a-58e7-4a8b-9e59-433e4c412757/federationmetadata/2007-06/federationmetadata.xml?appid=9b6e9159-c5ab-462b-8efa-3ecb46e8b6df

Also, let's separate this block out to its own section, and add a # SAML Config comment.

Thinking about this a bit more. It might be better to have all of the exports from the saml.server.ts module be async. This would allow us to pull the IdP metadata on startup and use that (i.e., first request would get the metadata and config the idp, subsequent runs would be faster since we already have it).

@humphd
I am trying the following but it is claiming that neither http://localhost:8081/simplesaml/saml2/idp/metadata.php or http://host.docker.internal:8081/simplesaml/saml2/idp/metadata.php exist

//Take the metadata stood up by the IDP and use it as the metadata for our IDP object const idp = samlify.IdentityProvider({ metadata: readFileSync(`${process.env.SAML_IDP_METADATA}`), });

What am I doing wrong?

Wait, I think I misunderstood, you would like to have the idp metadata stored as a file local to starchart, is that correct?

In the last commit I had it read from the idp config locally, when I try to load idp once in saml server at the top level it gives me the error that await can not be used at the top level.

humphd · 2023-02-17T14:37:33Z

.env.example

@@ -18,3 +18,7 @@ LOG_LEVEL=debug
 # Nodemailer config
 NOTIFICATIONS_EMAIL_USER="no-reply@senecacollege.ca"
 MAILHOG_SMTP_PORT=1025
+SAML_IDP_METADATA=http://localhost:8081/simplesaml/saml2/idp/metadata.php
+HOSTNAME = http://localhost`


Why does this not include the port?

Also, trailing backtick

.env.example

humphd · 2023-02-17T14:42:08Z

app/routes/login.tsx

 import { Form } from '@remix-run/react';

-import { createUserSession } from '~/session.server';
+import { getUsername } from '~/session.server';
+import { getIdp, sp } from '~/saml.server';


Let's not expose these, since the rest of our app doesn't need to know about Samlify. Instead, let's expose some higher-level API calls we can make. What about:

createLoginRequest() and have it deal with idp + sp + 'redirect' logic internally, and return the URL we need

createLogoutRequest() same idea

I added some functions in the saml server so we dont have to export sp and idp but export those instead. Is that sufficient or should there be a seperate file that is the only one to ever import saml.server?

humphd · 2023-02-17T14:42:43Z

app/routes/login.tsx

+    const idp = await getIdp();
+    const { context } = sp.createLoginRequest(idp, 'redirect');
+    return redirect(context);
+  } else {


No else after return. If eslint didn't flag this, we should add it to the rules.

humphd · 2023-02-17T14:51:39Z

app/routes/login/callback.tsx

+    });
+
+    //Try and extract the username and see if there is an existing user by that name
+    if (extract.nameID) {


According to what I shared with you from ITS, our metadata will look like this:

displayname

email

sAMAccountName (is the username)

group (currently the values would be: mycustomdomain-dev-admins, mycustomdomain-dev-faculty, mycustomdomain-dev-students)

So we better extract the sAMAccountName.

See https://login.microsoftonline.com/eb34f74a-58e7-4a8b-9e59-433e4c412757/federationmetadata/2007-06/federationmetadata.xml?appid=9b6e9159-c5ab-462b-8efa-3ecb46e8b6df for the metadata, and in particular, notice the claims it defines:

<fed:ClaimTypesOffered> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.xmlsoap.org/ws/2005/05/identity/claims/name" > <auth:DisplayName>Name</auth:DisplayName> <auth:Description>The mutable display name of the user.</auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier" > <auth:DisplayName>Subject</auth:DisplayName> <auth:Description> An immutable, globally unique, non-reusable identifier of the user that is unique to the application for which a token is issued. </auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname" > <auth:DisplayName>Given Name</auth:DisplayName> <auth:Description>First name of the user.</auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname" > <auth:DisplayName>Surname</auth:DisplayName> <auth:Description>Last name of the user.</auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.microsoft.com/identity/claims/displayname" > <auth:DisplayName>Display Name</auth:DisplayName> <auth:Description>Display name of the user.</auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.microsoft.com/identity/claims/nickname" > <auth:DisplayName>Nick Name</auth:DisplayName> <auth:Description>Nick name of the user.</auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.microsoft.com/ws/2008/06/identity/claims/authenticationinstant" > <auth:DisplayName>Authentication Instant</auth:DisplayName> <auth:Description> The time (UTC) when the user is authenticated to Windows Azure Active Directory. </auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.microsoft.com/ws/2008/06/identity/claims/authenticationmethod" > <auth:DisplayName>Authentication Method</auth:DisplayName> <auth:Description> The method that Windows Azure Active Directory uses to authenticate users. </auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.microsoft.com/identity/claims/objectidentifier" > <auth:DisplayName>ObjectIdentifier</auth:DisplayName> <auth:Description> Primary identifier for the user in the directory. Immutable, globally unique, non-reusable. </auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.microsoft.com/identity/claims/tenantid" > <auth:DisplayName>TenantId</auth:DisplayName> <auth:Description>Identifier for the user's tenant.</auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.microsoft.com/identity/claims/identityprovider" > <auth:DisplayName>IdentityProvider</auth:DisplayName> <auth:Description>Identity provider for the user.</auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress" > <auth:DisplayName>Email</auth:DisplayName> <auth:Description>Email address of the user.</auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.microsoft.com/ws/2008/06/identity/claims/groups" > <auth:DisplayName>Groups</auth:DisplayName> <auth:Description>Groups of the user.</auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.microsoft.com/identity/claims/accesstoken" > <auth:DisplayName>External Access Token</auth:DisplayName> <auth:Description> Access token issued by external identity provider. </auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.microsoft.com/ws/2008/06/identity/claims/expiration" > <auth:DisplayName>External Access Token Expiration</auth:DisplayName> <auth:Description> UTC expiration time of access token issued by external identity provider. </auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.microsoft.com/identity/claims/openid2_id" > <auth:DisplayName>External OpenID 2.0 Identifier</auth:DisplayName> <auth:Description> OpenID 2.0 identifier issued by external identity provider. </auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.microsoft.com/claims/groups.link" > <auth:DisplayName>GroupsOverageClaim</auth:DisplayName> <auth:Description> Issued when number of user's group claims exceeds return limit. </auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.microsoft.com/ws/2008/06/identity/claims/role" > <auth:DisplayName>Role Claim</auth:DisplayName> <auth:Description> Roles that the user or Service Principal is attached to </auth:Description> </auth:ClaimType> <auth:ClaimType xmlns:auth="http://docs.oasis-open.org/wsfed/authorization/200706" Uri="http://schemas.microsoft.com/ws/2008/06/identity/claims/wids" > <auth:DisplayName>RoleTemplate Id Claim</auth:DisplayName> <auth:Description> Role template id of the Built-in Directory Roles that the user is a member of </auth:Description> </auth:ClaimType> </fed:ClaimTypesOffered>

humphd · 2023-02-17T14:52:37Z

app/routes/login/callback.tsx

+    }
+
+    // return to access denied if redirect in createUserSession did not take
+    return redirect('/access_denied');


This page doesn't exist

app/routes/logout/callback.tsx

humphd · 2023-02-17T14:55:03Z

app/saml.server.ts

+samlify.setSchemaValidator(validator);
+
+//Here we configure the service provider: https://samlify.js.org/#/sp-configuration
+const spData = {


Move this object into line 27 (i.e., don't add a variable to pass it to the constructor). The reason for this is type checking. Here, you have an any type, but if you do it in the argument list of ServiceProvider({...}) it will get typed.

docker-compose.yml

sfrunza13 · 2023-02-18T15:54:13Z

@humphd I have been looking at this a lot over the past day and I don't know if it works exactly as intended. Validation of signature along with what is meant to be signed (right now I am trying to have the assertion signed but upon further inspection don't know if that's working and whether I'm supposed to be signing the assertions or the message), and the logout flow along with parsing the logout response might need to be added or changed. I don't think I can do all of that for this MS, I am going to do some cleaning up of the current code and try to get this in and change it moving forward if that's ok with you.

It seems to work but I think I have missed the mark on a lot of important things.

humphd · 2023-02-18T16:27:21Z

@humphd I have been looking at this a lot over the past day and I don't know if it works exactly as intended. Validation of signature along with what is meant to be signed (right now I am trying to have the assertion signed but upon further inspection don't know if that's working and whether I'm supposed to be signing the assertions or the message), and the logout flow along with parsing the logout response might need to be added or changed. I don't think I can do all of that for this MS, I am going to do some cleaning up of the current code and try to get this in and change it moving forward if that's ok with you.

It seems to work but I think I have missed the mark on a lot of important things.

I agree that nailing down all the various pieces of this is beyond the scope of 0.3.

Since what you have works in development, I'm inclined to say that we should fix the code-level issues I've highlighted, and file issues on the rest to do in 0.4+.

Let's not block this on being perfect. You've moved us ahead a long way, and that's what a weekly milestone is all about.

humphd

I don't see how to get this landed in 0.3, since this is pretty big and still needs work. We need PRs to come in sooner so there is time to review in future.

humphd · 2023-02-19T13:15:47Z

app/routes/login/callback.tsx

+  const body = Object.fromEntries(formData);
+  const extract = await parseLoginResponse(body);
+
+  //Try and extract the username and see if there is an existing user by that name


nit: sometimes you do //nospace and other times // space. Can you always do the latter (i.e., include a space after //)?

humphd · 2023-02-19T13:17:40Z

app/routes/login/callback.tsx

+  const extract = await parseLoginResponse(body);
+
+  //Try and extract the username and see if there is an existing user by that name
+  if (extract.attributes.sAMAccountName) {


Same advice as before, flip your logic and put the error/shorter case first:

if(!extract.attribues.sAMAccountName) { // TODO: Create some form of access denied notification to redirect to here return redirect('/access_denied'); } // bulk of code here, not indented...

humphd · 2023-02-19T13:18:44Z

app/routes/login/callback.tsx

+export const action = async ({ request }: ActionArgs) => {
+  const formData = await request.formData();
+
+  if (request.method != 'POST') {


Do this first, since a only a POST will have form data to parse.

Also, !== vs. != in JS please. The former does implicit type casting, which you never want.

humphd · 2023-02-19T13:19:10Z

app/routes/login/callback.tsx

+  }
+
+  const body = Object.fromEntries(formData);
+  const extract = await parseLoginResponse(body);


extract is an odd name. samlResponse?

humphd · 2023-02-19T13:19:49Z

app/routes/logout/callback.tsx

+
+  if (SAMLResponse) {
+    return await logout(request);
+  } else {


No else-after-return.

humphd · 2023-02-19T13:21:46Z

app/saml.server.ts

+
+//Take the metadata stood up by the IDP and use it as the metadata for our IDP object
+const idp = samlify.IdentityProvider({
+  metadata: readFileSync(`${process.env.SAML_IDP_METADATA}`),


If it's a filename vs. the contents of a file, you should name it as such: SAML_IDP_METADATA_FILE or _FILENAME.

humphd

This needs a few fixes, but I was able to make it work.

You should update CONTRIBUTING.md to include a section on how to login (i.e., a table with usernames/passwords) so we can point our devs at it. None of them are going to know how to login after you land this!

humphd · 2023-02-22T22:42:34Z

.env.example

@@ -18,3 +18,7 @@ LOG_LEVEL=debug
 # Nodemailer config
 NOTIFICATIONS_EMAIL_USER="no-reply@senecacollege.ca"
 MAILHOG_SMTP_PORT=1025
+SAML_IDP_METADATA_FILE='./config/idp-metadata.xml'


Put the contents of this file in dev-secrets/IDP_METADATA vs. here.

humphd · 2023-02-22T22:43:26Z

app/routes/login.tsx

+    const context = await createLoginRequest();
+    return redirect(context);
+  }
+  if (user) {


This is unnecessary, based on line 25. You can simply return ... here.

humphd · 2023-02-22T22:46:58Z

.env.example

@@ -18,3 +18,7 @@ LOG_LEVEL=debug
 # Nodemailer config
 NOTIFICATIONS_EMAIL_USER="no-reply@senecacollege.ca"
 MAILHOG_SMTP_PORT=1025
+SAML_IDP_METADATA_FILE='./config/idp-metadata.xml'


Also, please separate this section with a blank line and a comment:

# SSO Config ...

humphd · 2023-02-22T22:47:15Z

app/routes/login/callback.tsx

+
+  const formData = await request.formData();
+  const body = Object.fromEntries(formData);
+  const samlResponse = await parseLoginResponse(body);


Because you only use attributes, you might as well pull that out here:

const { attributes } = await parseLoginResponse(body);

humphd · 2023-02-22T22:50:19Z

app/routes/login/callback.tsx

+    request: request,
+    username: username,
+    remember: false,
+    redirectTo: '/',


Do we have access to the redirectTo param here? When you are on a page, and get redirected to the login, it usually keeps this param so you can get sent back to the place where you were. It should be in params I think, but check my math.

I remembered that Chris's example had something for this using RelayState so that the redirectUrl can persist to the callback. I added that.

app/saml.server.ts

humphd · 2023-02-22T22:51:24Z

app/saml.server.ts

+
+// Take the metadata stood up by the IDP and use it as the metadata for our IDP object
+const idp = samlify.IdentityProvider({
+  metadata: readFileSync(`${process.env.SAML_IDP_METADATA_FILE}`),


Pull this out of secrets

humphd

Looking good, a few small things.

humphd · 2023-02-23T00:56:08Z

app/routes/login.tsx

+  if (!user) {
+    const context = await createLoginRequest();
+    const url = new URL(request.url);
+    const returnTo = url.searchParams.get('redirectTo') || '/';


I think you are meant to do this (i.e., pass the relayState) in the SP code, see https://github.com/tngan/samlify/blob/87aa1cf7fc3729282d31be60dd248d4906dc491f/src/entity-idp.ts#L83-L91

humphd · 2023-02-23T00:56:43Z

app/routes/login/callback.tsx

+    request: request,
+    username: username,
+    remember: false,
+    // redirectTo: '/',


Remove this

app/saml.server.ts

humphd · 2023-02-23T00:58:18Z

package.json

@@ -6,7 +6,7 @@
    "build": "run-s build:*",
    "build:remix": "remix build",
    "build:server": "esbuild --platform=node --format=cjs ./server.ts --outdir=build --bundle",
-    "dev": "SECRETS_OVERRIDE=1 run-p dev:*",
+    "dev": "cross-env SECRETS_OVERRIDE=1 run-p dev:*",


Rebase to pick this up from main

humphd · 2023-02-23T01:01:15Z

app/saml.server.ts

+
+// Take the metadata stood up by the IDP and use it as the metadata for our IDP object
+const idp = samlify.IdentityProvider({
+  metadata: readFileSync(secrets.SAML_IDP_METADATA_FILE),


Three things:

You don't need to readFileSync() on the secrets, they are already re-hydrated from the filesystem

Rename it to SAML_IDP_METADATA (drop the _FILE since they are all files)

You should add some code at the top of this to check that this secret exists, and throw if not. We can't run the app without it.

const { SAML_IDP_METADATA } = secrets; if(!SAML_IDP_METADATA) { throw new Error('Missing SAML_IDP_METADATA secret'); }

sfrunza13 · 2023-02-23T03:41:44Z

Not entirely sure but after most recent changes I get the following whenever I try to log in:

TypeError: The "url" argument must be of type string. Received an instance of Object
    at new NodeError (node:internal/errors:399:5)
    at validateString (node:internal/validators:163:11)
    at Url.parse (node:url:176:3)
    at Object.urlParse (node:url:147:13)
    at buildRedirectURL (C:\Users\stefe\starchart\node_modules\samlify\src\binding-redirect.ts:55:25)
    at Object.loginRequestRedirectURL (C:\Users\stefe\starchart\node_modules\samlify\src\binding-redirect.ts:116:16)
    at ServiceProvider.createLoginRequest (C:\Users\stefe\starchart\node_modules\samlify\src\entity-sp.ts:71:32)
    at createLoginRequest (C:\Users\stefe\starchart\app\saml.server.ts:44:26)
    at action2 (C:\Users\stefe\starchart\app\routes\login.tsx:28:27)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

I'll take a look again tomorrow

humphd · 2023-02-23T13:39:42Z

Not entirely sure but after most recent changes I get the following whenever I try to log in:

TypeError: The "url" argument must be of type string. Received an instance of Object
    at new NodeError (node:internal/errors:399:5)
    at validateString (node:internal/validators:163:11)
    at Url.parse (node:url:176:3)
    at Object.urlParse (node:url:147:13)
    at buildRedirectURL (C:\Users\stefe\starchart\node_modules\samlify\src\binding-redirect.ts:55:25)
    at Object.loginRequestRedirectURL (C:\Users\stefe\starchart\node_modules\samlify\src\binding-redirect.ts:116:16)
    at ServiceProvider.createLoginRequest (C:\Users\stefe\starchart\node_modules\samlify\src\entity-sp.ts:71:32)
    at createLoginRequest (C:\Users\stefe\starchart\app\saml.server.ts:44:26)
    at action2 (C:\Users\stefe\starchart\app\routes\login.tsx:28:27)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

I'll take a look again tomorrow

That stack implies that for some reason the IDP's single sign on config is wrong, see https://github.com/tngan/samlify/blob/18ef1392eb92f9fcaa79d0fafe25e9d8d2fcb1cf/src/binding-redirect.ts#L93

const base = metadata.idp.getSingleSignOnService(binding.redirect);

sfrunza13 · 2023-02-23T15:33:42Z

I put the path to the xml file in config in the secret, when I should have probably just been putting the xml in the secret directly, that seems to do it.

sfrunza13 · 2023-02-23T15:36:27Z

Should I delete the metadata file in the config folder?

humphd · 2023-02-23T16:04:44Z

app/routes/login.tsx

+
+  // If not then create a login request to the IDP's redirect binding
+  if (!user) {
+    const samlRedirectURL = await createLoginRequest(new URL(request.url));


Question: what is request.url going to be here?

What about the case of http://localhost:8080/login?redirectTo=%2Fdev, where I want to go to /dev but get redirected to the login page?

humphd · 2023-02-23T16:06:41Z

app/routes/login/callback.tsx

+
+  const formData = await request.formData();
+  const body = Object.fromEntries(formData);
+  const { attributes } = await parseLoginResponse(body);


Can you also destructure RelateState at the same time there?

humphd · 2023-02-23T16:07:19Z

app/routes/login/callback.tsx

+    // TODO: Make this redirect to access denied page
+    return redirect('/');
+  }
+  const returnTo: string = body.RelayState ? body.RelayState.toString() : '/';


Don't specify a type if you don't have to (seems odd I know). TS can infer most types. Only decorate with types when it can't.

humphd · 2023-02-23T16:08:19Z

app/saml.server.ts

+
+export async function createLoginRequest(url?: URL) {
+  const { context } = sp.createLoginRequest(idp, 'redirect');
+  const returnTo = url ? url.searchParams.get('redirectTo') : '/';


sfrunza13 · 2023-02-23T17:12:46Z

should I be casting relayState to string?

Type 'FormDataEntryValue' is not assignable to type 'string'.
  Type 'File' is not assignable to type 'string'.ts(2322)

the above occurs on 78 in login/callback.tsx

humphd · 2023-02-23T17:19:27Z

should I be casting relayState to string?
Type 'FormDataEntryValue' is not assignable to type 'string'.
  Type 'File' is not assignable to type 'string'.ts(2322)
the above occurs on 78 in login/callback.tsx

What does parseLoginResponse() actually return?

All of the Samlify tests use this pattern:

  const { samlContent, extract } = await sp.parseLoginResponse(idpcustomNoEncrypt, 'redirect', parseRedirectUrlContextCallBack(context));
  t.is(typeof id, 'string');
  t.is(samlContent.startsWith('<samlp:Response'), true);
  t.is(samlContent.endsWith('/samlp:Response>'), true);
  t.is(extract.nameID, 'user@esaml2.com');
  t.is(extract.attributes.name, 'mynameinsp');
  t.is(extract.attributes.mail, 'myemailassociatedwithsp@sp.com');
  t.is(extract.response.inResponseTo, '_4606cc1f427fa981e6ffd653ee8d6972fc5ce398c4');

So is it on extract? That is: extract.RelayState or extract.relayState (not sure) and extract.attributes?

sfrunza13 · 2023-02-23T19:48:52Z

should I be casting relayState to string?
Type 'FormDataEntryValue' is not assignable to type 'string'.
  Type 'File' is not assignable to type 'string'.ts(2322)
the above occurs on 78 in login/callback.tsx

What does parseLoginResponse() actually return?

All of the Samlify tests use this pattern:

  const { samlContent, extract } = await sp.parseLoginResponse(idpcustomNoEncrypt, 'redirect', parseRedirectUrlContextCallBack(context));
  t.is(typeof id, 'string');
  t.is(samlContent.startsWith('<samlp:Response'), true);
  t.is(samlContent.endsWith('/samlp:Response>'), true);
  t.is(extract.nameID, 'user@esaml2.com');
  t.is(extract.attributes.name, 'mynameinsp');
  t.is(extract.attributes.mail, 'myemailassociatedwithsp@sp.com');
  t.is(extract.response.inResponseTo, '_4606cc1f427fa981e6ffd653ee8d6972fc5ce398c4');

So is it on extract? That is: extract.RelayState or extract.relayState (not sure) and extract.attributes?

I created a method that is named the same as the sp one, I take relayState out of the body. I don't think the relayState is returned by the sp method.

humphd · 2023-02-23T19:55:06Z

OK. I'm not as deep into the changes you've made on this as you are, so you tell me where this is at and let's get this finished.

sfrunza13 · 2023-02-23T19:58:30Z

export async function parseLoginResponse(body: { [k: string]: FormDataEntryValue }) {
  const { extract } = await sp.parseLoginResponse(idp, 'post', {
    body,
  });
  const relayState = body.RelayState;
  console.log(relayState);
  return { attributes: extract, relayState: relayState };
}

on line 54 of saml.server.ts

app/routes/login/callback.tsx

humphd · 2023-02-23T21:42:37Z

app/routes/login.tsx

+  // If not then create a login request to the IDP's redirect binding
+  if (!user) {
+    const url = new URL(request.url);
+    const redirectTo = url.searchParams.get('redirectTo');


const redirectTo = url.searchParams.get('redirectTo') ?? undefined;

This will give us undefined if we get back null, which is better for the createLoginRequest params. I'll comment below on how to do it.

app/routes/login/callback.tsx

humphd · 2023-02-23T21:45:23Z

app/saml.server.ts

+
+export async function createLoginRequest(redirectUrl?: string | null) {
+  const { context } = sp.createLoginRequest(idp, 'redirect');
+  const returnTo = redirectUrl ? redirectUrl : '/';


redirectUrl || '/'

humphd · 2023-02-23T21:49:31Z

app/saml.server.ts

+export async function createLoginRequest(redirectUrl?: string | null) {
+  const { context } = sp.createLoginRequest(idp, 'redirect');
+  const returnTo = redirectUrl ? redirectUrl : '/';
+  return context + '&RelayState=' + returnTo;


A better way to do this is to use the built-in URL constructor that the browser/node provide us (avoid hand-crafting complex strings like URLs, paths, etc as much as you can):

const url = new URL(context); url.searchParams.append('RelayState', returnTo); return url.href;

humphd · 2023-02-23T21:50:45Z

app/saml.server.ts

+    body,
+  });
+  const relayState = body.RelayState as string;
+  return { samlResponse: extract, relayState: relayState };


Don't repeat when key/value are the same:

return { samlResponse: extract, relayState };

app/saml.server.ts

humphd · 2023-02-23T21:56:31Z

app/saml.server.ts

+  return sp.getMetadata();
+}
+
+export async function createLoginRequest(redirectUrl?: string | null) {


You can avoid the ? and | null dance here if you always pass string or undefined (which my change above will do). Here you can set a default value:

export async function createLoginRequest(redirectUrl: string = '/') {

humphd · 2023-02-23T21:57:20Z

You also have a conflict with .env.example on main.

…all .env change SSO and SLO with SimpleSAMLPhp and Samlify Added SLO callback

…lServer, new samlServer functions to export funcionality without exposing sp/idp

…me guard clauses, added file to metadata file name

…relayState stuff in the logic inside of the saml server

…ectTo searchParams and if it comes from login directly we just use '/', added taking relaystate out of body in parseLoginResponse and destructure it in callback file.

…r in createLoginRequest

humphd

Excellent. Thanks for all your work writing and evolving this.

sfrunza13 · 2023-02-23T23:06:07Z

Excellent. Thanks for all your work writing and evolving this.

Thank you for the help, lets get one more review

SerpentBytes

I was able to test it successfully on my machine. Run into a small issue, and @sfrunza13 was able to help me figure out what I was doing wrong.

Great work!

* SAML Server, login callback, login page change, sp metadata route, small .env change SSO and SLO with SimpleSAMLPhp and Samlify Added SLO callback * linting concerns * else * POST check first, sAMAccountName extract, idp and sp only used in samlServer, new samlServer functions to export funcionality without exposing sp/idp * Add idp config locally * cleaned up the code comments, swaped the conditional logic to make some guard clauses, added file to metadata file name * forgot a console log * I think this addresses most things * Metadata secret check and remove fs in saml server, tried to add the relayState stuff in the logic inside of the saml server * Changed the metadata location properly * Changed the loginRequest a bit * passing a string into creating login request so that we can use redirectTo searchParams and if it comes from login directly we just use '/', added taking relaystate out of body in parseLoginResponse and destructure it in callback file. * narrowed down type of relayState to string * I forgot to save the changes last time * changed to string or default '/' for createLogin, used URL constructor in createLoginRequest * addition to env example --------- Co-authored-by: stefanaz2 <sfrunza@seneca.ca>

sfrunza13 force-pushed the #195 branch from 62feaca to 77b7035 Compare February 17, 2023 00:31

Genne23v assigned sfrunza13 Feb 17, 2023

sirinoks reviewed Feb 17, 2023

View reviewed changes

humphd requested changes Feb 17, 2023

View reviewed changes

Genne23v added this to the Milestone 0.3 milestone Feb 17, 2023

Genne23v added the category: authentication A service responsible for logging users in and maintaining their permissions based on credentials label Feb 17, 2023

SerpentBytes modified the milestones: Milestone 0.3, Milestone 0.4 Feb 19, 2023

SerpentBytes requested a review from a user February 19, 2023 02:00

humphd requested changes Feb 19, 2023

View reviewed changes

sfrunza13 force-pushed the #195 branch from fd37bc8 to b9b1187 Compare February 22, 2023 22:44

humphd requested changes Feb 22, 2023

View reviewed changes

humphd requested changes Feb 23, 2023

View reviewed changes

sfrunza13 force-pushed the #195 branch from 8d1bb89 to e322ff3 Compare February 23, 2023 02:54

humphd requested changes Feb 23, 2023

View reviewed changes

sfrunza13 commented Feb 23, 2023

View reviewed changes

app/routes/login/callback.tsx Show resolved Hide resolved

humphd requested changes Feb 23, 2023

View reviewed changes

SAML Server, login callback, login page change, sp metadata route, sm…

6d33b84

…all .env change SSO and SLO with SimpleSAMLPhp and Samlify Added SLO callback

stefanaz2 added 15 commits February 23, 2023 17:28

linting concerns

2b414b8

else

8cd710c

POST check first, sAMAccountName extract, idp and sp only used in sam…

c68becc

…lServer, new samlServer functions to export funcionality without exposing sp/idp

Add idp config locally

c2dd68d

cleaned up the code comments, swaped the conditional logic to make so…

bf970e8

…me guard clauses, added file to metadata file name

forgot a console log

226b593

I think this addresses most things

93b1bb4

Metadata secret check and remove fs in saml server, tried to add the …

e2d3877

…relayState stuff in the logic inside of the saml server

Changed the metadata location properly

656c67e

Changed the loginRequest a bit

2986885

passing a string into creating login request so that we can use redir…

a0be247

…ectTo searchParams and if it comes from login directly we just use '/', added taking relaystate out of body in parseLoginResponse and destructure it in callback file.

narrowed down type of relayState to string

c37c81b

I forgot to save the changes last time

5e01b39

changed to string or default '/' for createLogin, used URL constructo…

d47032d

…r in createLoginRequest

addition to env example

144aff6

sfrunza13 force-pushed the #195 branch from 201d791 to 144aff6 Compare February 23, 2023 22:31

humphd approved these changes Feb 23, 2023

View reviewed changes

SerpentBytes self-requested a review February 24, 2023 01:35

SerpentBytes approved these changes Feb 24, 2023

View reviewed changes

sfrunza13 merged commit f4e332a into DevelopingSpace:main Feb 24, 2023

#195 SSO and SLO additions #236

#195 SSO and SLO additions #236

Conversation

sfrunza13 commented Feb 17, 2023 • edited Loading

TO TEST THIS

sirinoks left a comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfrunza13 Feb 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfrunza13 commented Feb 18, 2023 • edited Loading

humphd commented Feb 18, 2023

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfrunza13 commented Feb 23, 2023

humphd commented Feb 23, 2023

sfrunza13 commented Feb 23, 2023

sfrunza13 commented Feb 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfrunza13 commented Feb 23, 2023

humphd commented Feb 23, 2023

sfrunza13 commented Feb 23, 2023

humphd commented Feb 23, 2023

sfrunza13 commented Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd commented Feb 23, 2023

humphd left a comment

Choose a reason for hiding this comment

sfrunza13 commented Feb 23, 2023

SerpentBytes left a comment

Choose a reason for hiding this comment

sfrunza13 commented Feb 17, 2023 •

edited

Loading

sfrunza13 Feb 18, 2023 •

edited

Loading

sfrunza13 commented Feb 18, 2023 •

edited

Loading

sfrunza13 commented Feb 23, 2023 •

edited

Loading