Skip to content

Conversation

MatKuhr
Copy link
Member

@MatKuhr MatKuhr commented May 27, 2024

Context

This PR improves the credential type detection to deal with a lowercase variant of x509_attested, which will be used by XSUAA.

  • E2E tested

Definition of Done

  • Functionality scope stated & covered
  • Tests cover the scope above
  • Error handling created / updated & covered by the tests above
  • Documentation updated
  • Release notes updated

@MatKuhr MatKuhr requested a review from newtork May 27, 2024 15:55
@MatKuhr MatKuhr added please merge Request to merge a pull request please review Request to review a pull request labels May 27, 2024
{
final String clientid = getOAuthCredentialOrThrow(String.class, "clientid");

final Option<String> exactCredentialType = getOAuthCredential(String.class, "credential-type");
Copy link
Member Author

Choose a reason for hiding this comment

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

see the impact of this refactoring. I figured since the feature is very new, SAP internal only and probably not used a lot yet we can accept this

@MatKuhr MatKuhr added do not merge Pull request must not be merged and removed please merge Request to merge a pull request labels May 27, 2024
return getCredentialType() == CredentialType.X509 ? getCertificateIdentity() : getSecretIdentity();
final String clientid = getOAuthCredentialOrThrow(String.class, "clientid");

return switch( getCredentialType() ) {
Copy link
Member Author

@MatKuhr MatKuhr May 27, 2024

Choose a reason for hiding this comment

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

In preparation of removing SecurityLibWorkarounds via #454 (see https://github.com/SAP/cloud-sdk-java-backlog/issues/428 )

Alternatively, one could apply this change standalone in this PR.

@@ -8,11 +8,11 @@

### 🔧 Compatibility Notes

-
- Using the `X509_ATTESTED` credential type now requires a version >= `3.4.0` of the [BTP Security Library](https://github.com/SAP/cloud-security-services-integration-library).
Copy link
Contributor

@newtork newtork May 29, 2024

Choose a reason for hiding this comment

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

I wonder what happens at runtime, or how "hard" this requirement is (or needs to be).

(Update to to xsuaa 3.4.0+ was in sdk 5.8.0 1month ago.)

Copy link
Member Author

Choose a reason for hiding this comment

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

We would log a warning that we don't recongise X509_ATTESTED, then try to apply BINDING_SECRET but fail with an IllegalSomethingException, because the binding doesn't contain a secret.

Copy link
Contributor

@newtork newtork Jun 4, 2024

Choose a reason for hiding this comment

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

I meant: in these lines we are referencing the enum values at runtime directly.
image

Rendering 3.4.0 the new lower bound for (transitive) security library dependency version. As I said, this may be fine. But we need to be aware of such implications.

Thinking about this, we may be required to define a default case handling in the switch statement, otherwise this code may break for future additions to the enum type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

I tried out what happens in a demo application. Turns out: This seems to be fine. I compiled the SDK with version 3.5.0 and then used it in the app with 3.3.5. Nothing bad happens 😄

CredentialType type = SecurityLibWorkarounds.getCredentialType("X509_GENERATED");
System.out.println(type); // x509
System.out.println(List.of(CredentialType.values())); // [x509, instance-secret, binding-secret]

Where I changed SecurityLibWorkarounds.getCredentialType to also include the switch statement, just to quickly run this test.

@MatKuhr MatKuhr added please merge Request to merge a pull request and removed do not merge Pull request must not be merged labels May 29, 2024
Copy link
Contributor

@newtork newtork left a comment

Choose a reason for hiding this comment

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

Please address / take note of my comment here
#453 (comment)

Looks good to me.

@MatKuhr MatKuhr mentioned this pull request Jun 4, 2024
6 tasks
@MatKuhr MatKuhr added do not merge Pull request must not be merged and removed please merge Request to merge a pull request labels Jun 4, 2024
@MatKuhr MatKuhr added please merge Request to merge a pull request and removed do not merge Pull request must not be merged labels Jun 4, 2024
@MatKuhr MatKuhr enabled auto-merge (squash) June 4, 2024 14:20
@MatKuhr MatKuhr merged commit d14d2d2 into main Jun 4, 2024
@MatKuhr MatKuhr deleted the feat/xsuaa-ztis-support branch June 4, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please merge Request to merge a pull request please review Request to review a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants