Bug 84023 – [EFL] Add Security Origin API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Thiago Marcos P. Santos]

Currently on Ewk_Frame there is no way to get the Security Origin, which is needed for fine grained security policy. Security Origin is used by APIs such as database for specifying quota per origin and others like geolocation to grant authorization. A convenience wrapper such as Ewk_Security_Origin should be implemented.

The current security model is based on whitelisting. A more fine tuned approach would be aligned to other ports and device APIs that we might implement some day.

Reference for implementation:
http://doc.qt.nokia.com/4.7-snapshot/qwebframe.html#securityOrigin
http://webkitgtk.org/reference/webkitgtk/stable/WebKitWebFrame.html#webkit-web-frame-get-security-origin

[Thiago Marcos P. Santos]

Created attachment 137959 [details]
patch adding the API

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 137959 [details]
patch adding the API

View in context: https://bugs.webkit.org/attachment.cgi?id=137959&action=review

> Source/WebKit/efl/ChangeLog:8
> +        to define quota and get usage information of a Web Storage based on it's

s/it's/its/

> Source/WebKit/efl/ewk/ewk_frame.cpp:296
> +    if (!smartData->frame->document() || !smartData->frame->document()->securityOrigin())
> +        return 0;

These could be wrapped into EINA_SAFETY_ON_NULL_RETURN_VAL()s too.

> Source/WebKit/efl/ewk/ewk_frame.cpp:298
> +    return ewk_security_origin_new(smartData->frame->document()->securityOrigin());

I wonder if it doesn't make sense to keep the same object around instead of always creating a new one, and updating it when the origin changes.

> Source/WebKit/efl/ewk/ewk_security_origin.cpp:97
> +    Ewk_Security_Origin* origin = new Ewk_Security_Origin();

Please use `new Foo' instead of `new Foo()'.

> Source/WebKit/efl/ewk/ewk_security_origin.cpp:101
> +    origin->host = 0;
> +    origin->protocol = 0;

Why not do the eina_stringshare_add() calls here and make the getters simply return the values set in this function?

[Thiago Marcos P. Santos]

(In reply to comment #2)
> (From update of attachment 137959 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137959&action=review
> 
> > Source/WebKit/efl/ChangeLog:8
> > +        to define quota and get usage information of a Web Storage based on it's
> 
> s/it's/its/
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:296
> > +    if (!smartData->frame->document() || !smartData->frame->document()->securityOrigin())
> > +        return 0;
> 
> These could be wrapped into EINA_SAFETY_ON_NULL_RETURN_VAL()s too.
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:298
> > +    return ewk_security_origin_new(smartData->frame->document()->securityOrigin());
> 
> I wonder if it doesn't make sense to keep the same object around instead of always creating a new one, and updating it when the origin changes.
> 

I thought about doing that (like GTK does) but IMO this wrapper is way too lightweight to deserve being cached somewhere and all the maintenance involved. The "heavy" part of it, which is the WebCore::SecurityOrigin is already refcounted.

> > Source/WebKit/efl/ewk/ewk_security_origin.cpp:97
> > +    Ewk_Security_Origin* origin = new Ewk_Security_Origin();
> 
> Please use `new Foo' instead of `new Foo()'.
> 
> > Source/WebKit/efl/ewk/ewk_security_origin.cpp:101
> > +    origin->host = 0;
> > +    origin->protocol = 0;
> 
> Why not do the eina_stringshare_add() calls here and make the getters simply return the values set in this function?

Lazy loading. In many cases you just need the origin object to set a database quota.

Thanks for reviewing. /me will fix the remaining issues

[Thiago Marcos P. Santos]

Created attachment 137989 [details]
patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 137989 [details]
patch

Looks fine to me.

[Grzegorz Czajkowski]

Is there any reason of adding a new separate private file for Security Origin feature (ewk_security_origin_private.h)? 

WebKit-EFL tends to keep one private file for all components (view, frame etc.) Maybe it's good point to consider it. 

CC'ing Tomasz who proposed this idea here  https://lists.webkit.org/pipermail/webkit-efl/2012-February/000132.html

[Thiago Marcos P. Santos]

(In reply to comment #6)
> Is there any reason of adding a new separate private file for Security Origin feature (ewk_security_origin_private.h)? 
> 
> WebKit-EFL tends to keep one private file for all components (view, frame etc.) Maybe it's good point to consider it. 
> 
> CC'ing Tomasz who proposed this idea here  https://lists.webkit.org/pipermail/webkit-efl/2012-February/000132.html

The points are exactly the ones mentioned by Tomasz Morawski, which are mainly code readability.

ewk_tiled.h already has an ewk_tiled_private.h, I'm trying to keep this positive trend.

[Thiago Marcos P. Santos]

Created attachment 138054 [details]
use ewk_private.h instead of ewk_security_origin_private.h

Using a big ewk_private.h is more EFLish but we should discuss on the mailing lists whenever this model scales to several APIs.

[Grzegorz Czajkowski]

(In reply to comment #8)
> Created an attachment (id=138054) [details]
> use ewk_private.h instead of ewk_security_origin_private.h
> 
> Using a big ewk_private.h is more EFLish but we should discuss on the mailing lists whenever this model scales to several APIs.

Actually I didn't mind your previous patch. Especially if Kubo has already approved it.
Anyway LGTM.

[WebKit Review Bot]

Comment on attachment 138054 [details]
use ewk_private.h instead of ewk_security_origin_private.h

Clearing flags on attachment: 138054

Committed r114749: <http://trac.webkit.org/changeset/114749>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.