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

[WFLY-17566] Deprecate legacy security attributes in resource adapter subsystem #16557

Merged
merged 2 commits into from May 10, 2023

Conversation

tadamski
Copy link
Contributor

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Feb 25, 2023
Copy link
Contributor

@bstansberry bstansberry left a comment

Choose a reason for hiding this comment

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

@tadamski Sorry it's taken so long to get to this one.

} else if (key.equals("ironjacamar.security.domain")) {
securitySettingDomain = value;
SUBSYSTEM_RA_LOGGER.legacySecurityNotSupported();
Copy link
Contributor

Choose a reason for hiding this comment

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

This method produces an OperationFailedException but it is not thrown. I don't think throwing OFE from here is correct; some other exception type is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -186,13 +182,13 @@ public void start(org.jboss.msc.service.StartContext context) throws org.jboss.m
Security security = null;
if (securitySetting != null) {
if ("".equals(securitySetting)) {
security = new SecurityImpl(null, null, false, false);
SUBSYSTEM_RA_LOGGER.legacySecurityNotSupported();
Copy link
Contributor

Choose a reason for hiding this comment

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

This method produces an OperationFailedException but it is not thrown. I don't think throwing OFE from here is correct; some other exception type is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

} else if ("application".equals(securitySetting)) {
security = new SecurityImpl(null, null, true, false);
SUBSYSTEM_RA_LOGGER.legacySecurityNotSupported();
Copy link
Contributor

Choose a reason for hiding this comment

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

This method produces an OperationFailedException but it is not thrown. I don't think throwing OFE from here is correct; some other exception type is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

} else if (securityDomain == null || securityDomain.trim().equals("")) {
return null;
if(securityDomain != null){
ConnectorLogger.SUBSYSTEM_RA_LOGGER.legacySecurityNotSupported();
Copy link
Contributor

Choose a reason for hiding this comment

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

This method produces an OperationFailedException but it is not thrown. I don't think throwing OFE from here is correct; some other exception type is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

} else {
return new PicketBoxSubjectFactory(subjectFactory.getValue());
if (securityDomain != null) {
DS_DEPLOYER_LOGGER.legacySecurityNotSupported();
Copy link
Contributor

Choose a reason for hiding this comment

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

This method produces an OperationFailedException but it is not thrown. I don't think throwing OFE from here is correct; some other exception type is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -181,13 +180,14 @@ static ModifiableDataSource from(final OperationContext operationContext, final


final String securityDomain = ModelNodeUtil.getResolvedStringIfSetOrGetDefault(operationContext, dataSourceNode, SECURITY_DOMAIN);
final boolean elytronEnabled = ModelNodeUtil.getBooleanIfSetOrGetDefault(operationContext, dataSourceNode, ELYTRON_ENABLED);
if (securityDomain != null) {
ConnectorLogger.DS_DEPLOYER_LOGGER.legacySecurityNotSupported();
Copy link
Contributor

Choose a reason for hiding this comment

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

This method produces an OperationFailedException but it is not thrown. Here an OFE is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -282,13 +282,14 @@ static ModifiableXaDataSource xaFrom(final OperationContext operationContext, fi
final String username = ModelNodeUtil.getResolvedStringIfSetOrGetDefault(operationContext, dataSourceNode, USERNAME);
final String password= ModelNodeUtil.getResolvedStringIfSetOrGetDefault(operationContext, dataSourceNode, PASSWORD);
final String securityDomain = ModelNodeUtil.getResolvedStringIfSetOrGetDefault(operationContext, dataSourceNode, SECURITY_DOMAIN);
final boolean elytronEnabled = ModelNodeUtil.getBooleanIfSetOrGetDefault(operationContext, dataSourceNode, ELYTRON_ENABLED);
if (securityDomain != null) {
ConnectorLogger.DS_DEPLOYER_LOGGER.legacySecurityNotSupported();
Copy link
Contributor

Choose a reason for hiding this comment

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

This method produces an OperationFailedException but it is not thrown. Here an OFE is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -335,7 +336,9 @@ static ModifiableXaDataSource xaFrom(final OperationContext operationContext, fi
final String recoveryUsername = ModelNodeUtil.getResolvedStringIfSetOrGetDefault(operationContext, dataSourceNode, RECOVERY_USERNAME);
final String recoveryPassword = ModelNodeUtil.getResolvedStringIfSetOrGetDefault(operationContext, dataSourceNode, RECOVERY_PASSWORD);
final String recoverySecurityDomain = ModelNodeUtil.getResolvedStringIfSetOrGetDefault(operationContext, dataSourceNode, RECOVERY_SECURITY_DOMAIN);
final boolean recoveryElytronEnabled = ModelNodeUtil.getBooleanIfSetOrGetDefault(operationContext, dataSourceNode, RECOVERY_ELYTRON_ENABLED);
if(recoverySecurityDomain != null){
ConnectorLogger.DS_DEPLOYER_LOGGER.legacySecurityNotSupported();
Copy link
Contributor

Choose a reason for hiding this comment

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

This method produces an OperationFailedException but it is not thrown. Here an OFE is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

OperationFailedException legacySecurityAttributeNotSupported(String attribute);

@Message(id = 132, value = "Legacy security is no longer supported. Please use Elytron configuration instead")
OperationFailedException legacySecurityNotSupported();
Copy link
Contributor

Choose a reason for hiding this comment

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

If you change this to OperationFailedRuntimeException it could be thrown from both an OperationStepHandler and from other contexts (like an MSC Service start method) that can properly handle a RuntimeException.

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 decided to return a String and create exceptions, which match other exceptions thrown, in the code of those classes

@tadamski
Copy link
Contributor Author

@bstansberry no problems, I have fixed the problems mentioned above

} else if (securityDomain == null || securityDomain.trim().equals("")) {
return null;
if (securityDomain != null) {
throw new DeployException(ConnectorLogger.SUBSYSTEM_RA_LOGGER.legacySecurityNotSupported());
Copy link
Contributor

Choose a reason for hiding this comment

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

@tadamski I suspect this causes test failures because securityMetadata.resolveSecurityDomain() is returning the name of the authentication context (e.g. RaOperationUtil passes that into CredentialImpl.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@bstansberry
Copy link
Contributor

@tadamski You've addressed the issues I saw, but that has resulted in test failures. I think my comment above is the reason for at least one. Some others may just mean some test deployments need updates to not reference legacy security. Others I don't know.

@tadamski tadamski force-pushed the WFLY-17566_ra branch 4 times, most recently from e3322b1 to 1c2d6ad Compare April 21, 2023 12:59
@bstansberry bstansberry merged commit c5f48f0 into wildfly:main May 10, 2023
14 checks passed
@bstansberry
Copy link
Contributor

Woohoo! Thanks @tadamski and @jamezp!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
29.Alpha1 deps-ok Dependencies have been checked, and there are no significant changes
Projects
None yet
4 participants