microsoft / MIMWAL

The MIMWAL is a Workflow Activity Library (WAL) solution for configuring complex Workflows in the Microsoft Identity Manager (MIM) 2016 and Forefront Identity Manager (FIM) 2010 R2 solution.
http://aka.ms/MIMWAL
Other
104 stars 36 forks source link

Documentation review #1

Closed missmiis closed 8 years ago

missmiis commented 8 years ago

Hi all. Jef asked if I could do some doco review - very happy to help. I've only got through the Activities section so far but wanted to post these comments - hopefully this is an ok place to do it.

Build and Deployment

Note: If you get any access denied errors from FIM Service during the creation of AIC's, you may have forgotten to correct the typo "IsAuthoriztionActivity" to "IsAuthorizationActivity" in the built-in MPR called "Administration: Administrators control configuration related resources".

I didn’t know myself that such a typo existed and I just create my own AIC policy. Is there a link to a technote about this? Rather than “You may have forgotten” instead link to the technote. (I’m surprised a patch would not have fixed this.)

Create Resource Activity

Iteration Optional. This is a lookup or function expression returning a collection of values to iterate over. use of iteration disables publication of created resource Id ("Target for Created Resource ID") and conflicting resource Id ("Target for Conflicting Resource ID").

In the “Delete Resource” activity there is a link to the Iteration page from this sub-section.

While the activity supports iteration, it is best to refrain from creating more than one resources in a single activity.

Should be “resource” not “resources”.

Why is it best? I only ever used this activity to create single resources myself, but the functionality must be there for some reason. Would be good to add a bit more explanation about why it is recommended – the main things I can think of right now is it’s much easier to track what’s going on, and there is no chance of accidentally spawning creation of thousands of objects. Or alternatively add a comment about whatever specific use case this feature was added for and state you wouldn’t use it otherwise.

BTW this group creation example is a really good one – I’ve recently been finding out just how difficult(/impossible) something like this is for certain competitor products.

Generate Unique Value Activity

Noticed one instance of “FIMServive” on the page.

It’s good you talk about bulk updates – I got into a terrible mess trying to generate a lot of AccountNames using this activity. I think it was made even worse by SQL doing an extra uniqueness lookup for that specific attribute.

Run PowerShell Script Activity

Powershell Script User: Worth mentioning that the FIMService service account is what is used to run the script by default.

Impersonation: there were problems with that and Craig’s PowerShell activity. The end result was a reg change was needed on each FIM Service server – if you didn’t make the reg change you could only impersonate members of the local Administrators group. The fact that a logon type has to be specified makes me think this might be the same. If so it would be worth expanding on the pre-reqs for the users that can be impersonated. It’s pretty common to want to say “any”, especially when it’s the original requestor we want to impersonate.

It would be really good to have a script template as a starting point, which shows how to send parameters to the script, and how to get information back from it.

I don’t agree with the comment about not using it in productions environments – in fact sometimes you have to (like when performing Exchange activities). Sounding a note of caution about spawning lots of sessions is worthwhile however.

On the comment about things failing – I use a lot of Invoke-Expression so I can set up the command I’m going to run, log the exact command, then run it, and capture the results. Try…Catch followed by testing we got a result is very helpful too. Anything to stop the script bombing out but returning a valid status. (Another thing to include – how to return a Failed status to the workflow – I normally just Throw the error message I want the request to store – that’s with Craig’s activity.)

I don’t see anything about the changes that need to be made to be able to run the FIMAutomation cmdlets. It’s possible that the config file changes are made as part of the setup, but we also need a person object for the FIMService service account that can login to the Portal and has the appropriate rights.

Finally – something on error reporting from the scripts would be good. Eg write to log fie, write the Event Log, return RequestStatusDetail to request.

Activity Advanced Features Iteration

Supported Activities does not mention Delete Resource.

NileshGhodekar commented 8 years ago

Thanks Carol. This is good. I'll keep it short for now as I'm working on some other check-ins, but just wanted to say keep going :)

missmiis commented 8 years ago

ConvertToBoolean Function

What string values can actually be converted? 0/1, true/false, Y/N, Yes/No...?

ConvertToNumber

I assume if it can't convert the string to a number it also throws an error. Does this just work on the digit as string? Actually for both of these Convert functionions, if it's just using a standard .NET method, then perhaps ust link to the MSDN doco on that.

DateTimeToFileTimeUTC Function

Example is for a differnt function.

DateTimeFormat Function

Would be useful to have an example showing how to format a datetime so it can be saved into a FIM datetime attribute. Not sure if you'd actually put that here anyway, but it would be useful to have a note somewhere on how dates are stored.

ParametersContain Function

Example does not have the attribute.

ParametersList Function

Example has ParametersContain function.

BTW these Paratemers functions are fantastic! Actually they're all great - really comprehensive!

LTrim Function

Example looks odd - double-check it's right.

NormalizeString Function

Might just need some extra clarification on the example (or perhaps two seperate exampels also showing sample before and after strings) - at first I thought this wasn't doing a standard remove diacrtics because of the substitution strings being listed, but then read it a couple more times and now I think it's removing diacritics except for those ones listed in the substituion string, which are handled by a string replace. Extra examples could help clarify (and a lot of people just look at the examples).

RTrim Function

Similar to LTrim - I'm not sure about the example. Also for Trim.

JefTek commented 8 years ago

Great responses!

Regarding the "isAuthoriztionActivity", I believe this has been the state of the rule since FIM 2010, and persists in FIM2010 R2. However, in MIM2016 it is corrected with "isAuthorizationActivity". If you upgrade FIM2010 to MIM2016, the Database upgrade process will correct the rule. In fact, if you have corrected it yourself prior to the update, you will see an error during the MIM DB upgrade process since it can't insert the value that already exists in the table. There is no tech note that I am aware of, but maybe we expand upon this in the FAQ?

I think we should expand on the potential risks of a poorly written powershell script as well.

NileshGhodekar commented 8 years ago

re: PowerShell and Exchange, there was a support case where PSS spent some 80+ hrs trying to investigate a memory leak issue which turned out to be a PowerShell / RPS bug. I also spent a few hours proving this was not a WAL issue and could repro it in PowerShell ISE :) IIRC, PowerShell team said easy work around exists (i.e. use external host), so don't think they took it up as an issue to be fixed.

EugeneSergeev commented 8 years ago

regarding DateTimeToFileTimeUTC function, the example says: DateTimeToFileTimeUTC (DateTimeAdd([//Target/EmployeeEndDate],1)) - adds one day to dismissal date and converts it to integer. What's wrong?

EugeneSergeev commented 8 years ago

added more examples to normalize string function https://github.com/Microsoft/MIMWAL/wiki/NormalizeString-Function

EugeneSergeev commented 8 years ago

stupid buttons...

NileshGhodekar commented 8 years ago

Yes, to be realistic I don't think we can close this before we go live, Now that my man Eugene is on this thread, I'd asked him to give some real world examples for an internal QRG document which he quickly produced, so out of context some of these examples may not make sense. Now for the wiki we need to provide some explanation as to what the example is doing for clarify just like we did for NormalizeString, but I myself cannot imaging why some want to L/R/Trim any thing apart from spaces and many other examples, so I'll leave such to Eugene to provide better examples / explanations who undoubtedly has the most real world experience among MCS. BTW while doing the documentation for Remarks sections, I felt some functions can behave better, so the last checking has it that NormalizeString makes second parameter optional :)

NileshGhodekar commented 8 years ago

Also Eugene, if you have any comments / tips, move them directly inline. In the case of NormalizeString to the Remarks section itself. Re: ToFileTimeUTC, I think we just need some explanation what and why example is doing. In this case "Time" is a big fat number.

EugeneSergeev commented 8 years ago

btw, regarding add delay function and provided example. I wonder what will happen when you will span lets say 100 workflow instances and all of them will execute a delay activity with 7 days duration. I would say this will affect other workflows as we can hit a limit of simultaneous workflows per instance, which is calculated by a formula * per cpu....

unless proven to be safe, I would remove that example.

EugeneSergeev commented 8 years ago

added a couple of comments, examples and use case explanation to Request Approval, Generate Unique Value

NileshGhodekar commented 8 years ago

The DelayActivity example is the best use case and best demonstration (i.e. do that for precision). The load should not any different that running workflows on set transitions or batch updates.

EugeneSergeev commented 8 years ago

no, wait a minute. let's say you load 1000 users into FIM Service causing 1000 workflows to start. under normal conditions FIM Service will start 140 instances on 2 cores machine (70 instances per 1 core, there's a special parameter for that in FIM Service config). after the first 20 will be completed FIM service will start another 20 and so on... the max number of simultaneously running workflows will remain 140.

now, imaging that you added a delay of 1 week. my understanding is that if WF instance is not suspended and stored in SQL then it will take more than a week to complete all those WFs...

p.s. if the SQL server edition running FIM Service DB is not Enterprise but Standard the things will be even worse, as FIM Service can do throttling only with SQL Enterprise.

missmiis commented 8 years ago

Scenario Generate Unique Account Name

Intro paragraph doesn't read well - needs some rewriting.

In the table the Value Expressions title is ot aligned with the values, and the text is all in bold in the final box.

I would add a note of warning that this approach should not be used for bulk operations - this was mentioned on the page describing the Generate Unique Value function, but should be repeated, or a note with a link to that page.

Scenario Generate EmployeeID

Are there pictures missing? I'm seeing a number of boxes which say "Scenario Generate Employee ID n".

The table needs line breaks between the activities to make it easier to read.

Complex approval scenario using an approval matrix and approval delegation

Looks like an interesting example but I don't think this page is complete so will leave it to Eugene for now.

New Accounts Approval

First, create a set !new users without AD accounts with criteria Provision to AD is not True (no users will be in this set until you or someone/something will set this attribute to true).

The comment in the brackets isn't right.

With the Mailboxes example - since Exchange 2010 it breaks enabling the mailbox if you export mailNickname so I never do that any more.

Automated Contractors End Date Extension with Approval

Re-word this: *Sure, you have to create one more policy that will grant permissions and will require approvals for requests submitted by contoso\fim.agent.

They are very regular MPR: !AD: approve requests from FIM Agent - account extension and Authorization Workflow: !AD: approve account end date extension.*

Suggest instead: "In addition you need to create a standard Approval policy to obtain the manager's approval. Create the Approval Workflow "!AD: approve account end date extension" and MPR "!AD: approve requests from FIM Agent - account extension"."

How to send notifications on dynamic groups membership changes

ALERT: THIS SOLUTION NOT TO BE USED.. change to "ALERT: THIS SOLUTION IS NOT TO BE USED"

I never thought of doing it that way - insteresting idea - but yes totally agree with your warning there!

NileshGhodekar commented 8 years ago

Eugene, Re: Delay Activity, not sure I get you question, if you added a delay of 1 week, yes workflows will not complete for at least 1 week, they may take 1 weeks plus few more minutes depending on how much stress on the system is and how many workflows are running when the FIM Workflow Host gets the notification that their timer is expired. It won’t schedule more than the configured number which is just ~4 workflows per core (5 * ProcessorCount) * 0.8 ). (So for 140 workflows in your example, I would need a ~10 core machine, but that’s not relevant for this discussion). I should note in the wiki that if the server which ran a delay workflow is not up and running when the timer is expired, the workflow will not resume until that server is up and running. But I will wait for someone to raise the DCR against the FIM Workflow Host for that to allow workflow to run on any server in the same service partition :). This is the main reason I don't recommend delay of more than 1 day even though theoretically it can be 60 days before FIM kills the workflow. ...And if your question is if the paused workflow is still counted in the concurrent limit, then the answer is no, as far as system is considered it's disappeared.

NileshGhodekar commented 8 years ago

Ignore my math! for 140 concurrent workflows you need 35 cores by default.

EugeneSergeev commented 8 years ago

Nilesh, I'm only worried about one thing. I've seen a solution where 50 instances of the powershell activity with start-sleep 1 day command had blocked everything else. all other requests stuck in the committed state. if you say it won't happen - good.

EugeneSergeev commented 8 years ago

Carol, 'First, create a set !new users without AD accounts with criteria Provision to AD is not True (no users will be in this set until you or someone/something will set this attribute to true). \ - changed.

NileshGhodekar commented 8 years ago

Implementation Guidance of the Delay Activity specifically calls out not to use PowerShell activity for that very reason :)

EugeneSergeev commented 8 years ago

updated DateTimeToFileTimeUTC description and example

EugeneSergeev commented 8 years ago

DateTimeFormat example updated

EugeneSergeev commented 8 years ago

Carol, Complex approval scenario page looks completed to me. it has the full listing in the very end.

EugeneSergeev commented 8 years ago

"since Exchange 2010 it breaks enabling the mailbox if you export mailNickname so I never do that any more." - I didn't get that. I do normally have persistent outbound flow of the mailnickName attribute and have never had any issues with it. this particular example is taken from the lab which is 3 years old now, and it runs with Ex2010 - no issues.

EugeneSergeev commented 8 years ago

Automated Contractors - changed the wording. btw, did you notice the request name update step? such a neat feature no one really uses

EugeneSergeev commented 8 years ago

updated Request Approval description with 3 most common (as for me) use cases and examples

EugeneSergeev commented 8 years ago

added request splitter example to Create Resource description

EugeneSergeev commented 8 years ago

added notes to Update Resource activity regarding aggregation and scope. shall we also mention if the value being written doesn't differ from an existing one - update will not happen?

EugeneSergeev commented 8 years ago

we need some nice text in here: https://github.com/Microsoft/MIMWAL/wiki/Activity-Advanced-Features-Activity-Execution-Condition - I only added a warning that queries will be executed regardless of condition (which I would say is a bug)

NileshGhodekar commented 8 years ago

Just the day before we spend some time with Allen to troubleshoot a bug that he reported with Queries and Update Resources were not working! Turned out this was due to the optimization in the UpdateResource logic that updates won't happen if the values are not actually modified. I've updated the ReadMe, but go ahead and move that to Activity Guidance itself.

EugeneSergeev commented 8 years ago

:) I knew that sooner or later it will happen. that's why I put that example into update resource activity description. check the very bottom of the page

NileshGhodekar commented 8 years ago

RE: AEC, it's a feature. Since you are calling it a bug, I revised the Verbiage / Help Text on the UI. it will cause "Core Task" to skip with an explanation on what the core task is :) The current design allows you to use queries in AEC and hence it's a feature feature.

EugeneSergeev commented 8 years ago

LoL. what's the point of running queries if AEC is false?

NileshGhodekar commented 8 years ago

The results of the queries be checked to decide if you want to execute the activity or not.

EugeneSergeev commented 8 years ago

then we need another explanation, that AEC supports [//Queries/...]

and while it's a nice feature, I would vote to turn it off for performance reasons and millions of failures I've seen.

see this, if there's a query /Person[Department=[//WorkflowData/X]] it could potentially fail if the X doesn't exist, and in this case AEC of IsPresent([//WorkflowData/X]) won't help. it will fail. due to this I had to fill all the variables used in queries with fake values or fake GUIDs (like if the manager is not found)... so queries won't fail

NileshGhodekar commented 8 years ago

Advance functions wiki is not written yet, but the activity already mentions this. Also there is a visual clue in the order of placement of the UI elements. Your workflows can fail for many reasons if you have not tested your expressions and looked up documentation or you write bad XPath filters or XPath not supported by FIM :)

EugeneSergeev commented 8 years ago

ok. let's rephrase it: can we have queries not failing because of lookup failures? e.g. //WfD/X doesn't exists but used in expression?

NileshGhodekar commented 8 years ago

Yes, you check IsPresent in your query expression just like you'd anywhere else.

JefTek commented 8 years ago

If the Query is based on Xpath, and that is executed before the AEC, where are you re doing the IsPresent on the potential Variable that is used in the query? Perhaps a simple ordered list of actions of what fires before each would make sense in the Advanced Features article?

EugeneSergeev commented 8 years ago

added https://github.com/Microsoft/MIMWAL/wiki/Request-Splitter example scenario to demonstrate iterations/ApplyAuthZ

NileshGhodekar commented 8 years ago

Jef, did you misss the XPath Search filter wiki? The usage is more generic that Queries. Also there were a couple of post on yammer on this subject, the latest being by Paul.

JefTek commented 8 years ago

Nilesh,

I must have missed this :)

So I can wrap the Xpath in something lie this, and if it returns Null does it not execute the query?

IIF(IsPresent([//WorkflowData/LocationCode]),"/Location[LocationCode="+[//WorkflowData/LocationCode] +"]",Null())

In other words, if I don't have a [//WorkflowData/LocationCode] value, don't issue the query? So the Query and Activity wouldn't fire if IsPresent([//WorkflowData/LocationCode]) returns false.

NileshGhodekar commented 8 years ago
Status - Build And Deployment Wiki

I've made additional clarifications around the IsAuthZ typo. We have also made some additional clarification on UpdateWorkflowXomls script after a great debate last night :). This item is considered closed.

NileshGhodekar commented 8 years ago
Status - Delay Activity Wiki

I've made some additional clarification in the implementation guidance on the FIM WF host behavior and added MSDN references. This item is can be opened only after Eugene does some testing :) :)

NileshGhodekar commented 8 years ago
Status - Create Resource Wiki

I've made some clarifications around the recommendation. I think this closed it, but you may defer on the recommendation or want to add more.

NileshGhodekar commented 8 years ago
Status - Generate Unique Value Wiki

I've moved Eugene's note inline and also added some addition clarifications. This items is considered closed.

NileshGhodekar commented 8 years ago

Jef, re:XPath expression, your syntax is a little wrong and the correct syntax will result it a "/" if the Null() is returned, which will then be an invalid XPath by FIM grammar. But WAL can always treat it as a null query. This feature may be for a future release :)

NileshGhodekar commented 8 years ago
Status - Run PowerShell Script Wiki

Updated wiki to address comments and much more.

@cwapshere re: Impersonation, I added a reference to the PS Connector for the permissions required. Also clarified that who gets impersonated and that there is no way to impersonate original requestor when running scripts.

re: script template, For the script input and output, I've added additional examples in the examples sections. The script RunPSLoggingSample.ps1 that gets copied to the SolutionOutput folder post build is pretty much the starting template that everyone should be using.

re: production recommendation, unfortunately it stays. But I've elaborated the reasoning in much more detail so one can take an informed decision.

re: error handling and logging, I've elaborated that try/catch/trap may not help with avoiding errors and a pessimistic approach to script development is better suited to avoid aborted workflow. The script error reporting / tracing was already covered but I've now moved the paragraph before the InvokeImmediateTermination script example for out-of-process scripts as I see that you missed it. The script InvokeImmediateTermination is already quite well instrumented even though it's hard to notice. Plug it in the WAL activity and run if you don't believe :)

re: FIMAutomation cmdlets, I've added a liner in the implementation guidance that if WAL's native activities can't do it, report the issue. We were not able to find a use case for it when WAL is used.

I'm marking all comments on PSH activity closed now.

EugeneSergeev commented 8 years ago

FIMAutomation could be loaded in WAL PS Activity session. requires FIM Service config to be changed from having an external name to be http://localhost:5725

NileshGhodekar commented 8 years ago

I think Carol is already aware of this since she mentioned it also needs a Person object for FIM Service.

EugeneSergeev commented 8 years ago

correct