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

Two potential bugs regarding bookmarking apps? #60

Open
pmoulos opened this issue Jan 13, 2020 · 10 comments
Open

Two potential bugs regarding bookmarking apps? #60

pmoulos opened this issue Jan 13, 2020 · 10 comments
Assignees
Labels
bookmarking Problems related to bookmarking enhancement New feature or request

Comments

@pmoulos
Copy link

pmoulos commented Jan 13, 2020

Hi,

Firstly, thanks for this excellent package! It made the whole auth0 setup so much easier and also gave ideas how to use httr with other authentication services.

Now, I think I have identified, two, if not bugs, then potential enhancements:

  1. When using bookmarks, Shiny adds _state_id_ in the bookmark URL. If we then logout (through the logout_url function for example), the logout redirect URI that is created contains _state_id_. If the user immediately logs back again, the previous bookmark is restored. I am not sure whether this is a desired behaviour. What if another user logs in with other credentials? Maybe I am missing some technical details here, but from my perspective this is not desirable and I implemented a fix by simply reconstructing the redirect URI without _state_id_ in.

  2. When using the remote_url parameter (in _auth0.yml) for deployment in a remote server and enforcing local is not an option because it would complicate other things (e.g. when using Apache mod_proxy to avoid displaying the Shiny port), bookmarks are not restored. Again, I 've implemented a fix/addition for this.

I 've forked the package and implemented these fixes/additions. You can review here.

If you think it's ok I could create a PR?

Thanks again.

@jtrecenti
Copy link
Member

Hi @pmoulos , thank you so much for the fixes!

I tried to install the package and it did not install. Apparently, it has some parenthesis/curly braces missing.

I've added them and successfully installed the package, but then I tried to run a simple app (the "logout" example) and it threw an error from mapply() operation.

Could you fix these please? Thanks again!

@pmoulos
Copy link
Author

pmoulos commented Jan 14, 2020

Hi @jtrecenti,

Sorry for the typos, it initially worked but seems I missed something from the comitted version.
I believe I fixed them. Could you please check? The "logout" app is working for me.

By the way, another thing that may need clarification in the docs is that if _auth0.yml has remote_url configured, this should not end with a slash (/) as auth0 URL parsing system seems to completely ignore the whole query (found out here).

Thanks again for all the work.

@pmoulos
Copy link
Author

pmoulos commented Feb 3, 2020

Hi @jtrecenti!
Did you have the chance to review the latest changes?
Thanks!

@jtrecenti
Copy link
Member

jtrecenti commented Feb 3, 2020 via email

@Kvit
Copy link

Kvit commented Feb 13, 2020

could this be related to #62?

@pmoulos
Copy link
Author

pmoulos commented Feb 13, 2020

Hi @Kvit,
It could be. Can you check if my fork fixes this?
Thanks.

@jtrecenti jtrecenti self-assigned this Feb 13, 2020
@jtrecenti jtrecenti added this to the Bookmarking issues milestone Feb 13, 2020
@jtrecenti jtrecenti added the enhancement New feature or request label Feb 13, 2020
@Kvit
Copy link

Kvit commented Feb 13, 2020

Hi @Kvit,
It could be. Can you check if my fork fixes this?
Thanks.

AWESOME, yes, it solves the problem, thanks a ton!

@Kvit
Copy link

Kvit commented Feb 14, 2020

There is an issue though: somewhere in process URL parameters become URL-decoded and this breaks the URL. For example, try parameter like/?param=two%20words , it will become /?param=two words

@jtrecenti jtrecenti added the bookmarking Problems related to bookmarking label Apr 20, 2020
@jtrecenti
Copy link
Member

Hi @Kvit,
It could be. Can you check if my fork fixes this?
Thanks.

Hi @pmoulos, I'll try to look at this in the following two weeks. Sorry for the absence and thank you for your help

@jtrecenti
Copy link
Member

Hi folks, just one update. We've dug a lot into the bookmarking issue , which led us to this PR on shiny: rstudio/shiny#2880

We think that it would be the best way to go. We've also added a branch which fixes the bookmarking problem if the shiny PR is ever accepted. https://github.com/curso-r/auth0/tree/bookmark-gambi. We won't merge it into master until shiny maintainers consider this fix.

Until then, it is possible to use bookmarking in a limited way, adding one observer that changes the input. Unfortunately, I don't think this is a stable solution, because it would need one to parse the input types correctly (for example, a checkbox should be parsed as a boolean).

Here's a minimal example.

library(shiny)
library(auth0)


ui <- function(request) {
  fluidPage(
    textInput("txt", "Enter text"),
    verbatimTextOutput("out"),
    bookmarkButton(),
    logoutButton()
  )
}
server <- function(input, output, session) {

  ## possible workaround without shiny PR
  shiny::observe({
    new_list <- shiny::parseQueryString(session$clientData$url_search)
    # for each identified input
    lapply(seq_along(new_list), function(ii) {
      # remove extra quotation marks
      val <- list(value = gsub("\"", "", new_list[[ii]]))
      # updates the inputs
      session$sendInputMessage(names(new_list)[ii], val)
    })
  })

  output$out <- renderText({
    input$txt
  })
}

options(shiny.port = 8080)
enableBookmarking(store = "url")
shinyAppAuth0(ui, server)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bookmarking Problems related to bookmarking enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants