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

New Enterprise Module : JAAS Security #244

Closed
wants to merge 10 commits into from

Conversation

dbrimley
Copy link
Contributor

@dbrimley dbrimley commented Jun 3, 2018

This takes the JAAS Security example in my personal repo. AFAIK, we do not currently have a JAAS Security example in the code samples project.

Initial commit
Java classes
Fixed license issues
Change URL in README.md for obtaining license
ignore .java-version from jenv

Initial commit

Java classes

Fixed license issues

Change URL in README.md for obtaining license

ignore .java-version from jenv
@devOpsHazelcast
Copy link
Collaborator

Test FAILed.

}

public void clientDisconnected(Client client) {

Copy link
Contributor

Choose a reason for hiding this comment

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

can be added some logging..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, added.

<client-login-modules>
<login-module class-name="ClientLoginModule" usage="REQUIRED">
<properties>
<property name="lookupFilePath">value3</property>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this property for?

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's a way for the JAAS ClientLoginModule to take outside properties. I guess this isn't very clear. I'll change the naming.

*
*/
public class ClientLoginModule implements LoginModule {

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest this impl to be reviewed by @tkountis @kwart

@mesutcelik
Copy link
Contributor

@dbrimley Thanks for the sample! Can you please fix checkstyle problems?

@Holmistr Holmistr requested review from kwart and removed request for Holmistr June 4, 2018 07:18
@devOpsHazelcast
Copy link
Collaborator

Test PASSed.

Copy link
Member

@kwart kwart left a comment

Choose a reason for hiding this comment

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

Some minor issues were commented. Otherwise LGTM.

import static com.hazelcast.examples.helper.LicenseUtils.ENTERPRISE_LICENSE_KEY;

/**
* Created by dbrimley on 19/05/2014.
Copy link
Member

Choose a reason for hiding this comment

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

Use a more descriptive Javadoc.

loginOk = doLoginCheck((UsernamePasswordCredentials) credentials);
}

return loginOk;
Copy link
Member

Choose a reason for hiding this comment

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

When authentication fails, then LoginException should be thrown. The FailedLoginException child class is probably the best candidate.

Returning false just means the login module should be ignored. It can be used for instance when the login module stack contains several modules, each accepting different parameters. Return false in case the actual parameters are not sufficient for current login module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will change.

/**
* Tidy up the Subject
*/
private void clearSubject() {
Copy link
Member

Choose a reason for hiding this comment

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

Login modules should only clean-up their stuff (i.e. avoid removing data introduced by other login modules). It's a minor thing in this case, but if users use code samples as a quick-start for their implementations this should be done right.

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 only clean up if the whole chain is aborted or if there is a logout event, so I think it's fine.

if(password.equals(credentials.getPassword())){
String userGroup = userGroups.get(username);
if (userGroup != null){
userGroupCredentials = new UserGroupCredentials(credentials.getEndpoint(),userGroup);
Copy link
Member

Choose a reason for hiding this comment

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

We don't add the authentication name part of the information into the Subject. Nonetheless, it's not a problem for this code sample.

@devOpsHazelcast
Copy link
Collaborator

Test FAILed.

@kwart
Copy link
Member

kwart commented Jul 6, 2018

David, could you rebase to the current master and squash the commits?

@devOpsHazelcast
Copy link
Collaborator

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@hazelcast hazelcast deleted a comment from devOpsHazelcast Apr 6, 2020
@hazelcast hazelcast deleted a comment from devOpsHazelcast Apr 6, 2020
@mmedenjak
Copy link
Contributor

@kwart do you think this code sample is still useful? If so, we can rebase and squash without David's involvement.

@kwart
Copy link
Member

kwart commented Apr 6, 2020

We don't need to include this code sample, I think.

We have 2 other samples implementing custom login modules:

  • enterprise/client-custom-credentials
  • enterprise/client-token-credentials

If there is a need for more, we can always resurrect this PR.

Closing for now, feel free to reopen if necessary.

@kwart kwart closed this Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants