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

Updating to Marathon(-ldap) 1.3 broke modal windows #6

Closed
DGollings opened this issue Oct 7, 2016 · 9 comments
Closed

Updating to Marathon(-ldap) 1.3 broke modal windows #6

DGollings opened this issue Oct 7, 2016 · 9 comments

Comments

@DGollings
Copy link

Updated marathon and ldap, can no longer use the GUI for creating new applications or editing existing ones.

Short stacktrace:
Oct 07 14:07:48 host marathon[2197]: java.lang.IllegalArgumentException: Unknown Action: ViewResource
Oct 07 14:07:48 host marathon[2197]: at io.containx.marathon.plugin.auth.type.Action.byAction(Action.java:38)
Oct 07 14:07:48 host marathon[2197]: at io.containx.marathon.plugin.auth.LDAPAuthorizor.isAuthorized(LDAPAuthorizor.java:26)
Oct 07 14:07:48 host marathon[2197]: at mesosphere.marathon.core.event.impl.stream.HttpEventStreamServlet.isAuthorized$1(HttpEventStreamServlet.scala:78)

@gondor
Copy link
Member

gondor commented Oct 7, 2016

Hi @DGollings

1.3 Only works with Marathon version 1.3+ which was recently release. If you are not running Marathon 1.3 you must use the Marathon-LDAP 1.1 release https://github.com/ContainX/marathon-ldap/releases/tag/1.1

Could you confirm your Marathon version

@DGollings
Copy link
Author

DGollings commented Oct 7, 2016

Marathon 1.3 and Marathon-ldap 1.3

Also, I 'fixed' it like this:
edited for clarity, see below

@DGollings
Copy link
Author

diff --git a/src/main/java/io/containx/marathon/plugin/auth/LDAPAuthorizor.java b/src/main/java/io/containx/marathon/plugin/auth/LDAPAuthorizor.java
index 0e03c72..4514c0e 100644
--- a/src/main/java/io/containx/marathon/plugin/auth/LDAPAuthorizor.java
+++ b/src/main/java/io/containx/marathon/plugin/auth/LDAPAuthorizor.java
@@ -30,7 +30,8 @@ public class LDAPAuthorizor implements Authorizer {
if (resource instanceof RunSpec) {
return isAuthorized(user, action, ((RunSpec) resource).id());
}

  •        return resource instanceof PathId && isAuthorized(user, action, (PathId) resource);
    

**+ return true;

  •        //return resource instanceof PathId && isAuthorized(user, action, (PathId) resource);**
       }
       return false;
    
    }
    diff --git a/src/main/java/io/containx/marathon/plugin/auth/type/Action.java b/src/main/java/io/containx/marathon/plugin/auth/type/Action.java
    index 30212c9..d99a65c 100644
    --- a/src/main/java/io/containx/marathon/plugin/auth/type/Action.java
    +++ b/src/main/java/io/containx/marathon/plugin/auth/type/Action.java
    @@ -13,6 +13,9 @@ public enum Action {
    DELETE_APP(DeleteRunSpec$.MODULE$, PermissionType.DELETE, EntityType.APP),
    VIEW_APP(ViewRunSpec$.MODULE$, PermissionType.VIEW, EntityType.APP),

**+ VIEW_RESOURCE(ViewResource$.MODULE$, PermissionType.VIEW, EntityType.APP),

  • UPDATE_RESOURCE(UpdateResource$.MODULE$, PermissionType.UPDATE, EntityType.APP),**
  • // Group Actions
    CREATE_GROUP(CreateGroup$.MODULE$, PermissionType.CREATE, EntityType.GROUP),
    UPDATE_GROUP(UpdateGroup$.MODULE$, PermissionType.UPDATE, EntityType.GROUP),

@gondor
Copy link
Member

gondor commented Oct 7, 2016

Ok thanks! Did you want to just submit this in a PR?

@DGollings
Copy link
Author

No, don't really consider it a fix (I have no idea what I'm doing) :)

I think it's secure-ish, but mostly it works which was good enough for now. (I think the *_RESOURCE stuff is close enough, but I doubt return true is acceptable)

gondor added a commit that referenced this issue Oct 8, 2016
@gondor
Copy link
Member

gondor commented Oct 8, 2016

@DGollings since your on source, would you mind testing this fix. The solution I did was to add the types missing from the plugin differences between 1.1 and 1.3 so we don't throw the IllegalArg. This sounded like the root cause and if it works will keep security in tact as it was prior.

@DGollings
Copy link
Author

See my 'fix', I tried the same but it didnt work. Something needs to be changed in LDAPAuthorizor.java as well but I couldnt't (quickly) figure out what. I think VIEW_RESOURCE is an instanceof something else?

Haven't tested it, but considering I tried the same I doubt commit 32aa110 is enough. Did you change more?

gondor added a commit that referenced this issue Oct 9, 2016
@gondor
Copy link
Member

gondor commented Oct 9, 2016

You are correct. I setup an environment running Marathon 1.3.2 and was able to reproduce this. Unfortunately when its of type ViewResource the actual resource is a SystemConfig which contains no PathId info, Prior calls validate you have rights to even make the "modal" action so in this scenario I return true.

Modals are now working for resources we are granted to see. Just update the 1.3 release with the fixes. You can grab that jar and test if you wish. Let me know if it works in your environment

@DGollings
Copy link
Author

Fixed, thanks a lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants