search

fedora-infra / noggin

Self-service user portal for open-source communities to use over FreeIPA.
MIT License
109 stars 59 forks source link

The Agreements page does not display signed agreements as signed #327

Closed abompard closed 4 years ago

abompard commented 4 years ago

Apparently the user dict that is returned from FreeIPA does not contain the memberof_fasagreement that the representation was relying on to list the agreements a user has signed. As a result the user's agreements are all shown as unsigned even if they actually are.

abompard commented 4 years ago

Maybe you can check it out @ryanlerch ?

ryanlerch commented 4 years ago

@abompard it seems it was a regression that was introduced in this commit:

https://github.com/fedora-infra/noggin/commit/349c96757df1c01be21a4c4d20b6d19a1478f9c8

if i roll back to this commit, i don't see any agreements in a users' list. Although, i fixed that with this change when i was rebuilding the cassettes:

https://github.com/fedora-infra/noggin/commit/5d099bceb623c5f47403a0b95f241705b10f63bf#diff-c5a6e11ee78de6b207bf30f8ba5214f9L304

I'm a little perplexed on how this ever worked, as there hasnt been any changes (major) changes to freeipa-fas to cause a regression like this.

nphilipp commented 4 years ago

I'll poke at this a little.

nphilipp commented 4 years ago

Here's what I understand of commit 349c967:

Unfortunately, memberof_agreements doesn't seem to be included in the result from the user_or_404() IPA query. Here's what I got when using the debugger to break at the end of test_ipa_client_fasagreement_add_user() in noggin/tests/unit/security/test_ipa.py:

ipdb> urec = user_or_404(ipa, "dummy")
ipdb> ppr.pprint(urec)
{'cn': ['Dummy User'],
 'displayname': ['Dummy User'],
 'dn': 'uid=dummy,cn=users,cn=accounts,dc=noggin,dc=test',
 'fascreationtime': [{'__datetime__': '20200811101511Z'}],
 'gecos': ['Dummy User'],
 'gidnumber': ['113800236'],
 'givenname': ['Dummy'],
 'has_keytab': True,
 'has_password': True,
 'homedirectory': ['/home/dummy'],
 'initials': ['DU'],
 'ipauniqueid': ['8a98f82e-dbbb-11ea-88f6-5254007be86f'],
 'krbcanonicalname': ['dummy@NOGGIN.TEST'],
 'krblastpwdchange': [{'__datetime__': '20200811101512Z'}],
 'krbpasswordexpiration': [{'__datetime__': '20201109101512Z'}],
 'krbprincipalname': ['dummy@NOGGIN.TEST'],
 'loginshell': ['/bin/bash'],
 'mail': ['dummy@example.com'],
 'memberof_group': ['ipausers'],
 'nsaccountlock': False,
 'objectclass': ['top',
                 'person',
                 'organizationalperson',
                 'inetorgperson',
                 'inetuser',
                 'posixaccount',
                 'krbprincipalaux',
                 'krbticketpolicyaux',
                 'ipaobject',
                 'ipasshuser',
                 'fasuser',
                 'ipaSshGroupOfPubKeys',
                 'mepOriginEntry'],
 'preserved': False,
 'sn': ['User'],
 'uid': ['dummy'],
 'uidnumber': ['113800236']}
ipdb>

At the moment I don't know what's needed to change in order that IPA returns the agreement memberships with the user, but I'll try to find out.

ryanlerch commented 4 years ago

@nphilipp i think i may have figured out what has happened here.

It was around this time that @abompard changed all the default branch names from master to dev.

It appears that there are a handful of PRs that are not in dev, but were merged into master, and were lost:

https://github.com/fedora-infra/freeipa-fas/pulls?q=is%3Apr+is%3Aclosed

AFAICT, this includes this one, that makes freeipa return what was being expected here:

https://github.com/fedora-infra/freeipa-fas/pull/122

ryanlerch commented 4 years ago

Okay, there is now the following PR ready to be merged, that puts back 4 commits that were lost, and should resolve this issue:

https://github.com/fedora-infra/freeipa-fas/pull/127

nphilipp commented 4 years ago

nphilipp moved this from To do to Done within Sprint in AAA 2 days ago ryanlerch moved this from Done within Sprint to Overall Done in AAA 6 hours ago

squints