PaulC91 / shinyauthr

R package with shiny authentication modules
https://paulc91.github.io/shinyauthr/
Other
428 stars 81 forks source link

Reload session when logging out? #46

Closed nicholaelaw closed 3 years ago

nicholaelaw commented 3 years ago

With current arrangement of pulling userBase for authentication, problem arises if you change the password while the user is in session. As the program does not refresh userBase, the old password would still be in effect until reloading the session.

I can think of two ways to fix this issue:

  1. Refresh userBase immediately after changeUserPassword. Or
  2. Force a refresh, or a session$reload() upon logout.

I can do 1 to get around this issue for now, but I think option 2 is also worth considering.

Edit: also, is there a way to refresh credentials?

michael-dewar commented 3 years ago

Have you tried wrapping the userBase in a reactive that depends on the login status? Then it will fetch a new set of passwords when needed.

nicholaelaw commented 3 years ago

Good idea. I'll try that as soon as it is practical.

nicholaelaw commented 3 years ago

Alright I've tried something. I wrapped userBase in a reactiveVal and it didn't work. Here's what I did:

shinyServer(function(input, output, session) {

userBase <- reactivedbReadTable(con, 'userBase') %>% data.table() %>% reactiveVal()
credentials <- callModule(shinyauthr::login, "login",
                            data = userBase(),
                            user_col = userName,
                            pwd_col = userPasswordHash,
                            sessionid_col = sessionID,
                            cookie_getter = getSessionID,
                            cookie_setter = saveSessionID,
                            sodium_hashed = TRUE,
                            log_out = reactive(logout_init()))

# other code
})

and here's what Shiny says:

Listening on http://127.0.0.1:4219
Warning: Error in : Operation not allowed without an active reactive context.
* You tried to do something that can only be done from inside a reactive consumer.
  65: <Anonymous>
Error : Operation not allowed without an active reactive context.
* You tried to do something that can only be done from inside a reactive consumer.
Warning: Error in logout_init: could not find function "logout_init"
  1: shiny::runApp

My guess, the login module is trying to get and set sessionID in a non-reactive way. Or maybe I messed up somewhere else?

michael-dewar commented 3 years ago

I'm sorry, but the approach I suggested won't work. The data=user_base object is not reactive. In order to make it reactive, the login module logic would have to be re-written.

You could add your own actionButton that triggers a session$reload().

You could try to make an observer get triggered by the change in credentials()$user_auth, but you'd have to use a reactiveVal to also make sure the user has already logged in at least once. Otherwise the app will immediately reload upon first load and get stuck in an infinite loop.

PaulC91 commented 3 years ago

I think we could probably handle this in the module logic but I'd need to look into the possibility of dealing wiith module arguments that could be reactive or static. @michael-dewar do you have experience with this? I'm thinking we could use shiny::is.reactive to test the object that is passed for reactivity and handle it differently depending on the result.

michael-dewar commented 3 years ago

I did a quick test and shiny::is.reactive seems plausible. But everything that is derived from the data object would need similar treatment. Having two cases for everything would be complicated. Maybe at the start use:

if(!is.reactive(data)) data <- reactive(data)

and then convert all of the module logic to assume data is reactive.

nicholaelaw commented 3 years ago

While investigating other issues, I am now convinced that a session reload upon logout is very important. Maybe add an onLogout argument to the logout module?

In a multi-user environment, some reactive values and calculations would stay in the session even if one user logs out and another logs in. Admittedly it was the result of my poor programming, but a simple page refresh would fix it.

PaulC91 commented 3 years ago

Agree a session reload makes sense on logout but I think this should be done from the main server function as I'm not sure we can trigger a full session reload from inside a module?

As @michael-dewar said you'd need to make sure it is not triggered on app launch when credentials()$user_auth == FALSE by default, only after the user has logged in successfully then logged out again. You could use ignoreInit = TRUE with observeEvent to do this:

observeEvent(credentials()$user_auth, ignoreInit = TRUE, {
  if (isFALSE(credentials()$user_auth)) {
    session$reload()
  }
})
michael-dewar commented 3 years ago

When I tried this I still got an infinite loop of reloads when I started the app. But that may have been due to something else in my app. However, this worked:

logged_in_at_least_once <- reactiveVal(value = FALSE)

observeEvent(credentials()$user_auth, ignoreInit = TRUE, {
  if(isTRUE(credentials()$user_auth)){
      logged_in_at_least_once(TRUE)
  } else if(isFALSE(credentials()$user_auth) && isTRUE(logged_in_at_least_once())){
      session$reload()
  }
})

Unfortunately the login UI flashes because it appears after the logout and then disappears at the start of the reload.

michael-dewar commented 3 years ago

Okay, this is really hacky, but if you add removeUI(paste0("#",NS("login", "panel"))) inside the observeEvent in my previous post, then the login panel gets removed entirely and so it won't flash on reload. Here "login" is the id you use when you call the login module, and "panel" is the internal id for the login panel UI.

nicholaelaw commented 3 years ago

Just tried your solution. A little bit hacky, but it works.

PaulC91 commented 3 years ago

I think in this case if you actually trigger the session reload by the logout button itself it will initiate the reload before the logout process is triggered inside the login module. This will avoid the problem of ensuring a successful login has already taken place and you won't have to hide the login panel etc.

Add this at the top of your server function:

observeEvent(logout_init(), {
  session$reload()
})
nicholaelaw commented 3 years ago

Hmm. I actually thought about attaching an observeEvent to the logout button, but I was afraid of breaking something else. I really should start learning about modules.

So I think this is a suitable solution to the problem, should I close?

michael-dewar commented 3 years ago

I just made a pull request in which I added an option to force reload the app on logout. What do you think?