Skip to content
This repository was archived by the owner on Nov 15, 2022. It is now read-only.

add privileged block in logout#22244

Merged
wmhopkins merged 1 commit intojavaee:masterfrom
sameer-pandit:privleged
Aug 23, 2017
Merged

add privileged block in logout#22244
wmhopkins merged 1 commit intojavaee:masterfrom
sameer-pandit:privleged

Conversation

@sameer-pandit
Copy link
Copy Markdown
Member

@sameer-pandit sameer-pandit commented Aug 23, 2017

Fixes #22243

    private void resetPolicyContext() {
       ((PolicyContextHandlerImpl)PolicyContextHandlerImpl.getInstance()).reset();
       PolicyContext.setContextID(null);
    }

The issue stacktrace catches permission issue PolicyContextHandlerImpl.getInstance(), but PolicyContext.setContextID() also need setPolicy permissions as per javadoc
which why the doPrivileged for whole resetPolicyContext().

@sameer-pandit
Copy link
Copy Markdown
Member Author

@glassfishrobot Run CI tests please

@glassfishrobot
Copy link
Copy Markdown
Contributor

Starting CI tests run

@wmhopkins
Copy link
Copy Markdown
Member

@sameer-pandit ,

This fix looks good, but I wonder if the privileged block can be moved down into the resetPolicyContext() method? If that method is private and used only by other RealmAdapter methods, that's a better implementation. If the method is public and potentially called by external callers, then the current placement is better/safer. (Not at my desk, so can't look for myself.)

@glassfishrobot
Copy link
Copy Markdown
Contributor

All CI tests successful

@wmhopkins
Copy link
Copy Markdown
Member

@sameer-pandit,

I had a closer look at the code when I got back to my desk, and I think it would be better to put the PrivilegedAction inside the resetPolicyContext() method. That method is private, so it can only be called by other methods of the RealmAdapter class. Putting it inside resetPolicyContext() would benefit all callers; the current approach means each caller has to have its own PrivilegedAction block if it calls resetPolicyContext().

That said, logout() is currently the only caller, so there's no practical difference at the moment, and we need to get this submitted asap. Approved the PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants