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

Use base64 encoding for usernames in http headers (shinyproxy specific) #123

Merged
merged 3 commits into from
Nov 4, 2024

Conversation

LDSamson
Copy link
Collaborator

@LDSamson LDSamson commented Nov 1, 2024

Closes #125. HTTP headers can only contain ascii signs, which causes issues when using names with less common names. Therefore, a solution is that user names are base64 encoded by the application serving the app (usually shinyproxy), and then decoded by clinsight once done.

…g if the result is not UTF8 (i.e. if the string was initially not encoded)
@LDSamson LDSamson force-pushed the ls_use_base64_encoded_usernames branch from 6ac735d to a22a5b5 Compare November 4, 2024 13:12
@LDSamson LDSamson marked this pull request as ready for review November 4, 2024 14:05
@jthompson-arcus
Copy link
Collaborator

Not really familiar with what's going on here. Are you encoding the values into base 64 in the deployment and this PR is just decoding/checking those values?

@jthompson-arcus
Copy link
Collaborator

Is the first test an example of a user name that would have caused issues previously?

@LDSamson
Copy link
Collaborator Author

LDSamson commented Nov 4, 2024

Yes exactly, to both of your questions. See the news description. I now ensure that all user names are base 64 encoded in shinyproxy, and then they need to be decoded in the app. This PR will do just that (decoding the user names). The provided user name in the test indeed previously caused issues.

@@ -46,6 +46,7 @@ Imports:
tools,
withr
Suggests:
base64enc,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have mixed feelings about this but since we are including base64enc in the suggests, I think decode_base64() should at the very list check if the packaged has been installed. (I have mixed feelings because it is a dependency for other packages, so I can't really think of a scenario where it wouldn't be available.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a function that will only run in production. It should be installed already since the package bslib depends on it. What other reservations do you have for adding it in suggests, other than checking in decode_base64 a check if the package is installed?

I will add the check mentioned by you in the function.

Copy link
Collaborator

@jthompson-arcus jthompson-arcus left a comment

Choose a reason for hiding this comment

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

LGTM

@jthompson-arcus jthompson-arcus merged commit 3290897 into dev Nov 4, 2024
3 checks passed
@jthompson-arcus jthompson-arcus deleted the ls_use_base64_encoded_usernames branch November 4, 2024 16:59
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.

2 participants