kylecorbelli / redux-token-auth

Redux actions and reducers to integrate easily with Devise Token Auth
MIT License
154 stars 80 forks source link

Check for hasVerificationBeenAttempted #35

Closed joshudev closed 6 years ago

joshudev commented 6 years ago

Check for hasVerificationBeenAttempted before redirecting in require sign in wrapper

Fixes #30

djvs commented 6 years ago

@joshudev I'm getting this error:

Uncaught TypeError: Cannot read property 'flushGetRequests' of undefined
    at generateAuthActions (actions.js:102)
    at Object.defineProperty.value (redux-token-auth-config.js:22)
    at __webpack_require__ (bootstrap ce7b60a4d02223d3308f:19)
    at Object.<anonymous> (examinees.js:15)
    at __webpack_require__ (bootstrap ce7b60a4d02223d3308f:19)
    at Object.<anonymous> (application.js:1) [.......]

Coming from here in actions.js:

var generateAuthActions = function (config) {
    var authUrl = config.authUrl, storage = config.storage, userAttributes = config.userAttributes, userRegistrationAttributes = config.userRegistrationAttributes;
    var Storage = Boolean(storage.flushGetRequests) ? storage : AsyncLocalStorage_1.default;

"storage" isn't specified in my config as per the README.md on this repo. I see flushGetRequests refers to ... React Native async local storage? Any tips?

djvs commented 6 years ago

Sorry, subbed it out with storage: window.localStorage. Might be an upstream problem.

joshudev commented 6 years ago

@djvs - I had the same problem with master, and added the following to my config:

storage: {
  flushGetRequests: false
}

e.g:

const config = { 
  authUrl,
  userAttributes: {
    firstName: 'first_name',
    imageUrl: 'image',
  },  
  userRegistrationAttributes: {
    firstName: 'first_name',
  },  
  storage: {
    flushGetRequests: false
  }
}
joshudev commented 6 years ago

@kylecorbelli - can you see any problem with the approach in this pull request?

kylecorbelli commented 6 years ago

@joshudev can we also get a semver patch version bump?

josephecombs commented 6 years ago

is this getting merged any time soon? I'm encountering this exact problem - redirected prior to completing a successful token check.

josephecombs commented 6 years ago

when I get this working I'll add a change to the readme that includes the flushGetRequests thing

kylecorbelli commented 6 years ago

@josephecombs thanks a bunch!

josephecombs commented 6 years ago

@kylecorbelli https://github.com/kylecorbelli/redux-token-auth/pull/42

rg-najera commented 6 years ago

Although this was merged, I can see this has has potentially opened a bug surface area I can't quite pin-point. For me using master, and adding the storage property to my config, just leads to the persist gate not redirecting to the path I set up with generateRequireSignInWrapper. Was there a breaking change Im just not seeing?

Current State when loading the screen that is protected and should redirect.

reduxTokenAuth: {
    currentUser: {
      isSignedIn: false,
      isLoading: false,
      hasVerificationBeenAttempted: true,
      attributes: {}
    }
  },

I can verify Im getting the empty container on https://github.com/kylecorbelli/redux-token-auth/commit/9d83b38b6cbcd53e2448fd96c7f694f429eeaf94#diff-0f483cb752108ae79a3aed8f3e94098eR43

After authentication, I can see the screen fine.

josephecombs commented 6 years ago

I THINK I encountered behavior like this myself and this is actually a bug with devisetokenauth - see https://github.com/lynndylanhurley/devise_token_auth/issues/1053

Try not changing the token on each request.

On Mon, Aug 6, 2018 at 1:07 PM, Rodrigo Garcia notifications@github.com wrote:

Although this was merged, I can see this has has potentially opened a bug surface area I can't quite pin-point. For me using master, and adding the storage property to my config, just leads to the persist gate not redirecting to the path I set up with generateRequireSignInWrapper. Was there a breaking change Im just not seeing?

Current State when loading the screen that is protected and should redirect.

reduxTokenAuth: { currentUser: { isSignedIn: false, isLoading: false, hasVerificationBeenAttempted: true, attributes: {} } },

I can verify Im getting the empty container on 9d83b38#diff- 0f483cb752108ae79a3aed8f3e94098eR43 https://github.com/kylecorbelli/redux-token-auth/commit/9d83b38b6cbcd53e2448fd96c7f694f429eeaf94#diff-0f483cb752108ae79a3aed8f3e94098eR43

After authentication, I can see the screen fine.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kylecorbelli/redux-token-auth/pull/35#issuecomment-410836835, or mute the thread https://github.com/notifications/unsubscribe-auth/ADX2SSjrvF1SlV_XiOMeXzgDBypXgnUGks5uOKIIgaJpZM4TOMLR .

--

Cell: 585-319-1067 www.josephecombs.com

rg-najera commented 6 years ago

@josephecombs I don’t think this due to the access token not being able to be accessed. Keep in mind that a user hasn’t been authenticated yet.

Maybe I’m just not getting it. What is the premise of the change in regards to verifyToken? If the state of “hasVerificationBeenAttempted” and that is true, and the user state “isSignedIn” is false then we should be redirected to the auth screen. Which it’s not what I’m getting. Maybe it has to do with where I’m adding the gate and token verification methods.

In regards to auth token. My project needs auth to be hardened due to PCI compliance. There’s no way we can allow user token to not be changed in each request without opening up major security loopholes.

My PR, https://github.com/kylecorbelli/redux-token-auth/pull/46 takes care of verifying when an empty auth token is sent back from the server - only happens with batch requests or when the user gets click happy (i was able to reproduce very easily). I’m using my fork with and without this merge effectively. Only adds a new auth token to storage when one is returned. It is a seperate issue but thought it needed to be noted in my PR.

rg-najera commented 6 years ago

Thanks by the way. Don’t mean to be dry about it. Hoping to contribute more as i start understanding more about how these challenges are dealt with.

TheStu commented 5 years ago

I've only been playing with react for a few weeks so maybe there's something wrong with this approach, but w/r/t the issue of not being redirected, I found that I needed to replace componentWillReceiveProps in generateRequireSignInWrapper with:

public componentDidMount(): void {
  const {
    history,
    hasVerificationBeenAttempted,
    isSignedIn,
  } = this.props

  if (hasVerificationBeenAttempted && !isSignedIn) {
    history.replace(redirectPathIfNotSignedIn)
  }
}

after that everything seems to be working as expected.

csalvato commented 4 years ago

This was merged 2 years ago but I don't think it ever made it to a release. NPM is reporting that 0.19.0 is the latest version, so these changes aren't pulled in when doing npm install or yarn install. AM I incorrect here?