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

Skip Loginview create when credentials already exist #275

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

qcserestipy
Copy link
Contributor

Fixes #267

Check for Existing Credentials and Bypass Login View Creation

  1. Implemented a verification step to detect existing Harbor credentials in the current configuration.
  2. If valid credentials are found, the creation of a new login view is skipped.
  3. Enhances login efficiency by reusing stored credentials and reducing unnecessary prompts.

ToDo: Add base64 encoding for password in config file?

@qcserestipy
Copy link
Contributor Author

To resolve the failing tests I will introduce a mock Keyring since the container has no keyring.

@qcserestipy qcserestipy changed the title !WIP: Skip Loginview create when credentials already exist Skip Loginview create when credentials already exist Dec 4, 2024
@Vad1mo Vad1mo requested a review from bupd December 10, 2024 14:50
Copy link
Member

@bupd bupd left a comment

Choose a reason for hiding this comment

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

There should be an easy way to change the login credentials directly.

  • Changing login credentials should not need users to change the config.yaml

@qcserestipy
Copy link
Contributor Author

qcserestipy commented Dec 20, 2024

@bupd Do you thing it would come in handy to create a config subcommand that lets us manipulate the config file?
Such that a user could do something like harbor config set Credential.Password <password> to update a field in the config file. In case not what do you think would be a better idea?

@bupd
Copy link
Member

bupd commented Dec 20, 2024

Yes this one's good. Should be able to get and set the config without touching the config file.

Also it would be great if we could set the current-credential name in config from CLI

@qcserestipy
Copy link
Contributor Author

@bupd I added the config sub command and several tests for its functionality. I added some screenshots:

Listing Config Items and Setting them

Screenshot 2024-12-22 at 13 53 59

Clearing config items

Screenshot 2024-12-22 at 13 59 58

Retrieving config items

Screenshot 2024-12-22 at 13 56 46

Setting credentials and credentialname supported with --name flag

Screenshot 2024-12-22 at 13 56 01 Screenshot 2024-12-22 at 13 56 11 Screenshot 2024-12-22 at 13 54 25

@qcserestipy
Copy link
Contributor Author

@bupd There is still a sign off error in one of the commits. Unfortunately, I would have to rebase over 34 commits from main. Is there an easier way to do this?

@bupd
Copy link
Member

bupd commented Dec 22, 2024

Try squashing the commits

Vad1mo and others added 19 commits December 22, 2024 14:20
Signed-off-by: Vadim Bauer <vb@container-registry.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
* fixed the issue#224

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

* fixed the issue#224

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

* some modification in registry

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

---------

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
This reverts commit d059ef2.

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
* generate credential name

Signed-off-by: bupd <bupdprasanth@gmail.com>

* feat: add support for the password-stdin flag in login flow

Signed-off-by: karanngi <karann.git@gmail.com>

* fix deps

- fixes dependencies

Signed-off-by: bupd <bupdprasanth@gmail.com>

* return stdout for tests

Signed-off-by: bupd <bupdprasanth@gmail.com>

* update workflow

Signed-off-by: bupd <bupdprasanth@gmail.com>

---------

Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: karanngi <karann.git@gmail.com>
Co-authored-by: karanngi <karann.git@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
print test output to screen

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: ALTHAF <althafasharaf02@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
* added flags for user list cmd

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

* changed cli flows to the id to name

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

* added flags for artifact list cmd

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

---------

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
* created search sub-command

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

* fix merge conflicts and lint issue

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

---------

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>
Signed-off-by: ALTHAF <114910365+Althaf66@users.noreply.github.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
* Added configpath to loginview; Added configpath validator; Added configpath flag check to determine if given

Signed-off-by: qcserestipy <patrickeschenbach96@gmail.com>

* Added update credentials function

Signed-off-by: qcserestipy <patrickeschenbach96@gmail.com>

* Removed config file from login view; moved credential update or creation to runLogin function

Signed-off-by: qcserestipy <patrickeschenbach96@gmail.com>

* Added PersistentPreRun function to handle config init; Added data file logic that keeps track of the last active config; Made DataFile and ConfigFile Available through global pointer. Adjusted harbor client to retrieve credentials through config pointer; Added funtionality to login run to update credentials after login

Signed-off-by: qcserestipy <patrickeschenbach96@gmail.com>

* Enhanced logging for login subcommand

Signed-off-by: qcserestipy <patrickeschenbach96@gmail.com>

* Improved handling of initConfig function; introduced HARBOR_CLI_CONFIG flag

Signed-off-by: qcserestipy <patrickeschenbach96@gmail.com>

* Improved Data and Config Pointer error handling; Included Mutex functionality for Config and Data Pointer

Signed-off-by: qcserestipy <patrickeschenbach96@gmail.com>

* Added custom sync.Once to handle concurrent config environments in tests; added config tests; adapted login tests to work with new config management

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>

* Added custom sync.Once to handle concurrent config environments in tests; added config tests; adapted login tests to work with new config management

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>

---------

Signed-off-by: qcserestipy <patrickeschenbach96@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: adwait-godbole <adwaitngodbole@gmail.com>
Signed-off-by: Vadim Bauer <vb@container-registry.com>
Co-authored-by: Vadim Bauer <vb@container-registry.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
* Support yaml output for 'registry list'

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

* Use gofmt to format all code

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

* fix: Support YAML output for additional commands

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

* fix: Support YAML format on artiface and repo command

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

* fix: Implement a generic function to format output

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

* chore: fix the problem about golangci-lint

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

* AutoGenerate credential name in login (goharbor#250)

* generate credential name

Signed-off-by: bupd <bupdprasanth@gmail.com>

* feat: add support for the password-stdin flag in login flow

Signed-off-by: karanngi <karann.git@gmail.com>

* fix deps

- fixes dependencies

Signed-off-by: bupd <bupdprasanth@gmail.com>

* return stdout for tests

Signed-off-by: bupd <bupdprasanth@gmail.com>

* update workflow

Signed-off-by: bupd <bupdprasanth@gmail.com>

---------

Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: karanngi <karann.git@gmail.com>
Co-authored-by: karanngi <karann.git@gmail.com>
Signed-off-by: JianMinTang <jmtangcs@gmail.com>

* print test output to screen (goharbor#254)

print test output to screen

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

* Support table format for repo view and add some comments on repo list

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

Add more detail on repo view

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

Support table format on registry view

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

Support table format on project view

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

Fixed tags list

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

Support table format and YAML/JSON output on artifact view

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

Fixed alignment problem

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

Fixed the code format

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

* AutoGenerate credential name in login (goharbor#250)

* generate credential name

Signed-off-by: bupd <bupdprasanth@gmail.com>

* feat: add support for the password-stdin flag in login flow

Signed-off-by: karanngi <karann.git@gmail.com>

* fix deps

- fixes dependencies

Signed-off-by: bupd <bupdprasanth@gmail.com>

* return stdout for tests

Signed-off-by: bupd <bupdprasanth@gmail.com>

* update workflow

Signed-off-by: bupd <bupdprasanth@gmail.com>

---------

Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: karanngi <karann.git@gmail.com>
Co-authored-by: karanngi <karann.git@gmail.com>
Signed-off-by: JianMinTang <jmtangcs@gmail.com>

* print test output to screen (goharbor#254)

print test output to screen

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

* Support yaml output for 'registry list'

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

Use gofmt to format all code

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

fix: Support YAML output for additional commands

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

fix: Implement a generic function to format output

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

chore: fix the problem about golangci-lint

Signed-off-by: JianMinTang <jmtangcs@gmail.com>

---------

Signed-off-by: JianMinTang <jmtangcs@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: karanngi <karann.git@gmail.com>
Co-authored-by: Prasanth B <89722848+bupd@users.noreply.github.com>
Co-authored-by: karanngi <karann.git@gmail.com>
Co-authored-by: Vadim Bauer <vb@container-registry.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
* created schedule cmd

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

* Update cmd.go

Signed-off-by: ALTHAF <114910365+Althaf66@users.noreply.github.com>

* Update cmd.go

Signed-off-by: ALTHAF <114910365+Althaf66@users.noreply.github.com>

* fix lint error

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

---------

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>
Signed-off-by: ALTHAF <114910365+Althaf66@users.noreply.github.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
…p login view creation if some credentials exist

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
ed config behavior to login docs

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
… in the keyring with a specified user and service

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
qcserestipy and others added 14 commits December 22, 2024 14:20
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
…ed test for encryption functions; Updated config and login tests with mock key ring provider

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
…0.0 (goharbor#189)

Bumps [github.com/charmbracelet/bubbles](https://github.com/charmbracelet/bubbles) from 0.18.0 to 0.20.0.
- [Release notes](https://github.com/charmbracelet/bubbles/releases)
- [Changelog](https://github.com/charmbracelet/bubbles/blob/master/.goreleaser.yml)
- [Commits](charmbracelet/bubbles@v0.18.0...v0.20.0)

---
updated-dependencies:
- dependency-name: github.com/charmbracelet/bubbles
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
…r config items; added a function to update config on disk

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: Alan Tang <jmtangcs@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
* created new command label and its subcommands

Signed-off-by: Althaf66 <althafasharaf02@gmail.com>

* modified label cmd

Signed-off-by: Althaf66 <althafasharaf02@gmail.com>

* created update label cmd

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

* modified label update cmd

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

* modified label update cmd

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

* added 32 color choice for label

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

---------

Signed-off-by: Althaf66 <althafasharaf02@gmail.com>
Signed-off-by: ALTHAF <114910365+Althaf66@users.noreply.github.com>
Signed-off-by: ALTHAF <althafasharaf02@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
* modified registy update command

Signed-off-by: Althaf66 <althafasharaf02@gmail.com>

* modified registry cmd

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

* modified registry cmd

Signed-off-by: ALTHAF <althafasharaf02@gmail.com>

---------

Signed-off-by: Althaf66 <althafasharaf02@gmail.com>
Signed-off-by: ALTHAF <114910365+Althaf66@users.noreply.github.com>
Signed-off-by: ALTHAF <althafasharaf02@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
…oharbor#217)

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
…mmands

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
@bupd
Copy link
Member

bupd commented Dec 22, 2024

@qcserestipy did you also add changing between different login accounts via changing the credential name command.

Which utilizes the current credential name.

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
…r config items; added a function to update config on disk

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
@qcserestipy qcserestipy force-pushed the 267_login_with_config branch from 69ba8e0 to a50950f Compare December 22, 2024 13:26
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
@qcserestipy
Copy link
Contributor Author

@bupd I went through the rebase and it worked with a little bit of manual work.
Concerning your second question can you give me an example what you mean?

You can set the current-credential-name with the command too:

Screenshot 2024-12-22 at 14 33 19

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
…in config with default config

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
@Standing-Man
Copy link
Contributor

Hi @qcserestipy and @bupd, If the command format is as follows, it might be even better.

  • harbor context list -> list all context
  • harbor context create -> create a new context for now
  • harbor context view -> view current context
  • harbor context delete -> delete context
  • ....

This is just a suggestion I have for this command. I'm not sure if it would be more user-friendly, but I’d love to hear your thoughts!

…tch after login execution and credential update

Signed-off-by: Patrick Eschenbach <patrickeschenbach96@gmail.com>
@qcserestipy
Copy link
Contributor Author

@bupd @Standing-Man I think a discussion about the context command would be interesting. However, I think than we would have to narrow down the reason for each of the commands. When I was working on the config settings recently and introduced I already thought that the login command also takes over some of those responsibilities. In case we use context and config what is the reason for login? I also added a small part when a user uses login with new credentials or updates credentials the current cred name is also automatically updated. In this way users could use the login command to manage credentials or the config command.

What is your opinion on this all? To me it is not 100% clear right now which command should take over which functionality since there is a little bit overlap right now.

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.

Storing Plaintext in Config Files is Insecure
7 participants