Skip to content

Adding tests to assert AAL values#11035

Merged
Sgtpluck merged 5 commits intomainfrom
dmm/add-tests
Aug 8, 2024
Merged

Adding tests to assert AAL values#11035
Sgtpluck merged 5 commits intomainfrom
dmm/add-tests

Conversation

@Sgtpluck
Copy link
Contributor

@Sgtpluck Sgtpluck commented Aug 6, 2024

WIP Currently just open so folks can look at the changes made in the attributes_asserter_spec file

🎫 Ticket

As part of https://gitlab.login.gov/lg-people/lg-people-appdev/Melba/backlog-fy24/-/issues/55, we discovered a number of issues with our returned AAL values. I wanted to write some tests to make sure our understanding of the current state vs the expected state was accurate.

This first commit is just a refactor of the spec file in order to make it easier to understand and work with,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm wondering why we are using this sp1_issuer, rather than the issuer of the service_provider created here, but i haven't looked into that yet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be called just once, and we can update any changing values in the context blocks

@Sgtpluck Sgtpluck marked this pull request as ready for review August 7, 2024 21:04
@Sgtpluck Sgtpluck requested a review from vrajmohan August 7, 2024 21:04
expect(get_asserted_attribute(user, :email)).to eq expected_email
end

it 'gets UUID (MBUN) from Service Provider' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that MBUN is referenced elsewhere, but what does it mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked it up. I think it is Meaningless But Unique Number. I think that it is confusing and should be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, thanks. I think I'd disagree that it's meaningless too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

early SAML docs used it more, we document it in the glossary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed in 29ea702 !

Copy link
Contributor

@vrajmohan vrajmohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@Sgtpluck Sgtpluck merged commit 59c76e7 into main Aug 8, 2024
@Sgtpluck Sgtpluck deleted the dmm/add-tests branch August 8, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants