[aerogear-dev] [Unified Push Server] Roles structure & password management

[Sebastien Blanc]

On Tue, Nov 5, 2013 at 8:28 PM, Bruno Oliveira <bruno at abstractj.org> wrote:

> As far as I recall (because we discussed it a long time ago). But I
> think you are talking about the following piece of code, right? I think
> the method below is still handy.
>
> /**
>      * Role validation against the IDM
>      *
>      * @param roles roles to be checked
>      * @return returns true if the current logged in has roles at the
> IDM, false otherwise
>      */
>     @Override
>     public boolean hasRoles(Set<String> roles) {
>
>         if (identity.isLoggedIn()) {
>             for (String role : roles) {
>                 Role retrievedRole = BasicModel.getRole(identityManager,
> role);
>                 if (retrievedRole != null &&
> BasicModel.hasRole(partitionManager.createRelationshipManager(),
> identity.getAccount(), retrievedRole)) {
>                     return true;
>                 }
>             }
>         }
>         return false;
>     }
>
> Speaking about the interceptor, here comes some criticism about what I did:
>
> private void authorize(Set<String> roles) {
>         boolean hasRoles = identityManagement.hasRoles(roles);
>
>         if (!hasRoles)
>             throw new
> AeroGearSecurityException(HttpStatus.CREDENTIAL_NOT_AUTHORIZED);
> }
>

Yes that was the piece of code I was thinking about, or more precisely ,
the code of the secure annotation
https://github.com/aerogear/aerogear-security/blob/master/src/main/java/org/jboss/aerogear/security/authz/Secure.java#L56


>
> The code above doesn't open a security flaw, but being completely
> paranoid that should be refactored to authorize accept a single role (I
> can be wrong). But think about the following scenario (out of the UPS
> box). If the developer mistakenly add "simple", "admin" to the some
> method (is not impossible) which does some sensitive operation, this
> might be a problem.
>
> As I told you guys, I'm not against it, but my job is to be picky and
> raise some concerns. AG Sec is not the holy grail of security and must
> be improved.
>
Sure, it's important to face your security concerns with
the functional needs :)

>
> Sebastien Blanc wrote:
> > But that is already something that we can do with AG PL , adding
> > multiple roles to the secure annotation. You said we should maybe
> > remove this from ag-pl ?
>
> --
> abstractj
>
>
>
> _______________________________________________
> aerogear-dev mailing list
> aerogear-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/aerogear-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/aerogear-dev/attachments/20131105/cfb7ab3a/attachment.html