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 https://github.com/inbo/aspbo/issues/8.

[codecov[bot]]

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 :warning:
R/get_username.R	0.00%	5 Missing :warning:

Additional details and impacted files

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: 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]

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:

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

• [x] 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()

• [x] 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()

• [x] hash within the evaluation of the httr request, so: password = openssl::md5(keyring::key_get("iasset_password")), getting rid of all this: https://github.com/inbo/iassetR/blob/2e938d3b536fbe6666ac626b301969ba0c04e603/R/get_access_token.R#L32-L37 -> I've already done this in 6bf2e9e4f4dd69733b26a127aaa64c06348cdea9

• [x] 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: cdc799599ae67de53bc17a3952883ed41a1d01d8

• [x] Check for keyring support -> done: 45d5e1e52662577a3df0be7758395bf1ea514f15

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'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!