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

Feature: Offer logout option in auth provider #6326

Conversation

andrico1234
Copy link
Contributor

Closes: #5882

Handles additional options to the checkError's Promise.reject.

redirect-on-error.mov

Notes:

  • Do we want to rename the useLogoutIfAccessDenied, since it can now sometimes not log the user out if access is denied.
  • Should we move the redirect building logic out to a reusable function, since it’s very similar to the one used in useLogout.
  • Do we also want to add similar logic to the checkAuth function?

@andrico1234
Copy link
Contributor Author

@djhi @fzaninotto hey folks! wondering if there's any update for this ticket? It's good for a review

Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Thanks!

docs/Authentication.md Outdated Show resolved Hide resolved
packages/ra-core/src/auth/useLogoutIfAccessDenied.ts Outdated Show resolved Hide resolved
@andrico1234
Copy link
Contributor Author

I've resolved the requested changes, @djhi

Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

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

Nice!

docs/Authentication.md Outdated Show resolved Hide resolved
docs/Authentication.md Outdated Show resolved Hide resolved
docs/Authentication.md Outdated Show resolved Hide resolved
packages/ra-core/src/auth/useLogoutIfAccessDenied.spec.tsx Outdated Show resolved Hide resolved
packages/ra-core/src/auth/useLogoutIfAccessDenied.ts Outdated Show resolved Hide resolved
@andrico1234
Copy link
Contributor Author

@djhi @fzaninotto

I'd like to get a Cypress test added for this behaviour, but can't figure out an appropriate way to do it.

Is there a way to monkey patch in a slightly modified authProvider? If not, I could:

  • create a really basic RA-app, but that seems like overkill.
  • create a new resource that specifically logs the user when clicking through to it (but we'd still need to alter the authProvider)

I added a unit test, but it didn't catch the problem when I called history.push(redirectTo) instead of history.push(newLocation).

What do you guys think?

Other than that, i've resolved the rest of the comments

@andrico1234
Copy link
Contributor Author

Note: I added a notification if a user was redirected but not logged out. I added a french translation that was made via google translate, since I don't speak French! Please let me know what you'd like the message to be 😄

Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

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

Awesome!

packages/ra-core/src/auth/useLogoutIfAccessDenied.ts Outdated Show resolved Hide resolved
packages/ra-language-french/src/index.ts Outdated Show resolved Hide resolved
@fzaninotto fzaninotto merged commit 15b54dc into marmelab:next Jul 7, 2021
@fzaninotto
Copy link
Member

Thanks a lot!

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