Appsilon / rhino

Build high quality, enterprise-grade Shiny apps at speed
https://appsilon.github.io/rhino
287 stars 24 forks source link

Support `shinymanager::secure_app()` #441

Closed muzerow closed 1 year ago

muzerow commented 1 year ago

Motivation

I try to migrate my Shiny app to Rhino which uses the authentication from shinymanager using secure_app and secure_server functions

Feature description

Currently I cannot use secure_app function on ui as it throws an error '$ operator is invalid for atomic vectors', so it would be helpful to support these functions natively (or writing a note if there is a way to use them even now)

Implementation ideas

No response

Impact

No response

Comments

No response

younessbahi commented 1 year ago

Same thing here, hope to get support with this.

sciordia commented 1 year ago

Any news?

kamilzyla commented 1 year ago

The issue boils down to the fact that shinymanager expects the result of shinymanager::secure_app() to passed directly to shiny::shinyApp(). However, Rhino does some additional handling of the ui object defined in your main.R, like attaching head tags to load app CSS and JS. Related PRs: #294, #384.

It seems likely that we'll be able to adjust how Rhino handles the UI of the app so that shinymanager can work. We'll post another update this week.

sciordia commented 1 year ago

Thank you very much Kamil. I am looking forward to testing the new release. Thank you for listening to our request.

Sergio

kamilzyla commented 1 year ago

It seems we have a fix, see the pass-request branch. We'll need to thoroughly test it as we've had some issues around this in the past (see PRs linked in my previous answer). If all goes well, the fix will land in the next release.

GeorgeOduor commented 1 year ago

any one with a sample code for this

ugurdar commented 1 year ago

I install package from pass-request branch but I couldn't implement. Can anyone give example code?

sciordia commented 1 year ago

Dear @ugurdar,

@kamilzyla told us that in principle it would be implemented in the next release of Rhino. It is true that the 'pass-request' branch was created but I don't know if it is really functional. It is true that we have been waiting for a couple of months and there is still no news.

I imagine that until the new version is released we won't have any example of how to implement Shinymanager with Rhino.

I need it too but we have to be patient, I imagine @kamilzyla and @jakubnowicki are working on it (among many other issues).

Sergio

kamilzyla commented 1 year ago

Rhino 1.3.1 has just landed on CRAN and it now supports shinymanager! See our How-to: Use shinymanager guide for details.

sciordia commented 1 year ago

Dear @kamilzyla ,

Thank you very much for including shinymanager support in Rhino.

I have already upgraded Rhino to the latest release (v1.3.1) and followed your guide to incorporate shinymanager in a simple example. When I run it, I get the following error: "environments cannot be coerced to other types".

I have uploaded my example to github: https://github.com/sciordia/Rhino_Shinymanager/

Could you take a look at it to see where the error is, please?.

Thanks again for your effort.

Bests regards, Sergio

kamilzyla commented 1 year ago

@sciordia Please take another look at the example in How-to: Use shinymanager :wink: The shinymanager$secure_app() call must wrap the entire UI of your Rhino app, e.g.:

#' @export
ui <- shinymanager$secure_app(
  kmeans_plot$ui("kmeans_plot")
)

Note that:

  1. The result of shinymanager$secure_app() is assigned to directly to ui.
  2. It's not a Shiny module, i.e. the UI definition is not wrapped in function(id) { ... } and there is no ns <- NS(id) (namespace definition). This is what the legacy_entrypoint: box_top_level setting does: instead of a Shiny module, Rhino expects a definition which can be passed to shinyApp() directly.
sciordia commented 1 year ago

Thank you @kamilzyla ,

I have updated my example with your suggestions and now it works correctly. I had to remove also the function(id) and the moduleServer definition in the server and that's it. It would be interesting to have a simple "complete" example in the Shinymanager guide (you can take my example if it helps you).

So in the main.R we can't use directly the Shiny modules but calling them from other R scripts (which work as modules).

Do you think this new "configuration" would work if I integrate it with Shiny.router?. That's the scheme I'm looking for: Rhino + Shinymanager + Shiny.router

Thanks again for your work. Sergio

kamilzyla commented 1 year ago

@sciordia The code in How-to: Use shinymanager is a complete example: after setting legacy_entrypoint in rhino.yml you can replace your whole app/main.R with the code there and it should work.

I'm not sure what you mean by "in the main.R we can't use directly the Shiny modules". See Explanation: Application structure for a general overview of how Shiny and box modules play together in Rhino.

I'm not 100% about Rhino + shinymanager + shiny.router, but I think it will work. Rhino is compatible with shiny.router so it's only a matter of compatibility of shinymanager and shiny.router.

sciordia commented 1 year ago

Thank you very much @kamilzyla. Sorry, I didn't mean that you need to change your example code. It is well understood.

I'm adapting my example to integrate all three elements (Rhino + Shinymanager + Shiny.Router), I hope it works. I just hope there are no compatibility problems between Shinymanager and Shiny.Router.

Thanks. Sergio

sciordia commented 1 year ago

Hi @kamilzyla ,

I am writing to you again about shinymanager and its integration with Rhino and Shiny.Router. I have followed your tutorial describing how to work with Rhino and Router together in the same project, nice example. The thing is that I have taken as a base your finished example and I have tried to include Shinymanager.

The first problem I encountered is the one I tried to explain in a previous message. To include Shinymanager (I think) I can't use namespace(ns) in the main.R file and that makes it difficult to add Router. I've tried to do it myself but clearly I don't have that much knowledge of the Rhinoverse yet.

I have in github the example in this link: Example_Rhino_Router_Shinymanager

Could you check the code in the main.R and home.R file to give me some hint, please?.

I get this error message: _"Warning: Error in $: $ operator is invalid for atomic vectors"_.

I hope you can help me, I've been waiting for a long time for the integration of the 3 packages.

Thanks Kamyl.

Yours sincerely, Sergio

kamilzyla commented 1 year ago

Hi @sciordia! Thanks for preparing the example; it seems to work fine, you only need to add legacy_entrypoint: box_top_level line to your rhino.yml :slightly_smiling_face:

I have also noticed that your repository doesn't have an .Rprofile file. Rhino creates one for you when you run rhino::init() and you should git-track it as well. It is responsible for setting up renv and box packages properly.

sciordia commented 1 year ago

You're absolutely right @kamilzyla , I forgot to include legacy_entrypoint. I noticed it just before you wrote to me this morning. I've updated the example on github with all the files. Now everything is running fine. I really appreciate that you have integrated Shinymanager with Rhino. Now the front office looks much more professional. If it helps you can use the example for your talk at the end of June.

Thanks again for helping me. Sergio