batchee-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Romain Manni-Bucau <rmannibu...@gmail.com>
Subject Re: bye bye security service?
Date Wed, 17 Aug 2016 19:44:21 GMT
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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message