PaulC91 / shinyauthr

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

added cookie-based authentication #41

Closed michael-dewar closed 3 years ago

michael-dewar commented 3 years ago

This adds cookie-based authentication. If the user has already logged in recently, then this module will detect the browser cookie and automatically re-login during subsequent visits.

This is based on a gist by calligross and an earlier pull request. But that PR is two years old and out of sync with the current branch.

PaulC91 commented 3 years ago

Hi Michael,

Great job, working well on my end.

A nice addition to this would be to ensure the login box does not have the chance to appear if a cookie login is successful. When I tested it appears briefly before moving through to the main app. I think this should be achievable with a uiOutput in the UI function and then only rendering the login wellpanel via the server function if there is no cookie login.

Also, perhaps updating the login time every time the user logs in (including a cookie login) so that the cookie expiry only takes effect if they have not visited the app after n days would be better? I think this is the typical behavior of other authentication systems I use.

Let me know your thoughts.

Thanks, Paul

michael-dewar commented 3 years ago

Thanks. These are great ideas. I’ll incorporate them on Monday.

michael-dewar commented 3 years ago

I made the changes. If the app tries to show things only when credentials()$user_auth is false, then those ui elements will briefly flash before the cookie gets checked. The best way to handle it is for the app to pass the ui elements as additional_ui to the login module. But if the user really wants/needs to manage it themself, then I have returned credentials()$cookie_already_checked. But I'm not sure if this is a great idea to expose something so granular. What do you think?

PaulC91 commented 3 years ago

Hey Michael,

I'm not 100% sure what you mean but I just tested again with your new changes and it seems moving the cookie checking code ("possibility 2: ...") above the login button code and the following observer solves the issue of the login box showing before a cookie login occurs.

  shiny::observe({
    if (credentials$user_auth){
      shinyjs::hide(id = "panel")
    } else if (credentials$cookie_already_checked){
      shinyjs::show(id = "panel")
    }
  })

Let me know if you get the same results on your end.

Thanks, Paul

michael-dewar commented 3 years ago

It seems to be working fine for me on this end.

PaulC91 commented 3 years ago

Hi @michael-dewar,

On further testing it seems the cookie login only works during a single app session. For example, if I run the example app then refresh the page the cookie login works. But if I kill the app and launch it again there is no cookie login and I need to login again.

Is this the intended behavior?

Thanks, Paul

michael-dewar commented 3 years ago

The example app in inst/shiny-examples/shinyauthr_example/app.R only uses an in-memory SQLite database. I did this because I wasn't really sure how/where a database file would be saved when the user was running the example from the package in the computer.

The example app from the README.md must be copy-and-pasted to a new file on the user's computer. Both the app and the user can easily find the file that gets created. Because this file persists, it will last across sessions.

Do you know how to handle the database file saving for the app at inst/shiny-examples/shinyauthr_example/app.R?

nicholaelaw commented 3 years ago

Forgive me but is there a time table for merging this PR? Is there any problem besides the issue of cookie-based auth not working perfectly with the builtin example?

PaulC91 commented 3 years ago

Hi both. Sorry for my lack of activity, life has been busy lately.

@michael-dewar I forgot the example used an in memory DB! That explains my question and I think that's fine for the example. Is everything else good to go on your end?

@nicholaelaw I'll look to have this merged by this weekend.

nicholaelaw commented 3 years ago

👍👍👍

michael-dewar commented 3 years ago

Everything looks good to me

yogat3ch commented 2 years ago

Hey @PaulC91 & @michael-dewar , Thanks for taking the time to add this cookie functionality! I'm currently attempting to implement it, but I'm finding that the implementation is relatively clunky because the cookie checking code is bundled with the loginUI. I'm showing a non-closeable modal with the loginUI, to prevent further use of the app without authentication, but given this is also where the logging and checking of cookies occurs, it forces this modal to load every time. When a cookie is present, it closes it seconds later - so there's a confusing blip of the modal popup when a cookie is detected.

Can we separate and export functions for the various steps involved in cookie checking such that they can be embedded in different layers of the UI?

js_cookie_to_r_code, jscookie_script, and perhaps adding a function for explicitly running shinyjs.getcookie() and binding the result? I also noticed that when a cookie is detected by shinyjs.getcookie, it binds "true" rather than the cookie hash itself. Can this bind the cookie hash instead, such that the input$jscookie can be checked against the sessionid database before showing the login modal?

It might also be a good idea to have js_cookie_to_r_code just return the JS as is, rather than wrapping it in head and script tags, such that the user can embed it wherever they see fit.