Skip to content

Conversation

@selalererjfrog
Copy link
Contributor

  • [ V ] All tests passed. If this feature is not already covered by the tests, I added new tests.
  • [ V ] I used npm run format for formatting the code before submitting the pull request.

@github-actions
Copy link

github-actions bot commented Jul 17, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@selalererjfrog
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

}

if (!accessToken) {
throw new Error('No access token available for authentication');

Choose a reason for hiding this comment

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

Since Basic Auth is still supported (JF_USER and JF_PASSWORD), I would throw a different error.
Additionally, Won't an error fail the step and pipeline? I don't think we want to do it in case of Basic Auth

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added support to basic auth too

Choose a reason for hiding this comment

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

I meant GitHub action supports basic auth (legacy) while evidence doesn't (API will fail) but we need to make it transparent at the actions level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the code to check if authentication is using user-password at the beginning and just issue a log message and skip evidence collection

accessToken = yield OidcUtils.exchangeOidcToken(credentials);
}
if (!accessToken) {
throw new Error('No access token available for authentication');

Choose a reason for hiding this comment

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

Since we are supporting Basic Auth with JF_USER and JF_PASSWORD we should give a meaningful error in this case.

for (const filePath of filePaths) {
try {
const fileStats = yield fs_1.promises.stat(filePath);
const fileSizeMB = fileStats.size / (1024 * 1024); // Convert bytes to MB

Choose a reason for hiding this comment

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

See comment in config API.
If we will use it in Azure for example I don't think we want to convert each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think converting makes sense. It leaves the API readable to humans

continue;
}
core.info(`Creating evidence for: ${filePath}`);
const output = yield utils_1.Utils.runCliAndGetOutput(['evd', 'create', '--sigstore-bundle', filePath]);

Choose a reason for hiding this comment

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

What happens in the case JF_PROJECT is defined? don't we want to use --project here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see that we pass project in the jf cli:

func NewCreateEvidenceCustom(serverDetails *config.ServerDetails, predicateFilePath, predicateType, markdownFilePath, key, keyId, subjectRepoPath,
	subjectSha256, sigstoreBundlePath, providerId string) evidence.Command {
	return &createEvidenceCustom{
		createEvidenceBase: createEvidenceBase{
			serverDetails:     serverDetails,
			predicateFilePath: predicateFilePath,
			predicateType:     predicateType,
			providerId:        providerId,
			markdownFilePath:  markdownFilePath,
			key:               key,
			keyId:             keyId,
		},
		subjectRepoPath:    subjectRepoPath,
		subjectSha256:      subjectSha256,
		sigstoreBundlePath: sigstoreBundlePath,
	}
}

Am I missing something?

Choose a reason for hiding this comment

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

We might need to change it but good for now

// Check if evidence collection is supported by the server
const evidenceConfig = await getEvidenceConfiguration();
if (!evidenceConfig.external_evidence_collection_supported) {
core.info('Evidence collection is not supported by this Artifactory server. Skipping evidence collection.');

Choose a reason for hiding this comment

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

I believe more specific message is needed as to why we are not supporting it (License, Basic_Auth etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is only the license that don't allow it. Changed the log message

// Check if evidence collection is supported by the server
const evidenceConfig = await getEvidenceConfiguration();
if (!evidenceConfig.external_evidence_collection_supported) {
core.info("Evidence collection is not supported by Artifactory's license type. Skipping evidence collection.");

Choose a reason for hiding this comment

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

I prefer "Evidence collection is supported only with Enterprise Plus license. Skipping evidence collection".
It avoids future questions and also similar to other messages of this type across platform.

@asafgabai asafgabai added improvement Automatically generated release notes safe to test Approve running integration tests on a pull request labels Aug 6, 2025
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Aug 6, 2025
@github-actions
Copy link

github-actions bot commented Aug 6, 2025

👍 Frogbot scanned this pull request and did not find any new security issues.


@asafgabai asafgabai merged commit 92008cd into jfrog:master Aug 6, 2025
41 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Automatically generated release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants