Community
Participate
Working Groups
I had played around "ReadAccessHandler", which supposed to interrupt loading of particular revision. It is good starting point for implementing some security principles in CDO. Unfortunately, I have noticed that, if part of revisions is not permitted to load and they are mixed among the revisions which you are allow to load, on client side, I am getting exceptions, even if I attempt to work with permitted objects. For example, I have EList of CDOObject which are not loaded (list contains CDOIDs). One CDOIDs are allowed to read other are not. I know which one is allowed to access for me and I intend to remove one of them (Using EcoreUtil.delete or just removing from container). And I get exception, because that EMF code is touching forbidden elements in a list.
"ReadAccessHandler" now is controlling, if CDO revision shall be passed or not (control can be made by throwing exception), but for reasons explained in previous message it is not sufficient to work with model, where some peaces of model is allowed to read but other not. Thus "ReadAccessHandler" must have ability not only to reject READ action, but substitute returned revisions with different one (small change). In READ protection case we can simple fill revision with empty values. When revisions are passed to client, we need somehow to give a hint for client to recognize which one revision was protected and which one is not. As possible solution, I could see to introduce additional revision member field for such information. I do not know, if community shall desire this field-marker in default implementation, but in any way implementator can provide his own revision implementation with desired field-marker. Few changes will be needed to make registering of revision factory on client-server side.
Created attachment 191956 [details] Patch v1 Light refactoring which will allow to inject custom revision and test which shows how it can work.
Created attachment 192200 [details] Patch v2 Added posibility to replace outgoing revisions.
Created attachment 192436 [details] Patch v3
I've added the test case to AllConfigs.java
I could imagine a new field in BaseCDORevision. Something like Set<EStructuralFeature> featuresChangedByReadAccessHandler = EMPTY_SET; // TODO Better name needed!
Committed to trunk, revisions 7585
Hmm... I didn't see benefit of having "Set<EStructuralFeature> featuresChangedByReadAccessHandler"... What would be advantage for client to know that some features was changed by server read handler and other not? In my thought was to deliver some kind of protection level (READ, WRITE protection), which will state that whole revision has specific protection and user should expect error, if he attempt to change revisions in WRITE protected state. Or if state is READ protected, then client side shouldn't expect to get desired information.
What if the ReadAccessHandler is used to enforce an authorization strategy that's based on per-feature permissions?
Bugzilla_340961_Test fails e.g. for the H2 Branching test suite. Please investigate: org.eclipse.emf.cdo.tests.config.impl.ConfigTestException: Error in Bugzilla_340961_Test.testObjectChilds [Combined, DBStore: H2 (branching), JVM, Native] at org.eclipse.emf.cdo.tests.config.impl.ConfigTest.runBare(ConfigTest.java:489) at junit.framework.TestResult$1.protect(TestResult.java:110) at junit.framework.TestResult.runProtected(TestResult.java:128) at junit.framework.TestResult.run(TestResult.java:113) at junit.framework.TestCase.run(TestCase.java:124) at org.eclipse.net4j.util.tests.AbstractOMTest.run(AbstractOMTest.java:260) at junit.framework.TestSuite.runTest(TestSuite.java:232) at org.eclipse.emf.cdo.tests.config.impl.ConfigTestSuite$TestWrapper.runTest(ConfigTestSuite.java:126) at junit.framework.TestSuite.run(TestSuite.java:227) at junit.framework.TestSuite.runTest(TestSuite.java:232) at junit.framework.TestSuite.run(TestSuite.java:227) at junit.framework.TestSuite.runTest(TestSuite.java:232) at junit.framework.TestSuite.run(TestSuite.java:227) at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197) Caused by: java.lang.OutOfMemoryError: Java heap space at java.util.ArrayList.<init>(ArrayList.java:112) at org.eclipse.emf.cdo.internal.net4j.protocol.LoadRevisionsRequest.confirming(LoadRevisionsRequest.java:151) at org.eclipse.emf.cdo.internal.net4j.protocol.LoadRevisionsRequest.confirming(LoadRevisionsRequest.java:1) at org.eclipse.emf.cdo.internal.net4j.protocol.CDOClientRequest.confirming(CDOClientRequest.java:90) at org.eclipse.net4j.signal.RequestWithConfirmation.doExtendedInput(RequestWithConfirmation.java:123) at org.eclipse.net4j.signal.Signal.doInput(Signal.java:326) at org.eclipse.net4j.signal.RequestWithConfirmation.doExecute(RequestWithConfirmation.java:103) at org.eclipse.net4j.signal.SignalActor.execute(SignalActor.java:51) at org.eclipse.net4j.signal.Signal.runSync(Signal.java:251) at org.eclipse.net4j.signal.SignalProtocol.startSignal(SignalProtocol.java:396) at org.eclipse.net4j.signal.RequestWithConfirmation.doSend(RequestWithConfirmation.java:87) at org.eclipse.net4j.signal.RequestWithConfirmation.send(RequestWithConfirmation.java:73) at org.eclipse.emf.cdo.internal.net4j.protocol.CDOClientProtocol.send(CDOClientProtocol.java:401) at org.eclipse.emf.cdo.internal.net4j.protocol.CDOClientProtocol.send(CDOClientProtocol.java:434) at org.eclipse.emf.cdo.internal.net4j.protocol.CDOClientProtocol.loadRevisions(CDOClientProtocol.java:163) at org.eclipse.emf.cdo.internal.common.revision.CDORevisionManagerImpl.loadRevisions(CDORevisionManagerImpl.java:352) at org.eclipse.emf.cdo.tests.util.TestRevisionManager.loadRevisions(TestRevisionManager.java:116) at org.eclipse.emf.cdo.internal.common.revision.CDORevisionManagerImpl.getRevisions(CDORevisionManagerImpl.java:263) at org.eclipse.emf.cdo.tests.util.TestRevisionManager.getRevisions(TestRevisionManager.java:71) at org.eclipse.emf.cdo.internal.common.revision.CDORevisionManagerImpl.getRevision(CDORevisionManagerImpl.java:246) at org.eclipse.emf.cdo.internal.common.revision.CDORevisionManagerImpl.getRevision(CDORevisionManagerImpl.java:239) at org.eclipse.emf.internal.cdo.view.CDOViewImpl.getRevision(CDOViewImpl.java:336) at org.eclipse.emf.internal.cdo.view.AbstractCDOView.createObject(AbstractCDOView.java:787) at org.eclipse.emf.internal.cdo.view.AbstractCDOView.getObject(AbstractCDOView.java:702) at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.getObject(CDOTransactionImpl.java:1018) at org.eclipse.emf.internal.cdo.view.AbstractCDOView.getObject(AbstractCDOView.java:668) at org.eclipse.emf.internal.cdo.view.AbstractCDOView.getRootResource(AbstractCDOView.java:206) at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.attachNewResourceNode(CDOTransactionImpl.java:896) at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.getOrCreateResourceFolder(CDOTransactionImpl.java:868) at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.attachNewResource(CDOTransactionImpl.java:825) at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.attachResource(CDOTransactionImpl.java:815) at org.eclipse.emf.cdo.eresource.impl.CDOResourceImpl.basicSetResourceSet(CDOResourceImpl.java:1255) org.eclipse.emf.cdo.tests.config.impl.ConfigTestException: Error in Bugzilla_340961_Test.testMultiUserWork [Combined, DBStore: H2 (branching), JVM, Native] at org.eclipse.emf.cdo.tests.config.impl.ConfigTest.runBare(ConfigTest.java:489) at junit.framework.TestResult$1.protect(TestResult.java:110) at junit.framework.TestResult.runProtected(TestResult.java:128) at junit.framework.TestResult.run(TestResult.java:113) at junit.framework.TestCase.run(TestCase.java:124) at org.eclipse.net4j.util.tests.AbstractOMTest.run(AbstractOMTest.java:260) at junit.framework.TestSuite.runTest(TestSuite.java:232) at org.eclipse.emf.cdo.tests.config.impl.ConfigTestSuite$TestWrapper.runTest(ConfigTestSuite.java:126) at junit.framework.TestSuite.run(TestSuite.java:227) at junit.framework.TestSuite.runTest(TestSuite.java:232) at junit.framework.TestSuite.run(TestSuite.java:227) at junit.framework.TestSuite.runTest(TestSuite.java:232) at junit.framework.TestSuite.run(TestSuite.java:227) at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197) Caused by: java.lang.OutOfMemoryError: Java heap space at java.util.ArrayList.<init>(ArrayList.java:112) at org.eclipse.emf.cdo.internal.net4j.protocol.LoadRevisionsRequest.confirming(LoadRevisionsRequest.java:151) at org.eclipse.emf.cdo.internal.net4j.protocol.LoadRevisionsRequest.confirming(LoadRevisionsRequest.java:1) at org.eclipse.emf.cdo.internal.net4j.protocol.CDOClientRequest.confirming(CDOClientRequest.java:90) at org.eclipse.net4j.signal.RequestWithConfirmation.doExtendedInput(RequestWithConfirmation.java:123) at org.eclipse.net4j.signal.Signal.doInput(Signal.java:326) at org.eclipse.net4j.signal.RequestWithConfirmation.doExecute(RequestWithConfirmation.java:103) at org.eclipse.net4j.signal.SignalActor.execute(SignalActor.java:51) at org.eclipse.net4j.signal.Signal.runSync(Signal.java:251) at org.eclipse.net4j.signal.SignalProtocol.startSignal(SignalProtocol.java:396) at org.eclipse.net4j.signal.RequestWithConfirmation.doSend(RequestWithConfirmation.java:87) at org.eclipse.net4j.signal.RequestWithConfirmation.send(RequestWithConfirmation.java:73) at org.eclipse.emf.cdo.internal.net4j.protocol.CDOClientProtocol.send(CDOClientProtocol.java:401) at org.eclipse.emf.cdo.internal.net4j.protocol.CDOClientProtocol.send(CDOClientProtocol.java:434) at org.eclipse.emf.cdo.internal.net4j.protocol.CDOClientProtocol.loadRevisions(CDOClientProtocol.java:163) at org.eclipse.emf.cdo.internal.common.revision.CDORevisionManagerImpl.loadRevisions(CDORevisionManagerImpl.java:352) at org.eclipse.emf.cdo.tests.util.TestRevisionManager.loadRevisions(TestRevisionManager.java:116) at org.eclipse.emf.cdo.internal.common.revision.CDORevisionManagerImpl.getRevisions(CDORevisionManagerImpl.java:263) at org.eclipse.emf.cdo.tests.util.TestRevisionManager.getRevisions(TestRevisionManager.java:71) at org.eclipse.emf.cdo.internal.common.revision.CDORevisionManagerImpl.getRevision(CDORevisionManagerImpl.java:246) at org.eclipse.emf.cdo.internal.common.revision.CDORevisionManagerImpl.getRevision(CDORevisionManagerImpl.java:239) at org.eclipse.emf.internal.cdo.view.CDOViewImpl.getRevision(CDOViewImpl.java:336) at org.eclipse.emf.internal.cdo.view.AbstractCDOView.createObject(AbstractCDOView.java:787) at org.eclipse.emf.internal.cdo.view.AbstractCDOView.getObject(AbstractCDOView.java:702) at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.getObject(CDOTransactionImpl.java:1018) at org.eclipse.emf.internal.cdo.view.AbstractCDOView.getObject(AbstractCDOView.java:668) at org.eclipse.emf.internal.cdo.view.AbstractCDOView.getRootResource(AbstractCDOView.java:206) at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.attachNewResourceNode(CDOTransactionImpl.java:896) at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.getOrCreateResourceFolder(CDOTransactionImpl.java:868) at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.attachNewResource(CDOTransactionImpl.java:825) at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.attachResource(CDOTransactionImpl.java:815) at org.eclipse.emf.cdo.eresource.impl.CDOResourceImpl.basicSetResourceSet(CDOResourceImpl.java:1255)
(In reply to comment #9) > What if the ReadAccessHandler is used to enforce an authorization strategy > that's based on per-feature permissions? Per feature authorization could cause additional complexity on client side. Protection always brings usually two level of protection: read and write. Then client perhaps should know what protection level is for each feature - then we should have map here. Any way, if I understood correctly, it is just a thought what could be added by implementator. Major thing of this bugzilla is to allow edit revisions on read handler and deliver to client. Maybe in future it could be developed to general CDO protection strategy.
(In reply to comment #11) > Per feature authorization could cause additional complexity on client side. Granted. > Protection always brings usually two level of protection: read and write. That's an unproven assertion. > Then > client perhaps should know what protection level is for each feature - then we > should have map here. > Any way, if I understood correctly, it is just a thought what could be added by > implementator. Yes ;-)
Created attachment 192631 [details] Patch for test Exception was caused because revisions was left from previous test (proper pre-condition). Left revisions in cache was not compatible with provided revisions by bugzilla test. Previously introduced test cleanup was not covering situation, when the same Repository was shared among tests. Additionally, was needed to cover situation, when after test Repository is left in improper state for the rest tests (post-condition). For this I used annotation @NeedsCleanRepo. While it was usable only for solving first issue (with proper pre-condition), I introduced annotation member "dropRepoAfter", which helps to solve "post-condition" issue
The "Needs" in @NeedsCleanRepo clearly states that it's a pre condition. I don't think we should reuse this annotation for a typical post condition or post action. A new annotation would be better. I'm inclined to reject the approval. Caspar, what do you think?
Egidijus, please rename the annotation from NeedsCleanRepo" to "CleanRepo" and change your new boolean field to an enum type with two literals BEFORE and AFTER (BEFORE being the default value).
Created attachment 193012 [details] Patch for test v2 Added second annotatin LeavesRepoClean
Actually annotation name is LeavesCleanRepo
Created attachment 193013 [details] Test v3
Thanks, Egidijus!
Fixed in trunk, revision 7609
This new annotation @LeavesCleanRepo (or its name) doesn't make sense to me. In the code I see that at the end of a testcase that has the annotation, the repositories are deactivated, which ensures... what? AFAICS, it only ensures that they will be rebuilt clean at the start of the next testcase. But that's what we already had the annotation @NeedsCleanRepo for. Perhaps it is because "after test Repository is left in improper state for the rest tests (post-condition)"? Actually this isn't a post condition, but a post state. (There's a serious difference, I'm not just mincing words here: a condition is constraint that must be met; a state is a situation that happens to be the case.) So I wonder what state this test puts the repo in so that it knows for sure the repo is unusable for any other tests after that. Most tests perform their operations in a test-specific resource; so what is it that his new test does to the repo, that makes it impossible for any other test to perform its ops in its own resource? Is the resource tree somehow intentionally damaged by the test? Moreover, the name of the annotation currently doesn't make sense. "LeavesCleanRepo", read as normal English, would mean: this test leaves a clean repo behind after execution. But if it leaves it clean, then why must it be destroyed and reconstructed?
Although that name was my suggestion I also already noticed it does not properly reflect the intended semantics. Am I right that what we really want for both annotations is deleting the database? What about a single annotation: @DropDatabase(before=true, after=false)
Reopening to discuss/address the annotation issue. (See my previous comment.)
Oops, mid-air collision. Reopening again.
(In reply to comment #22) > @DropDatabase(before=true, after=false) I'd prefer @CleanRepositories(before=true, after=false), because I think it should be left to to the repository to decide what that involves. (This also makes more sense in the code, where the annotation basically triggers deactivation of the repos, not deactivation of the underlying store.) Still, I'm curious to hear of an example when after=true would truly make sense...
> Still, I'm curious to hear of an example when after=true would truly make > sense... Given test creates and leaves custom revisions in a cache which are not compatible with revisions provided by the rest CDO test. While repository along with cache is shared among other tests, thus cache must be cleared. @CleanRepositories(before=true, after=false) is ok for me. Perhaps name should be singular "CleanRepository" as we talk about one repository cleaning. And I prefer "before=true" to make a default value for annotation. Annotation with "before=false" doesn't make sense to use in test logic as it is default behavior.
(In reply to comment #26) > > Still, I'm curious to hear of an example when after=true would truly make > > sense... > > Given test creates and leaves custom revisions in a cache I should have realized this earlier. If that's true, that's certainly evil by itself and should probably not be worked around by test annotations that fix the bad state for the following tests. > @CleanRepositories(before=true, after=false) is ok for me. Perhaps name should > be singular "CleanRepository" as we talk about one repository cleaning. I think this refers to all repositories that have been created for a test case. Most of the tests create only one repo, but some create more than one. > And I > prefer "before=true" to make a default value for annotation. Annotation with > "before=false" doesn't make sense to use in test logic as it is default > behavior. I'd prefer an enum { BEFORE, AFTER, BEFORE_AND_AFTER } with BEFORE being the default literal. But maybe we should first see if we still need that at all or if the underlying problem needs to be addressed where it's caused, as Caspar suggested.
I find the whole idea of substituting 'custom revisions' very troubling. Allowing the readAccessHandler to do this isn't too difficult; but what logic do we have in place to maintain any kind of sanity when working with these custom revisions? I wonder why we don't try to develop/maintain solutions that are analogous with well-established, time-tested systems. In the case of security, we could look at DBMS's and file systems. I know of no system that involves the creation of 'custom records' or 'custom inodes' to implement security. Or generally: I know of no systems that implement security by creating copies of data, then changing those copies and handing them off to other parts of the system that are ignorant of this. But that's just my 2 cents worth.
(In reply to comment #28) > I find the whole idea of substituting 'custom revisions' very > troubling. Allowing the readAccessHandler to do this isn't too > difficult; but what logic do we have in place to maintain any kind of > sanity when working with these custom revisions? > > I wonder why we don't try to develop/maintain solutions that are > analogous with well-established, time-tested systems. In the case > of security, we could look at DBMS's and file systems. I know of > no system that involves the creation of 'custom records' or 'custom > inodes' to implement security. Or generally: I know of no systems > that implement security by creating copies of data, then changing > those copies and handing them off to other parts of the system that > are ignorant of this. > > But that's just my 2 cents worth. I didn't catch idea "In the case of security, we could look at DBMS's and file systems". How could it help here? Anyway, what security could be provided by CDO, we could discus in another separate bugzilla. This issue is about feature, which will allow to replace revisions with custom one. Perhaps, we should focus to solve it.
> I should have realized this earlier. If that's true, that's certainly evil by > itself and should probably not be worked around by test annotations that fix > the bad state for the following tests. This test shows capability of this feature to create custom revisions and share them among server-client. Can we call that custom revisions in a cache is "bad state"? Currently for this test, it is a "good state". Obviously, if such state is delivered to another test, it is normal, that another test should not be capable to work with data, left by previous test. Maybe, each test must work on clear state? IMO, to leave state, known by default as "good state", for rest of test system, is not an workaround, but just another testing framework functionality (like clean revision before test, which seems to be not "evil") > But maybe we should first see if we still need that at all or if the underlying > problem needs to be addressed where it's caused, as Caspar suggested. How could we fix it in another way?
Revisions are always read in the context of a session. Security is also supposed to judge in this context. Because revisions are expected to be immutable and always/everywhere the same we currently put them in a cross-session cache. From there they will be taken and handed to any requesting session. I'm pretty sure that we can safely call that bad state ;-) > How could we fix it in another way? If we decide that replacing or modifying revisions is a good thing at all then we certainly must make this fact known to the general core framework. That would require to add security related infos at least to InternalCDORevision. That would be easy. A bit harder would be to identify all the places that would have to react to this new information.
(In reply to comment #29) > Anyway, what security could be provided by > CDO, we could discus in another separate bugzilla. We could, but I get the impression that your justification for *this* feature is that it is required to implement security. Is this not the case? > I didn't catch idea "In the case of security, we could look at DBMS's and file > systems". How could it help here? Generally, by providing models/approaches that have been shown to work well. As I said, to the best of my knowledge, those approaches don't include substituting fabricated data for real data. Specifically, to take the example from your first comment on this Bugzilla: if one object/record references another object/record in another table, and that second record/object cannot be accessed because the user doesn't have the requisite privileges for that, then a DBMS will somehow communicate the failure to select, rather than return a fabricated record. This seems like an adequate model to me. Why can't we follow such an approach? Note that I'm not saying that CDO is currently adequately equippred to make this possible. I'm just saying: this seems like a simple, proven approach, and so I don't see why we don't pursue that instead of an new mechanism that will be entirely idiosyncratic to CDO and has no analogies in existing systems with security features.
(In reply to comment #31) > Revisions are always read in the context of a session. Security is also > supposed to judge in this context. Because revisions are expected to be > immutable and always/everywhere the same we currently put them in a > cross-session cache. From there they will be taken and handed to any requesting > session. I'm pretty sure that we can safely call that bad state ;-) Still do not agree. Revisions are immutable and are the same, but they are the same only in the same context. I see here two context: server and client. Let the server have it's own type, state of revisions and client side will be on his own revisions. It is not bad state, if we are talking about different context. Talking about test, each test can be viewed as separate context with it's own revision kind setup. > If we decide that replacing or modifying revisions is a good thing at all then > we certainly must make this fact known to the general core framework. That > would require to add security related infos at least to InternalCDORevision. > That would be easy. A bit harder would be to identify all the places that would > have to react to this new information. While we provide easy way for revision replacement, then implementator is free for providing secured context from server to client with custom revisions. I have nothing against, that it would be general CDO security mechanism, but for this we need much more work to do (will it be possible in CDO 4.0?). I think, we should discuss first, what kind of protection it should carry, how could it be extended, how protection will act on client code, how do we need to make CDO client aware about protection. I think, we have for this https://bugs.eclipse.org/bugs/show_bug.cgi?id=343084
(In reply to comment #32) > (In reply to comment #29) > > > Anyway, what security could be provided by > > CDO, we could discus in another separate bugzilla. > > We could, but I get the impression that your justification for *this* > feature is that it is required to implement security. Is this not the > case? Yes, it is possibility to implement security, but it doesn't keep security concept for CDO. I thing, these concepts should rice here https://bugs.eclipse.org/bugs/show_bug.cgi?id=343084 > Generally, by providing models/approaches that have been shown to work > well. As I said, to the best of my knowledge, those approaches don't > include substituting fabricated data for real data. > > Specifically, to take the example from your first comment on this Bugzilla: > if one object/record references another object/record in another table, > and that second record/object cannot be accessed because the user doesn't > have the requisite privileges for that, then a DBMS will somehow communicate > the failure to select, rather than return a fabricated record. This seems > like an adequate model to me. Why can't we follow such an approach? > > Note that I'm not saying that CDO is currently adequately equippred to > make this possible. I'm just saying: this seems like a simple, proven > approach, and so I don't see why we don't pursue that instead of an > new mechanism that will be entirely idiosyncratic to CDO and has no > analogies in existing systems with security features. I do not think, that it will work with model. Model is built from objects, tightly related to each other. It could be that I can modify one element in a list, but do not have permission to read another. If we will attempt to fail on every revision reading, which do not have permission to be read, EMF model won't work.
(In reply to comment #33) > (In reply to comment #31) > > Revisions are always read in the context of a session. Security is also > > supposed to judge in this context. Because revisions are expected to be > > immutable and always/everywhere the same we currently put them in a > > cross-session cache. From there they will be taken and handed to any requesting > > session. I'm pretty sure that we can safely call that bad state ;-) > > Still do not agree. Revisions are immutable and are the same, but they are the > same only in the same context. I see here two context: server and client. Let > the server have it's own type, state of revisions and client side will be on > his own revisions. It is not bad state, if we are talking about different > context. Talking about test, each test can be viewed as separate context with > it's own revision kind setup. I'm only talking about the server, the revision cache there in particular. If revisions end up in this cache that are only valid in the context of a specific session that is bad state for all the other sessions. > > If we decide that replacing or modifying revisions is a good thing at all then > > we certainly must make this fact known to the general core framework. That > > would require to add security related infos at least to InternalCDORevision. > > That would be easy. A bit harder would be to identify all the places that would > > have to react to this new information. > > While we provide easy way for revision replacement, then implementator is free > for providing secured context from server to client with custom revisions. Still these customo revisions must not end up in the shared revision cache if they have been created for a specific session. > I have nothing against, that it would be general CDO security mechanism, but > for this we need much more work to do (will it be possible in CDO 4.0?). Everything except API change is still allowed after M7. > I think, > we should discuss first, what kind of protection it should carry, how could it > be extended, how protection will act on client code, how do we need to make > CDOclient aware about protection. > I think, we have for this bug 343084 Yes.
(In reply to comment #35) > I'm only talking about the server, the revision cache there in particular. If > revisions end up in this cache that are only valid in the context of a specific > session that is bad state for all the other sessions. > Still these customo revisions must not end up in the shared revision cache if > they have been created for a specific session. I think it is misunderstanding. Lets assume that we do not have Read handler at all. Test is using it's own *CustomCDORevision*. CustomCDORevision has one additional field which is used for serialization from server to client. CustomCDORevision type and instances are the same for all sessions in the server (as Eike says). But other test uses CDORevision, which are not aware about CustomCDORevision and about it's serialized custom field. Letting to mix different revisions, we will get error in protocol exchange on revision loading. Thus, we need clean repo before test and we need to leave clean after test. Read handler in this test shows one of the way how revision could be modified (ensures that revisions are replaced in read handler). Note, that modified revision is *copied* before modification and therefore it doesn't change revision existing in the cache. Thus *all* sessions on the server uses the same revisions, only for different clients are send different revisions.
It seems I was wrong and the ReadAccessHandler replaces the revisions *after* they've already been cached. Then I wonder what made the tests fail so that we felt the need to introduce the post-cleansing?
If the failures just result from the custom CDORevisionFactory or ReadAccessHandler that you leave in the repository, then we should probably just remove them in a finally block in those tests rather than triggering creation of a new repo.
(In reply to comment #37) > It seems I was wrong and the ReadAccessHandler replaces the revisions *after* > they've already been cached. Then I wonder what made the tests fail so that we > felt the need to introduce the post-cleansing? In test is used custom revision, which serializes one additional field via stream. While revision is responsible to read and write bytes by its own policy, then mixing two different revisions kinds, mixes serialization policies.
(In reply to comment #38) > If the failures just result from the custom CDORevisionFactory or > ReadAccessHandler that you leave in the repository, then we should probably just > remove them in a finally block in those tests rather than triggering creation of > a new repo. If you do not desire to have re-usable control for clearing repository after test, then I could try to make it just in a test. But I don't think it is good way. I would add it as an option for test configuration, especially when this option is already coded, just need to adjust naming.
(In reply to comment #40) > If you do not desire to have re-usable control for clearing repository after > test, then I could try to make it just in a test. But I don't think it is good > way. I would add it as an option for test configuration, especially when this > option is already coded, just need to adjust naming. I don't have an objection in principle against controllable post-test cleanup, but it should be discouraged. It's tempting to "just tell the framework to clean the repos" instead of re- working the test so that such cleanup isn't necessary. But then in a while our DB testsuites will once again be so slow that nobody will want to run them anymore. In the case of this particular test, I still don't get it. Perhaps I'm a little thick, but I don't understand why subsequent tests would be accessing this test's revisions at all. A subsequent test should set up its own resource and its own test data in that resource, unless the test is by its very nature one that cannot isolate its data into a test-specific resource. The point is: this decision can only be made by the subsequent test. In short, I still wonder: how does *this* test know that the *next* test cannot avoid running into the mess that it is leaving behind? If you/someone can explain to me why *only* this test can make that judgment, then clearly we do need the new annotation and I will drop any objections.
I can't explain it either, although I still have the suspicion I mentioned in comment 38.
Given test creates and leaves custom revisions in a cache, which are not compatible with revisions provided by the rest CDO test. While repository, along with cache, is shared among other tests, thus cache must be cleared from custom revisions. For example, there is revision for ROOT in server cache. This revision is of type X. Client works with revisions Y, which is not compatible with revision X in serialization meaning. How do you think, what will happen, if client, will ask revision for ROOT object from server? Server will serialize revision data by the X revision's rules, and client will start to read from stream by Y revision rules. You just need to clean cache and problem is gone.
I think I got it now. You'd have to replace the revisionFactory, the readAccessHandler and purge the repository cache after these tests. I think this annotation would be justified.
I concur :-)
ok, then, what is verdict for annotation? Leave or change? @CleanRepositories(before=true, after=false) @CleanRepositories({ BEFORE, AFTER, BEFORE_AND_AFTER })
(In reply to comment #46) > ok, then, what is verdict for annotation? Leave or change? Change for sure. > @CleanRepositories(before=true, after=false) > @CleanRepositories({ BEFORE, AFTER, BEFORE_AND_AFTER }) I prefer the option with the booleans, because the enum will end up being something overly verbose like CleanRepositories.Timing.BEFORE_AND_AFTER
I had talked with Eike few days ago and he proposed Enum with two values {BEFORE, AFTER} and annotation field of array type, where annotation field should have default {BEFORE}. Excerpt from the chat: Eike Stepper 4/29/11 11:22 AM what about an array of the enum type as parameter? 4/29/11 11:22 AM then the enum only needs two literals 4/29/11 11:23 AM empty array or null:=BEFORE 4/29/11 11:23 AM would that work?
(In reply to comment #48) It's fine either way. I was just trying to move this along because I didn't see any activity.
Annotation array leaves unpredictable state, when annotation is used with empty array. Of course, code can explicitly treat as it is BEFORE set in this case, even because that BEFORE is default, when annotation member is omitted. But this would be different from the expected behavior and leaves place for misunderstanding. I think, boolean values are more common and clear.
(In reply to comment #50) Hehe, this is getting pretty funny. It all seems needlessly complicated to me. Introducing 1 new enum type with 3 values, so 4 new identifiers, or introducing 2 booleans whose meaning can only be understood if one opens the annotation's declaration; all of this in order to identify 1 single condition?? :S How about: @CleanRepositoriesBefore @CleanRepositoriesAfter I pat myself on the back for thinking of this ;-)
(In reply to comment #51) > How about: > > @CleanRepositoriesBefore > @CleanRepositoriesAfter Let's go for it ;-)
Created attachment 195836 [details] Patch for annotation naming Circling around and got to the same point :). Ok, I have changed NeedsCleanRepo -> CleanRepositoriesBefore; LeavesCleanRepo -> CleanRepositoriesAfter
Committed revision 7764: - trunk/plugins/org.eclipse.emf.cdo.dawn.tests - trunk/plugins/org.eclipse.emf.cdo.tests - trunk/plugins/org.eclipse.emf.cdo.tests.db
Thanks Egidijus! I committed it for you because I need to get our RC1 build out...
Available in R20110608-1407
