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.

Sign up for GitHub

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

Jump to bottom

Add messages and tests for DatabaseIdentityStore #1410

Merged
merged 1 commit into from
Jan 5, 2018
Merged

Add messages and tests for DatabaseIdentityStore #1410

merged 1 commit into from
Jan 5, 2018

Conversation

kristip17
Copy link
Contributor

Add error and warning messages for DatabaseIdentityStore. Add additional FAT tests.

@kristip17 kristip17 self-assigned this Dec 28, 2017
@kristip17 kristip17 force-pushed the AddMessagesIdentityStore branch 4 times, most recently from 9ecd0c7 to 2cdae48 Compare January 3, 2018 18:24
@kristip17 kristip17 changed the base branch from master to integration January 3, 2018 20:43
@kristip17
Copy link
Contributor Author

#build

@LibbyBot
Copy link

LibbyBot commented Jan 3, 2018

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_bBqH4PC-Eee7E8dg7u8gzA

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

LibbyBot commented Jan 4, 2018

The build kristip17-1410-20180103-2053
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_bBqH4PC-Eee7E8dg7u8gzA
completed successfully!

@kristip17 kristip17 requested review from jvanhill and ryanesch January 4, 2018 14:07
@aguibert
Copy link
Contributor

aguibert commented Jan 4, 2018

#run-libby

@LibbyBot
Copy link

LibbyBot commented Jan 4, 2018

Code analysis and actions

DO NOT DELETE THIS COMMENT.

WARNING: This pull request contains binary files, which should be managed in a Maven repository.

  • 3 product code files were changed.

  • Please describe in a separate comment how you tested your changes.

  • 11 FAT files were changed, added, or removed.

  • Check that the build did not break the affected FAT suite(s).

  • 1 messages files were changed and need an L2 review.

  • @OpenLiberty/message-reviewer Please review.

  • dev/com.ibm.ws.security.javaeesec/resources/com/ibm/ws/security/javaeesec/internal/resources/JavaEESecMessages.nlsprops

  • 1 NLS files were changed and need an ID review.

  • @OpenLiberty/message-reviewer Please review.

  • dev/com.ibm.ws.security.javaeesec/resources/com/ibm/ws/security/javaeesec/internal/resources/JavaEESecMessages.nlsprops

jvanhill
jvanhill reviewed Jan 4, 2018
View reviewed changes
...ecurity.javaeesec/src/com/ibm/ws/security/javaeesec/identitystore/DatabaseIdentityStore.java Outdated
if (tc.isEventEnabled()) {
Tr.event(tc, "Exception validating caller: " + caller, e);
}
Tr.error(tc, "JAVAEESEC_WARNING_GEN_DB", new Object[] { caller, idStoreDefinition.getCallerQuery(), e });
Copy link
Contributor

Choose a reason for hiding this comment

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

Tr.warning instead of Tr.error

jvanhill
jvanhill reviewed Jan 4, 2018
View reviewed changes
...ecurity.javaeesec/src/com/ibm/ws/security/javaeesec/identitystore/DatabaseIdentityStore.java Outdated
@@ -255,10 +255,17 @@ public CredentialValidationResult validate(Credential credential) {

private ResultSet runQuery(PreparedStatement prep, String caller) throws SQLException {
long startTime = System.currentTimeMillis();
ResultSet result = prep.executeQuery();
long endTime = System.currentTimeMillis();
Copy link
Contributor

Choose a reason for hiding this comment

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

Move system call into debug / trace if block.

jvanhill
jvanhill reviewed Jan 4, 2018
View reviewed changes
...c_fat/fat/src/com/ibm/ws/security/javaeesec/fat/HttpAuthenticationMechanismDBNoUserTest.java Outdated
import componenttest.topology.impl.LibertyServer;
import componenttest.topology.impl.LibertyServerFactory;

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong copyright. If you copied find where this might have come from and fix that one too.

jvanhill
jvanhill reviewed Jan 4, 2018
View reviewed changes
...vaeesec_fat/fat/src/com/ibm/ws/security/javaeesec/fat/HttpAuthenticationMechanismDBTest.java Outdated
@Mode(TestMode.LITE)
@Test
public void testJaspiAnnotatedDBBasicNullUserPwd() throws Exception {
// Log.info(logClass, getCurrentTestName(), "-----Entering " + getCurrentTestName());
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove commented code

jvanhill
jvanhill suggested changes Jan 4, 2018
View reviewed changes
Copy link
Contributor

@jvanhill jvanhill left a comment

Choose a reason for hiding this comment

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

See review comments for findings.

@kristip17 kristip17 force-pushed the AddMessagesIdentityStore branch 2 times, most recently from 28a2933 to 44ab0a6 Compare January 4, 2018 18:05
@kristip17 kristip17 force-pushed the AddMessagesIdentityStore branch from 44ab0a6 to 0a0fd76 Compare January 4, 2018 19:16
helyarp
helyarp reviewed Jan 4, 2018
View reviewed changes
Copy link

@helyarp helyarp left a comment

Choose a reason for hiding this comment

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

Hi, we are supposed to use the "the {variable} noun" format unless the "noun {variable}" format is already used for the component's messages. Also, we are supposed to use "error" instead of "exception," although I'm okay with "exception" if you want to keep it in the messages. I've tried to simplify the message text. If my suggested changes won't look good, then use text that you think is best. I'll approve after adding my comments.

Suggested changes:

JAVAEESEC_WARNING_GEN_DB=CWWKS1918W: The {1} query to get the {0} caller failed on the DatabaseIdentityStore with an error: {2}
JAVAEESEC_WARNING_GEN_DB.explanation=The search for the provided caller failed with an error.
JAVAEESEC_WARNING_GEN_DB.useraction=Review the provided error.

JAVAEESEC_WARNING_EXCEPTION_ON_GROUPS=CWWKS1919W: The {1} query to get the groups for the {0} caller failed on the IdentityStore. The partial list of groups is {2}. The error is {3}.
JAVAEESEC_WARNING_EXCEPTION_ON_GROUPS.explanation=The search for groups for the caller failed with an error.
JAVAEESEC_WARNING_EXCEPTION_ON_GROUPS.useraction=Review the error. A partial list of groups is returned.

JAVAEESEC_WARNING_NO_PWD=CWWKS1923W: The {1} query did not return a password for the {0} caller on the DatabaseIdentityStore.
JAVAEESEC_WARNING_NO_PWD.explanation=The query did not return a password for the provided caller query.
JAVAEESEC_WARNING_NO_PWD.useraction=If a password was expected, review the caller query and database contents.

JAVAEESEC_WARNING_MULTI_CALLER=CWWKS1924W: The {1} query returned multiple results for the {0} caller on the DatabaseIdentityStore.
JAVAEESEC_WARNING_MULTI_CALLER.explanation=Multiple results were returned for the requested caller. The caller query should only return a single result.
JAVAEESEC_WARNING_MULTI_CALLER.useraction=Review the caller query and database contents. Change the caller query so that it returns one result.

@kristip17 kristip17 force-pushed the AddMessagesIdentityStore branch from 0a0fd76 to cbcb2f2 Compare January 5, 2018 14:23
@kristip17
Copy link
Contributor Author

I made Pam's changes to the message file.

@kristip17 kristip17 force-pushed the AddMessagesIdentityStore branch from cbcb2f2 to 47f946d Compare January 5, 2018 15:22
@kristip17 kristip17 merged commit 866cc77 into OpenLiberty:integration Jan 5, 2018
@kristip17
Copy link
Contributor Author

fixes #1322

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@helyarp helyarp helyarp left review comments

@ryanesch ryanesch ryanesch approved these changes

@jvanhill jvanhill jvanhill approved these changes

Assignees

@helyarp helyarp

@kristip17 kristip17

Labels
ID reviewed release-18.0.0.1 team:Wendigo East
Projects
None yet
Milestone
[Iteration 18.1] January 1 - January 12
Development

Successfully merging this pull request may close these issues.
6 participants
@kristip17 @LibbyBot @aguibert @helyarp @jvanhill @ryanesch