maxmantz / redux-oidc

A package for managing OpenID Connect authentication in ReactJS / Redux apps
MIT License
400 stars 111 forks source link

Oidc base component #59

Closed PeterKottas closed 7 years ago

PeterKottas commented 7 years ago

Hey Max, hope you are well!

While working with this lib I realized I need easy access to a callback triggered when user logs in. While my previous issue provided a bit of a workaround, I was not too happy with that. Now I found even more places that would definitely benefit from having this built-in. So my proposal.

Create a OidcComponentBase that extends React.Component. It adds a function "userLoggedIn" passes user as a parameter. You would place your ajax stuff that requires access token into this function. I intend yo use react context just like in my guestbell/forms. Therefore one context creator. OidcProvider is candidate for placing this code as it's already there and serves as a "container" if you will. It would discourage people from inject oidc redux state where it don't belong. It would also discourage writing something like

    public componentWillReceiveProps(newProps: LayoutProps) {
        if (!this.props.oidc.user && newProps.oidc.user) {
            // all authenticated requests go here
        }
    }

By extending OidcComponentBase, instead of this, you would simply write

    public userLoggedIn(user: ApplicationUser) {
        // all authenticated requests go here
    }

And the need to inject oidc state disappears making the code easier to read and maintain.

I am more than happy to implement this (I would anyways), my question is if you would be interested in having this added to your library.

What do you think?

PeterKottas commented 7 years ago

To see the point even clearer, note that I am using redux-axios-middleware therefore I really don't need to inject oidc state. I simply need to know when it's safe to make the request. I could also add state with "isLoggedIn:boolean" to the base component.

PeterKottas commented 7 years ago

Functionality is almost done on my fork tests are missing as I want to discuss first before getting down to that.

sveinpg commented 7 years ago

Does https://github.com/mjrussell/redux-auth-wrapper solve your problem?

PeterKottas commented 7 years ago

Thanks @sveinpg . Interesting lib. I must admit I was looking at that for a while to really get what and how it provides. It seems to me it might be a bit of a overkill. I really just need callback when user logs in. Code I've added to this library is probably 30 lines total. I am not sure I want to plugin and setup whole new library for the purpose of that. It seems it's providing other things as well. But I am not sure I care about those that much. Biggest drawback for me is it seems there's quite a lot of config to make that simple callback happen.

maxmantz commented 7 years ago

Hi again @PeterKottas. From your description I cannot fully understand what you are trying to do :).

However there are two ways you could make a userLoggedIn funcitonality:

  1. Listen to the USER_FOUND action type. This is dispatched by this library when a new user has been found (either the user logged in or the access token ahs been renewed).
  2. Have the successCallback prop of the callback component do something specific once the user logs in.
PeterKottas commented 7 years ago

Think about a component that just needs to call some function when user logs in. This component should be decoupled from user store as you don't really care about it, but it still needs to know when a user loggedIn. That's it really.

  1. Listening to action won't work cause this needs to be in a reducer, we want this to trigger something in component.
  2. There can be hundreds of pages and only the one that is currently rendered needs to perform an action. I can't call all actions on all pages from callback component. That wouldn't make sense.

Easiest way to think about it is in terms of existing react life-cycle functions.

What would you do if you didn't have componentDidLoad? You would probably hack your way around it and maybe put timeout in constructor or something. And repeat this everywhere. In a same way, I want to avoid any hacks, and just add userLoggedIn "life-cycle" method that get's called automatically once user logs in for a given component.

Hope it makes sense now. Looking at that fork of mine will make it clear. It's literally 30 lines of code. I'll create a PR once we settle on it's use cases.

sveinpg commented 7 years ago

@maxmantz suggestions should cover most cases. Otherwise I would simply create a HOC (Higher order component) and connect that to the desired components.

PeterKottas commented 7 years ago

@sveinpg Yes, HOC might be another way to go. I come from OOP background so base component seemed like more natural choice for me.

It simply registers all baseComponents within react context and then calls desired method from OidcProvider once a new user is acquired.

Plus I am slightly terrified of writing HOC in typescript as it seems there's a log of types magic going on there. Will read into it now though as it can be generally useful. While I like react context, they say it might change in next release of react so this might be safer.

PeterKottas commented 7 years ago

While one part of me still thinks this has some value to it. I think I'll just take the path of least resistance and go for an easier approach. This is what I ended up with and I am pretty happy with that tbh.

export function loggedInHoc<P>(
    Comp: React.ComponentClass<P>,
    loading: JSX.Element
): React.ComponentClass<P> {
    class WrappedComponent extends React.Component<
        P & { oidc: OidcStore.State }, void> {
        public render() {
            return this.props.oidc.user ? <Comp {...this.props}/> : loading;
        }
    }

    return connect<{ oidc: OidcStore.State }, {}, any>((state: ApplicationState) => ({ oidc: state.oidc }), {})(WrappedComponent)
}

export default loggedInHoc;

You would use it like this:

loggedInHoc(Subscriptions, <p>Waiting to log in</p>)

Where Subscriptions is a react component.