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

[ELY-1150] - Revisit branches in handleOne method in ServerAuthenticationContext.createCallbackHandler() #862

Merged
merged 1 commit into from Jun 8, 2017

Conversation

pedroigor
Copy link
Contributor

No description provided.

Copy link
Contributor

@darranl darranl left a comment

Choose a reason for hiding this comment

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

Are you sure this is needed? I think in some cases it was a deliberate decision that once this callback has been handled we should handle no more.

Copy link
Contributor

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

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

The CachedIdentityAuthorizeCallback branch still looks wrong to me. I don't think it should be returning, since that would prevent handling subsequent callbacks.

@@ -1031,6 +1034,7 @@ private void handleOne(final Callback[] callbacks, final int idx) throws IOExcep
log.tracef("Handling CachedIdentityAuthorizeCallback: principal = %s authorizedIdentity = %s", principal, authorizedIdentity);
Copy link
Contributor

Choose a reason for hiding this comment

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

Three lines before this one is a return statement. Should it instead be a handleOne()/continue? And there's another as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry,it is wrong. It should go in the finally block where AI is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmlloyd I've removed the try/finally block and return statements. In case an exception is thrown we should not call handleOne.

@pedroigor
Copy link
Contributor Author

@darranl I can't remember any reason for not calling handleOne within CachedIdentityAuthorizeCallback branch.

@pedroigor pedroigor force-pushed the ELY-1150 branch 2 times, most recently from 94d6cad to d0ca39f Compare June 7, 2017 19:29
@dmlloyd dmlloyd added the +1 DML label Jun 7, 2017
@darranl darranl added the +1 DAL label Jun 8, 2017
@darranl darranl merged commit bf9a5bf into wildfly-security:master Jun 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants