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.

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

feat: add samlify example #130

Closed
wants to merge 30 commits into from

Conversation

christopherpickering
Copy link

No description provided.

@MichaelDeBoey MichaelDeBoey changed the title added remix saml auth example feat: add samlify example Jan 16, 2023
Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

Please create an as small as possible example.

This means you should start from the remix template & add only the really necessary things.

We don't want to make these examples too complicated/bloated.

@christopherpickering
Copy link
Author

hmm I started with the blues stack and added a few files. Should I start with a different one, or did I change it too much? Thanks!

@MichaelDeBoey
Copy link
Member

@christopherpickering Best is to start with the __template folder of this repo as a basis and add only the bare minimum to get your example working

@christopherpickering
Copy link
Author

@MichaelDeBoey how's it looking now? You can squash when you merge?

Thanks!

machour
machour previously approved these changes Jan 20, 2023
Copy link
Collaborator

@machour machour left a comment

Choose a reason for hiding this comment

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

Can we renamed the folder to "auth-saml-ldap" ? remix is implicit in this repo 😅
(don't forget to update the sandbox link)

Also, is the git ubmodule saml-idp really needed here?

@christopherpickering
Copy link
Author

Cool, I'll rename. Yeah, the submodule is used for the dev to show the example SAML login.

@christopherpickering
Copy link
Author

Sorry that was a messy name change, but should be good now.

Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

Please also fix all ESLint errors/warnings

CC/ @machour @mcansh What would you think of renaming this to saml-ldap-auth instead?

Comment on lines +46 to +47
<!-- TODO: update this link to the path for your example: -->

Copy link
Member

@MichaelDeBoey MichaelDeBoey Jan 22, 2023

Choose a reason for hiding this comment

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

This comment should be deleted

Suggested change
<!-- TODO: update this link to the path for your example: -->

Copy link
Member

Choose a reason for hiding this comment

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

auth-saml-ldap/README.md Outdated Show resolved Hide resolved
auth-saml-ldap/app/root.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/root.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/auth/asc.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/auth/asc.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/auth/slo.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/auth/slo.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/auth/slo.tsx Outdated Show resolved Hide resolved
@christopherpickering
Copy link
Author

Thanks, I removed the submodule (now cloning the idp instead) and updated the lint.

auth-saml-ldap/app/root.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/index.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/login.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/auth/asc.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/auth/asc.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/login.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/login.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/login.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/login.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/logout.tsx Outdated Show resolved Hide resolved
@christopherpickering
Copy link
Author

christopherpickering commented Jan 23, 2023

@MichaelDeBoey thanks bro, wish I coulda been a programmer like you 😀 unfortunately, some of us never get out of kindergarten

@christopherpickering
Copy link
Author

shucks didn't mean to close it.

@MichaelDeBoey
Copy link
Member

unfortunately, some of us never get out of kindergarten

@christopherpickering No need to make these kind of comments imo
I'm just trying to make sure your code adheres to the standards we use in our code/this repo, that's all 🤷‍♂️

So if you feel offended by them: I'm sorry, 'cause that's not what I'm trying to do at all.

Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

Please also run ESLint on the files

auth-saml-ldap/app/root.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/root.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/auth/slo.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/auth/slo.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/auth/asc.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/models/user.server.ts Outdated Show resolved Hide resolved
auth-saml-ldap/app/models/user.server.ts Outdated Show resolved Hide resolved
auth-saml-ldap/app/models/user.server.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,86 @@
import type { User } from "@prisma/client";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import type { User } from "@prisma/client";
import type { Group, User } from "@prisma/client";

Copy link
Member

Choose a reason for hiding this comment

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

auth-saml-ldap/app/models/user.server.ts Outdated Show resolved Hide resolved
@christopherpickering
Copy link
Author

unfortunately, some of us never get out of kindergarten

@christopherpickering No need to make these kind of comments imo I'm just trying to make sure your code adheres to the standards we use in our code/this repo, that's all 🤷‍♂️

So if you feel offended by them: I'm sorry, 'cause that's not what I'm trying to do at all.

😁 I'm not offended, just jealous that I didn't see all that stuff you recommended when I went through it :) Wishing you could review all my code!

@MichaelDeBoey
Copy link
Member

I'm not offended

Oh OK, then I misunderstood your comment 🙈

auth-saml-ldap/app/ldap.server.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/models/user.server.ts Outdated Show resolved Hide resolved
auth-saml-ldap/app/models/user.server.ts Outdated Show resolved Hide resolved
auth-saml-ldap/app/models/user.server.ts Outdated Show resolved Hide resolved
auth-saml-ldap/app/models/user.server.ts Outdated Show resolved Hide resolved
auth-saml-ldap/app/models/user.server.ts Outdated Show resolved Hide resolved
auth-saml-ldap/app/routes/auth/slo.tsx Outdated Show resolved Hide resolved
auth-saml-ldap/app/session.server.ts Outdated Show resolved Hide resolved
auth-saml-ldap/app/ldap.server.tsx Outdated Show resolved Hide resolved
@christopherpickering
Copy link
Author

@MichaelDeBoey thanks, is there another linter you run to find more ways to improve the code, or have a better set of es rules than the default? Or have a style guide I can check out? I'm wondering how I can improve my code from the beginning. Thanks!

@christopherpickering
Copy link
Author

@MichaelDeBoey I think I've gotten everything taken care of 👍🏽

@@ -0,0 +1,21 @@
import { RemixBrowser } from "@remix-run/react";
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this file (see #159)

@@ -0,0 +1,110 @@
import { PassThrough } from "stream";
Copy link
Member

Choose a reason for hiding this comment

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

Same (see #159)

/* can't do idp initiated logout w/ cookie sessions, but can still use
this point to logout if we wanna
*/
export const action = async (request: Request) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const action = async (request: Request) => {
export const action = async ({ request }: ActionArgs) => {

@@ -0,0 +1,8 @@
import { logout } from "~/session.server";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { logout } from "~/session.server";
import type { ActionArgs } from "@remix-run/node";
import { logout } from "~/session.server";

"saml:cert": "openssl req -x509 -new -newkey rsa:2048 -nodes -subj '/C=US/ST=California/L=San Francisco/O=JankyCo/CN=Test Identity Provider' -keyout test/saml-idp/idp-private-key.pem -out test/saml-idp/idp-public-cert.pem -days 7300",
"saml:setup": "rm -rf test/saml-idp; mkdir test/saml-idp; git clone https://github.com/mcguinness/saml-idp.git test/saml-idp; cd test/saml-idp/; npm install",
"start": "remix-serve build",
"setup": "prisma generate && prisma migrate dev --name \"initial\""
Copy link
Member

Choose a reason for hiding this comment

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

Let's add typecheck script to be in line with all other examples (see #98)

Suggested change
"setup": "prisma generate && prisma migrate dev --name \"initial\""
"setup": "prisma generate && prisma migrate dev --name \"initial\"",
"typecheck": "tsc"

@MichaelDeBoey
Copy link
Member

Hi @christopherpickering!

Are you still interested in getting this one merged?

If so, please rebase onto latest main & implement remarks

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label Jun 6, 2023
@christopherpickering
Copy link
Author

@MichaelDeBoey I ended up writing an auth strategy for remix-auth. This pr can probably be closed unless you would like it added.

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants