Skip to content

Add CredentialManager absctract class #71

Merged
sduenas merged 1 commit into
chaoss:mainfrom
alberefe:Library-Standalone-Credential-manager
Apr 16, 2026
Merged

Add CredentialManager absctract class #71
sduenas merged 1 commit into
chaoss:mainfrom
alberefe:Library-Standalone-Credential-manager

Conversation

@alberefe
Copy link
Copy Markdown
Contributor

@alberefe alberefe commented Mar 3, 2026 •
edited
Loading

Add CredentialManager base class that orchestrates login, secret fetching, field extraction, and logout
in a single call. This allows any caller (BackendCommand, Perceval, scripts) to resolve secrets without depending on Perceval's CLI layer.

  • Add CredentialManager base class for credential managers
  • Add resolve_credentials() with field extraction loop and login/logout
    lifecycle management
  • Fix BitwardenManager logout to clean up temporary appdata directory
  • Document usage in README

@alberefe alberefe force-pushed the Library-Standalone-Credential-manager branch 3 times, most recently from 984f30a to a91b6fb Compare March 8, 2026 14:44
@alberefe
Copy link
Copy Markdown
Contributor Author

alberefe commented Mar 9, 2026

@sduenas Changes for dealing with credentials with same format of perceval are up.

sduenas
sduenas requested changes Mar 12, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@sduenas sduenas left a comment

Choose a reason for hiding this comment

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

I think it would make more sense if this code is part of the credentials manager of each provider/service. I think how to access to the credentials/fields should be encapsulated on each provider's class. The behavior is different for each manager, so I think the access should be transparent for the client.

You can create an abstract class that BitwardenManager and HashicorpManager need to implement, now that we know what similarities they have. The resolve_credentials function duplicates code for both managers. I think you can make it more generic if needed, by including the specific code of each manager in a method classes have to implement, and that resolve_credentials will call.

Also, I don't think we need a factory because we only have two different classes.

@alberefe alberefe force-pushed the Library-Standalone-Credential-manager branch from a91b6fb to 1bbb0bf Compare March 16, 2026 15:43
@alberefe
Copy link
Copy Markdown
Contributor Author

alberefe commented Mar 16, 2026 •
edited
Loading

Hi.
So I've tried to implement your ideas. Kept the resolve_credentials() function in credential_manager.py creating a unified interface for all the managers.

I say I've tried cause not sure if it's what you meant. It does work by itself, tests pass and it works with perceval. Also works as standalone library and does not need cli to work.

Removed the factory, left the ABC CredentialManager.

Tell me your thoughts!

Also, i added a temp file creation by bw_manager and modified the logout cause sometimes the previous stale session would give problems, so solved it with this using info from the docs of Bitwarden CLI on how to run parallel sessions of the tool.

@alberefe alberefe requested a review from sduenas March 16, 2026 15:46
@alberefe alberefe changed the title Add resolve_credentials() for standalone credential resolution Add CredentialManager absctract class Mar 17, 2026
sduenas
sduenas requested changes Apr 9, 2026
View reviewed changes
Comment thread grimoirelab_toolkit/credential_manager/credential_manager.py Outdated
Comment thread releases/unreleased/standalone-credential-resolution.yaml Outdated
Comment thread grimoirelab_toolkit/credential_manager/credential_manager.py Outdated
@alberefe alberefe force-pushed the Library-Standalone-Credential-manager branch 2 times, most recently from 01c9bc6 to 2a6db40 Compare April 9, 2026 20:52
@alberefe alberefe requested a review from sduenas April 10, 2026 10:59
sduenas
sduenas requested changes Apr 10, 2026
View reviewed changes
Comment thread grimoirelab_toolkit/credential_manager/bw_manager.py Outdated
Comment thread README.md Outdated
@alberefe alberefe force-pushed the Library-Standalone-Credential-manager branch from 2a6db40 to ba740be Compare April 13, 2026 12:49
@alberefe alberefe requested a review from sduenas April 13, 2026 20:42
sduenas
sduenas requested changes Apr 14, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@sduenas sduenas left a comment

Choose a reason for hiding this comment

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

You forgot to update the commit message. There are references to ABC. Also KingArthur is not used anymore.

jjmerchante
jjmerchante requested changes Apr 14, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jjmerchante jjmerchante left a comment

Choose a reason for hiding this comment

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

LGTM. Just a small change

Comment thread tests/test_hc_manager.py Outdated
@alberefe alberefe force-pushed the Library-Standalone-Credential-manager branch 2 times, most recently from d12caf6 to 557819d Compare April 14, 2026 20:03
@alberefe alberefe requested a review from jjmerchante April 15, 2026 06:36
sduenas
sduenas approved these changes Apr 15, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@sduenas sduenas left a comment

Choose a reason for hiding this comment

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

LGTM

@sduenas sduenas force-pushed the Library-Standalone-Credential-manager branch from 557819d to d942e5e Compare April 15, 2026 10:45
jjmerchante
jjmerchante approved these changes Apr 15, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jjmerchante jjmerchante left a comment

Choose a reason for hiding this comment

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

LGTM

@sduenas
Copy link
Copy Markdown
Member

sduenas commented Apr 15, 2026

You forgot to update the commit message. There are references to ABC. Also KingArthur is not used anymore.

This hasn't been updated on the commit message.

@alberefe alberefe force-pushed the Library-Standalone-Credential-manager branch from d942e5e to 0a66690 Compare April 15, 2026 20:24
@alberefe
Copy link
Copy Markdown
Contributor Author

alberefe commented Apr 15, 2026

Sorry changed the commit message!

@alberefe alberefe requested review from jjmerchante and sduenas April 16, 2026 08:30
@jjmerchante
Copy link
Copy Markdown
Contributor

jjmerchante commented Apr 16, 2026

Sorry changed the commit message!

I think you wrote the commit message twice

@alberefe alberefe force-pushed the Library-Standalone-Credential-manager branch from 0a66690 to f9c0521 Compare April 16, 2026 13:02
@alberefe
Copy link
Copy Markdown
Contributor Author

alberefe commented Apr 16, 2026

Sorry changed the commit message!

I think you wrote the commit message twice

Woops, sorry. I think it's fixed!

@alberefe @jjmerchante
Add resolve_credentials() for standalone credential resolution
0584e68 
Add CredentialManager base class that orchestrates login, secret
fetching, field extraction, and logout in a single call. This allows
any caller (BackendCommand, Perceval, scripts) to resolve secrets
without depending on Perceval's CLI layer.

Signed-off-by: Alberto Ferrer Sánchez <alberefe@gmail.com>
@jjmerchante jjmerchante force-pushed the Library-Standalone-Credential-manager branch from f9c0521 to 0584e68 Compare April 16, 2026 13:27
jjmerchante
jjmerchante approved these changes Apr 16, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jjmerchante jjmerchante left a comment

Choose a reason for hiding this comment

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

LGTM

sduenas
sduenas approved these changes Apr 16, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@sduenas sduenas left a comment

Choose a reason for hiding this comment

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

LGTM

@sduenas sduenas merged commit 4120218 into chaoss:main Apr 16, 2026
6 checks passed
@sduenas
Copy link
Copy Markdown
Member

sduenas commented Apr 16, 2026

Merged. Thanks for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sduenas sduenas sduenas approved these changes

@jjmerchante jjmerchante jjmerchante approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alberefe @sduenas @jjmerchante