authorization update for self editors #106

hudajkhan · 2018-12-13T03:06:50Z

Thank you for submitting a pull request! Title this pull request with a brief description of what the pull request fixes/improves/changes. Please describe the pull request in detail using the template below.

No linked JIRA issue:
We didn't create a specific JIRA issue although discussed this using other channels

Other Relevant Links (Mailing list discussion, related pull requests, etc.)

What does this pull request do?

Ensures self-editors cannot access edit or add links using URLs

What's new?

The code which checks authorization now adds a check to see if this particular property can be edited or added for the current user. This check adds to the front end editing permission check.

How should this be tested?

I tested this using the following steps: When logged in as a site admin, I copied the URLs for adding and editing a data property and an object property. I then logged out and logged back in as a self editor and then tried to access these URLs. The page should not allow the form to display but should state that the user does not have access to this page.

Additional Notes:

Additional testing would be preferable for more editing scenarios across different roles.

Interested parties

@VIVO-project/vivo-committers and specifically @gneissone and @j2blake

gneissone · 2018-12-13T22:26:43Z

...du/cornell/mannlib/vitro/webapp/edit/n3editing/controller/EditRequestDispatchController.java

@@ -13,11 +13,15 @@

 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-
+import org.apache.jena.ontology.OntModel;
+import org.apache.jena.rdf.model.ResourceFactory;


Suggested change

import org.apache.jena.rdf.model.ResourceFactory;

Can this import be removed?

gneissone

It is working, but I think it is possible it's working a little too well. I am not able to add or edit any faux properties as a self-editor.

awoods · 2019-01-14T22:01:22Z

@hudajkhan : would you mind creating a JIRA for this PR so that it does not get lost in the shuffle?

gneissone · 2019-11-07T02:33:24Z

@hudajkhan I have not forgotten about your efforts. I wonder if @brianjlowe could help us here? I know at the very least he has leveraged VIVO's permission code before. @brianjlowe this was previously discussed via the committers channels, happy to clarify the issue if anything is not clear.

awoods · 2020-01-15T14:20:20Z

@brianjlowe ? @vivo-project/vivo-committers ? any comments on this bug fix?

awoods · 2020-01-23T05:21:30Z

@hudajkhan / @gneissone : While reproducing the behavior of a self-editor not being able to edit their own faux property, I captured the DEBUG log: https://gist.github.com/awoods/157eab4ab31b844be014669ac374915b

It appears that both AddObjectPropertyStatement and AddDataPropertyStatement actions are unrecognized by any policy and therefore "INCONCLUSIVE".

My question is: What permission do we expect to be in the session that would recognized (and allow) the above actions?

awoods · 2020-01-24T19:21:53Z

Investigating further, @hudajkhan 's logic appears to work as expected (user can update their own profile, properties and faux properties... and not other profiles) only when the base-property from which the faux property is created has a #selfEditor update level.

Using the debugger, the edit requests on faux properties are declined due to the PropertyRestrictionBeanImpl returning nobody instead of selfEditor. When I change the value of these properties to "self-editor and above" in the Faux Property editor form in the "site admin"... the functionality appears to be correct.

I would expect that the appropriate changes could be made in vitroAnnotations.n3 as well.

The remaining question is, are these properties' "prohibitedFromUpdateBelowRoleLevelAnnot" values set to "nobody" for some other reason?

gneissone · 2020-01-24T22:44:11Z

The remaining question is, are these properties' "prohibitedFromUpdateBelowRoleLevelAnnot" values set to "nobody" for some other reason?

The authorization code should respect the settings in PropertyConfig.n3, right? The editing levels for each individual faux property need to be controlled separately, even if they share a base property.

brianjlowe · 2020-01-24T22:47:13Z

Benjamin is correct. Faux property visibility and editing levels override those of their parent property. It is a common occurrence that very generic base properties like "relates" are set to nobody (i.e. only root) while the more specific faux properties have greater visibility and editability.

gneissone · 2020-02-05T16:08:42Z

Reopening this pull request. In an effort to follow current best practices, the VIVO project will now be developed against the master branch. As a result of removing the develop branch, this pull request was automatically closed by GitHub, apologies for any confusion this may have caused.

brianjlowe · 2020-02-06T14:26:21Z

After taking a quick look at this, I think the problem is that predicateProp doesn't have its domainVClassURI and rangeVClassURI values set so that it can be recognized as a faux property if applicable.

If we're lucky, it may be just a matter of adding after line 73:

predicateProp.setDomainVClassURI(EditConfigurationUtils.getDomainUri(vreq));
predicateProp.setRangeVClassURI(EditConfigurationUtils.getRangeUri(vreq));

I'll try to test this out when I get a chance.

gneissone · 2020-02-07T06:36:40Z

Thanks for taking a look @brianjlowe. Unfortunately I'm still not able to edit faux properties with the change. I get redirected to the home page and an error is displayed We're sorry, but you are not authorized to view the page you requested. If you think this is an error, please contact us and we'll be happy to help.

brianjlowe · 2020-02-07T12:01:05Z

Thanks, Benjamin. Turns out the domain and range were only part of it. This version appears to be working as expected:

    @Override
    protected AuthorizationRequest requiredActions(VitroRequest vreq) {
        //Check if this statement can be edited here and return unauthorized if not
        String subjectUri = EditConfigurationUtils.getSubjectUri(vreq);
        String predicateUri = EditConfigurationUtils.getPredicateUri(vreq);
        String objectUri = EditConfigurationUtils.getObjectUri(vreq);
        String domainUri = EditConfigurationUtils.getDomainUri(vreq); 
        String rangeUri = EditConfigurationUtils.getRangeUri(vreq);
        Property predicateProp = new Property();
        predicateProp.setURI(predicateUri);
        predicateProp.setDomainVClassURI(domainUri);
        predicateProp.setRangeVClassURI(rangeUri);
        OntModel ontModel = ModelAccess.on(vreq).getOntModel();        
        AbstractObjectPropertyStatementAction objectPropertyAction;
        if (objectUri == null) {
            objectPropertyAction = new AddObjectPropertyStatement(
                    ontModel, subjectUri, predicateProp, RequestedAction.SOME_URI);            
        } else {            
            if (isDeleteForm(vreq)) {
                objectPropertyAction = new DropObjectPropertyStatement(
                        ontModel, subjectUri, predicateProp, objectUri);
            } else {
                objectPropertyAction = new EditObjectPropertyStatement(
                        ontModel, subjectUri, predicateProp, objectUri);
            }
        }        
        boolean isAuthorized = PolicyHelper.isAuthorizedForActions(vreq, 
                new EditDataPropertyStatement(ontModel, subjectUri, predicateUri, objectUri).
                or(objectPropertyAction));
        if (isAuthorized) {
            return SimplePermission.DO_FRONT_END_EDITING.ACTION;
        } else { 
            return AuthorizationRequest.UNAUTHORIZED;
        }
}

awoods · 2020-02-10T15:15:26Z

@brianjlowe : Can you submit a pull-request against @hudajkhan 's branch:
https://github.com/hudajkhan/Vitro/tree/updateEditing

awoods · 2020-02-11T15:23:17Z

@brianjlowe / @hudajkhan / @gneissone : My local testing verifies Brian's PR both allows for self-edit of all properties, while prohibiting edit of other profiles.

hudajkhan · 2020-02-19T05:26:50Z

The file has been updated based on suggestions from Brian Lowe and Benjamin Gross

awoods · 2020-02-19T14:41:54Z

...du/cornell/mannlib/vitro/webapp/edit/n3editing/controller/EditRequestDispatchController.java

@@ -43,71 +51,101 @@
 * by the N3 editing system.  It will examine the request parameters, determine
 * which form to use, execute a EditConfiguration setup, and evaluate the
 * view indicated by the EditConfiguration.
- * 


@hudajkhan : there are a lot of whitespace changes that clutter this pull-request. Would you be able to limit the updates to the functional bits?

awoods

Works as expected. I would be happy to merge now... unless anyone else would like to review first.

awoods · 2020-02-20T23:38:47Z

I believe @gneissone still has an outstanding "request for change"... which has likely been addressed.

gneissone

Works as intended, thank you Huda and Brian.

authorization update for self editors

a43da3e

gneissone reviewed Dec 13, 2018

View reviewed changes

gneissone requested changes Dec 13, 2018

View reviewed changes

awoods closed this Jan 28, 2020

gneissone reopened this Feb 5, 2020

gneissone changed the base branch from develop to master February 5, 2020 16:08

hudajkhan added 2 commits February 18, 2020 23:42

updating with Brian Lowe's suggestions

81665f9

removing extra import per Benjamin Gross's suggestion

554d9f7

awoods requested changes Feb 19, 2020

View reviewed changes

hudajkhan added 4 commits February 19, 2020 10:32

updating indentations

e3388a4

hoping indentation fixed

89584bd

merging in from master

1960fac

updating code after merging in from master

b2f8d42

awoods approved these changes Feb 20, 2020

View reviewed changes

gneissone self-requested a review February 21, 2020 00:43

gneissone approved these changes Feb 21, 2020

View reviewed changes

gneissone merged commit 7420957 into vivo-project:master Feb 21, 2020

gneissone mentioned this pull request Jul 20, 2020

[VIVO-1907] - Re-add logic from commit 7420957 #176

Merged

gneissone mentioned this pull request Jul 12, 2022

fix: allow self editors to create individuals in faux property forms #317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

authorization update for self editors #106

authorization update for self editors #106

hudajkhan commented Dec 13, 2018

gneissone Dec 13, 2018

gneissone left a comment

awoods commented Jan 14, 2019

gneissone commented Nov 7, 2019

awoods commented Jan 15, 2020

awoods commented Jan 23, 2020 •

edited

Loading

awoods commented Jan 24, 2020

gneissone commented Jan 24, 2020

brianjlowe commented Jan 24, 2020

gneissone commented Feb 5, 2020

brianjlowe commented Feb 6, 2020

gneissone commented Feb 7, 2020

brianjlowe commented Feb 7, 2020

awoods commented Feb 10, 2020

awoods commented Feb 11, 2020

hudajkhan commented Feb 19, 2020

awoods Feb 19, 2020

awoods left a comment

awoods commented Feb 20, 2020

gneissone left a comment

authorization update for self editors #106

authorization update for self editors #106

Conversation

hudajkhan commented Dec 13, 2018

What does this pull request do?

What's new?

How should this be tested?

Additional Notes:

Interested parties

gneissone Dec 13, 2018

Choose a reason for hiding this comment

gneissone left a comment

Choose a reason for hiding this comment

awoods commented Jan 14, 2019

gneissone commented Nov 7, 2019

awoods commented Jan 15, 2020

awoods commented Jan 23, 2020 • edited Loading

awoods commented Jan 24, 2020

gneissone commented Jan 24, 2020

brianjlowe commented Jan 24, 2020

gneissone commented Feb 5, 2020

brianjlowe commented Feb 6, 2020

gneissone commented Feb 7, 2020

brianjlowe commented Feb 7, 2020

awoods commented Feb 10, 2020

awoods commented Feb 11, 2020

hudajkhan commented Feb 19, 2020

awoods Feb 19, 2020

Choose a reason for hiding this comment

awoods left a comment

Choose a reason for hiding this comment

awoods commented Feb 20, 2020

gneissone left a comment

Choose a reason for hiding this comment

awoods commented Jan 23, 2020 •

edited

Loading