Skip to content

Secure entry and storage for SharePoint creds#7

Open
brendan-newlon wants to merge 2 commits intoLukasK13:masterfrom
brendan-newlon:brendan-newlon-patch-secure-credentials
Open

Secure entry and storage for SharePoint creds#7
brendan-newlon wants to merge 2 commits intoLukasK13:masterfrom
brendan-newlon:brendan-newlon-patch-secure-credentials

Conversation

@brendan-newlon
Copy link
Copy Markdown

I added an option to enter Username and Password securely and store both in the system key store when a credential isn't provided.

  1. more secure than entering a password as an argument.
  2. convenient for successive API calls with the same credentials.

I also added a function sp_clearKeys() to delete those saved keys from the system key store to allow connecting with different credentials from the same system account.

Added option to enter Username and Password securely and store in system key store.
Added function to delete those saved keys from system key store.
Comment thread R/sharepoint_driver.R
Username = credentials$Username # save username
Password = credentials$Password # save password
}
}
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sorry, this closing curly brace should be removed.

Comment thread R/sharepoint_driver.R
# Last chance to enter Username
if (is.null(Username)) { # No Username is given
if ("sharepointr_username" %in% key_list("sharepointr_username")$service) { # Check system key store
Username <- key_get("sharepointr_username")
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
Username <- key_get("sharepointr_username")
Username <- keyring::key_get("sharepointr_username")

Comment thread R/sharepoint_driver.R
Username <- key_get("sharepointr_username")
} else { # Username not in key store
Username <- rstudioapi::askForSecret("Enter SharePoint Username")
key_set_with_value("sharepointr_username", password = Username)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
key_set_with_value("sharepointr_username", password = Username)
keyring::key_set_with_value("sharepointr_username", password = Username)

Comment thread R/sharepoint_driver.R
# Last chance to enter Password
if (is.null(Password)) { # No Password is given
if ("sharepointr_password" %in% key_list("sharepointr_password")$service) { # Check system key store
Password <- key_get("sharepointr_password")
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
Password <- key_get("sharepointr_password")
Password <- keyring::key_get("sharepointr_password")

Comment thread R/sharepoint_driver.R
Password <- key_get("sharepointr_password")
} else { # Password not in key store
Password <- rstudioapi::askForSecret("Enter SharePoint Password")
key_set_with_value("sharepointr_password", password = Password)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
key_set_with_value("sharepointr_password", password = Password)
keyring::key_set_with_value("sharepointr_password", password = Password)

Comment thread R/sharepoint_driver.R
if ("sharepointr_username" %in% key_list("sharepointr_username")$service) { # Check system key store
Username <- key_get("sharepointr_username")
} else { # Username not in key store
Username <- rstudioapi::askForSecret("Enter SharePoint Username")
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What happens during non interactive session?

Comment thread R/sharepoint_driver.R
if ("sharepointr_password" %in% key_list("sharepointr_password")$service) { # Check system key store
Password <- key_get("sharepointr_password")
} else { # Password not in key store
Password <- rstudioapi::askForSecret("Enter SharePoint Password")
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What happens during non interactive session?

Comment thread R/sharepoint_driver.R
#' sp_clearKeys()
sp_clearKeys <- function() {
# Delete the Username if stored in system key store.
if ("sharepointr_username" %in% key_list("sharepointr_username")$service) {key_delete("sharepointr_username")}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
if ("sharepointr_username" %in% key_list("sharepointr_username")$service) {key_delete("sharepointr_username")}
if ("sharepointr_username" %in% keyring::key_list("sharepointr_username")$service) {keyring::key_delete("sharepointr_username")}

Comment thread R/sharepoint_driver.R
# Delete the Username if stored in system key store.
if ("sharepointr_username" %in% key_list("sharepointr_username")$service) {key_delete("sharepointr_username")}
# Delete the Password if stored in system key store.
if ("sharepointr_password" %in% key_list("sharepointr_password")$service) {key_delete("sharepointr_password")}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
if ("sharepointr_password" %in% key_list("sharepointr_password")$service) {key_delete("sharepointr_password")}
if ("sharepointr_password" %in% keyring::key_list("sharepointr_password")$service) {keyring::key_delete("sharepointr_password")}

@LukasK13
Copy link
Copy Markdown
Owner

Hey @brendan-newlon
Thank you very much for your idea and the related pull request.
I think this is a great idea and should be included. However, I am missing two points:

  1. What happens, if rstudioapi::askForSecret("Enter SharePoint Username") is called during a non interactive session?
  2. Please include keyring:: in your calls for methods from the package keyring (see my review)

Fix dependencies
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