batchee-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Struberg <strub...@yahoo.de.INVALID>
Subject Re: bye bye security service?
Date Wed, 17 Aug 2016 20:12:38 GMT
That’s a pretty strong argument ;)

Go for it, waiting for the JIRA ticket.

LieGrue,
strub


> Am 17.08.2016 um 21:44 schrieb Romain Manni-Bucau <rmannibucau@gmail.com>:
> 
> Le 17 août 2016 21:42, "Mark Struberg" <struberg@yahoo.de.invalid> a écrit :
>> 
>> Would work.
>> 
>> Just keep in mind that throwing one halfway (well rather quarter-way)
> decent solution out is only a good idea if we know that it’s technically
> possible to implement a superior solution later ;)
>> 
> 
> Well we start from something not used. Pretty sure we can do it as good as
> current one ;)
> 
>> LieGrue,
>> strub
>> 
>> 
>>> Am 17.08.2016 um 21:38 schrieb Romain Manni-Bucau <rmannibucau@gmail.com
>> :
>>> 
>>> Think it works. Can we just jeep the topic of removing current security
> on
>>> this thread then happy to discuss new impl on another one. Would that
> work?
>>> 
>>> Le 17 août 2016 21:23, "Mark Struberg" <struberg@yahoo.de.invalid> a
> écrit :
>>> 
>>>> I like your idea with wrapping the whole JobOperator IF security is
> needed.
>>>> Would that be possible? Or do we not have all the information on that
> high
>>>> level?
>>>> Just brainstorming…
>>>> 
>>>> LieGrue,
>>>> strub
>>>> 
>>>> 
>>>>> Am 17.08.2016 um 20:39 schrieb Romain Manni-Bucau <
> rmannibucau@gmail.com
>>>>> :
>>>>> 
>>>>> Le 17 août 2016 20:30, "Mark Struberg" <struberg@yahoo.de.invalid>
a
>>>> écrit :
>>>>>> 
>>>>>> Hah, I already wondered if somebody would really use that
>>>> SecurityService
>>>>> ;)
>>>>>> 
>>>>>> Well, I can see that it _might_ make sense in certain integration
>>>>> scenarios for big irons.
>>>>>> But otoh I am totally with Scott that securing getJobNames() is
>>>>> debateable.
>>>>>> 
>>>>>> What would a user do with the job name even if he would not be
> allowed
>>>> to
>>>>> look at job instances?
>>>>>> Imo not much. He might click on the job name and then he would end
up
>>>>> with an empty list.
>>>>>> Not perfect, but also not much of a security problem.
>>>>>> 
>>>>>> I’m personally +0 for removing the SecurityService.
>>>>>> While I personally never used it (nor ever seen it being used), I
can
>>>>> imagine that it _might_ be of use for some scenarios.
>>>>>> And it doesn’t add that much overhead (beside in getJobNames).
>>>>>> 
>>>>> 
>>>>> @reinhard: any opinion with the new inputs?
>>>>> 
>>>>> If we all agree I can try to remove it before next release.
>>>>> 
>>>>>> LieGrue,
>>>>>> strub
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>>> Am 17.08.2016 um 20:21 schrieb Scott Kurz <skurz3@gmail.com>:
>>>>>>> 
>>>>>>> Given that the batch spec says nothing about the specifics of
>>>>>>> authorization, the function in the original jbatch RI was driven
by
>>>>>>> requests from the Glassfish team.
>>>>>>> 
>>>>>>> From my real-world IBM experience I've seen value in associating
an
>>>>>>> instance with a submitter and doing further authorization checks
>>>> against
>>>>>>> that user (e.g. on stop(), etc.).   But for *getJobNames()*,
the
> jbatch
>>>>>>> approach is debatable; it could also work to allow everyone to
see
>>>> every
>>>>>>> job name and authorize in other ways... e.g. against the instance.
>>>>>>> 
>>>>>>> Actually I just chimed in because I'd been wondering what you
did
> with
>>>>> this
>>>>>>> security service but had never really looked.
>>>>>>> 
>>>>>>> So are you suggesting removing it completely or just for
>>>>>>> *getJobNames()?  *
>>>>>>> 
>>>>>>> No strong opinion, just curious.
>>>>>>> 
>>>>>>> Scott
>>>>>>> 
>>>>>>> 
>>>>>>> On Wed, Aug 17, 2016 at 2:01 PM, Romain Manni-Bucau <
>>>>> rmannibucau@gmail.com>
>>>>>>> wrote:
>>>>>>> 
>>>>>>>> Le 17 août 2016 19:33, "Reinhard Sandtner" <
>>>> reinhard.sandtner@gmail.com>
>>>>> a
>>>>>>>> écrit :
>>>>>>>>> 
>>>>>>>>> Hey folks,
>>>>>>>>> 
>>>>>>>>> i’m not sure i get the use case right now ;)
>>>>>>>>> 
>>>>>>>>> As i understand, you can have read, update ([re]start,
stop)
>>>>> permissions…
>>>>>>>> and i think it makes sense to do this for some jobs only.
Maybe i
> can
>>>>> only
>>>>>>>> read ‚MyJob1‘ but update ‚MyJob2‘.
>>>>>>>>> 
>>>>>>>>> is it really a use case to be able to update ‚MyJob3:instance2‘
> but
>>>>> can
>>>>>>>> only read ‚MyJob3:instance1‘?
>>>>>>>>> maybe it depends on the status or the ‚age‘ of the
job? i don’t
> know,
>>>>> but
>>>>>>>> it could make sense ;)
>>>>>>>> 
>>>>>>>> Yep, basically allowing to filter by state which can fully
be user
>>>>>>>> dependent (did the team which launched the job os allowed
to see my
>>>>> job for
>>>>>>>> instance)
>>>>>>>> 
>>>>>>>>> but if you only want to list the jobs, the only thing
i need to
> know
>>>>> is,
>>>>>>>> does the user have the permission to read this job. i don’t
want to
>>>>> know
>>>>>>>> any detail of it’s instances or executions - help me if
i’m
> completely
>>>>>>>> wrong pls
>>>>>>>> 
>>>>>>>> Works since you can the  re-join on the name but back to
my point
> on
>>>>> API
>>>>>>>> and releases
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> i think the security checks should not be done in the
persistence
>>>>> layer.
>>>>>>>> i’d prefer if i call persistenceService.getJobNames() to
get all
> job
>>>>> names
>>>>>>>> which are currently in the storage. i would do this check
in the
>>>>>>>> jobOperator (like it is done now) or on top of it.
>>>>>>>>> 
>>>>>>>> 
>>>>>>>> +1
>>>>>>>> 
>>>>>>>>> Another drawback would be, that we have to implement
the security
>>>>> check
>>>>>>>> in every implementation of our  PersistenceManagerService.
Mark
> and i
>>>>>>>> thought about a FilePersistenceService a few weeks (or months)
> ago. Or
>>>>> lets
>>>>>>>> say a user decides to implement his own PersistenceManagerService
> - so
>>>>> he
>>>>>>>> has to consider that checks also - and if the behavior is
> different on
>>>>> 2
>>>>>>>> PersistenceManagerServices it is imo also not a really good
idea
> (lets
>>>>> say
>>>>>>>> you use FilePersistence for local testing and JPA in production)
>>>>>>>>> 
>>>>>>>> 
>>>>>>>> That is why it was done on top of it
>>>>>>>> 
>>>>>>>>> Summary:
>>>>>>>>> I’d like to keep the SercurityService but rethink the
SPI (maybe
> it
>>>> is
>>>>>>>> good as it now)…
>>>>>>>>> The argument, it is slow lets remove it is imo not that
good.
> (btw:
>>>>> our
>>>>>>>> problem was, that getJobNames() listed ALL our batches because
of
>>>>>>>> https://issues.apache.org/jira/browse/BATCHEE-107 <
>>>>>>>> https://issues.apache.org/jira/browse/BATCHEE-107>)
>>>>>>>>> 
>>>>>>>> 
>>>>>>>> This was not the argument. The real one was: always wanted
to
> remove
>>>> it
>>>>>>>> cause not at the level security should be IMO. Also the fact
it is
> not
>>>>>>>> portable tend to make me think we should build an extension
> wrapping
>>>>> jbatch
>>>>>>>> joboperator.
>>>>>>>> 
>>>>>>>> Im happy to have a security integration but we need to discuss
the
>>>>> goal and
>>>>>>>> how we handle it.
>>>>>>>> 
>>>>>>>> Whatever we chose the security service should stay stable
for next
>>>>> release
>>>>>>>> then question is: do we remove current flavor them add a
real
> security
>>>>>>>> layer (integrated with web frontend - jaxrs and servlet)
or do we
> just
>>>>> say
>>>>>>>> it is not our responsability which is also valid IMO.
>>>>>>>> 
>>>>>>>> In summary this thread is able ack-ing current security service
is
> not
>>>>> that
>>>>>>>> useful and remove it but it doesnt prevent to handle security
but
>>>> would
>>>>>>>> need more work and discussion so another thread and shouldnt
hold
>>>>> releases
>>>>>>>> IMHO.
>>>>>>>> 
>>>>>>>> Hope it makes sense.
>>>>>>>> 
>>>>>>>>> just my 2 cents
>>>>>>>>> 
>>>>>>>>> lg
>>>>>>>>> reini
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> Am 17.08.2016 um 18:21 schrieb Romain Manni-Bucau
<
>>>>>>>> rmannibucau@gmail.com
>>>>>>>>> :
>>>>>>>>>> 
>>>>>>>>>> 2016-08-17 18:17 GMT+02:00 Mark Struberg
> <struberg@yahoo.de.invalid
>>>>> :
>>>>>>>>>> 
>>>>>>>>>>> For now I just added another method
>>>>>>>> SecurityService#isAuthorizedJobName(String
>>>>>>>>>>> jobName);
>>>>>>>>>>> 
>>>>>>>>>>> That allows us to implement the scan really quickly.
>>>>>>>>>>> Our problem is that we have batch jobs started
by UC4 all 15
>>>>> minutes.
>>>>>>>> And
>>>>>>>>>>> that summed up in the last 2 years. Now we have
over 200k
>>>>> JobInstance
>>>>>>>>>>> entries.
>>>>>>>>>>> 
>>>>>>>>>>> Now one could argue that this could be cleaned
up in the db -
> and
>>>>> this
>>>>>>>> is
>>>>>>>>>>> certainly right ;)
>>>>>>>>>>> But I still would prefer to have getJobNames()
perform really
> well.
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>> Well this means you introduce a new API and the proposal
is
> actually
>>>>> to
>>>>>>>>>> completely get rid of it. Would do something like:
>>>>>>>>>> 
>>>>>>>>>> 0.3: SecurityService.isAuthorized(id)
>>>>>>>>>> 0.4: SecurityService.isAuthorizedName(name)
>>>>>>>>>> 0.5: -
>>>>>>>>>> 
>>>>>>>>>> instead of:
>>>>>>>>>> 
>>>>>>>>>> 0.3: SecurityService.isAuthorized(id)
>>>>>>>>>> 0.4: @Deprecated SecurityService.isAuthorized(id)
>>>>>>>>>> 0.5: -
>>>>>>>>>> 
>>>>>>>>>> Don't get me wrong, I'm in favor of your impl of
getJobNames and
> we
>>>>> can
>>>>>>>>>> support it for 0.4 checking if the securityservice
is our default
>>>>> one.
>>>>>>>> This
>>>>>>>>>> is enough for most cases I think but still allows
us to get
> feedback
>>>>>>>> before
>>>>>>>>>> 0.5 and the removal of that API. You current work
breaks 0.4 and
>>>> will
>>>>>>>> break
>>>>>>>>>> again 0.5. I know we are incubating but I'm not sure
it is good
> to
>>>>>>>> proceed
>>>>>>>>>> this way, in particular when the path to avoid it
is quite clear.
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> Side note:
>>>>>>>>>>> 
>>>>>>>>>>> The other thing we discovered while working on
this is that we
>>>>>>>> currently
>>>>>>>>>>> do not lock in the DB.
>>>>>>>>>>> 
>>>>>>>>>>> Imo we should aim to be able to have something
like start a Job
>>>>> with a
>>>>>>>>>>> PartitionMapper and then have all 5 cluster nodes
grab job by
> job
>>>>> and
>>>>>>>> work
>>>>>>>>>>> them off.
>>>>>>>>>>> For this to happen we need to properly detect/prevent
concurrent
>>>>>>>>>>> modifications in the DB. Happy for feedback and
brainstorming ;)
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>> Other thread ;)
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> LieGrue,
>>>>>>>>>>> strub
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>>> On Wednesday, 17 August 2016, 17:53, Romain
Manni-Bucau <
>>>>>>>>>>> rmannibucau@gmail.com> wrote:
>>>>>>>>>>>>> 2016-08-17 17:47 GMT+02:00 Scott Kurz
<skurz3@gmail.com>:
>>>>>>>>>>>> 
>>>>>>>>>>>>> If preserving this ability were a goal
the filtering could be
>>>> done
>>>>>>>> by
>>>>>>>>>>>>> refining/adding DB queries in the JDBC/JPA
persistence and
> maybe
>>>>> the
>>>>>>>>>>>>> in-memory persistence could treat performance
as a non-goal.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>>>>>>>>>>>> Hi Scott,
>>>>>>>>>>>> 
>>>>>>>>>>>> this is pretty hard to keep it with the same
API and do it
>>>>> correctly
>>>>>>>> on
>>>>>>>>>>> the
>>>>>>>>>>>> backend - agree the in memory impl is not
concerned.
>>>>>>>>>>>> 
>>>>>>>>>>>> My 2cts are that if we break the API we could
directly remove
> it
>>>>>>>> since
>>>>>>>>>>> the
>>>>>>>>>>>> security is easy to add on top of JobOperator
for any user and
> we
>>>>>>>> never
>>>>>>>>>>> get
>>>>>>>>>>>> any feedback of a user SecurityService for
now. This allows to
> get
>>>>>>>> rid
>>>>>>>>>>> of a
>>>>>>>>>>>> vendor SPI which didn't proove until now
to be user-useful.
>>>>>>>>>>>> 
>>>>>>>>>>>> @Scott: do you recall any real-world concern
on that area or
> was
>>>> it
>>>>>>>> an
>>>>>>>>>>> IBM
>>>>>>>>>>>> feature inheritance?
>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>>> On Wed, Aug 17, 2016 at 10:41 AM, Romain
Manni-Bucau <
>>>>>>>>>>>>> rmannibucau@gmail.com>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Hi guys,
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Mark and Reinhard spotted a perf
issue in getJobNamed() -
>>>>> basically
>>>>>>>>>>> we
>>>>>>>>>>>>> were
>>>>>>>>>>>>>> fetching all jobs to then filter
the names.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Origin is the ability to filter the
shown jobs per *id*. This
>>>> was
>>>>>>>>>>> very
>>>>>>>>>>>>>> flexible cause it allows to get job
metadata and filter with
> all
>>>>>>>>>>> these
>>>>>>>>>>>>>> meta. In these meta you have jbatch
meta (name, startedAt
> etc..)
>>>>>>>> but
>>>>>>>>>>>> also
>>>>>>>>>>>>>> user meta since the id is accessible
through the batch so it
> can
>>>>> be
>>>>>>>>>>>> used
>>>>>>>>>>>>>> for business logic as well.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Reviwing the security service usage
it seems to me we could
> drop
>>>>> it
>>>>>>>>>>>>>> completely since I think we should
enable to secure
> JobOperator
>>>>> and
>>>>>>>>>>>>> nothing
>>>>>>>>>>>>>> under that level to give business
value to the security API.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> So here is the proposal I have:
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> - to not break users who could rely
on that we keep current
>>>>>>>>>>>>> SecurityService
>>>>>>>>>>>>>> API for next release (R) but @Deprecate
it (impl can use
>>>>>>>>>>>> shortcuts/flags
>>>>>>>>>>>>> to
>>>>>>>>>>>>>> deactiavte it for jobNames temporarly)
>>>>>>>>>>>>>> - in release R+1 we completely remove
it
>>>>>>>>>>>>>> - *if* we get request to secure JobOperator
we build a
> security
>>>>>>>>>>> either
>>>>>>>>>>>> on
>>>>>>>>>>>>>> top of JobOperator or directly on
persistence layer (= at
> query
>>>>>>>> level
>>>>>>>>>>>> and
>>>>>>>>>>>>>> not java level which prevents any
perf optim).
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Any objection? Other idea?
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Romain Manni-Bucau
>>>>>>>>>>>>>> @rmannibucau <https://twitter.com/rmannibucau>
|  Blog
>>>>>>>>>>>>>> <https://blog-rmannibucau.rhcloud.com>
| Old Wordpress Blog
>>>>>>>>>>>>>> <http://rmannibucau.wordpress.com>
| Github
>>>>>>>>>>>> <https://github.com/
>>>>>>>>>>>>>> rmannibucau> |
>>>>>>>>>>>>>> LinkedIn <https://www.linkedin.com/in/rmannibucau>
|
> Tomitriber
>>>>>>>>>>>>>> <http://www.tomitribe.com>
| JavaEE Factory
>>>>>>>>>>>>>> <https://javaeefactory-rmannibucau.rhcloud.com>
>>>> 
>>>> 
>> 


Mime
View raw message