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

Add openidconnect authentication demo #1500

Merged
merged 15 commits into from
Oct 20, 2023
Merged

Conversation

Stygmates
Copy link
Contributor

@Stygmates Stygmates commented Sep 27, 2023

This is a follow up on #1152, this example shows how to implement authentication using OpenID connect.

I am not an expert on openid/dioxus so I'll require a thorough review if possible!

I also feel like we need a dioxus-auth module for openid/oauth2 to make it easier for anyone to use it, I'm open to help implement it if we have an idea on what the API should look like.
Edit: Happy 1500th pull request!

@Stygmates
Copy link
Contributor Author

Most of the tests fail because the required env variables are not set, the ones that are needed are in the readme.

Copy link
Member

@ealmloff ealmloff left a comment

Choose a reason for hiding this comment

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

Overall this looks amazing! Auth examples are some of the most complex (but very useful) examples we need for Dioxus, thanks for putting this together! There are a few places in the code that could cause issues in the future or could be simplified.

Most of the tests fail because the required env variables are not set, the ones that are needed are in the readme.

Could you exclude/skip the test in the CI workflows?

@Stygmates
Copy link
Contributor Author

Thanks for the feedback, I'll have a look at it when I can!

@ealmloff ealmloff added enhancement New feature or request example Update examples labels Oct 14, 2023
@Stygmates
Copy link
Contributor Author

Stygmates commented Oct 16, 2023

Overall this looks amazing! Auth examples are some of the most complex (but very useful) examples we need for Dioxus, thanks for putting this together! There are a few places in the code that could cause issues in the future or could be simplified.

You are welcome, I also needed it for myself and my company, so it's also good for me to get a code review from someone who knows dioxus :).

I also rewrote some of the parts to try to make it simpler/reflect the authentication flow better, so I'm open to feedback to make it simpler.
The one thing I was thinking about was to pass fermi atoms like the client as a borrowed prop to get rid of the match pattern in the inner components, would it be a good idea?

Most of the tests fail because the required env variables are not set, the ones that are needed are in the readme.

Could you exclude/skip the test in the CI workflows?

I didn't need to exclude the example from the CI workflow in the end.

Also, I'm still getting this warning when running dx build and I'm not sure what to do with it:
[WARN] failed to parse `name` custom section Invalid name type (at offset 99523911)

@ealmloff
Copy link
Member

Also, I'm still getting this warning when running dx build and I'm not sure what to do with it: [WARN] failed to parse `name` custom section Invalid name type (at offset 99523911)

I think that is an artifact from wasm-bindgen that we currently don't strip. It is safe to ignore for now.

@ealmloff
Copy link
Member

The one thing I was thinking about was to pass fermi atoms like the client as a borrowed prop to get rid of the match pattern in the inner components, would it be a good idea?

That sounds like a good idea. There is a lot of duplicate pattern matching between components.

You could try to use borrowed props but you might run into some issues with temporary lifetimes since you would be borrowing from a Ref. If you can't borrow the state, you could clone it before passing it as props. It seems like the objects are relatively small so it shouldn't effect performance too much. (and you save some work matching in child components)

@Stygmates
Copy link
Contributor Author

Stygmates commented Oct 17, 2023

@ealmloff Could you maybe please chime in on the related issue if you have something to add (or if I wasn't clear with something) 🙏

@ealmloff
Copy link
Member

You can implement PartialEq manually (without the #[derive] macro) on the props struct that uses Client. If you know that you never change client you could implement PartialEq as true or you could get some unique id from the Client and compare the props by that id

@Stygmates
Copy link
Contributor Author

Stygmates commented Oct 18, 2023

You can implement PartialEq manually (without the #[derive] macro) on the props struct that uses Client. If you know that you never change client you could implement PartialEq as true or you could get some unique id from the Client and compare the props by that id

Oh good idea, I think I prefer this solution, cloning the Client is probably less expensive than instantiating it, I'll use the ClientId to implement PartialEq, thanks!

@Stygmates Stygmates requested a review from ealmloff October 20, 2023 14:08
@Stygmates
Copy link
Contributor Author

Stygmates commented Oct 20, 2023

How would I pass the Client as a prop to the login component through the router? Is the only solution to load it before we instantiate the router itself? Maybe the best solution would be to have two different routers depending on whether the client is initalized or not.

Edit: Maybe the best way would be to keep using fermi for the components that are direct children to the header and use props for the descendents of these children

@ealmloff
Copy link
Member

How would I pass the Client as a prop to the login component through the router? Is the only solution to load it before we instantiate the router itself? Maybe the best solution would be to have two different routers depending on whether the client is initalized or not.

Edit: Maybe the best way would be to keep using fermi for the components that are direct children to the header and use props for the descendents of these children

You need to either use the context API or Fermi. You can use the context API or Fermi to get the router within each route you need it in and then use props to pass it to the children of the route once you get the state you need out of the client

@Stygmates
Copy link
Contributor Author

How would I pass the Client as a prop to the login component through the router? Is the only solution to load it before we instantiate the router itself? Maybe the best solution would be to have two different routers depending on whether the client is initalized or not.
Edit: Maybe the best way would be to keep using fermi for the components that are direct children to the header and use props for the descendents of these children

You need to either use the context API or Fermi. You can use the context API or Fermi to get the router within each route you need it in and then use props to pass it to the children of the route once you get the state you need out of the client

That's what I thought, thanks! I guess this PR is up for review then. If you see anything else that should be corrected I'm open for feedback, otherwise I think It's good enough to be merged.

Copy link
Member

@ealmloff ealmloff left a comment

Choose a reason for hiding this comment

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

Thank you!

@ealmloff ealmloff merged commit b836851 into DioxusLabs:master Oct 20, 2023
10 checks passed
@jkelleyrtp jkelleyrtp added this to the 0.5 Release milestone Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request example Update examples
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants