Inconsistent API IgniteClient and REST

[RyzhovSV]

Hello!
Now the work of permissions for API IgniteClient and REST is different.
To create/delete a cache:
IgniteClient authorises CACHE_CREATE/CACHE_DESTROY.(GridCacheProcessor#authorizeCacheCreate <https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java#L3983>, authorizeCacheDestroy <https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java#L3973>)
REST authorises ADMIN_CACHE.(GridRestProcessor#authorize <https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/rest/GridRestProcessor.java#L841>)
I think this is inconsistent.

I suggest ADMIN_CACHE mark @Deprecated
and replace it in the GridRestProcessor with CACHE_CREATE / CACHE_DESTROY
while maintaining backward compatibility for ADMIN_CACHE.

This will allow us to remove ADMIN_CACHE in the future.



Sergei Ryzhov
[hidden email]

[Nikolay Izhikov-2]

Hello, Sergey.


I’m +1 to make this change.

I think we should make security consistent across all APIs.

> 31 марта 2020 г., в 12:14, Sergei Ryzhov <[hidden email]> написал(а):
>
> Hello!
> Now the work of permissions for API IgniteClient and REST is different.
> To create/delete a cache:
> IgniteClient authorises CACHE_CREATE/CACHE_DESTROY.(GridCacheProcessor#authorizeCacheCreate <https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java#L3983>, authorizeCacheDestroy <https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java#L3973>)
> REST authorises ADMIN_CACHE.(GridRestProcessor#authorize <https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/rest/GridRestProcessor.java#L841>)
> I think this is inconsistent.
>
> I suggest ADMIN_CACHE mark @Deprecated
> and replace it in the GridRestProcessor with CACHE_CREATE / CACHE_DESTROY
> while maintaining backward compatibility for ADMIN_CACHE.
>
> This will allow us to remove ADMIN_CACHE in the future.
>
>
>
> Sergei Ryzhov
> [hidden email]
>
>
>
>
>
>
>
>

[Andrey Kuznetsov]

I'd prefer marking ADMIN_CACHE as deprecated, but postpone its removal from
GridRestProcessor till next Ignte release (2.10 or 3.0?). For now we could
just add checks for CACHE_CREATE / CACHE_DESTROY there along
with ADMIN_CACHE.

вт, 31 мар. 2020 г. в 12:30, Nikolay Izhikov <[hidden email]>:

> Hello, Sergey.
>
>
> I’m +1 to make this change.
>
> I think we should make security consistent across all APIs.
>
> > 31 марта 2020 г., в 12:14, Sergei Ryzhov <[hidden email]>
> написал(а):
> >
> > Hello!
> > Now the work of permissions for API IgniteClient and REST is different.
> > To create/delete a cache:
> > IgniteClient authorises
> CACHE_CREATE/CACHE_DESTROY.(GridCacheProcessor#authorizeCacheCreate <
> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java#L3983>,
> authorizeCacheDestroy <
> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java#L3973
> >)
> > REST authorises ADMIN_CACHE.(GridRestProcessor#authorize <
> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/rest/GridRestProcessor.java#L841
> >)
> > I think this is inconsistent.
> >
> > I suggest ADMIN_CACHE mark @Deprecated
> > and replace it in the GridRestProcessor with CACHE_CREATE /
> CACHE_DESTROY
> > while maintaining backward compatibility for ADMIN_CACHE.
> >
> > This will allow us to remove ADMIN_CACHE in the future.
> >
> >
> >
> > Sergei Ryzhov
> > [hidden email]
> >
> >
> >
> >
> >
> >
> >
> >
>
>

--
Best regards,
  Andrey Kuznetsov.

[Anton Vinogradov-2]

Folks,

The question is not about "+1" or "-1".
The question is "why?".

This looks like some special feature to solve some special security issue
but may be just a bad/broken/obsolete/unrefactored code.
Changing this semantic we'll fix bad code or break some contract. 50% to
50%.

Let's keep REST case as is for now but start an investigation to gain
security consistent across all APIs, if possible.

On Tue, Mar 31, 2020 at 4:59 PM Andrey Kuznetsov <[hidden email]> wrote:

> I'd prefer marking ADMIN_CACHE as deprecated, but postpone its removal from
> GridRestProcessor till next Ignte release (2.10 or 3.0?). For now we could
> just add checks for CACHE_CREATE / CACHE_DESTROY there along
> with ADMIN_CACHE.
>
> вт, 31 мар. 2020 г. в 12:30, Nikolay Izhikov <[hidden email]>:
>
> > Hello, Sergey.
> >
> >
> > I’m +1 to make this change.
> >
> > I think we should make security consistent across all APIs.
> >
> > > 31 марта 2020 г., в 12:14, Sergei Ryzhov <[hidden email]>
> > написал(а):
> > >
> > > Hello!
> > > Now the work of permissions for API IgniteClient and REST is different.
> > > To create/delete a cache:
> > > IgniteClient authorises
> > CACHE_CREATE/CACHE_DESTROY.(GridCacheProcessor#authorizeCacheCreate <
> >
> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java#L3983
> >,
> > authorizeCacheDestroy <
> >
> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java#L3973
> > >)
> > > REST authorises ADMIN_CACHE.(GridRestProcessor#authorize <
> >
> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/rest/GridRestProcessor.java#L841
> > >)
> > > I think this is inconsistent.
> > >
> > > I suggest ADMIN_CACHE mark @Deprecated
> > > and replace it in the GridRestProcessor with CACHE_CREATE /
> > CACHE_DESTROY
> > > while maintaining backward compatibility for ADMIN_CACHE.
> > >
> > > This will allow us to remove ADMIN_CACHE in the future.
> > >
> > >
> > >
> > > Sergei Ryzhov
> > > [hidden email]
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> >
> >
>
> --
> Best regards,
>   Andrey Kuznetsov.
>

[Nikolay Izhikov-2]

Hello, Anton.

What is «contract» for you?
Do we have this contract written somewhere?

> 1 апр. 2020 г., в 11:35, Anton Vinogradov <[hidden email]> написал(а):
>
> Folks,
>
> The question is not about "+1" or "-1".
> The question is "why?".
>
> This looks like some special feature to solve some special security issue
> but may be just a bad/broken/obsolete/unrefactored code.
> Changing this semantic we'll fix bad code or break some contract. 50% to
> 50%.
>
> Let's keep REST case as is for now but start an investigation to gain
> security consistent across all APIs, if possible.
>
> On Tue, Mar 31, 2020 at 4:59 PM Andrey Kuznetsov <[hidden email]> wrote:
>
>> I'd prefer marking ADMIN_CACHE as deprecated, but postpone its removal from
>> GridRestProcessor till next Ignte release (2.10 or 3.0?). For now we could
>> just add checks for CACHE_CREATE / CACHE_DESTROY there along
>> with ADMIN_CACHE.
>>
>> вт, 31 мар. 2020 г. в 12:30, Nikolay Izhikov <[hidden email]>:
>>
>>> Hello, Sergey.
>>>
>>>
>>> I’m +1 to make this change.
>>>
>>> I think we should make security consistent across all APIs.
>>>
>>>> 31 марта 2020 г., в 12:14, Sergei Ryzhov <[hidden email]>
>>> написал(а):
>>>>
>>>> Hello!
>>>> Now the work of permissions for API IgniteClient and REST is different.
>>>> To create/delete a cache:
>>>> IgniteClient authorises
>>> CACHE_CREATE/CACHE_DESTROY.(GridCacheProcessor#authorizeCacheCreate <
>>>
>> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java#L3983
>>> ,
>>> authorizeCacheDestroy <
>>>
>> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java#L3973
>>>> )
>>>> REST authorises ADMIN_CACHE.(GridRestProcessor#authorize <
>>>
>> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/rest/GridRestProcessor.java#L841
>>>> )
>>>> I think this is inconsistent.
>>>>
>>>> I suggest ADMIN_CACHE mark @Deprecated
>>>> and replace it in the GridRestProcessor with CACHE_CREATE /
>>> CACHE_DESTROY
>>>> while maintaining backward compatibility for ADMIN_CACHE.
>>>>
>>>> This will allow us to remove ADMIN_CACHE in the future.
>>>>
>>>>
>>>>
>>>> Sergei Ryzhov
>>>> [hidden email]
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>> --
>> Best regards,
>>  Andrey Kuznetsov.
>>

[Anton Vinogradov-2]

That's exactly the question I have.
Since no tests and documentation can be found - then we have no contract
and it's safe to change the semantic.
But we should recheck search results twice because we're talking about the
security.

On Wed, Apr 1, 2020 at 11:44 AM Nikolay Izhikov <[hidden email]> wrote:

> Hello, Anton.
>
> What is «contract» for you?
> Do we have this contract written somewhere?
>
>
> > 1 апр. 2020 г., в 11:35, Anton Vinogradov <[hidden email]> написал(а):
> >
> > Folks,
> >
> > The question is not about "+1" or "-1".
> > The question is "why?".
> >
> > This looks like some special feature to solve some special security issue
> > but may be just a bad/broken/obsolete/unrefactored code.
> > Changing this semantic we'll fix bad code or break some contract. 50% to
> > 50%.
> >
> > Let's keep REST case as is for now but start an investigation to gain
> > security consistent across all APIs, if possible.
> >
> > On Tue, Mar 31, 2020 at 4:59 PM Andrey Kuznetsov <[hidden email]>
> wrote:
> >
> >> I'd prefer marking ADMIN_CACHE as deprecated, but postpone its removal
> from
> >> GridRestProcessor till next Ignte release (2.10 or 3.0?). For now we
> could
> >> just add checks for CACHE_CREATE / CACHE_DESTROY there along
> >> with ADMIN_CACHE.
> >>
> >> вт, 31 мар. 2020 г. в 12:30, Nikolay Izhikov <[hidden email]>:
> >>
> >>> Hello, Sergey.
> >>>
> >>>
> >>> I’m +1 to make this change.
> >>>
> >>> I think we should make security consistent across all APIs.
> >>>
> >>>> 31 марта 2020 г., в 12:14, Sergei Ryzhov <[hidden email]>
> >>> написал(а):
> >>>>
> >>>> Hello!
> >>>> Now the work of permissions for API IgniteClient and REST is
> different.
> >>>> To create/delete a cache:
> >>>> IgniteClient authorises
> >>> CACHE_CREATE/CACHE_DESTROY.(GridCacheProcessor#authorizeCacheCreate <
> >>>
> >>
> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java#L3983
> >>> ,
> >>> authorizeCacheDestroy <
> >>>
> >>
> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java#L3973
> >>>> )
> >>>> REST authorises ADMIN_CACHE.(GridRestProcessor#authorize <
> >>>
> >>
> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/rest/GridRestProcessor.java#L841
> >>>> )
> >>>> I think this is inconsistent.
> >>>>
> >>>> I suggest ADMIN_CACHE mark @Deprecated
> >>>> and replace it in the GridRestProcessor with CACHE_CREATE /
> >>> CACHE_DESTROY
> >>>> while maintaining backward compatibility for ADMIN_CACHE.
> >>>>
> >>>> This will allow us to remove ADMIN_CACHE in the future.
> >>>>
> >>>>
> >>>>
> >>>> Sergei Ryzhov
> >>>> [hidden email]
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>
> >> --
> >> Best regards,
> >>  Andrey Kuznetsov.
> >>
>
>

[Nikolay Izhikov-2]

Got it. Thank you.

> But we should recheck search results twice because we're talking about the security.

+1

> 1 апр. 2020 г., в 11:56, Anton Vinogradov <[hidden email]> написал(а):
>
> That's exactly the question I have.
> Since no tests and documentation can be found - then we have no contract
> and it's safe to change the semantic.
> But we should recheck search results twice because we're talking about the
> security.
>
> On Wed, Apr 1, 2020 at 11:44 AM Nikolay Izhikov <[hidden email]> wrote:
>
>> Hello, Anton.
>>
>> What is «contract» for you?
>> Do we have this contract written somewhere?
>>
>>
>>> 1 апр. 2020 г., в 11:35, Anton Vinogradov <[hidden email]> написал(а):
>>>
>>> Folks,
>>>
>>> The question is not about "+1" or "-1".
>>> The question is "why?".
>>>
>>> This looks like some special feature to solve some special security issue
>>> but may be just a bad/broken/obsolete/unrefactored code.
>>> Changing this semantic we'll fix bad code or break some contract. 50% to
>>> 50%.
>>>
>>> Let's keep REST case as is for now but start an investigation to gain
>>> security consistent across all APIs, if possible.
>>>
>>> On Tue, Mar 31, 2020 at 4:59 PM Andrey Kuznetsov <[hidden email]>
>> wrote:
>>>
>>>> I'd prefer marking ADMIN_CACHE as deprecated, but postpone its removal
>> from
>>>> GridRestProcessor till next Ignte release (2.10 or 3.0?). For now we
>> could
>>>> just add checks for CACHE_CREATE / CACHE_DESTROY there along
>>>> with ADMIN_CACHE.
>>>>
>>>> вт, 31 мар. 2020 г. в 12:30, Nikolay Izhikov <[hidden email]>:
>>>>
>>>>> Hello, Sergey.
>>>>>
>>>>>
>>>>> I’m +1 to make this change.
>>>>>
>>>>> I think we should make security consistent across all APIs.
>>>>>
>>>>>> 31 марта 2020 г., в 12:14, Sergei Ryzhov <[hidden email]>
>>>>> написал(а):
>>>>>>
>>>>>> Hello!
>>>>>> Now the work of permissions for API IgniteClient and REST is
>> different.
>>>>>> To create/delete a cache:
>>>>>> IgniteClient authorises
>>>>> CACHE_CREATE/CACHE_DESTROY.(GridCacheProcessor#authorizeCacheCreate <
>>>>>
>>>>
>> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java#L3983
>>>>> ,
>>>>> authorizeCacheDestroy <
>>>>>
>>>>
>> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProcessor.java#L3973
>>>>>> )
>>>>>> REST authorises ADMIN_CACHE.(GridRestProcessor#authorize <
>>>>>
>>>>
>> https://github.com/apache/ignite/blob/aefad946ebd7720f81b460aa39e205c10dc24b26/modules/core/src/main/java/org/apache/ignite/internal/processors/rest/GridRestProcessor.java#L841
>>>>>> )
>>>>>> I think this is inconsistent.
>>>>>>
>>>>>> I suggest ADMIN_CACHE mark @Deprecated
>>>>>> and replace it in the GridRestProcessor with CACHE_CREATE /
>>>>> CACHE_DESTROY
>>>>>> while maintaining backward compatibility for ADMIN_CACHE.
>>>>>>
>>>>>> This will allow us to remove ADMIN_CACHE in the future.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Sergei Ryzhov
>>>>>> [hidden email]
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>> --
>>>> Best regards,
>>>> Andrey Kuznetsov.
>>>>
>>
>>