Include cause in HTTP response ProblemDetails

[gstaa]

Cause is a mandatory attribute of the ProblemDetails IE.

Cause is set according to particular NF standard or TS 29.500.

Additionally:

• 30 misused OGS_SBI_HTTP_STATUS_FORBIDDEN statuses changed.
• OGS_SBI_HTTP_STATUS_MEHTOD_NOT_ALLOWED typo fixed.
• [PCF] Fixed SM Policy establishment error handling

In some cases cause does not need to be included
(e.g. 404, 405, 501).

[acetcom]

@gstaa Is the Cause of ProblemDetails a required value? I think we should first check if it is required by the standard documentation.

[gstaa]

The attribute itself is not mandatory:
TS 29.571, Table 5.2.4.1-1: Definition of type ProblemDetails

• Attribute name: cause
• P: C
• Description: A machine-readable application error cause specific to this occurrence of the problem
  This IE should be present and provide application-related error information, if available.

As seen in the table for the ProblemDetails, the Cause value is C (conditional), but if the information is available, the Cause should be present!

Each NF has situations specified where the Cause is mandatory.

e.g. PCF: TS 129 512:

• 4.2.2 Npcf_SMPolicyControl_Create Service Operation
  If the user information received within the "supi" attribute is unknown, the PCF shall reject the request and include in an HTTP "400 Bad Request" response message the "cause" attribute of the ProblemDetails data structure set to "USER_UNKNOWN".
• 4.2.4.2 Requesting the update of the Session Management related policies
  … the PCF shall reject the request and include in an HTTP "403 Forbidden" response message the "cause" attribute of the ProblemDetails data structure set to "ERROR_TRAFFIC_MAPPING_INFO_REJECTED".
• 4.2.4.17 UE initiates a resource modification support
  …the PCF shall reject the request and indicate the cause for the rejection including the "cause" attribute of the ProblemDetails data structure set to "ERROR_TRAFFIC_MAPPING_INFO_REJECTED" in an HTTP "403 Forbidden" response message.
• 5.7.3 Application Errors
  The application errors defined for the Npcf_SMPolicyControl API are listed in table 5.7.3-1 and 5.7.3-2. The PCF shall include in the HTTP status code a "ProblemDetails" data structure with the "cause" attribute indicating the application error as listed in table 5.7.3-1 when PCF acts as a server.

Let's see the "USER_UNKNOWN" example solved by this PR:
As seen in the table for the ProblemDetails, the Cause value is C (conditional), but in this case we have the information available – USER UNKNOWN – and therefore we think the Cause should be present.
And explicitly from TS 129 512:
The entire Npcf_SMPolicyControl API shall provide "cause", but in this particular case the PCF shall reject the request and include the "cause" attribute of the ProblemDetails data structure set to "USER_UNKNOWN".

[acetcom]

@gstaa

So why not add a comment to the source code as shown below?

$ diff --git a/src/pcf/pcf-sm.c b/src/pcf/pcf-sm.c
index 2e1973860..cce05ebf3 100644
--- a/src/pcf/pcf-sm.c
+++ b/src/pcf/pcf-sm.c
@@ -207,10 +207,19 @@ void pcf_state_operational(ogs_fsm_t *s, pcf_event_t *e)
 
             if (!sess) {
                 ogs_error("Not found [%s]", message.h.uri);
+        /*
+         * TS29.512
+         * 4.2.2.2 SM Policy Association establishment
+         *
+         * If the user information received within the "supi" attribute is
+         * unknown, the PCF shall reject the request with an HTTP "400 Bad
+         * Request" response message including the "cause" attribute
+         * of the ProblemDetails data structure set to "USER_UNKNOWN".
+         */
                 ogs_assert(true ==
                     ogs_sbi_server_send_error(stream,
                         OGS_SBI_HTTP_STATUS_NOT_FOUND,
-                        &message, "Not found", message.h.uri));
+                        &message, "Not found", message.h.uri, "USER_UNKNOWN"));
                 break;
             }

If you don't, I can't verify that the cause value is correct.

If it's too much, I think it's better to start with NULL at first.

Thanks a lot!
Sukchan

[gstaa]

@acetcom
I added a comment to the source code for each rejection where the cause is not NULL.
I left NULL for some rejections:

• where cause is empty according to standard (e.g. 404, 405, 501) or
• I was not able to justify the right cause

[acetcom]

@gstaa

The comments you added appear to pull from the standard, but mostly appear to be your interpretation.

If I were you, I would first set all causes to NULL, and only add cause values in obvious cases defined by the standard.

[gstaa]

@acetcom
If I understand correctly your suggestion is not to follow TS 29.500, but only the special cases for which cause is defined by particular NF standard?

[acetcom]

@gstaa

That's pretty much it, but to be more precise, I think it's a good idea to define cause only where it's explicitly stated in the standard documentation first, otherwise it can be difficult for others to understand why you wrote the cause value the way you did.

[gstaa]

@acetcom
Now cause is set just where it is mandatory according to particular NF standard ("shall") in all other cases it is set to NULL.
Additionally:

• OGS_SBI_HTTP_STATUS_MEHTOD_NOT_ALLOWED typo fixed.
• [PCF] Fixed SM Policy establishment error handling

[acetcom]

@gstaa

Is ERROR_INITIAL_PARAMETERS correct for pcf_nudr_dr_handle_query_am_data in src/pcf/nudr-handler.c? You are referring to the one in 4.2.2.2 SM Policy Association establishment, not AM_DATA.

If you are trying to implement this feature for the first time now, it is recommended to minimize the example. If there are wrong usage during review, this pull request will not be merged.

Thanks a lot!
Sukchan

[gstaa]

@acetcom
It is not, I added it by mistake.
It is removed now.
Thanks a lot!