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

Add support to provide "Reason" for password retrieval #16

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

ywei2017
Copy link
Contributor

It's great to see a fully featured python library. I was doing a PoC by running the tests, and found the missing of "Reason" in password retrievals, so I put in the PR.

The testing has some of the settings, and I updated the doc to help future testing.

** Note, I don't have the AIM module, so nothing changed for the AIM APIs.

Please let me know anything missing on the PR.

- Note: not tested with AIM functions.
- Create the associated safes : sample-it-dept,sample-iaadmins,sample-coolteam
- Create safe "BSA-SYS-PTT-R", and grant user "admin_bot" (see below) to the "Safe Management" permissions (for safe
Copy link
Owner

Choose a reason for hiding this comment

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

We will rename this safe with a more significant name eg: "RENAME_ME"

@safepost
Copy link
Owner

Thanks a lot for your contribution.

Let me know why you logoff on tests rather than closing the session ?
Closing the session keeps the token active and we only need to reopen an http session to the PVWA instead of performing a full logon, so for me this is suboptimal.

@gleveille-lbp gleveille-lbp merged commit bc4058e into safepost:dev Jan 25, 2024
@ywei2017
Copy link
Contributor Author

ywei2017 commented Jan 25, 2024

@safepost Sorry missed the earlier message. I was closing the session, more closely minic what a regular script would do.

  • During my testing, I had many erros (my fault), and I got the error or too many(like 300 I think) open sessions, after some investigation I realized the session wasn't closed. That was the direct reason I was calling logoff.

I also ran into an issue with the logon account index. The code uses 2, and the default is 1 (I think).

  • Then I realized that the index is at each platform level. I was thinking of wire some logic in the link_account logic. If you think that is a reasonable approach, let me know and I can submit a PR.

Another thing that confused me a bit is the xml file keywards, upper case vs lower case vs camel case. What do you think of making it case insensitive?

  • It will be a totally backward compatible change.
  • And users don't need to worry about it.

Thanks

@gleveille-lbp
Copy link
Collaborator

Hi @ywei2017

  1. Regarding the sessions not closed properly I think we have this problem : Not waiting for connection_lost() when transport closed aio-libs/aiohttp#1925
    So there's nothing we can do, I tried to add some await asyncio.sleep(0) in asyncTearDown to see if it helps closing sessions

  2. For the account index yes you can go ahead and have a try but i'm a little bit scared about performance with the scenario of getting platform infos before linking.
    Perhaps we can apply this logic for the utility function that refers explictely to logon and reconcile accounts, and let the user have a chance to specify his index (link_account) if performance is a concern.

  3. We have done several changes in the config file with the last version (0.0.30) and you can see keys are now case insensitive, and you'll have a warning message if you specify keys that are not recognized.
    Please have a try and of course if you think there's still something we can do better, we'll be happy to hear it.

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.

3 participants