Bug 60469 - RealmBase#hasRole() imposes too much boilerplate code if principal is not of type GenericPrincipal
Summary: RealmBase#hasRole() imposes too much boilerplate code if principal is not of ...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.5.x-trunk
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-12-12 13:20 UTC by Michael Osipov
Modified: 2017-03-03 20:22 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Osipov 2016-12-12 13:20:19 UTC
If your custom realm extends RealmBase but uses a custom principal, just like mine, you need to duplicate the entire #hasRole(Wrapper, Princpal, String) method.

The only interesting part for an implementor is
!(principal instanceof GenericPrincipal)
and
GenericPrincipal gp = (GenericPrincipal) principal;
boolean result = gp.hasRole(role);

A verbatim copy of the method is need just to adapt three lines. Which basically means to double-check every Tomcat release for changes in this method.

I am proposing to introduce a new sub-method: hasRoleInternal(Wrapper, Principal, String) (or alike) which performs just the code above. The entire boilerplate code remains in #hasRole()

My custom realm would simply do:

public boolean hasRoleInternal(Wrappr wrapper, Principal principal, String role) {
  if(!(principal instanceof CustomPrincipal))
    return false;

  CustomPrincipal cp = (CustomPrincipal) principal;
  return cp.hasRole(role);
}

I can provide a patch for that if you agree with.
Comment 1 Remy Maucherat 2016-12-12 13:23:40 UTC
I disagree, it's not worth changing the API for it and it's worse than "checking for changes" which actually never happens.
BTW, this is an "enhancement", not a "major" bug.
Comment 2 Michael Osipov 2016-12-12 13:32:50 UTC
(In reply to Remy Maucherat from comment #1)
> I disagree, it's not worth changing the API for it and it's worse than
> "checking for changes" which actually never happens.
> BTW, this is an "enhancement", not a "major" bug.

Why do you close this issue without having a discussion first?

No one is changing the API. You can still continue to override #hasRole(), it will have the same effect. The change can happen and are not a part of an API at at all. The code of RealmBase#hasRole() is implementation specific, not to be known to the user at best.
Comment 3 Michael Osipov 2017-02-10 09:53:11 UTC
Anyone else care to share an opinion on that?
Comment 4 Christopher Schultz 2017-02-10 18:50:04 UTC
Feel free to post a patch. I think it's a reasonable change.
Comment 5 Michael Osipov 2017-02-11 11:01:45 UTC
(In reply to Christopher Schultz from comment #4)
> Feel free to post a patch. I think it's a reasonable change.

Thanks, I will prepare one next week.
Comment 6 Mark Thomas 2017-03-03 14:25:18 UTC
Fixed in:
- trunk for 9.0.0.M18 onwards
- 8.5.x for 8.5.12 onwards
Comment 7 Michael Osipov 2017-03-03 20:22:12 UTC
(In reply to Mark Thomas from comment #6)
> Fixed in:
> - trunk for 9.0.0.M18 onwards
> - 8.5.x for 8.5.12 onwards

Per(In reply to Mark Thomas from comment #6)
> Fixed in:
> - trunk for 9.0.0.M18 onwards
> - 8.5.x for 8.5.12 onwards

Perfect, couldn't be better. Thank you!