Bug 84023 - [EFL] Add Security Origin API
Summary: [EFL] Add Security Origin API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thiago Marcos P. Santos
URL:
Keywords:
Depends on:
Blocks: 84170
  Show dependency treegraph
 
Reported: 2012-04-16 05:23 PDT by Thiago Marcos P. Santos
Modified: 2012-04-20 09:08 PDT (History)
9 users (show)

See Also:


Attachments
patch adding the API (13.78 KB, patch)
2012-04-19 13:16 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
patch (13.76 KB, patch)
2012-04-19 15:10 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
use ewk_private.h instead of ewk_security_origin_private.h (13.05 KB, patch)
2012-04-20 00:27 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago Marcos P. Santos 2012-04-16 05:23:24 PDT
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
Comment 1 Thiago Marcos P. Santos 2012-04-19 13:16:36 PDT
Created attachment 137959 [details]
patch adding the API
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-04-19 13:54:05 PDT
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?
Comment 3 Thiago Marcos P. Santos 2012-04-19 14:05:07 PDT
(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
Comment 4 Thiago Marcos P. Santos 2012-04-19 15:10:30 PDT
Created attachment 137989 [details]
patch
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-04-19 15:25:53 PDT
Comment on attachment 137989 [details]
patch

Looks fine to me.
Comment 6 Grzegorz Czajkowski 2012-04-19 23:15:22 PDT
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
Comment 7 Thiago Marcos P. Santos 2012-04-19 23:32:36 PDT
(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.
Comment 8 Thiago Marcos P. Santos 2012-04-20 00:27:39 PDT
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.
Comment 9 Grzegorz Czajkowski 2012-04-20 01:21:46 PDT
(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.
Comment 10 WebKit Review Bot 2012-04-20 09:08:24 PDT
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>
Comment 11 WebKit Review Bot 2012-04-20 09:08:30 PDT
All reviewed patches have been landed.  Closing bug.