Skip to content

[Zeppelin-1561] Improve sync for multiuser environment #1537

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.

Sign up for GitHub

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

Closed
khalidhuseynov wants to merge 8 commits into from
Closed

[Zeppelin-1561] Improve sync for multiuser environment #1537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cd1c3fa
don't sync on start
khalidhuseynov Sep 22, 2016
585a675
reload, sync and broadcast on login
khalidhuseynov Sep 22, 2016
1a54cc0
set perms for pulling notes - make them private
khalidhuseynov Sep 23, 2016
b104249
add isAnonymous
khalidhuseynov Oct 18, 2016
05afa2a
remove syncOnStart
khalidhuseynov Oct 19, 2016
bd1a44a
address comment + test
khalidhuseynov Oct 23, 2016
0f2ade7
reformat style
khalidhuseynov Oct 23, 2016
b3e6ed3
add userAndRoles
khalidhuseynov Oct 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions zeppelin-interpreter/src/main/java/org/apache/zeppelin/user/AuthenticationInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,20 @@

package org.apache.zeppelin.user;

import org.apache.commons.lang.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/***
*
*/
public class AuthenticationInfo {
private static final Logger LOG = LoggerFactory.getLogger(AuthenticationInfo.class);
String user;
String ticket;
UserCredentials userCredentials;
public static final AuthenticationInfo ANONYMOUS = new AuthenticationInfo("anonymous",
"anonymous");

public AuthenticationInfo() {}

Expand Down Expand Up @@ -66,4 +73,17 @@ public void setUserCredentials(UserCredentials userCredentials) {
this.userCredentials = userCredentials;
}

public static boolean isAnonymous(AuthenticationInfo subject) {
if (subject == null) {
LOG.warn("Subject is null, assuming anonymous. "
+ "Not recommended to use subject as null except in tests");
return true;
}
return subject.isAnonymous();
}

public boolean isAnonymous() {
return ANONYMOUS.equals(this) || "anonymous".equalsIgnoreCase(this.getUser())
|| StringUtils.isEmpty(this.getUser());
}
}
10 changes: 10 additions & 0 deletions zeppelin-server/src/main/java/org/apache/zeppelin/realm/ZeppelinHubRealm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.net.MalformedURLException;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.HashSet;
import java.util.concurrent.atomic.AtomicInteger;

import org.apache.commons.httpclient.HttpClient;
Expand All @@ -36,6 +37,7 @@
import org.apache.shiro.authz.AuthorizationInfo;
import org.apache.shiro.realm.AuthorizingRealm;
import org.apache.shiro.subject.PrincipalCollection;
import org.apache.zeppelin.server.ZeppelinServer;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -135,6 +137,7 @@ protected User authenticateUser(String requestBody) {
}
responseBody = put.getResponseBodyAsString();
put.releaseConnection();

} catch (IOException e) {
LOG.error("Cannot login user", e);
throw new AuthenticationException(e.getMessage());
Expand All @@ -147,6 +150,13 @@ protected User authenticateUser(String requestBody) {
LOG.error("Cannot deserialize ZeppelinHub response to User instance", e);
throw new AuthenticationException("Cannot login to ZeppelinHub");
}

/* TODO(khalid): add proper roles and add listener */
HashSet<String> userAndRoles = new HashSet<String>();
userAndRoles.add(account.login);
ZeppelinServer.notebookWsServer.broadcastReloadedNoteList(
new org.apache.zeppelin.user.AuthenticationInfo(account.login), userAndRoles);

return account;
}

Expand Down
47 changes: 36 additions & 11 deletions zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/repo/NotebookRepoSync.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

import org.apache.zeppelin.conf.ZeppelinConfiguration;
import org.apache.zeppelin.conf.ZeppelinConfiguration.ConfVars;
Expand Down Expand Up @@ -92,14 +94,6 @@ public NotebookRepoSync(ZeppelinConfiguration conf) {
LOG.info("No storages could be initialized, using default {} storage", defaultStorage);
initializeDefaultStorage(conf);
}
if (getRepoCount() > 1) {
try {
AuthenticationInfo subject = new AuthenticationInfo("anonymous");
sync(0, 1, subject);
} catch (IOException e) {
LOG.warn("Failed to sync with secondary storage on start {}", e);
}
}
}

@SuppressWarnings("static-access")
Expand Down Expand Up @@ -172,6 +166,10 @@ public void remove(String noteId, AuthenticationInfo subject) throws IOException
/* TODO(khalid): handle case when removing from secondary storage fails */
}

void remove(int repoIndex, String noteId, AuthenticationInfo subject) throws IOException {
getRepo(repoIndex).remove(noteId, subject);
}

/**
* Copies new/updated notes from source to destination storage
*
Expand All @@ -197,7 +195,7 @@ void sync(int sourceRepoIndex, int destRepoIndex, AuthenticationInfo subject) th
for (String id : pushNoteIDs) {
LOG.info("ID : " + id);
}
pushNotes(subject, pushNoteIDs, srcRepo, dstRepo);
pushNotes(subject, pushNoteIDs, srcRepo, dstRepo, false);
} else {
LOG.info("Nothing to push");
}
Expand All @@ -207,7 +205,7 @@ void sync(int sourceRepoIndex, int destRepoIndex, AuthenticationInfo subject) th
for (String id : pullNoteIDs) {
LOG.info("ID : " + id);
}
pushNotes(subject, pullNoteIDs, dstRepo, srcRepo);
pushNotes(subject, pullNoteIDs, dstRepo, srcRepo, true);
} else {
LOG.info("Nothing to pull");
}
Expand All @@ -230,16 +228,43 @@ public void sync(AuthenticationInfo subject) throws IOException {
}

private void pushNotes(AuthenticationInfo subject, List<String> ids, NotebookRepo localRepo,
NotebookRepo remoteRepo) {
NotebookRepo remoteRepo, boolean setPermissions) {
for (String id : ids) {
try {
remoteRepo.save(localRepo.get(id, subject), subject);
if (setPermissions && emptyNoteAcl(id)) {
makePrivate(id, subject);
}
} catch (IOException e) {
LOG.error("Failed to push note to storage, moving onto next one", e);
}
}
}

private boolean emptyNoteAcl(String noteId) {
NotebookAuthorization notebookAuthorization = NotebookAuthorization.getInstance();
return notebookAuthorization.getOwners(noteId).isEmpty()
&& notebookAuthorization.getReaders(noteId).isEmpty()
&& notebookAuthorization.getWriters(noteId).isEmpty();
}

private void makePrivate(String noteId, AuthenticationInfo subject) {
if (AuthenticationInfo.isAnonymous(subject)) {
LOG.info("User is anonymous, permissions are not set for pulled notes");
return;
}
NotebookAuthorization notebookAuthorization = NotebookAuthorization.getInstance();
Set<String> users = notebookAuthorization.getOwners(noteId);
users.add(subject.getUser());
Copy link
Member

@Leemoonsoo Leemoonsoo Oct 23, 2016

Choose a reason for hiding this comment

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

Could you elaborate why 'makePrivate()' routine, which is being used inside of sync() adds current user as a owner, reader, writer?

Copy link
Member Author

@khalidhuseynov khalidhuseynov Oct 23, 2016

Choose a reason for hiding this comment

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

right, it sets it when pulling notes from secondary storage to your main one

Copy link
Member

@Leemoonsoo Leemoonsoo Oct 23, 2016

Choose a reason for hiding this comment

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

Can you explain why it sets when pulling notes from secondary storage to the main one?

For example, what happen if a note permission has a user as a reader, but not as a writer and owner?

Copy link
Member Author

@khalidhuseynov khalidhuseynov Oct 23, 2016

Choose a reason for hiding this comment

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

if it's pulling, that means there was no such a note in main storage and it's pulling new note from secondary storage. Thus, that new note not supposed to have previous owners, readers, writers. So generally it's same as creating note and only owner should be set. But I also set readers and writers in order other users don't see that new note (make it private).

notebookAuthorization.setOwners(noteId, users);
users = notebookAuthorization.getReaders(noteId);
users.add(subject.getUser());
notebookAuthorization.setReaders(noteId, users);
users = notebookAuthorization.getWriters(noteId);
users.add(subject.getUser());
notebookAuthorization.setWriters(noteId, users);
}

private void deleteNotes(AuthenticationInfo subject, List<String> ids, NotebookRepo repo)
throws IOException {
for (String id : ids) {
Expand Down
68 changes: 68 additions & 0 deletions zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/repo/NotebookRepoSyncTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@

import java.io.File;
import java.io.IOException;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;

import org.apache.commons.io.FileUtils;
import org.apache.zeppelin.conf.ZeppelinConfiguration;
Expand Down Expand Up @@ -314,6 +316,72 @@ public void testCheckpointOneStorage() throws IOException, SchedulerException {
notebookRepoSync.remove(note.getId(), anonymous);
}

@Test
public void testSyncWithAcl() throws IOException {
/* scenario 1 - note exists with acl on main storage */
AuthenticationInfo user1 = new AuthenticationInfo("user1");
Note note = notebookSync.createNote(user1);
assertEquals(0, note.getParagraphs().size());

// saved on both storages
assertEquals(1, notebookRepoSync.list(0, null).size());
assertEquals(1, notebookRepoSync.list(1, null).size());

/* check that user1 is the only owner */
NotebookAuthorization authInfo = NotebookAuthorization.getInstance();
Set<String> entity = new HashSet<String>();
entity.add(user1.getUser());
assertEquals(true, authInfo.isOwner(note.getId(), entity));
assertEquals(1, authInfo.getOwners(note.getId()).size());
assertEquals(0, authInfo.getReaders(note.getId()).size());
assertEquals(0, authInfo.getWriters(note.getId()).size());

/* update note and save on secondary storage */
Paragraph p1 = note.addParagraph();
p1.setText("hello world");
assertEquals(1, note.getParagraphs().size());
notebookRepoSync.save(1, note, null);

/* check paragraph isn't saved into first storage */
assertEquals(0, notebookRepoSync.get(0,
notebookRepoSync.list(0, null).get(0).getId(), null).getParagraphs().size());
/* check paragraph is saved into second storage */
assertEquals(1, notebookRepoSync.get(1,
notebookRepoSync.list(1, null).get(0).getId(), null).getParagraphs().size());

/* now sync by user1 */
notebookRepoSync.sync(user1);

/* check that note updated and acl are same on main storage*/
assertEquals(1, notebookRepoSync.get(0,
notebookRepoSync.list(0, null).get(0).getId(), null).getParagraphs().size());
assertEquals(true, authInfo.isOwner(note.getId(), entity));
assertEquals(1, authInfo.getOwners(note.getId()).size());
assertEquals(0, authInfo.getReaders(note.getId()).size());
assertEquals(0, authInfo.getWriters(note.getId()).size());

/* scenario 2 - note doesn't exist on main storage */
/* remove from main storage */
notebookRepoSync.remove(0, note.getId(), user1);
assertEquals(0, notebookRepoSync.list(0, null).size());
assertEquals(1, notebookRepoSync.list(1, null).size());
authInfo.removeNote(note.getId());
assertEquals(0, authInfo.getOwners(note.getId()).size());
assertEquals(0, authInfo.getReaders(note.getId()).size());
assertEquals(0, authInfo.getWriters(note.getId()).size());

/* now sync - should bring note from secondary storage with added acl */
notebookRepoSync.sync(user1);
assertEquals(1, notebookRepoSync.list(0, null).size());
assertEquals(1, notebookRepoSync.list(1, null).size());
assertEquals(1, authInfo.getOwners(note.getId()).size());
assertEquals(1, authInfo.getReaders(note.getId()).size());
assertEquals(1, authInfo.getWriters(note.getId()).size());
assertEquals(true, authInfo.isOwner(note.getId(), entity));
assertEquals(true, authInfo.isReader(note.getId(), entity));
assertEquals(true, authInfo.isWriter(note.getId(), entity));
}

static void delete(File file){
if(file.isFile()) file.delete();
else if(file.isDirectory()){
Expand Down