John Dennis wrote:
On 11/04/2015 10:25 PM, Rob Crittenden wrote:
> John Dennis wrote:
>> In the Web UI if you go to the admin page, then click on the "Identity
>> Providers" tab, then click on "saml2 manage" you'll be brought
to the
>> "Service Providers" page where there is a "Add New" clickable
link and a
>> list of existing service providers below it.
>>
>> What I don't understand and would like someone to explain to me (because
>> I'm trying to modify the code) is why the template for adding a new SP
>> is a hand crafted page (templates/admin/providers/saml2_sp_new.html)
>> that only permits a slim set of the configuration options but if you
>> click on one of the links to view an existing SP (or edit it) and
>> entirely different template is used
>> (templates/admin/option_config.html). Unlike the template for adding an
>> SP this template is not specific to the data (e.g. adding a new SP),
>> rather it is a generic data driven template that populates itself from
>> the ServiceProvider config items. In fact the option config template
>> paradigm (templates/admin/option_config.html) is used in a variety of
>> places to build a config page.
>>
>> So why does the NewSPAdminPage class have a significantly different
>> implementation than the SPAdminPage class?
>>
>> Validation of the data is handled differently, the data that can be
>> specified is different and the generation of the HTML is different. Why?
>>
>> Why am I asking this question? It's not just academic :-) I'm trying to
>> fix ticket #130 and also alleviate some of the major headaches we had
>> getting ECP to work in rippowan. We need to be able to show the SP
>> metadata on the SP config page and optionally allow it to be edited (if
>> it's not signed). This is because many SAML problems have their root
>> cause in the metadata and if you can't see (or fix) the metadata it's
>> really hard to diagnose the problem. The problem with implementing the
>> fix is the mechanism for specifying the metadata on the "New Service
>> Provider" page is entirely different from what the "Service Provider
>> Configuration" page uses and that's creating a lot of awkward weirdness
>> in implementing the fix. It seems like things would be much simpler and
>> more robust if the way you add an SP and the way you view an SP used the
>> same code and template. Is there some compelling reason they don't? Is
>> there some reason the two pages have disjoint sets of data that can be
>> specified?
>>
>
> Yeah, it's a bad situation and I've run into similar problems it's just
> a rather huge task to tackle. I was going to bring that up for 1.3.
>
> The whole writing and reading data is a nightmare right now using either
> mechanism. I think the util.config method is the way to go forward we
> just need to improve the import/export handling to hide the data
> implementation better. The data normalization still leaves a lot to be
> desired. There have just been bigger fish to fry.
>
> That, and I think the new SP page will always need some extra hand
> holding but it should still be possible to do a generic-ish page for it.
O.K. good so I'm not nuts :-)
I agree this is probably not the moment for a significant code
modification and that could wait some team group think on how best to
approach it.
However in the specific case of the SP "new" and "config" pages they
could be folded together continuing to use the existing mechanisms and
at least remove 1 layer of redundancy.
In this context my primary question is is there a reason the two are
different, perhaps from a UI perspective? Is there a reason when you
create a new SP you can only specify a subset of values? Because if we
fold the two pages together we'll lose that distinction. My take is it
doesn't make a lot of sense to create an SP on one page, only partially
configuring it and then have to navigate to a different page to finish
configuring it. Plus I think there is a value in having the UI be
consistent. I don't want to do the work of merging them together only to
learn in code review it violates some principle I'm not aware of and go
back to the drawing board.
I think the merging will be part of the re-design. I'd advocate leaving
it as-is for now, even as undesirable it is to have the duplication.
Now on to the real problem and my thoughts on how to fix it. I'd
love
some early feedback.
The metadata needs to be displayed in an HTML <textarea> widget. The
"new sp" page hardcodes the <textarea> into the HTML template. But the
"sp config" page builds all it's contents from util.config.Config items.
The script code in options_config.html looks at the class of the Config
item (e.g. String, List, Choice, Pick, etc.) and builds a UI element
based on the Config type. But it's hard to tailor the UI at this level
of granularity. For example how do you display a string? Is the string
logically a single line of text or is it a multiline text block?
My thought, and this is where I'd like to see if there is agreement, is
to add optional properties to the Config items which guide what UI
widget is used to display it and how it's formatted. Thus for example
the String config item might pick up the 2 new properties "rows",
"columns" where rows defaults to 1 indicating it's intended to be a
single line and gets rendered in a text input widget. But if rows > 1 it
gets rendered in a textarea widget. The basic mechanism is pretty simple
and flexible (doesn't IPA do something similar?)
I'm not sure how IPA does it, Petr^1 would know though.
I think the properties idea is a good one. Currently there is no generic
way to do validation. I think making that part of the config object in
the future would be a good idea. That would definitely mirror what IPA does.
The alternative to adding properties is to subclass. For example instead
of just having a String config item we would derive a new class
TextBlock from String. My only concern with the subclassing approach is
each time we discover we need finer control we end up with a new class,
perhaps too many classes that other code then has to become aware of.
I think we have a much smaller universe of input types but I see where
you're going. I think properties is a good place to start and we can
subclass when necessary.
Adding properties could also make validation easier and more
generic,
e.g. properties for legal character set, min/max values, etc.
Thoughts?
BTW, at the moment there is no way the option_config page can display
metadata because the contents need to be rendered in a textarea widget.
But option_config renders Strings as a single line text input, and for
reasons that are not obvious to me the only use of a textarea is in
rendering a list where each list item is joined by a newline and
inserted into the textarea (fwiw it seems to me properties would be a
better way to control how lists are displayed rather than assume every
element is a single line of text).
I saw that code too and scratched my head a bit and then moved onto
other things. I think there is still a bit of code left over from early
one-man development that lacks some error checking, or where a new
approach was required some duplication. I can totally see creating a
new_sp page and then saying "ah crud, now I need to modify it" and
creating a new page using the shiny new config idea. Parts of Ipsilon
have grown organically and it was good for 1.0, we just need to
recognize where this has happened and and try to improve on it.
I think properties would be a good fix for the String problem, I'd start
there.
rob