my attempt at allowing storage of password in Sys env

[SanderDevisscher]

fixes #20

probably not really best practice but since we are not going to use iAsset for much longer I think we just need to make some progress. Can you take a look at this PR and also #31 ? I need both to make progress with inbo/aspbo#8.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 0.00%. Comparing base (a501cb1) to head (f504bd3).

Files	Patch %	Lines
R/get_access_token.R	0.00%	28 Missing ⚠️
R/get_username.R	0.00%	5 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main     #38   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          4       5    +1     
  Lines        131     160   +29     
=====================================
- Misses       131     160   +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[PietrH]

Something like this is best practise: https://blog.r-hub.io/2024/02/28/key-advantages-of-using-keyring/

rgbif works similar to your implementation.

[PietrH]

Keep in mind that the password will be exposed to anything that can read the sys env, the unsalted md5 hash is no protection against rainbow tables. Keyring would be a much, much better option. I'm however completely taken up by other projects at the moment, so I can't help implement it for the foreseeable future.

With this in mind, do you think this risk is acceptable? If, so, the code you have written should work. If not, you'll need to look at keyring.

Another, unrelated, risk is that users of this package will store sensitive data in unencrypted outputs, like csv or excel, or in their global environment. We can't really help this except not supplying access to this data at all. But I want to emphasize that storing personal data is not something that should be taken lightly.

[PietrH]

For the record, I would it if you could rewrite using keyring. But understand if you, like me, are out of time budget.

[SanderDevisscher]

@PietrH in my honest opinion, if a widely used package like rgbif works in a similar way, why should we do more? especially since we will be using this package for a relative short time (at least until june). Also I don't have enough time to properly dive into the specifics of keyring. So if you could just approve this PR so I can continue?

[PietrH]

Because GBIF doesn't handle personal information like iAsset does.

[SanderDevisscher]

Because GBIF doesn't handle personal like iAsset does.

TRUE, I'll try my best to implement keyring

[PietrH]

I'm attending a hackathon all of next week, but if you set me as reviewer I'll get to it as soon as I can

[SanderDevisscher]

@PietrH I've implemented (or at least attempted to implement) keyring. Can you review again ?

[PietrH]

• Users still need to enter their username every time, that might get old too. If we are going trough the trouble of using a system keyring, we might as well cache the username... It's fine storing this in sys env (.Renviron) -> You can store this alongside the key using keyring::key_set(username = "my_username"), in that case, get_access_token() can drop username as an argument, much cleaner
• Let's use a custom prompt referring to iAsset to query the password the first time OPTIONAL: documentation instructs using keyring::key_set(), which is fine, but in that case we need a way to for users to receive this information if they didn't read the documentation, for example by improving the error message when they didn't set a key.
• We must not store the key value in sys env after fetching it, this defeats the point of using a keyring.
• I think we should try not to store the hashed value in the function environment if possible, as interrupting the function at this stage could leak the secret. This is only a nice to have... You could still get to this by somehow interrupting the httr call after the request is built but before it is completed, or maybe even after it is completed. So security wise this remains suboptimal anyway.

I propose the following:

• Check if a username is stored in sys env, if not, ask for one.

• Store the username in the keyring, if there isn't one stored, prompt for one. (And add it to the keyring??) --> it might be more elegant to just wrap the whole interactive prompting business into get_access_token(), so check if a key exists, if not, prompt for one and set it with key_set_with_value()

• remove the username argument from get_access_token()

• Check if the key exists, if not prompt for it with key_create() -> OR: improve upon the default error when no keyring is set:
  Error: keyring error (file-based keyring), cannot get secret: The specified item could not be found in the keychain., I believe we can check if the key exists with key_list()

• hash within the evaluation of the httr request, so: password = openssl::md5(keyring::key_get("iasset_password")), getting rid of all this:
  

  iassetR/R/get_access_token.R

  Lines 32 to 37 in 2e938d3

  	if(pwd == openssl::md5("")){
  	hash <- askpass::askpass() %>%
  	openssl::md5()
  	}else{
  	hash <- pwd
  	}

  
  -> I've already done this in 6bf2e9e

• We need to handle the situation where a user has multiple keys stored for the iasset_password service, the easy way out is to check for this situation with keyring::key_list() and just use the first one with a warning() -> errored out instead: cdc7995

• Check for keyring support -> done: 45d5e1e

---

Tell me if this is too much, or if you get stuck, let me know and I'll help. If it's me causing extra work, I might as well do some of it.

[PietrH]

I've made some changes directly to this PR while reviewing, I hope you don't mind. This isn't exactly "good reviewer" behaviour to be honest, it's better to work with code suggestions. As compensation I'll finish up with what I've started and you can review my work on Tuesday. I hope that is alright? .

My main thoughts are in the comment above, but it boils down to:

• I think we should store the username as well, and get rid of the username argument entirely
• I think it's neater and clearer to wrap the setting of the key if it's missing, OR to use a custom error if it's missing with better instructions on how to fix it. I prefer having the function work without the users having to read the documentation (since we use it as a helper quite a bit) and without an error the first time; so wrapping seems best to me.

[PietrH]

I'm having second thoughts about depreciating the username argument... I don't want to break existing scripts, or complicate the usage in github actions. So perhaps making it optional is an option, and then adding a helper that can fetch the username from the keyring...

[SanderDevisscher]

I'm having second thoughts about depreciating the username argument... I don't want to break existing scripts, or complicate the usage in github actions. So perhaps making it optional is an option, and then adding a helper that can fetch the username from the keyring...

ok for me

[PietrH]

@SanderDevisscher I'm done with my changes, can you take a look at this?

[SanderDevisscher]

@PietrH I expanded the error you get when you have more than 1 username associated with the "iasset_password" - service just so users (mainly myself) knows what to do to delete the unneeded usernames.

For the rest Its ok for me ! So whats next ? I Merge this PR ? Do you redo your review ?

[PietrH]

Nicely done!

If it's ok by you, it's ok by me. Go ahead and merge!