adding identity token as an approximate alias of password by sbdtu5498 · Pull Request #1040 · oras-project/oras

sbdtu5498 · 2023-07-31T09:23:46Z

What this PR does / why we need it:
These changes make the identity token an approximate alias of a password. The underlying implementation still depends on the password implementation, but a few cases to handle exceptions and errors have been added, thus technically speaking, it is not an alias but can be considered an approximate alias to enhance UX in terms of user's understandability.

Which issue(s) this PR fixes
Fixes #742

Please check the following list:

Does the affected code have corresponding tests, e.g. unit test, E2E test?
Does this change require a documentation update?
Does this introduce breaking changes that would require an announcement or bumping the major version?

Signed-off-by: Sanskar Bhushan <sbdtu5498@gmail.com>

codecov-commenter · 2023-07-31T14:07:36Z

Codecov Report

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

Project coverage is 81.63%. Comparing base (5608eeb) to head (976db98).

Files	Patch %	Lines
cmd/oras/internal/option/remote.go	53.84%	8 Missing and 4 partials ⚠️
cmd/oras/root/login.go	53.33%	4 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1040      +/-   ##
==========================================
- Coverage   81.97%   81.63%   -0.35%     
==========================================
  Files          83       83              
  Lines        3995     4024      +29     
==========================================
+ Hits         3275     3285      +10     
- Misses        497      509      +12     
- Partials      223      230       +7

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

TerryHowe · 2023-07-31T14:58:47Z


-// readPassword tries to read password with optional cmd prompt.
+// readPassword tries to read password and identity-token with optional cmd prompt.
 func (opts *Remote) readPassword() (err error) {


I'm fine with this, but there is a Cobra feature for mutually exclusive

Oh, thanks for the info. I will check it and get back on this one.

Please refer to https://pkg.go.dev/github.com/spf13/cobra#Command.MarkFlagsMutuallyExclusive

BTW identity-token is exclusive to both password and username

qweeah · 2023-08-01T12:36:38Z

-		notePrefix    string
+		shortUser          string
+		shortPassword      string
+		shortIdentityToken string


Should not enable shorthand for --identity-token

qweeah · 2023-08-01T12:36:52Z

 	)
 	if prefix == "" {
-		shortUser, shortPassword = "u", "p"
+		shortUser, shortPassword, shortIdentityToken = "u", "p", "i"


Should not enable shorthand for --identity-token

Sure will do it

qweeah · 2023-08-01T12:37:14Z

 	}
 	fs.StringVarP(&opts.Username, flagPrefix+"username", shortUser, "", notePrefix+"registry username")
 	fs.StringVarP(&opts.Password, flagPrefix+"password", shortPassword, "", notePrefix+"registry password or identity token")
+	fs.StringVarP(&opts.IdentityToken, flagPrefix+"identity-token", shortIdentityToken, "", notePrefix+"identity token for registry")


Should not enable shorthand for --identity-token

Suggested change

fs.StringVarP(&opts.IdentityToken, flagPrefix+"identity-token", shortIdentityToken, "", notePrefix+"identity token for registry")

fs.StringVarP(&opts.IdentityToken, flagPrefix+"identity-token", "", "", notePrefix+"identity token for registry")

qweeah · 2023-08-01T12:38:36Z

 }

-// readPassword tries to read password with optional cmd prompt.
+// readPassword tries to read password and identity-token with optional cmd prompt.


NIT: make line length less than 80

Suggested change

// readPassword tries to read password and identity-token with optional cmd prompt.

// readPassword tries to read password and identity-token with optional cmd

// prompt.

qweeah · 2023-08-01T12:41:05Z


-// readPassword tries to read password with optional cmd prompt.
+// readPassword tries to read password and identity-token with optional cmd prompt.
 func (opts *Remote) readPassword() (err error) {


Please refer to https://pkg.go.dev/github.com/spf13/cobra#Command.MarkFlagsMutuallyExclusive

qweeah · 2023-08-01T12:54:27Z


-// readPassword tries to read password with optional cmd prompt.
+// readPassword tries to read password and identity-token with optional cmd prompt.
 func (opts *Remote) readPassword() (err error) {


BTW identity-token is exclusive to both password and username

qweeah · 2023-08-01T12:57:00Z

 	Username          string
 	PasswordFromStdin bool
 	Password          string
+	IdentityToken     string


Don't need this new value, the --registry-token can be applied to Password directly. The tricky part is whether we need --registry-token-stdin? Please wait for #742 (comment) to be answered?

I see. I will remove the field and would implement identity-token-stdin instead.

shizhMSFT · 2023-08-01T15:11:17Z

 	PasswordFromStdin bool
 	Password          string
+	IdentityToken     string


Instead of having IdentityToken, we can have something like Secret and SecretFromStdin.

Oh, So should I add a field secret that will hold the Identity Token, Instead of Password?

shizhMSFT · 2023-08-01T15:13:55Z

+	if opts.IdentityToken != "" {
+		// If IdentityToken is provided, use it as the credential without a username
+		return auth.Credential{
+			Username: "",
+			Password: opts.IdentityToken,
+		}
+	} else {
+		// If IdentityToken is not provided, use the username and password as credentials
+		return auth.Credential{
+			Username: opts.Username,
+			Password: opts.Password,
+		}
+	}


This block of code should be reverted since credential.Credential() already handles everything.

Okay, will fix it.

shizhMSFT · 2023-08-01T15:14:53Z

+	// If IdentityToken is provided, use it as the credential without a username
+	if opts.IdentityToken != "" {
+		opts.Username = ""                 // Set the username to empty since it's not required when using identity token
+		opts.Password = opts.IdentityToken // Use the identity token as the password
+	} else if opts.Password == "" {


Those part of code should be reverted if we introduce opts.Secret.

Oh Okay. I will revert them. Let me know should I go with Secrets and SecretFromStdin or identity-token-stdin and password.

TerryHowe · 2023-09-06T13:35:46Z

Where is this going? Be nice to get this out.

github-actions · 2023-10-22T01:47:57Z

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 30 days.

Signed-off-by: Terry Howe <tlhowe@amazon.com>

FeynmanZhou · 2023-11-27T03:17:38Z

@sbdtu5498 Will you be able to continue to work on this PR? It has been stale for a long time and might be closed if no follow-ups on it in a few weeks.

github-actions · 2024-01-12T01:48:46Z

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions · 2024-02-28T01:46:18Z

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 30 days.

shizhMSFT · 2024-04-12T11:05:42Z

Succeeded and closed by #1294

adding identity token as an approximate alias of password

c226617

Signed-off-by: Sanskar Bhushan <sbdtu5498@gmail.com>

sbdtu5498 requested review from SteveLasker, TerryHowe, qweeah, sajayantony and shizhMSFT as code owners July 31, 2023 09:23

TerryHowe reviewed Jul 31, 2023

View reviewed changes

qweeah requested changes Aug 1, 2023

View reviewed changes

shizhMSFT reviewed Aug 1, 2023

View reviewed changes

github-actions Bot added the stale Inactive issues or pull requests label Oct 22, 2023

Merge branch 'main' into adding-identity-token-as-a-password-alias

f1ea0e5

Signed-off-by: Terry Howe <tlhowe@amazon.com>

TerryHowe removed the stale Inactive issues or pull requests label Oct 28, 2023

github-actions Bot added stale Inactive issues or pull requests and removed stale Inactive issues or pull requests labels Jan 12, 2024

github-actions Bot added the stale Inactive issues or pull requests label Feb 28, 2024

Terry Howe added 2 commits February 28, 2024 06:43

Merge branch 'main' into adding-identity-token-as-a-password-alias

d2b598a

Merge branch 'main' into adding-identity-token-as-a-password-alias

976db98

github-actions Bot removed the stale Inactive issues or pull requests label Feb 29, 2024

TerryHowe mentioned this pull request Mar 19, 2024

feat: make --identity-token an alias of --password #1294

Merged

4 tasks

shizhMSFT closed this Apr 12, 2024

	fs.StringVarP(&opts.IdentityToken, flagPrefix+"identity-token", shortIdentityToken, "", notePrefix+"identity token for registry")
	fs.StringVarP(&opts.IdentityToken, flagPrefix+"identity-token", "", "", notePrefix+"identity token for registry")

	// readPassword tries to read password and identity-token with optional cmd prompt.
	// readPassword tries to read password and identity-token with optional cmd
	// prompt.

Conversation

sbdtu5498 commented Jul 31, 2023

Uh oh!

codecov-commenter commented Jul 31, 2023 • edited by codecov Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TerryHowe commented Sep 6, 2023

Uh oh!

github-actions Bot commented Oct 22, 2023

Uh oh!

FeynmanZhou commented Nov 27, 2023

Uh oh!

github-actions Bot commented Jan 12, 2024

Uh oh!

github-actions Bot commented Feb 28, 2024

Uh oh!

shizhMSFT commented Apr 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented Jul 31, 2023 •

edited by codecov Bot

Loading