Bug 1108660
| Summary: | [AAA] NPE when creating user with empty domain | ||
|---|---|---|---|
| Product: | [Retired] oVirt | Reporter: | Ondra Machacek <omachace> |
| Component: | ovirt-engine-core | Assignee: | Mooli Tayer <mtayer> |
| Status: | CLOSED CURRENTRELEASE | QA Contact: | Ondra Machacek <omachace> |
| Severity: | low | Docs Contact: | |
| Priority: | unspecified | ||
| Version: | 3.5 | CC: | alonbl, bazulay, bugs, gklein, iheim, juan.hernandez, omachace, oourfali, rbalakri, yeylon |
| Target Milestone: | --- | ||
| Target Release: | 3.5.0 | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Whiteboard: | infra | ||
| Fixed In Version: | vt3 | Doc Type: | Bug Fix |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2014-10-17 12:43:34 UTC | Type: | Bug |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | Infra | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
| Bug Depends On: | |||
| Bug Blocks: | 1076964 | ||
|
Description
Ondra Machacek
2014-06-12 12:04:03 UTC
Juan, is an emtpy domain valid when it comes to API? As far as I understand, the mandatory fields are the domain id and the user name. The mandatory fields are the user name and the domain, but they can be provided in different ways:
1. All in the <user_name> tag:
<user>
<user_name>joe</user_name>
</user>
2. The user name and the domain name:
<user>
<user_name>joe</user_name>
<domain>
<name>example.com</name>
</domain>
</user>
3. The user name and the domain id:
<user>
<user_name>joe</user_name>
<domain id="12313123123123"/>
</user>
In this case the caller is using a mix of 1 and 2, with an empty domain name. If we are strict we could reject this as an invalid request, but it would be more useful to accept it, as the domain name has already been provided as part of the <user_name> tag.
Alon - what are your thoughts around that? (In reply to Oved Ourfali from comment #3) > Alon - what are your thoughts around that? <domain><name></name></domain> should result in error as domain "" is not available. Juan, Do we formally support user@domain? as we have a problem if user already has '@' within his name. Can we support only the explicit domain notations? Thanks! We can't stop supporting the domain name as part of <user_name> as that would break backwards compatibility. (In reply to Juan Hernández from comment #5) > We can't stop supporting the domain name as part of <user_name> as that > would break backwards compatibility. we already broke some, not sure this is special. oved, your call... if we are to support that, and if <domain> is not specified, we take the last '@' and if matches some profile name we take that profile as if <domain> was specified. I think we should not support this notation any more, we can clearly state that in release docs, there had already been <domain> no reason for anyone to use this notation. (In reply to Alon Bar-Lev from comment #6) > (In reply to Juan Hernández from comment #5) > > We can't stop supporting the domain name as part of <user_name> as that > > would break backwards compatibility. > > we already broke some, not sure this is special. > > oved, your call... if we are to support that, and if <domain> is not > specified, we take the last '@' and if matches some profile name we take > that profile as if <domain> was specified. > > I think we should not support this notation any more, we can clearly state > that in release docs, there had already been <domain> no reason for anyone > to use this notation. I think we should go with the same way we went in the UI, and to remove the support of using user@domain. We can have a release note explaining it. Barak - do you approve? This isn't special. No backwards compatibility breakage is allowed in the RESTAPI. If you know that you broke something in these regards please explain so that we are aware, as those breakages need to be fixed. (In reply to Juan Hernández from comment #8) > This isn't special. No backwards compatibility breakage is allowed in the > RESTAPI. If you know that you broke something in these regards please > explain so that we are aware, as those breakages need to be fixed. Didn't say it is special. Just say that it was something wrong that we weren't supposed to support in the first place, and I think that we need to remove this support. We can remove things that we think are wrong but users are using in 4.0, not before. Alon why do you think we should not support it ? I have a feeling that a lot of customers use it. (In reply to Barak from comment #11) > Alon why do you think we should not support it ? > I have a feeling that a lot of customers use it. if you think that many customer use this, although there is explicit field for domain, then we should support that. but my question is where is it documented? am I reading the rsdl_metadata.yaml? this is undocumented feature, no contract, can be removed. - name: /users|rel=add description: add a new user to the system request: body: parameterType: User signatures: - mandatoryArguments: {user.user_name: 'xs:string', user.domain.id|name: 'xs:string'} description: add a new user to the specified domain with the specified user name urlparams: {} headers: Content-Type: {value: application/xml|json, required: true} Expect: {value: 201-created, required: false} Correlation-Id: {value: 'any string', required: false} The support for user@domain was added explicitly, as part of the solution for bug 751983. Its use is documented in the documentation, since at least 3.0: https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Virtualization/3.4/html/Technical_Guide/sect-Methods-12.html The RSDL is incorrect, please fix it. (In reply to Juan Hernández from comment #13) > The support for user@domain was added explicitly, as part of the solution > for bug 751983. Its use is documented in the documentation, since at least > 3.0: > > > https://access.redhat.com/documentation/en-US/ > Red_Hat_Enterprise_Virtualization/3.4/html/Technical_Guide/sect-Methods-12. > html > > The RSDL is incorrect, please fix it. In my opinion documentation is not a contract, and sometimes things have to be changed to make the project better, even in the cost of changing the behavior in the past. I also don't think that it is widely used, and even if so we can give a special release note and in addition send an E-mail to users, explaining the change and why it was done. Barak - your call. This same excuse can be used to introduce any backwards compatibility breaking change. In my opinion this isn't taking backwards compatibility seriously. (In reply to Juan Hernández from comment #15) > This same excuse can be used to introduce any backwards compatibility > breaking change. In my opinion this isn't taking backwards compatibility > seriously. As you know, we usually don't do it, so can't say that we don't take this seriously. This isn't an excuse. We're discussing this issue, and different opinions pop-up. I've just expressed my opinion. (In reply to Ondra Machacek from comment #0) > Description of problem: > > > Version-Release number of selected component (if applicable): > ovirt-engine-backend-3.5.0-0.0.master.20140605145557.git3ddd2de.el6.noarch > > How reproducible: > always > > Steps to Reproduce: > curl -k -X POST -H "Accept: application/xml" -H "Content-Type: > application/xml" -H "Filter: $filter" -d > "<user><domain><name></name></domain><user_name>vdcalladmin.lab. > eng.brq.redhat.com</user_name><roles/></user>" -u $U $URL/users > Ondra - with regards to the issue you raise in this patch, does it it work when you use the same request, without the domain section? i.e. Body is: <user><user_name>vdcalladmin.lab.eng.brq.redhat.com</user_name><roles/></user> ? For the body:
<user><user_name>vdcalladmin.lab.eng.brq.redhat.com</user_name><roles/></user>
Response is:
<html><head><title>JBoss Web/7.0.13.Final - Error report</title><style><!--H1 {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;font-size:22px;} H2 {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;font-size:16px;} H3 {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;font-size:14px;} BODY {font-family:Tahoma,Arial,sans-serif;color:black;background-color:white;} B {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;} P {font-family:Tahoma,Arial,sans-serif;background:white;color:black;font-size:12px;}A {color : black;}A.name {color : black;}HR {color : #525D76;}--></style> </head><body><h1>HTTP Status 500 - Bad arguments passed to public abstract javax.ws.rs.core.Response org.ovirt.engine.api.resource.UsersResource.add(org.ovirt.engine.api.model.User) ( org.ovirt.engine.api.model.User org.ovirt.engine.api.model.User@7531f21e )</h1><HR size="1" noshade="noshade"><p><b>type</b> Status report</p><p><b>message</b> <u>Bad arguments passed to public abstract javax.ws.rs.core.Response org.ovirt.engine.api.resource.UsersResource.add(org.ovirt.engine.api.model.User) ( org.ovirt.engine.api.model.User org.ovirt.engine.api.model.User@7531f21e )</u></p><p><b>description</b> <u>The server encountered an internal error (Bad arguments passed to public abstract javax.ws.rs.core.Response org.ovirt.engine.api.resource.UsersResource.add(org.ovirt.engine.api.model.User) ( org.ovirt.engine.api.model.User org.ovirt.engine.api.model.User@7531f21e )) that prevented it from fulfilling this request.</u></p><HR size="1" noshade="noshade"><h3>JBoss Web/7.0.13.Final</h3></body></html>[
By mistake removed needinfo. Putting back. The error in comment 18 is a result bug 1113485, not related to this issue. Ondra, Can you please retest (comment #17) with latest build (one that includes a fix for bug 1113485). Issue in comment #17 is ok now, user is added. curl -k -X POST -H "Accept: application/xml" -H "Content-Type: application/xml" -H "Filter: $filter" -d "<user><user_name>vdcalladmin.lab.eng.brq.redhat.com</user_name><roles/></user>" -u $U $URL/users <?xml version="1.0" encoding="UTF-8" standalone="yes"?> <user href="/api/users/98336a79-04c4-45f2-a81d-bd0408209168" id="98336a79-04c4-45f2-a81d-bd0408209168"> <name>vsechny</name> <link href="/api/users/98336a79-04c4-45f2-a81d-bd0408209168/permissions" rel="permissions"/> <link href="/api/users/98336a79-04c4-45f2-a81d-bd0408209168/roles" rel="roles"/> <link href="/api/users/98336a79-04c4-45f2-a81d-bd0408209168/tags" rel="tags"/> <domain href="/api/domains/6272712d-6970-612e-7268-65762e6c6162" id="6272712d-6970-612e-7268-65762e6c6162"/> <domain_entry_id>38666164353131362D343434332D313165312D616539302D303031613461303133663131</domain_entry_id> <namespace>*</namespace> <last_name>skupiny</last_name> <user_name>vdcalladmin.lab.eng.brq.redhat.com</user_name> <groups> <group> <name>brq-ipa.rhev.lab.eng.brq.redhat.com/accounts/groups/ipausers</name> </group> </groups> </user> (In reply to Ondra Machacek from comment #22) > Issue in comment #17 is ok now, user is added. > > curl -k -X POST -H "Accept: application/xml" -H "Content-Type: > application/xml" -H "Filter: $filter" -d > "<user><user_name>vdcalladmin.lab.eng.brq.redhat.com</ > user_name><roles/></user>" -u $U $URL/users > > <?xml version="1.0" encoding="UTF-8" standalone="yes"?> > <user href="/api/users/98336a79-04c4-45f2-a81d-bd0408209168" > id="98336a79-04c4-45f2-a81d-bd0408209168"> > <name>vsechny</name> > <link href="/api/users/98336a79-04c4-45f2-a81d-bd0408209168/permissions" > rel="permissions"/> > <link href="/api/users/98336a79-04c4-45f2-a81d-bd0408209168/roles" > rel="roles"/> > <link href="/api/users/98336a79-04c4-45f2-a81d-bd0408209168/tags" > rel="tags"/> > <domain href="/api/domains/6272712d-6970-612e-7268-65762e6c6162" > id="6272712d-6970-612e-7268-65762e6c6162"/> > > <domain_entry_id>38666164353131362D343434332D313165312D616539302D303031613461 > 303133663131</domain_entry_id> > <namespace>*</namespace> > <last_name>skupiny</last_name> > <user_name>vdcalladmin.lab.eng.brq.redhat.com</user_name> > <groups> > <group> > > <name>brq-ipa.rhev.lab.eng.brq.redhat.com/accounts/groups/ipausers</name> > </group> > </groups> > </user> Thanks for re-testing. So the only issue remaining is using a domain section, with an empty name. In my opinion we shouldn't support that. If he wants an empty domain, then he shouldn't have such a section at all. (In reply to Oved Ourfali from comment #23) > So the only issue remaining is using a domain section, with an empty name. > In my opinion we shouldn't support that. If he wants an empty domain, then > he shouldn't have such a section at all. empty, invalid or missing should return a proper error. Mooli - go ahead and solve the NPE. We shouldn't allow passing a domain section without a name. In addition, Barak - please decide as for the wrong heuristic we're doing to derive the user and domain today. If you agree on fixing it then I'll open a new bug on that. (In reply to Oved Ourfali from comment #25) > Mooli - go ahead and solve the NPE. > We shouldn't allow passing a domain section without a name. > > In addition, Barak - please decide as for the wrong heuristic we're doing to > derive the user and domain today. If you agree on fixing it then I'll open a > new bug on that. I would open a different bug on the huristic. This bug should only be about the NPE This was fixed as while fixing bug 1117488. Result is now: <?xml version="1.0" encoding="UTF-8" standalone="yes"?> <fault> <reason>Operation Failed</reason> <detail>extension could not be found</detail> </fault> verified vt3.1
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<fault>
<reason>Incomplete parameters</reason>
<detail>User [domain.id|name] required for add</detail>
</fault>
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<fault>
<reason>Operation Failed</reason>
<detail>extension could not be found</detail>
</fault>
oVirt 3.5 has been released and should include the fix for this issue. |